Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utilize Role isAdmin() functionality #225

Closed
wants to merge 6 commits into from
Closed

Utilize Role isAdmin() functionality #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e388600
Utilize Role isAdmin() functionality
damiankloip Jun 9, 2016
197cb0e
Fix OgAccessEntityTest
damiankloip Jun 9, 2016
cbb0ee9
Remove default assignment for permissions cache
damiankloip Jun 9, 2016
0b2c693
Add an Og::getUserMembership() method
Jun 9, 2016
6f6b6fd
Fix OgAccess::userAccess caching all group membership permissions for…
damiankloip Jun 9, 2016
e67b43c
Break early if an is_admin flag is found in roles, rename getUserMemb…
damiankloip Jun 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/schema/og.schema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ og.og_role.*:
group_bundle:
type: string
label: 'Group bundle'
is_admin:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damiankloip This is conceptually changing the logic from:

This is a role that might, or might not have the administer group permission. It's just another role like all the others

into:

This is an admin role. The permissions it has under it are not so important, because the role itself is marked as admin.

I think I prefer the existing solution, because it's taking a similar approach to drupal core, where a role is simply a collection of permissions with some metadata (i.e. the role name).

With this approach we'll now have to check for permission both on the Role level, and on the permissions level.

Thoughts?

Copy link
Collaborator Author

@damiankloip damiankloip Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amitaibu Yes, that is what I'm shifting here.

With this approach we'll now have to check for permission both on the Role level, and on the permissions level.

Hmm, wouldn't we need to do that anyway? As the permissions generated for OG could E.g. apply to one group, and therefore roles have to be check per group like that? The permissions for OG are kind of sub permissions I thought (OgRole !== global administer groups permission)? So this utilises all the isAdmin() stuff that is already a part of core Roles. So Core is already doing this with it's roles. I.e. if it's marked as a full administrator role, you can do anything. That same assumption applies on the OG level. E.g. a user membership has a role attached that is an admin role (is_admin = true) and they can then be a group admin. So all checks in OgAccess with check that anyway. E.g Role::hasPermission() (used by OgRole) will also check if the role isAdmin() first.

See OgAccess::userAccess - the global admin perm is checked, then if that is not true, we get user memberships and check those.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. see user.role.administrator.yml config in a D8 install.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also adopt fromthing like core's RoleStorage, a isPermissionInRoles method. That would check each role for x permission or admin.

Copy link
Collaborator Author

@damiankloip damiankloip Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amitaibu OH, so OgAccess::userAccess collects all permissions, which would be specific to a node bundle for example? I thought users can just have an admin role for e.g. 1 group and not another?

Basically, Og::userAccess does:

      foreach (Og::getUserMemberships($user) as $membership) {
        foreach ($membership->getRoles() as $role) {
        }
     }

but it caches them per group, per user. This doesn't seem correct. Should we only be checking the roles/permissions for the current group membership only?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, just to make sure we are on the same page, in OG7:

  1. For every group or group type you have your own roles.
  2. Roles are just a collection of permissions, the same way roles are in D7
  3. There is an administer groups site-wide permission, which allows a user to administer all groups
  4. There is an administer group OG permission, which allows a user to administer a specific group they might have the role in.

So it seems that in D8, Role has evolved a bit and now holds a boolean is_admin. As OG is following core's logic this means that in OG8 we should remove the administer group OG permission in favor of the is_admin flag.

Also just to clarify in general how we deal with the permissions:

  1. an OgRole is now being referenced from an OgMembership. So it means a user can have a member role in Group A, and a administer role in Group B.
  2. If a group content (e.g. Article) belongs to both groups, and the user wants to edit it, we iterate over those two groups and check for access based on that group. Enough to get one allow to grant access.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and it's awesome having you back :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amitaibu ! And yes, that is pretty much how I interpreted the use of roles and permissions. Which is useful :)

type: boolean
label: 'User is group admin'
permissions:
type: sequence
label: 'Permissions'
Expand Down
1 change: 1 addition & 0 deletions src/Entity/OgRole.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
* "id",
* "label",
* "weight",
* "is_admin",
* "group_type",
* "group_bundle",
* "group_id",
Expand Down
24 changes: 24 additions & 0 deletions src/Og.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,30 @@ public static function getUserMemberships(AccountInterface $user, array $states
return static::$cache[$identifier];
}

