Skip to content

Fix permissions for non-administrators #214

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

Merged
amitaibu merged 4 commits into from
Jun 8, 2016
Merged

Fix permissions for non-administrators #214

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c30346b
Fix permissions for non-administrators.
pfrenssen May 27, 2016
a2c2dda
Also allow access on viewing for group administrators.
pfrenssen May 27, 2016
46ea003
Only care about entities that are either groups or group content.
pfrenssen May 27, 2016
863090b
Restore neutral access for view operations.
pfrenssen May 27, 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
20 changes: 8 additions & 12 deletions og.module
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ function og_entity_field_access($operation, FieldDefinitionInterface $field_defi
* Implements hook_entity_access().
*/
function og_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
// We only care about content entities.
// We only care about content entities that are groups or group content.
if (!$entity instanceof ContentEntityInterface) {
return AccessResult::neutral();
}
Expand All @@ -116,21 +116,17 @@ function og_entity_access(EntityInterface $entity, $operation, AccountInterface
$entity_type_id = $entity->getEntityTypeId();
$bundle_id = $entity->bundle();

$access = OgAccess::userAccessEntity('administer group', $entity, $account);

if ($access->isNeutral()) {
// The node isn't in an OG context, so no need to keep testing.
return $access;
}
else {
// Any and own content.
$access = $access->orIf(OgAccess::userAccessEntity($operation, $entity, $account));
if (!Og::isGroup($entity_type_id, $bundle_id) && !Og::isGroupContent($entity_type_id, $bundle_id)) {
return AccessResult::neutral();
}

if (!$access->isAllowed() && ($operation === 'update') && Og::isGroup($entity_type_id, $bundle_id)) {
$access = OgAccess::userAccessEntity($operation, $entity, $account);
// If the user has permission to administer all groups, allow access.
if ($account->hasPermission('administer group')) {
return AccessResult::allowed();
}

$access = OgAccess::userAccessEntity($operation, $entity, $account);

@amitaibu amitaibu Jun 7, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sure, I'm ok with keeping this PR with a small scope (and we can have tests as follow up), but I think this part is still wrong. For example, if the bundle == 'article' and operation == 'update', it should be converted to something like:

$access = OgAccess::userAccessEntity('update own article content', $entity, $account) && $entity>getUserId() == $account->uid || OgAccess::userAccessEntity('update any article content', $entity, $account);

@pfrenssen pfrenssen Jun 7, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah! I'm actually tackling that in #217. In this issue this is not touched, this is exactly the same code as it was before.

#217 is depending on #196 so it is unreviewable at the moment, but the commit that will interest you is this one: 6aca08c

@amitaibu amitaibu Jun 7, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

@amitaibu amitaibu Jun 7, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

in that case, it's ready for merge, right?

@pfrenssen pfrenssen Jun 8, 2016

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes this should be good to go. I'll make a followup issue to provide test coverage.


if ($access->isAllowed()) {
return $access;
}
Expand Down