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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bulldozer2003’s picture

Still need the access administration pages permission since other modules can add items to the group tab.

bulldozer2003’s picture

Title: Individual permissions for each item on group tab » og_ui permissions overhaul
Status: Active » Needs review
FileSize
7.73 KB

This 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:

  • Use the administration pages
    Allows user to use the group tab on group entities. Added link back to group entity if the page is empty.
  • Add user
    Allows user to use the group add-user page.
  • Manage members
    Allows user to use the group people page to remove users, alter roles, and modify member status.restrict access = true
  • Add roles
    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
  • Edit permissions
    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
amitaibu’s picture

I like this approach. haven't tested yet, just from a quick review?

+++ b/og_ui/og_ui.moduleundefined
@@ -1038,6 +1060,14 @@ function og_ui_get_group_admin($entity_type, $etid) {
+  if (empty($data)) {
+    $data['empty'] = array(
+      'title' => t('Go back'),
+      'description' => t('Nothing to do here. Contact the group or site administrator if you see this message.'),
+      'href' => NULL,
+    );

Why not 403?

Status: Needs review » Needs work

The last submitted patch, og_ui-permissions_overhaul-1580132-2.patch, failed testing.

bulldozer2003’s picture

Why not 403?

The 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.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
7.76 KB

Ok, 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.

$ diff ~/og_ui-permissions_overhaul-1580132-2.patch ~/og_ui-overhaul_permissions-1580132-6.patch 
2c2
< index 778ab3e..adde60e 100644
---
> index 778ab3e..cf1820f 100644
20c20
< +        if ($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) {
---
> +        if (($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) && user_access('administer group')) {

Status: Needs review » Needs work

The last submitted patch, og_ui-overhaul_permissions-1580132-6.patch, failed testing.

amitaibu’s picture

The 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.

Then I think the access callback for the tab, should do this check, and not show the tab in that case.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Then I think the access callback for the tab, should do this check, and not show the tab in that case.

Done.
Fixed errors found in testing.

amitaibu’s picture

Status: Needs review » Needs work

Looks good.

+++ b/og_ui/og_ui.moduleundefined
@@ -1016,16 +1017,39 @@ function og_ui_og_permission() {
 /**
+ * Return TRUE or FALSE is there are group admin menu items for the user.
+ */
+function og_ui_group_admin_menu($entity_type, $etid) {
+  $data = og_ui_get_group_admin($entity_type, $etid);
+  return !empty($data);

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 :)

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
7.76 KB

We don't really need this, you can make og_ui_get_group_admin() your access callback.

Ok, this works. I didn't think it would since it returns non-boolean.

Let's add a small test, to make sure the tab is indeed hidden if use has no access.

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():

Placed at the end of testOgUiUserPermissionChanges():
    // Check that the group tab will have no content, therefore not displayed.
    $this->drupalLogin($web_user);
    $this->assertFalse(og_ui_get_group_admin('entity_test', $entity1->pid), t('User has no group admin menu items.'));

Setting the return value to a variable does not work either:

    // Check that the group tab will have no content, therefore not displayed.
    $this->drupalLogin($web_user);                                               <--- Function hard coded to check logged in user.
    $data = og_ui_get_group_admin('entity_test', $entity1->pid);
    $this->assert(empty($data), t('User has no group admin menu items.'));        <--- FAILS

    // Add a group admin permission and check for group tab content.
    $this->drupalLogin($admin_user);
    $this->drupalPost('admin/config/group/permissions/entity_test/main', array('1[add user]' => TRUE), t('Save permissions'));
    $this->drupalLogin($web_user);
    $data = og_ui_get_group_admin('entity_test', $entity1->pid);
    $this->assert(!empty($data), t('User has group admin menu items.'));      <--- PASSES, but not likely for the right reasons.

Status: Needs review » Needs work

The last submitted patch, og_ui-permissions_overhaul-1580132-11.patch, failed testing.

bulldozer2003’s picture

Status: Needs work » Needs review

Having trouble with this.

amitaibu’s picture

> Having trouble with this.

Where are you stuck?

Moo64c’s picture

Fixed tests.

amitaibu’s picture

Status: Needs review » Needs work
+++ b/og_ui/og_ui.admin.incundefined
@@ -561,6 +561,11 @@ function og_ui_admin_permissions($form, $form_state, $group_type = '', $gid = 0,
+        // If the user does not have administer group, don't populate restricted
+        // permissions.
+        if (($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) && !user_access('administer group')) {
+          continue;

This doesn't seem to be related to this patch.

+++ b/og_ui/og_ui.moduleundefined
@@ -81,6 +81,7 @@ function og_ui_menu() {
+    // Require administer group permission to edit and delete roles.
     'access arguments' => array('administer group', 1, 2, 5),

Why? Seems to me this belongs under "manage roles".

+++ b/og_ui/og_ui.moduleundefined
@@ -1018,10 +1019,25 @@ function og_ui_og_permission() {
+    'title' => t('Edit permissions'),

Lets call it "Manage permissions"

+++ b/og_ui/og_ui.moduleundefined
@@ -1031,7 +1047,9 @@ function og_ui_og_permission() {
+  global $user;
+
+  $data = module_invoke_all('og_ui_get_group_admin', $entity_type, $etid, $user);

This is wrong. If we need the $data we should call og_ui_get_group_admin() that also takes care of calling drupal_alter().

+++ b/og_ui/og_ui.testundefined
@@ -58,6 +58,59 @@ class OgUiUserPermissionsTestCase extends DrupalWebTestCase {
+      og_role_revoke_permissions($auth_rid, array($perm));

Please add comment why we revoke the permission.

Moo64c’s picture

Cleaned up code.

amitaibu’s picture

Status: Needs work » Needs review

You need to set correct status, otherwise simpletest won't be triggered

amitaibu’s picture

Committed, thanks.

bulldozer2003’s picture

+++ b/og_ui/og_ui.admin.incundefined
@@ -561,6 +561,11 @@ function og_ui_admin_permissions($form, $form_state, $group_type = '', $gid = 0,
+        // If the user does not have administer group, don't populate restricted
+        // permissions.
+        if (($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) && !user_access('administer group')) {
+          continue;

This doesn't seem to be related to this patch.

@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.

bulldozer2003’s picture

@Moo64c - Thank you

amitaibu’s picture

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.

Ok, 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.

bulldozer2003’s picture

Ok, 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.

amitaibu’s picture

Status: Needs review » Needs work

In an unrelated note, I did some security testing with the firefox tamper data add-on.

Interesting, I'd like to hear more about this tool and how you operate it. Sounds like a great blog post :)

+++ b/og_ui/og_ui.admin.incundefined
@@ -561,6 +561,12 @@ function og_ui_admin_permissions($form, $form_state, $group_type = '', $gid = 0,
+        if (($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) && !user_access('administer group')) {

og_user_access() is already doing the user_access('administer group'); check

+++ b/og_ui/og_ui.testundefined
@@ -107,6 +107,12 @@ class OgUiAdminPermissionsTestCase extends DrupalWebTestCase {
+      // Check that restricted permissions are not displayed to the user with
+      // manage permissions but not administer group.
+      if ($perm == 'manage permissions') {
+        $this->drupalGet('group/node/' . $node->nid . '/admin/permissions');

Let'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.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
2.73 KB

Tests moved out into their own function, checking appropriate display for both privileged and unprivileged users.

Sounds like a great blog post :)

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.

bulldozer2003’s picture

og_user_access() is already doing the user_access('administer group');

I 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.

amitaibu’s picture

Status: Needs review » Needs work

Minor issues:

+++ b/og_ui/og_ui.admin.incundefined
@@ -561,6 +561,12 @@ function og_ui_admin_permissions($form, $form_state, $group_type = '', $gid = 0,
+        if (($perm_item['restrict access'] && !og_user_access($group_type, $gid, 'administer group')) && !user_access('administer group')) {

Yes, please remove the user_access()

+++ b/og_ui/og_ui.testundefined
@@ -112,6 +112,34 @@ class OgUiAdminPermissionsTestCase extends DrupalWebTestCase {
+  function testOgUiAdminPermissionsAccess() {

Add PHPdoc to explain in short the test.

+++ b/og_ui/og_ui.testundefined
@@ -112,6 +112,34 @@ class OgUiAdminPermissionsTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Warning: Give to trusted roles only; this permission has security implications in the group context.'), t('Restricted permissions are not displayed to the unprivileged user.'));

Let's put the t('Warning... in a variable, as we use it twice.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
2.7 KB

Done

amitaibu’s picture

Status: Needs review » Fixed

Great, thanks. committed.

drupalycious’s picture

These new permission options are great, but there is a little bug that I am reporting here: #1664780: Missing options in Global OG default permissions

bulldozer2003’s picture

Thats 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.