Permissions are confusing

Committed! thanks :)

http://drupal.org/cvs?commit=462678

+++ skinr_ui.module
@@ -158,7 +158,7 @@ function skinr_ui_menu() {
-    'access arguments' => array('administer skinr'),
+    'access arguments' => array('access skinr'),

It is *very* unusual that a menu path starting with 'admin/' is accessible with a non-administrative user permission. It sounds like this almost invisible change revealed/resolved some other issue. Therefore, it would be a good idea to prepend an inline comment to explain why this path can be accessed without administrative permissions. Otherwise, the next one touching these lines might "fix" the permission.

Powered by Dreditor.

The problem here is that the permissions are named poorly. It's is not a non-administrative permission. This is not the first time the "access skinr" permission has confused people, so we need to fix that.

BEFORE

/**
 * Implements hook_permission().
 */
function skinr_ui_permission() {
  return array(
    'administer skinr' => array(
      'title' => t('Administer Skinr'),
      'description' => t('Administer Skinr\'s settings.'),
    ),
    'access skinr' => array(
      'title' => t('Access Skinr\'s settings'),
      'description' => t('Set Skinr options for individual themes.'),
    ),
    'access skinr classes' => array(
      'title' => t('Access Skinr\'s advanced settings'),
      'description' => t('Set advanced Skinr options, such as custom CSS classes.'),
    ),
  );
}

I was thinking something along these lines would be better...

AFTER

/**
 * Implements hook_permission().
 */
function skinr_ui_permission() {
  return array(
    'administer skinr' => array(
      'title' => t('Administer Skinr'),
    ),
    'edit skin settings' => array(
      'title' => t('Edit skin settings.'),
    ),
    'edit advanced skin settings' => array(
      'title' => t('Edit advanced skin settings'),
      'description' => t('Edit advanced skin settings, such as custom CSS classes.'),
    ),
  );
}

Once we flesh this out, we can work on an actual patch.

Isn't that implied? Looking at core permissions, the only "administer x" permissions that contain a description have warnings that giving the permission has security implications. Skinr doesn't fall in that category, so that's why I didn't add anything. We could add something anyway, but it might be a little much.

I agree that the permissions are confusing and need to be reworded... i also think it couldn't hurt to add a note for some about 'administer skinr' just because some people don't realize that..

Ok, well suggestions for what to write there are welcome. :)

The are no security implications, so we don't need a warning. Whether we want to turn the administer skinr permission into a superglobal-one-eats-all permission is something we can decide freely. It probably makes sense, but I guess some code needs to be updated to actually apply that meaning.

Lastly, please note that 'description' keys in permissions should be avoided, if possible. Instead, the title should be phrased in a descriptive way. Developers can deal with internal and not so descriptive permission names like access skinr or edit skin settings, but on the permissions screen, users should always see permission labels that are crystal-clear.

I like what you suggested above in post #4... makes sense, worded well.

Ok... So, I should take #9 and #10 to mean the suggestion in #4 is ok, or should I remove the description there as well.

Also, as far as I understand, "administer skinr" is already a superglobal-one-eats-all permission.

#4 seems good, though I think the 'edit advanced skin settings' description could be dropped. At the moment are there advanced settings besides CSS classes?

#4 seems good, though I think the 'edit advanced skin settings' description could be dropped. At the moment are there advanced settings besides CSS classes?

No, not ATM since we removed template file support. I'm not ready to drop this yet because eventually I'd like to add template support back there, as well as some other features (like choosing a wrapper HTML5 tag) and people do use this functionality. I'm not totally against removing it, but we should discuss in a separate issue.

This needs is a patch, so setting to active, since there isn't one. This is an easy one... Any takers?

Ok, here's a patch for this.

Tests are failing, because the only existing test we have fails to grant the user permissions. YAY for tests! :)

+++ skinr_ui.module	Fri Dec 17 18:32:25 EST 2010
@@ -158,7 +156,7 @@
-    'access arguments' => array('access skinr'),
+    'access arguments' => array('edit skin settings'),

With this patch, the administer skinr permission is not "global", but we can leave that for a separate follow-up issue. Actually, superuser-permissions are not really easy to implement.

Powered by Dreditor.

Oopsie!! Forgot about the test! :P

This patch certainly applies fine, but I can't say if it did everything its supposed to. I'm trying to use it in conjuction to the class lost issue (http://drupal.org/node/891942)

If you need anything more specifically tested, just say so and I'll try.

Ok, committed: http://drupal.org/cvs?commit=467354