/**
* Returns the group membership for a given user and group.
*
* @param \Drupal\Core\Session\AccountInterface $user
* The user to get the membership for.
* @param \Drupal\Core\Entity\EntityInterface $group
* The group to get the membership for.
* @param array $states
* (optional) Array with the state to return. Defaults to active.
* @param string $field_name
* (optional) The field name associated with the group.
*
* @return \Drupal\og\Entity\OgMembership|NULL
* The OgMembership entity, or NULL if the user is not a member of the
* group.
*/
public static function getMembership(AccountInterface $user, EntityInterface $group, array $states = [OgMembershipInterface::STATE_ACTIVE], $field_name = NULL) {
foreach (static::getUserMemberships($user, $states, $field_name) as $membership) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a user might have many memberships, so I think better to query for the specific membership we want instead of iterating over all of them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, now that OgMembership is associated only with users maybe we should change the name to just getMembership?

Copy link
Collaborator Author

@damiankloip damiankloip Jun 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me! We can have a follow up to rename getUserMemberships.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amitaibu I think we should leave this method as-is for now, as @pfrenssen wrote it. I think we can optimise it later. This keeps it simple, and keeps it statically cached too. If we load single stuff here we might want to look at how some stuff could be shared between getting all memberships and a single one. Maybe a lot of requests would need to get all memberships anyway?

if ($membership->getGroupEntityType() === $group->getEntityTypeId() && $membership->getEntityId() === $group->id()) {
return $membership;
}
}
}

/**
* Returns all group IDs associated with the given group content entity.
*
Expand Down
28 changes: 19 additions & 9 deletions src/OgAccess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class OgAccess {
*/
protected static $permissionsCache = [];


/**
* Administer permission string.
*
Expand Down Expand Up @@ -112,18 +111,25 @@ public static function userAccess(EntityInterface $group, $operation, AccountInt
// To reduce the number of SQL queries, we cache the user's permissions
// in a static variable.
if (!$pre_alter_cache) {
$permissions = array();

foreach (Og::getUserMemberships($user) as $membership) {
$permissions = [];
$user_is_group_admin = FALSE;

if ($membership = Og::getMembership($user, $group)) {
foreach ($membership->getRoles() as $role) {
// Check for the is_admin flag.
if ($role->isAdmin()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if a role isAdmin, we don't need to continue iterating. We can just add all the permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I think if the user is_admin, permissions don't really matter.

$user_is_group_admin = TRUE;
break;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/** @var $role RoleInterface */
$permissions = array_unique(array_merge($permissions, $role->getPermissions()));
$permissions = array_merge($permissions, $role->getPermissions());
}
}

static::setPermissionCache($group, $user, TRUE, $permissions, $cacheable_metadata);
$permissions = array_unique($permissions);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amitaibu a single array_unique is kept here. Then we are just doing it once, instead of once per role.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, missed that :)


static::setPermissionCache($group, $user, TRUE, $permissions, $user_is_group_admin, $cacheable_metadata);
}

if (!$skip_alter && !in_array($operation, $post_alter_cache)) {
Expand All @@ -138,12 +144,13 @@ public static function userAccess(EntityInterface $group, $operation, AccountInt
);
\Drupal::moduleHandler()->alter('og_user_access', $alterable_permissions['permissions'], $cacheable_metadata, $context);

static::setPermissionCache($group, $user, FALSE, $alterable_permissions['permissions'], $cacheable_metadata);
static::setPermissionCache($group, $user, FALSE, $alterable_permissions['permissions'], $alterable_permissions['is_admin'], $cacheable_metadata);
}

$altered_permissions = static::getPermissionsCache($group, $user, FALSE);

$user_is_group_admin = !empty($altered_permissions['permissions'][static::ADMINISTER_GROUP_PERMISSION]);
$user_is_group_admin = !empty($altered_permissions['is_admin']);

