Utilize Role isAdmin() functionality

[damiankloip]

Currently we extend Role with OgRole but remove the is_admin flag usage, but all the methods are available. We are also using all the Role methods to check a role has a permission etc.. isAdmin() is built into all that. As if a role have that, it will always return true. So this is a simpler way of implementing admin permission checking, and being more in line with core.

[damiankloip]

I think we need to keep OgAccess::ADMINISTER_GROUP_PERMISSION as that is a global thing.

[amitaibu]

Wow, it's @damiankloip! :)

[damiankloip]

@amitaibu Surprise!

[amitaibu]

I think you need to keep the array_unique, because you might have the same permissions defined in different roles.

[damiankloip]

@amitaibu this is also a pretty critical fix, before this was adding permissions for a users group from all groups the user is a part of. uh oh. Test coverage also included below. I verified locally by first writing the tests, which failed. This then fixed them again.

We are caching this stuff per group/per user so the permissions cached based on this need to reflect that too.

[amitaibu]

👍

[amitaibu]

you can use \Drupal\og\Og::isMember here instead

[damiankloip]

isMember only returns a bool though? We need the membership below to check the roles on it.

[pfrenssen]

I really like the DX of this.

[amitaibu]

Indeed. This reminds me that we should allow setting is-admin in the default roles definition

[amitaibu]

Ok, in front of a computer, so here's the reference \Drupal\og\GroupManager::createPerBundleRoles. That is, \Drupal\og\GroupManager::getDefaultRoles should allow having roles that are is_admin

[damiankloip]

@amitaibu wouldn't this already be supported I'd someone defines a default role that is_admin?

[amitaibu]

Currently not, we have

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
    foreach ($this->getDefaultRoles() as $role_name => $default_properties) {
      $properties = [
        'group_type' => $entity_type_id,
        'group_bundle' => $bundle_id,
        'id' => $role_name,
        'role_type' => OgRole::getRoleTypeByName($role_name),
      ];

(no is_admin). I'll create a follow up issue for this.

[damiankloip]

Oh. I see. Cool!
On 10 Jun 2016 19:05, "Amitai Burstein" notifications@github.com wrote:

In tests/src/Kernel/Access/OgEntityAccessTest.php
#225 (comment):

@@ -133,6 +177,15 @@ protected function setUp() {
->grantPermission($this->randomMachineName())
->save();

• $this->ogAdminRole = OgRole::create();
• $this->ogAdminRole

•  ->setId($this->randomMachineName())
  

•  ->setLabel($this->randomString())
  

•  ->setGroupType($this->group1->getEntityTypeId())
  

•  ->setGroupBundle($this->groupBundle)
  

•  ->setIsAdmin(TRUE)
  

•  ->save();
  

Currently not, we have

protected function createPerBundleRoles($entity_type_id, $bundle_id) { foreach ($this->getDefaultRoles() as $role_name => $default_properties) { $properties = [ 'group_type' => $entity_type_id, 'group_bundle' => $bundle_id, 'id' => $role_name, 'role_type' => OgRole::getRoleTypeByName($role_name), ];

(no is_admin). I'll create a follow up issue for this.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/amitaibu/og/pull/225/files/e67b43cb7803205e91601fdaafe60e548a43f199#r66656440,
or mute the thread
https://github.com/notifications/unsubscribe/ABAUw_0l8PVfwIeJQK-rTwejZv8ov2Spks5qKadogaJpZM4Ix0vw
.

[pfrenssen]

This looks good to me!

[pfrenssen]

Note that getMembership() is not tested here, but this has been cherry-picked from #217. A test for this method will be added in that PR.

[pfrenssen]

Small remark: I think it's a good idea to rename Og::getUserMembership() to Og::getMembership(). For consistency, can you also rename Og:getUserMemberships() to Og::getMemberships()?

[damiankloip]

@pfrenssen yes, we already discussed that on here but it's on one of the 'out of date diff' comments, see #225 (comment) . Basically renaming that will extend the scope of this further than we need so I think that can be a simple follow up. That keeps things easier to review.

[pfrenssen]

👍

[damiankloip]

I think no conflicts with thw og access service PR. wow! :)

[amitaibu]

needs a re-roll

[damiankloip]

Yeah, this is a nasty re-roll. I can't even be bothered to do this now... It will have to wait.

[amitaibu]

I can give it a try. I can take the shortcut and do a git merge 8.x-1.x. I hate rebasing..

[amitaibu]

@damiankloip since it's on your own repo, I'll create a new PR for this. Is that ok with you?

[damiankloip]

Yes sure. Just base it from this branch and it should be all good?