The patch creates two new permissions in core: administer themes and administer modules. They are fairly self explanatory. Users with the 'administer themes' permission can get to admin/build/themes/* and users with the 'administer modules' permission can get to admin/build/modules/*

This patch also changes the method of display for the administration pages.

If a user has permission to, for example, administer modules, that user should be able to get to the administration page. Menu items for system.module that previously checked for 'access administration pages' now check for any of the administrative permissions to be enabled (see line 802 after applying the patch to see what permissions are checked)

CommentFileSizeAuthor
#31 446666_31.patch10.58 KBcweagans
#28 446666_28.patch10.59 KBcweagans
#27 446666_27.patch8.17 KBcweagans
#26 446666_26.patch8.1 KBcweagans
#24 446666_24.patch7.96 KBcweagans
#14 permission_fixes_3.patch8.04 KBcweagans
#11 permission_fixes_2.patch6.31 KBcweagans
#7 permissions_fixes.patch3.35 KBcweagans
permissions_fixes.patch6.42 KBcweagans

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Anonymous (not verified) commented

I like this idea. You'll need to update the tests to get them to pass.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

Update the tests? How do I do that?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
-    'access arguments' => array('access administration pages'),
+    'access callback' => '_system_admin_pages_access',

"access administration pages" you have to leave as is. It's a completely different permission than "administer site configuration", for good reason.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

Well sure, but it's kind of a usability thing. Why should an admin have to assign two separate permissions to someone if they want to allow that user to administer the modules on the site. What is the reasoning to have two permissions?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

"access administration pages" grants access to admin/. There is more than administering modules and themes below admin/ - starting with the administration dashboard directly on the path "admin". Followed by administrative help pages, site reports, and arbitrary other stuff you may add via contributed or custom modules. Basically, you could say that it grants overall access to Drupal's back-end, while the back-end is whatever you want to have as back-end.

This permission exists on purpose, has nothing to do with the scope of this patch, and therefore you have to leave it alone.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs work » Needs review
StatusFileSize
new permissions_fixes.patch3.35 KB

Alright, I removed that part of the patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

joachim’s picture

joachim commented

To my surprise today I found that 'administer site configuration' allows a user to use the site while in maintenance mode, something I thought was only for UID 1. So that's something else to take into account if rethinking this permission.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@joachim: #201415: Add permission to access site in maintenance mode

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
StatusFileSize
new permission_fixes_2.patch6.31 KB

It took a while, but I finally got around to fixing the tests. On my machine, all tests pass, so let's hope it stays that way on the testing slaves =D

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs work » Needs review

Woops....needs review >.<

Status: Needs review » Needs work

The last submitted patch failed testing.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs work » Needs review
StatusFileSize
new permission_fixes_3.patch8.04 KB

One more...

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

Conversation with cwgordon7 in #drupal

cwgordon7: "Administer modules" is basically a permission that would allow a user to compromise a site. Why, then, should that be separate from "administer site configuration"?

cweagans: administer site configuration is a HUGE permission. There's so much stuff to it....it's ridiculous, really...I've got sites where I would want a user to be able to administer modules and themes, but not the rest of the stuff that administer site configuration opens up.

cwgordon7: cweagans: But why would anyone ever want to be enabling/disabling modules and not be able to configure them?

cwgordon7: I guess my complaint is mainly from a security standpoint - Drupal has had some negative attention due to there being more than one permission that can lead to complete administrative access of the site

cwgordon7: I'm reluctant at best to adding yet another site-compromising permission that people can easily mess up and assign to users without fully realizing the implications

cwgordon7: cweagans: Though I must say, I admire your efforts in the struggle against the test bot even if I'm not fully convinced by the patch yet

--cwgordon7 went offline at this point. Druplicon should have passed this message for me:

cweagans: a lot of modules have their own permission for allowing a user to configure that particular module (eg. the block module: administer blocks). The reason that I see for it is a case in which a Drupal installation is mostly managed by somebody that knows what they are doing, but wants to allow certain users to, for example, enable the blog module. Or the forum module. A case in which an admin can offload some of the responsibility of building the site to other capable people and not allow them to, for example, put the site into maintenance mode. (haha, you went offline just as I posted this)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

First of all, with "administer permissions" there are already multiple permissions, that let you compromise the site.
That having been said, I don't think it makes sense to prevent people of delegating administrative tasks to trusted users only because people would start to use it unwisely. Doing that is not fixing the problem, and keeping Drupal less flexible at that.

The solution to people potentially handing out permissions without caution is educating them properly about what they are doing.
We already have security warnings in the description of the permissions, and I like that trend, but clearly this isn't enough. Maybe we should color the affected rows red and in the descriptions link to a help page which describes in which way you are shooting yourself in the foot if you proceed or something.

Anyways, that is for another issue, and from a conceptual POV it definitely makes sense to split the "administer site configuration" permission.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

I couldn't agree with you more tstoeckler.

Also, splitting administer site configuration into 'administer site configuration', 'administer themes', and 'administer modules' is based on a conversation that I had with webchick in IRC. Initially, I thought that it would be a good idea to basically merge http://drupal.org/project/config_perms into core. webchick thought that this was way too granular for most people, hence the current patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs work » Needs review

Huh, the patch was fine before. Retest.

matt2000’s picture

matt2000 commented

+1

Status: Needs review » Needs work

The last submitted patch failed testing.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Assigned: cweagans » Unassigned
Category: feature » task
Issue tags: +#d7ux

It's not looking like I'm going to have time to update this patch to HEAD. If someone wants to take this on, that would be freaking awesome! 'administer site configuration' is a HUGE permission. Tagging with d7ux...this is very confusing for a new user ("Erm...that permission enables WHAT!?")

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 446666_24.patch7.96 KB

Updated to head. Hope the tests pass..

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Critical

Thanks! Looks great!

+++ modules/system/system.module	1 Dec 2009 20:22:36 -0000
@@ -207,6 +207,12 @@ function system_permission() {
+    'administer themes' => array(
+      'title' => t('Administer site themes'),
+    ),
+    'administer modules' => array(
+      'title' => t('Administer site modules'),
+    ),

1) Can we remove "site" from the title, please?

2) What about descriptions?

I'm on crack. Are you, too?

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
StatusFileSize
new 446666_26.patch8.1 KB

Fixed both issues. A sidenote: the other system permissions don't have descriptions. They will need to be added in another issue.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
StatusFileSize
new 446666_27.patch8.17 KB

Ok, last one, I think.

tha_sun: The general rule is to order permissions by administer, access, view, edit, delete
tha_sun: and within those groups, we currently order them alphabetically

I moved the administer modules permission above administer site configuration

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
StatusFileSize
new 446666_28.patch10.59 KB

Last one for real. Removed the descriptions for the new permissions...they are fairly self explanatory. #620446: Rewrite permission titles and descriptions is the issue that removed most of the descriptions.

Also, I think I fixed the tests.

matt2000’s picture

matt2000 commented 
+++ modules/color/color.test	1 Dec 2009 20:56:10 -0000
@@ -23,7 +23,7 @@ class ColorTestCase extends DrupalWebTes
   function setUp() {
     parent::setUp('color');
     // Create users.
-    $this->big_user = $this->drupalCreateUser(array('administer site configuration'));
+    $this->big_user = $this->drupalCreateUser(array('administer site configuration', 'administer themes'));
   }

Why do color tests still require administer site config?

This review is powered by Dreditor.

yoroy’s picture

yoroy commented

I like this patch. The offloading of responsibilities that it will allow is a good thing to have, I can see the more complex site workflows benefit from this added granularity. That this is about really high-power permissions is not necessarily something that should stop us from doing it. To the contrary, anything that keeps you from having to grant the really big 'administer site configuration' permission is a good thing. Anyway, +1 from UX perspective as well

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
StatusFileSize
new 446666_31.patch10.58 KB

oh, you're right matt2000

matt2000’s picture

matt2000 commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Indeed!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -permissions, -DrupalWTF, -#d7ux

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

Issue tags: +permissions, +DrupalWTF, +#d7ux

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.