Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I'm going to submit a patch that will allow permissions to be set for each of the four options on the group tab page.
Once this is done, the 'access administration pages' permission should probably be removed and check if a user has one of the individual permissions in order to display the group tab.
Comments
Comment #1
bulldozer2003Still need the access administration pages permission since other modules can add items to the group tab.
Comment #2
bulldozer2003This patch overhauls the og_ui permissions interface. Prior, most OG administrative actions required the administer group permission. Now, administer group is only needed to perform the most dangerous actions.
The following permissions are affected and can be given individually without the administer group permission:
Allows user to use the group tab on group entities. Added link back to group entity if the page is empty.
Allows user to use the group add-user page.
Allows user to use the group people page to remove users, alter roles, and modify member status.restrict access = true
Allows user to view the group roles page and add roles but they cannot edit existing roles without the administer group permission. restrict access = true
Allows user to view the permissions page and edit permissions if default roles are overridden. If user does not have administer group permission, permissions with restrict access = true are hidden.restrict access = true
Comment #3
amitaibuI like this approach. haven't tested yet, just from a quick review?
Why not 403?
Comment #5
bulldozer2003The group tab will display if the user has 'access administration pages', but will be empty if they don't have any of the permissions for the individual items. The data['empty'] simply renders an item onto that page if there are no other items. An empty page would probably confuse users who might think there is a site problem. The message to contact an admin is not so much to report an error, but so the admin can figure out why the user has 'access administration pages' but not any of the individual permissions.
Comment #6
bulldozer2003Ok, failed test was due to what could be called a bug in og_user_access. For the permissions overview pages, group_type and gid are not set, calling og_user_access will return FALSE when not sent a group_type and gid even if the user has the global drupal 'administer group' permission. I added a check to see if the user has user_access('administer group') in the code.
Comment #8
amitaibuThen I think the access callback for the tab, should do this check, and not show the tab in that case.
Comment #9
bulldozer2003Done.
Fixed errors found in testing.
Comment #10
amitaibuLooks good.
We don't really need this, you can make og_ui_get_group_admin() your access callback.
Let's add a small test, to make sure the tab is indeed hidden if use has no access. With tests we know we got it right :)
Comment #11
bulldozer2003Ok, this works. I didn't think it would since it returns non-boolean.
Having trouble with this. Thinking that the above worked, all I should need to do is check the return value of og_ui_get_group_admin():
Setting the return value to a variable does not work either:
Comment #13
bulldozer2003Comment #14
amitaibu> Having trouble with this.
Where are you stuck?
Comment #15
Moo64c CreditAttribution: Moo64c commentedFixed tests.
Comment #16
amitaibuThis doesn't seem to be related to this patch.
Why? Seems to me this belongs under "manage roles".
Lets call it "Manage permissions"
This is wrong. If we need the $data we should call og_ui_get_group_admin() that also takes care of calling drupal_alter().
Please add comment why we revoke the permission.
Comment #17
Moo64c CreditAttribution: Moo64c commentedCleaned up code.
Comment #18
amitaibuYou need to set correct status, otherwise simpletest won't be triggered
Comment #19
amitaibuCommitted, thanks.
Comment #20
bulldozer2003@amitaibu
I was attempting to reconcile the security implication that a user given "manage permissions" can give themselves the "administer group" permission, or any of the other privileged permissions. This code achieves that by not populating restrict access permissions on the edit permissions page.
With the above caveat, you can safely allow a user to manage permissions without worrying about them escalating their own privileges.
Comment #21
bulldozer2003@Moo64c - Thank you
Comment #22
amitaibuOk, I understand it. Please attach a follow up patch, with a complimenting test. Also I think the comment should be improved to reflect the above explanation.
Comment #23
bulldozer2003Ok, here is a patch with a better comment and an associated test.
In an unrelated note, I did some security testing with the firefox tamper data add-on. I wanted to make sure that a user with manage permissions, but not administer group, could not forge the submission and add one of the restricted permissions. I got a nifty error message courtesy of the _form_validate() function that detects and prevents that kind of input forgery.
Comment #24
amitaibuInteresting, I'd like to hear more about this tool and how you operate it. Sounds like a great blog post :)
og_user_access()
is already doing theuser_access('administer group');
checkLet's complete the test by also asserting that the text exists for a privileged user. Also let's move this test into its own test under
OgUiAdminPermissionsTestCase::testAdminPermissionsAccess
.Comment #25
bulldozer2003Tests moved out into their own function, checking appropriate display for both privileged and unprivileged users.
I should start one, or buy a domain and put drupal on it. In the interim there are some youtube videos that show script kiddies how to use tamper data to increase their score in flash games to give a general idea of how to use it.
Comment #26
bulldozer2003I forgot to remove the redundant user_access('administer group'), I can submit another patch if necessary. I remember there was a specific reason I made that seemingly redundant, but I cannot think of why now.
Comment #27
amitaibuMinor issues:
Yes, please remove the user_access()
Add PHPdoc to explain in short the test.
Let's put the t('Warning... in a variable, as we use it twice.
Comment #28
bulldozer2003Done
Comment #29
amitaibuGreat, thanks. committed.
Comment #30
drupalycious CreditAttribution: drupalycious commentedThese new permission options are great, but there is a little bug that I am reporting here: #1664780: Missing options in Global OG default permissions
Comment #31
bulldozer2003Thats why I had the redundant user_access('administer group'), because og_user_access() exits if there is no group object!
Using $gid, as amitaibu did, might be a better choice though.