if (($user_is_group_admin && !$ignore_admin) || in_array($operation, $altered_permissions['permissions'])) {
// User is a group admin, and we do not ignore this special permission
// that grants access to all the group permissions.
Expand Down Expand Up @@ -233,16 +240,19 @@ public static function userAccessEntity($operation, EntityInterface $entity, Acc
* Determines if the type of permissions is pre-alter or post-alter.
* @param array $permissions
* Array of permissions to set.
* @param bool @is_admin
* Whether or not the user is a group administrator.
* @param \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheable_metadata
* A cacheable metadata object.
*/
protected static function setPermissionCache(EntityInterface $group, AccountInterface $user, $pre_alter, array $permissions, RefinableCacheableDependencyInterface $cacheable_metadata) {
protected static function setPermissionCache(EntityInterface $group, AccountInterface $user, $pre_alter, array $permissions, $is_admin, RefinableCacheableDependencyInterface $cacheable_metadata) {
$entity_type_id = $group->getEntityTypeId();
$group_id = $group->id();
$user_id = $user->id();
$type = $pre_alter ? 'pre_alter' : 'post_alter';

static::$permissionsCache[$entity_type_id][$group_id][$user_id][$type] = [
'is_admin' => $is_admin,
'permissions' => $permissions,
'cacheable_metadata' => $cacheable_metadata,
];
Expand Down
83 changes: 81 additions & 2 deletions tests/src/Kernel/Access/OgEntityAccessTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,20 @@ class OgEntityAccessTest extends KernelTestBase {
*/
protected $user3;

/**
* @var \Drupal\user\Entity\User
*/
protected $adminUser;

/**
* @var \Drupal\entity_test\Entity\EntityTest
*/
protected $group1;

/**
* @var \Drupal\entity_test\Entity\EntityTest
*/
protected $group2;

/**
* The machine name of the group's bundle.
Expand All @@ -52,21 +60,34 @@ class OgEntityAccessTest extends KernelTestBase {
*/
protected $groupBundle;


/**
* The OG role that has the permission we check for.
*
* @var OgRole
*/
protected $ogRoleWithPermission;

/**
* The OG role that has the permission we check for.
*
* @var OgRole
*/
protected $ogRoleWithPermission2;

/**
* The OG role that doesn't have the permission we check for.
*
* @var OgRole
*/
protected $ogRoleWithoutPermission;

/**
* The OG role that doesn't have the permission we check for.
*
* @var OgRole
*/
protected $ogAdminRole;

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -100,6 +121,10 @@ protected function setUp() {
$this->user3 = User::create(['name' => $this->randomString()]);
$this->user3->save();

// Admin user.
$this->adminUser = User::create(['name' => $this->randomString()]);
$this->adminUser->save();


// Define the group content as group.
Og::groupManager()->addGroup('entity_test', $this->groupBundle);
Expand All @@ -112,6 +137,15 @@ protected function setUp() {
]);
$this->group1->save();

// Create another group to help test per group/per account permission
// caching.
$this->group2 = EntityTest::create([
'type' => $this->groupBundle,
'name' => $this->randomString(),
'user_id' => $group_owner->id(),
]);
$this->group2->save();

/** @var OgRole ogRoleWithPermission */
$this->ogRoleWithPermission = OgRole::create();
$this->ogRoleWithPermission
Expand All @@ -123,6 +157,16 @@ protected function setUp() {
->grantPermission('some_perm')
->save();

$this->ogRoleWithPermission2 = OgRole::create();
$this->ogRoleWithPermission2
->setId($this->randomMachineName())
->setLabel($this->randomString())
->setGroupType($this->group1->getEntityTypeId())
->setGroupBundle($this->groupBundle)
// Associate an arbitrary permission with the role.
->grantPermission('some_perm_2')
->save();

/** @var OgRole ogRoleWithoutPermission */
$this->ogRoleWithoutPermission = OgRole::create();
$this->ogRoleWithoutPermission
Expand All @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the DX of this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I see. Cool!
On 10 Jun 2016 19:05, "Amitai Burstein" [email protected] 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
.



/** @var OgMembership $membership */
$membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]);
Expand All @@ -143,6 +196,16 @@ protected function setUp() {
->addRole($this->ogRoleWithPermission->id())
->save();

// Also create a membership to the other group. From this we can verify that
// permissions are not bled between groups.
$membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]);
$membership
->setUser($this->user1->id())
->setEntityId($this->group2->id())
->setGroupEntityType($this->group2->getEntityTypeId())
->addRole($this->ogRoleWithPermission2->id())
->save();

/** @var OgMembership $membership */
$membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]);
$membership
Expand All @@ -151,6 +214,15 @@ protected function setUp() {
->setGroupEntityType($this->group1->getEntityTypeId())
->addRole($this->ogRoleWithoutPermission->id())
->save();

/** @var OgMembership $membership */
$membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]);
$membership
->setUser($this->adminUser->id())
->setEntityId($this->group1->id())
->setGroupEntityType($this->group1->getEntityTypeId())
->addRole($this->ogAdminRole->id())
->save();
}

/**
Expand All @@ -159,13 +231,20 @@ protected function setUp() {
public function testAccess() {
// A member user.
$this->assertTrue(OgAccess::userAccess($this->group1, 'some_perm', $this->user1)->isAllowed());
// This user should not have access to 'some_perm_2' as that was only
// assigned to group 2.
$this->assertTrue(OgAccess::userAccess($this->group1, 'some_perm_2', $this->user1)->isForbidden());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, very important to test this!


// A member user without the correct role.
$this->assertTrue(OgAccess::userAccess($this->group1, 'some_perm', $this->user2)->isForbidden());

// A non-member user.
$this->assertTrue(OgAccess::userAccess($this->group1, 'some_perm', $this->user3)->isForbidden());

// Group admin user should have access regardless.
$this->assertTrue(OgAccess::userAccess($this->group1, 'some_perm', $this->adminUser)->isAllowed());
$this->assertTrue(OgAccess::userAccess($this->group1, $this->randomMachineName(), $this->adminUser)->isAllowed());

// Add membership to user 3.
$membership = OgMembership::create(['type' => OgMembershipInterface::TYPE_DEFAULT]);
$membership
Expand All @@ -179,4 +258,4 @@ public function testAccess() {
}


}
}
3 changes: 1 addition & 2 deletions tests/src/Unit/OgAccessTestBase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,9 @@ public function setUp() {
$reflection_property = $r->getProperty('permissionsCache');
$reflection_property->setAccessible(TRUE);


$values = [];
foreach (['pre_alter', 'post_alter'] as $key) {
$values[$group_type_id][$this->group->id()][2][$key] = ['permissions' => ['update group']];
$values[$group_type_id][$this->group->id()][2][$key] = ['permissions' => ['update group'], 'is_admin' => FALSE];
}

$reflection_property->setValue($values);
Expand Down