Add a third default role to core for handling Administrative duties

Marking as duplicate of http://drupal.org/node/64861

I agree, there are more than enough good reasons for this! Too late for D6 maybe, but this should not be neglected in D7.

Benjamin stated several times that this is a trivial thing to implement. I disagree.

Adding another default role to core must presume to change every site owner's definition of rid3. What is that rid right now, and what's it being used for? To presume that is doesn't exist would be folly. To assume that you can take ownership of it for your own need would be tragix, to say the least.

But, it can, and should be done. Here's how. Leave rid 1 and rid2 alone. We've all written so much code that assumes rid 1 & 2 are anon and auth users, respectively, that nobody wants to go back in time and change all that logic.

Instead, we'll add an rid3 called 'administrator'. If rid3 already exists, we move it to ridX+1 and store it there. Care most be taken to comb the popular contrib modules that deal with roles and rid's to make sure nothing gets missed before this is committed to core.

The idea of a third default role within core is not new, and I remember it coming up during my Drupalcon Boston talk about "Making Modules More Admin-Friendly". I actually even proposed a hook_configuration() system to help out the process of installing, enabling and configurating modules.

I'd be willing to write a D7 patch for this, but before I do, I need to make sure that we cannot achieve Meta Roles for core, cause that would be so much sweeter and better than just making a third role. Only when the Meta Roles proposal is won't-fixed will I begin working on this particular issue's patch. Sound good? We don't need any duplication of efforts.

This issue is closely linked to #248598: Label permissions which are warned about in the user interface. Because the other issue has a broader scope and aims to build security foundations that could be use to build a default "administrator role", I'm tempted to mark this one as duplicate, or at least to postpone it.

Ok, I'll bite. Let's postpone this thread until we need to revisit it again.

How about setting anonymous user to 0, administrator to 1, and authenticated user to 2. This would map with the same thing we do for user IDs for those things, and not screw with any of the existing role IDs. There are defined constants DRUPAL_AUTHENTICATED_RID and DRUPAL_ANONYMOUS_RID. If any code is not using them, file a (critical) bug. No reason to postpone this issue, imo, unless I'm completely on crack (which is always possible).

Also I strongly disagree that this issue should have any bearing whatsoever on #248598: Label permissions which are warned about in the user interface. These two issues are addressing two completely different things.

I'll roll a patch for the suggestion in #13 right away. I like that idea, a lot.

I just came on over here from the other issue (#248598: Label permissions which are warned about in the user interface). I agree that these are definitely not duplicates, but I think Damien is right that they are related. They are kind of opposite sides of the same coin:

• Here the goal is to let modules require, or at least recommend, that certain permissions be (initially) assigned to an administrative role.
• At the other issue, the goal was to let modules require, or at least recommend, that certain permissions NOT be assigned to NON-administrative roles.

The same sort of information is needed for both issues; the only question is what you do with it when you have it.

Hardcoding a single DRUPAL_ADMINISTRATIVE_RID would definitely be useful to a large number of sites, but in the long run, I think this extra hardcoding might be a bit of a step backwards. The ideas we were starting to develop at #248598: Label permissions which are warned about in the user interface would give you all this for free without any hardcoding needed, and in a way that would work for any site. In particular, we learned as a result of the discussion there that it's a design flaw to ask modules to know anything about how roles are being used; we tried it that way initially, but it was quickly pointed out to us that on some sites (intranets), anonymous users might actually be the administrators. Alternatively, some sites might have multiple "administrator-level" roles ("manager", "webmaster", etc). Hardcoding DRUPAL_ADMINISTRATIVE_RID and then asking modules to assign permissions to this role can never address these kinds of possibilities.

So the question I would have is the following: Is it worth it (and is it realistic) to try to do this the "100% right way" within the D7 development cycle, or is it better to go with the somewhat-flawed-but-still-useful approach of a hardcoded DRUPAL_ADMINISTRATIVE_RID?

Hardcoding something that's only useful for 80% of sites is by definition at least a little bit of a flaw, unless the other 20% have an easy way to override it... then it becomes a brilliant usability improvement instead ;)

So in that spirit: I'm curious about how "hardcoded" this administrative role would actually need to be. You've said above that it should be possible to delete this role (which sounds good), but you've also said you want modules to be able to assume it's there and interact with it by setting "sensible defaults." How exactly do you envision this? Are you thinking that a module should assign permissions directly to this role, e.g. via db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", DRUPAL_ADMINISTRATIVE_RID, 'administer mymodule'), or were you imagining a more elaborate mechanism?

If modules are being encouraged to insert permissions directly, then I'd worry that's a major security flaw... unless Drupal hardcodes that role extremely tightly and treats it as a very special case, with bells, whistles, etc. (Which is certainly not the end of the world, but it does then mean Drupal is less flexible and the 20% minority will get annoyed.)

Or maybe I'm just missing the point entirely?

David, this will pave the way for something like a hook_configuration() proposal to allow contrib modules the ability to easily install themselves with pre-configured role permissions.

Wow, hook_configuration() sounds like a really great idea.

For the issue at hand, though, it's still necessary to think about how hardcoded the administrative role should be, for the following reason. Suppose the only thing that's hardcoded is a fixed role ID for modules to use, as proposed above. The problem I see with that is this: Let's say I'm setting up my site and I go to add some roles. I have a whole list of roles I want to add, but "Administrator" isn't one of the ones I need. So I decide to rename the "Administrator" role provided by Drupal to "People I Don't Trust Very Much" and start putting people I don't trust in that role. Seems like a perfectly reasonable thing to do; after all, Drupal gives me complete freedom to decide how I want to use a role. However, once I start enabling new modules, some of those modules are then going to be trying to assign their administrative permissions to the "People I Don't Trust Very Much" role! If the permission assignments are done automatically, this could be catastrophic. If done in a managed way via hook_configuration(), then it's less catastrophic, maybe, but still annoying; every time I install a new module, Drupal will recommend that I give all the module's administrative permissions to these untrusted users.

So that's why I think more might need to be hardcoded then just the role ID, to prevent the above scenario. (How much, exactly, I'm not sure...)

Thinking about this more, what if we took a slightly different approach... maybe we could borrow some small ideas from #248598: Label permissions which are warned about in the user interface without needing the whole thing:

Rather than having "administrator" be a role that is treated specially, how about making it a "tag" (for lack of a better word) that can be applied to existing roles? The site owner ultimately gets to decide which roles the tag is applied to. There could easily then be a simple API for modules to assign permissions to these roles; e.g., calling user_assign_administrative_permissions(array('permission1', 'permission2')) would assign the listed permissions to all the roles on the site that have been tagged as "administrative". And if hook_configuration() happens, that could use the same API just as easily.

This would provide much more flexibility. Sites that have no use for this feature could simply not tag any of their roles. And sites with unusual setups, e.g. intranets where anonymous users are the administrators, could tag the anonymous user role as administrative, and then it would work fine for them too.

I haven't thought through all the implications of this, but does that seem like it might be a better approach?

Ok, here's the patch I promised in #15. It moves the rid for anonymouse userz from 1 to 0, adds an rid for' administrator' as 1, and leaves rid2 as the authenticated user.

Patch Testing Instructions
To test this patch, drop your existing database, apply the patch, and then perform a new install. Navigate to /admin/user/roles. Observe the new role called 'administrator, then go to admin/user/permissions and try out some settings. Any permissions you change should stick.

Also notice that there are no users yet assigned to this administrator role, which means that uid1 *must* explicitly assign users to this role in order to open up any semblance of security risk. Cool.

Two Notes:
1. If this approach is somewhat acceptable, I will be providing Simpletests for all of this. No, the simpletest doesn't pass right now, so please don't even think of applying this to core yet.

2. There needs to be an upgrade hook for system.install. That process needs some debate from you guys as to how best to support all database types.

This looks pretty good to me (although I don't think this is the patch to get 'node' back into user interface text sadly).

If we had an administrator role, I'd quite like to take 'create pages' away from authenticated users and give it to admin users on install - would differentiate the two content types a bit more than they currently are.

The help text needs work, and I'll volunteer to give it a try. I share catch's concern about node, plus, for better or worse, this goes against the trend began in Drupal 6 (that some disagree with) where we separated out some of the HTML (like li, etc) from translatable text).

Keith Smith: I would *love* some help on the help text. I rewrote it to try and explain the new role, then realized it was eye-glazingly long. Help a brother out?

@Catch: What are you referring to by "get 'node' back into user interface text"?

@All: Here are the roles that an 'administrator' would have by default (i.e., not uid1, but any other user who was granted this new role by the site's creator):

* administer blocks
* access comments (Normally given to authenticated users as well)
* administer comments
* post comments (Normally given to authenticated users as well)
* post comments without approval (Normally given to authenticated users as well)
* administer filters
* administer menu
* access content (Normally given to authenticated users as well)
* administer content types
* administer nodes
* access administration pages
* administer site configuration
* select different theme
* administer taxonomy
* access user profiles
* administer permissions
* administer users

Let's discuss the merits or pitfalls of these default perms for the Administrator role.

(I'm changing the status back to CNR to get more eyes on this. Keith, submit your patch to change the text at anytime. This issue won't get committed to core until we work on the text a bit, I'm sure, but we need some testers as well.)

Senpai: Your help text mentions the word 'node', there's been a couple of year process removing 'node' from the UI (which I'd like to see reversed), it should probably say 'content' or 'posts' or something instead for now.

I'd personally be in favour of the administrator role having all permissions by default, to encourage people to use an account other than user/1 for day-to-day administration.

a good idea. i'd even go a step further and create a an editor role that has administer nodes and maybe a couple more.

@Moshe: I'd thought about that too, but the trixy thing is this; rid3 is probably already used up on an existing site. I really didn't wanna delete the existing rid3 in favor of creating a default 'editor' role, even though that is one of the roles that most people create on every new site that gets installed.

That's also part of why the hook_update() is being so difficult right now. You can see in the existing patch that
system.install performs a

<?php
  db_query("UPDATE {role} SET rid = rid - 1");
  // Followed by:
  db_query("ALTER TABLE {role} AUTO_INCREMENT = 3");
?>

which takes the three newly installed roles and change them from 1,2,3 to 0,1,2 then resets the table's auto_increment counter so that the next admin-created role gets a 3, thereby avoiding a hole a position 3.

During a site update, however, rid 3 is most likely in use, and thus, this same approach doesn't werk so goot as a hook_update(). I need ideas, people.

I don't think there is a problem with having a "hole" for the role id 3. That's exactly what you would get if you created a few new roles then deleted the one with role id 3. Auto-incrementing just guarantees that every new entry in a table has a unique value for the specified column, not that all values in that column are consecutive.

@flobruit: Ahh, but the system.install *can* easily avoid a hole at position 3, whereas an update hook must respect an already existing rid3. That's the dilemma. We can install a series of rids on a clean slate, but during a hook_update, how do I successfully implement a database agnostic way of creating rid0, rid1, and rid2 without messing up the existing rid3?

Maybe move rid3 out of the way temporarily?

Why bother with an upgrade for this feature? Seems to me that it will do more harm than good. Let those sites be.

I agree with Moshe. Leave this for new sites only - sites upgrading to D7 should have their roles fixed up by the time they get there. And also having an editor role in there would be great.

Having said that, if it's possible, I'd rather see this in the default install profile than system.install - so we can the minimum install profile won't get them if it goes in, and other install profiles can define their own roles and permissions for particular use cases.

Oh wait. You're talking about making this another reserved role, that's guaranteed to be there for modules etc., in that case we probably need an upgrade path since a lot of code is going to depend on these constants. hmm.

Yeah, I was thinking that I needed to make an upgrade path from D6 for this new feature, or we'll end up with a lot of conversations that go like this:

"Ok, navigate to admin/user/permissions, and uncheck the 'administer users' box for your Administrator role. That'll fix you right up."

"Huh, I don't have an 'administrator' role. What now?"

Here's my thoughts while driving to work this morning. During the upgrade, run a db_query to get the contents of rid3, store it in an array, perform the UPDATE that installs rid0, 1 and 2, reset the AUTO_INCREMENT value to 3, then INSERT the contents of the array, thereby preserving the original rid3.

This does not help with the proposed creation of an rid for 'editor'. Hmm, needs more thought.

Why not just increment every non-core rid +1 (or 2 if editor gets added)? The only place rid should ever be checked is in code, and we don't need to provide backwards compatibility for code, just upgrade instructions.

Bdragon mentioned access control lists via IRC. That's something I hadn't even thought of. It needs some checking into, to be sure.

Sorry, but, an editor role is a) not in the scope of this issue, and b) IMHO, not very Drupalish. If you need one, add it to your install profile. If we would follow this path, we could also turn *all* roles into meta-roles. What would we gain from that? Lots of code in modules that need to check for even more special roles. Just have a look at all the modules that need to deal with DRUPAL_ANONYMOUS_RID and DRUPAL_AUTHENTICATED_RID already and you (hopefully) get the point, why this would suck.

Incrementing rids will be a mess for existing custom code. I'd rather avoid changing rids and potentially wrecking custom access control code.

A better solution is how the filter module does it. It uses a constant called FILTER_FORMAT_DEFAULT which points to a dynamic format as determined by filter_resolve_format($format).

Maybe something like this might work:

<?php
function system_update_7010() {
  // Add the new role at position x.
  db_query("INSERT INTO {role} (name) VALUES ('%s')", 'administrator');
  // Change the rid of anonymous from 1 to 0, leaving a hole at position 1.
  db_query("UPDATE {role} SET rid = rid - 1 WHERE (name) = ('%s')", 'anonymous user');
  // Move the newly created Administrator role into position 1.
  db_query("UPDATE {role} SET rid = 1 WHERE (name) = ('%s')", 'administrator');
  // Now close the hole by resetting increment value to one less than it is now.
  db_query("ALTER TABLE {role} AUTO_INCREMENT = AUTO_INCREMENT - 1");
?>

In this way, authenticated users stay at rid = 2, just like they always were. Webchick suspects that AUTO_INCREMENT might be a MySQL only function. I wonder if that means this statement breaks on non-MySQL databases. Does db_query() take things like that into account?

##Patch number 2

This one satisfies Catch's comment #26 by removing the word 'node' from the help text, and changes some of the help text that I'd previously missed. It addresses Keith.Smith's comment #24 by separating the HTML markup from the t() strings. Also included is an upgrade path from D6 that should, to the best of my knowledge, work across all databases.

The text is almost there. I'll have to read it again. I notice one inconsistency in capitalizing Role vs. role in user-facing text. Usually these days, when we refer to something that occurs exactly the same way in the interface we just italicize it, so that may be an option.

The last patch in this issue was no longer applying to the latest dev version, so I attempted to recreate it (attached), with two small changes. The first change was that there was already a function system_update_7010() so I made the new one system_update_7011(). The second change relates to the problem I had trying to run the update. The original patch had:

+  if ($GLOBALS['db_type'] == 'mysql') {
+    db_query("ALTER TABLE {role} AUTO_INCREMENT = AUTO_INCREMENT - 1");
+  }

However, that gave me an error so I tried

+  if ($GLOBALS['db_type'] == 'mysql' || $GLOBALS['db_type'] == 'mysqli') {
+    db_query("ALTER TABLE {role} AUTO_INCREMENT = AUTO_INCREMENT - 1");
+  }

which I also see within system.install, however it spits out an error ( Undefined index: db_type in drupal7\modules\system\system.install on line 3087. ) and aborts the update.

If I manually apply the aborted updates then the changes work as expected and are a much needed improvement to the Drupal default roles. Can someone help me with the system.install error so we can get this patch back on track?

Sorry, I forgot to run the last patch through dos2unix. The attached patch has been run through it...

Alex, that's almost certainly been changed with the new database layer. I'm not sure what you'd need to do with the new db layer though for it to play nice.

Just to provide another example of why something like this is important, the website of a local politician was recently "cracked." The reason: they left it open for users to register and then granted all permissions to the authenticated role. If they had an admin role on the site, they probably wouldn't have given all the permission to the Authenticated role. So, I agree with the arguments that this is a usability and security enhancement.

Setting back to needs work given the pdo changes.

A quick IRC convo with Bdragon to refresh my memory on why ACLists was important to this patch, which I referenced in #36. The jist of it is this,
it was about setting defaults easily. Adding defaults to hook_perm so that an official "admin" role would get the right permissions added automatically when installing a contrib module.

I've written this down here so that when i come back to this patch, I'll be able to check this angle out. Oh, and yeah, like AlexUA said, I'm gonna need help with the PDO database layer changes. I've read the books, but AFAIK, this isn't straight PDO is it. It's Drupal PDO! ;-)

Agree with #17: "Webchick's #13 is perfectly elegant" :-)

Food for thought, or perhaps a useless comment: Those familiar with Linux/UNIX system administration may consider this analogous to the concept of a "sudoers" group.

On Debian-based systems login as the superuser (user 0, usually named "root") is not enabled by default. Changing to the superuser on the command line is also highly discouraged. Instead, users who are to have system administration privileges are added to a sudo group. By issuing commands preceded by sudo, these users are able to perform actions permitted only to the superuser.

Assigning an admin role to certain Drupal users accomplishes similar things. The advice that UID 1 be used only for a very limited set of administration tasks (i.e. accessing update.php) mirrors "(almost) never login as root", and is sensible. While Drupal has no equivalent to the "sudo" command, confirmation forms (in Drupal core at least, e.g. "Do you really wish to delete X? This cannot be undone!") prevent users with admin permissions from accidentally performing irreversible changes.

Lesson? If an admin role patch goes through, a caution should be added to the module developer's handbook to use confirmation forms to protect actions that would be catastrophic if triggered accidentally, i.e. anything which permanently removes user data from the database.

Lesson? If an admin role patch goes through, a caution should be added to the module developer's handbook to use confirmation forms to protect actions that would be catastrophic if triggered accidentally, i.e. anything which permanently removes user data from the database.

Actually, regardless of whether we ship with an admin role or not, module developers need to do this. Otherwise, a malicious user could craft a link and trick someone with advanced permissions into clicking it (for example get your free porn here! -- or, more insidious, <img src="http://www.example.com/admin/user/delete/1" /> on a busy thread), thus causing them to inadvertently cause harm on their site. This type of attack is called a cross-site request forgery (CSRF).

The Drupal Security handbook does have a page on this at http://drupal.org/node/178896 but it looks like it could use some fleshing out with specific "do" and "don't" code. Right now it merely points to the patch for Drupal 5.1 -> 5.2 where two such vulnerabilities were fixed. Paul, if this is something you feel comfortable doing I think it'd be really valuable.

Is it ok to just make rid 0 the DRUPAL_ADMINISTRATOR_ROLE ?

No, it is not. Because PHP is a (very) weakly typed language, 0 should never be used, because it compares to NULL. The only legitimate case to use 0 is when "all things non 0" and "the 0 thing" are two very different classes of stuff (for example, the anonymous user is 0, so that if ($user->uid) is a test for an authenticated user).

Because DRUPAL_ADMINISTRATOR_ROLE is nothing more than a new role, it shouldn't be 0.

I think Damien is right. There is no need to use a defined constant here at all, in fact. The role ID can simply be stored in a variable. Then if modules need to know which role it is, they could simply use a function like this:

/**
 * Return a list of administrator-level roles on the site.
 */
function drupal_administrator_roles() {
  return variable_get('drupal_administrator_roles', array());
}

Note the plural -- with this scheme, there is actually no reason to limit it to a single role.

I also realized that another problem with using 0 for the administrator role ID might be this: #138799: checkbox(es): key 0 is mistakingly always on. As far as I understand it, you would have problems anywhere in Drupal where the user roles are presented as a set of checkboxes.... And it seems that switching existing role IDs around is dicey too, because although they are defined constants (codewise), modules might store the roles in the database (where they are integers), so you'd need a major DB update for every table that stored them (in Drupal core alone, the above patch would need to be updated to deal with the {block_role}, {filter_format}, and many other tables, for sites upgrading from Drupal 6).

I think it's clear that a new approach to this patch is needed. I intend to write one over the weekend (hope it's not rude to "steal" this issue from someone else, but I'd really like to get this done and get it off my tracker :)... The goal of the patch will be to accomplish three things:

1. Sites which install Drupal using the default installation profile automatically get an "Administrator" role with the 'magical administrative properties' (by which I mean a role that modules can use to automatically assign permissions and such). However, they can delete this role if they don't need it.
2. Sites which install Drupal using the "minimal" installation profile do not get this role. But if they want to add one later and assign it the 'magical administrative properties', they can do so.
3. Sites upgrading from Drupal 6 do not get any roles added -- we can safely assume that they've set up their roles and permissions the way they want and don't want us to add any new ones for them, thank you very much :) However, if they want to add the 'magical administrative properties' to any of their existing roles (or to a new one), they can do that whenever they want to.

Check back here in the next couple of days for the patch...

David_Rothstein: Sounds like a very sensible approach! :) Looking forward to reviewing the patch!

Tagging as usability.

(Yes, I know usability is also a component, but "user system" is the proper component for this issue.)

This issue has come a long way and I like the idea of David Rothstein, which sounds great!

Better title.

My take on this:

1. Enhance hook_perm() to allow each module to define which permissions are recommended and should be set for administrative user roles:
   

   /**
    * Implementation of hook_perm().
    */
   function block_perm() {
     return array(
       'administer blocks' => array(
         'title' => t('Administer blocks'),
         'description' => t('Select which blocks are displayed, and arrange them on the page.'),
         'administrative' => TRUE,
       ),
     );
   }
   

2. Enhance the user roles overview page on admin/user/roles with a checkbox to mark a role as administrative. Enabled roles are stored in a variable. There is no checkbox for the anonymous user and authenticated user roles.
3. Upon installation of a module, the recommended permissions are granted to the configured administrative roles. This happens only once during installation, so administrators can mark multiple roles as administrative and alter the permissions afterwards.
4. Only the default profile adds a "administrator" role, which is marked as administrative role by default.

sun: sorry, but IMO #1 & #3 is killing kittens. Let's get the role there first as a pure duplicate of UID 1, and then talk about neat things we could do with it.

OK, here is a patch which accomplishes all the goals discussed above. It also incorporates some of the help text improvements that were in the earlier version of the patch above. I'm not 100% sure about the UI -- I was playing with ways to inform the site admin which roles are "administrative" in a few places where it seemed important to do so, but I'm not sure what I came up with so far is that great.

In terms of automatically assigning administrative permissions when a module is installed, the patch allows any module to do this by calling something like this in its hook_install() implementation:

user_assign_administrative_permissions(array('administer mymodule'));

This would basically give the effect that @sun suggests. Currently, the patch does not do that for any modules in core, though... I thought about including that in this patch (and maybe we should), but it seemed a bit tricky to even figure out which permissions are the appropriate ones to assign, so I left it out for the moment. The default installation profile still adds permissions for its modules, though.

As for eventually labeling these permissions in hook_perm(), I think @sun may very well be on the right track, but yeah, that should probably be for another issue, perhaps #230059: Allow modules to provide optional configuration profiles (e.g., for use in a "setup wizard").

Also, as @sun suggested, the UI does not allow anonymous or authenticated roles to be flagged as administrative, because for 99% of sites that would be a really bad idea. However, there is nothing in the underlying code that prevents this, so for the rare sites where this would be desirable (e.g., certain types of intranets), you could easily have a module that did this. Or actually, all the site would really have to do is put something like this in their settings.php file, and it should work fine:

$conf['drupal_administrative_roles'] = array(1 => 1, 2 => 2);

Finally, @Benjamin: Take a look at the patch and see what you think... I don't think we're introducing anything here that acts like a "meta role"? These are really just normal roles, and the permission inheritance works exactly the same for them as it does in the rest of Drupal.

I tested the patch in #58, and it appears to have some serious drawbacks, not to mention it's taking the issue in a direction the O.P. wasn't intending. First of all, after applying this patch and then visiting admin/user/permissions, I cannot assign the 'administer comments' permission to anyone except anonymous or authenticated users. Why? Uber fail in terms of Drupalability, at least without a major restructuring of the Permission Page's checkbox nightmare, which is wwaaaay beyond the scope of D7. ;)

Also, If I were to write a new module that wanted to enable, upon installation, all of the "administrators" of the site to create content of the type provided by my module, I can't see any way to do it. David, please provide instructions on how to test this fundamental requirement for this issue? The way i see it, there *might* be an admin role, if somone had bothered to create a new role in their new site, and then edit that role to make it an 'admin' role. But what if they haven't, and my module needs to configure itself a certain way upon installation? Fail.

[EDIT] These last few discussions are great and all, but they're really steering the focus of the issue away from it's fundamental goal, which was to provide a foolproof way for modules and coders to know which role in a site was 'safe' for auto-assigning certain extra permissions. All the patch needed was a little PDO help, and instead we've gone off the deep end into a meta-role area which is already covered by at least three other issues. I think the issue should steer back on track to the few posts before #52.

Thanks for testing the patch.

First of all, after applying this patch and then visiting admin/user/permissions, I cannot assign the 'administer comments' permission to anyone except anonymous or authenticated users.

I can't reproduce this issue. When I try it, the admin/user/permissions page seems to work perfectly fine. Can you post step-by-step instructions for how to reproduce this? (I'm somewhat surprised that my patch could have anything to do with this, since the only change I made on the permissions page was a minor theming change, as I recall.)

Also, If I were to write a new module that wanted to enable, upon installation, all of the "administrators" of the site to create content of the type provided by my module, I can't see any way to do it. David, please provide instructions on how to test this fundamental requirement for this issue?

All you need to do is something like this in the module's install file:

function mymodule_install() {
  user_assign_administrative_permissions(array('create mymodule content'));
}

The way i see it, there *might* be an admin role, if somone had bothered to create a new role in their new site, and then edit that role to make it an 'admin' role.

Or if they installed Drupal 7 with the default install profile (as most people will do). With my patch, the default install profile creates such a role for them.

But what if they haven't, and my module needs to configure itself a certain way upon installation? Fail.

I don't see why any module needs to do this absolutely -- it's a convenience, not a requirement. Currently in Drupal 6, no module can do this, and it works fine. If a site doesn't have any administrative roles defined, it probably means they are not using this feature. These sites can still assign permissions the normal way by hand, just like always.

These last few discussions are great and all, but they're really steering the focus of the issue away from it's fundamental goal, which was to provide a foolproof way for modules and coders to know which role in a site was 'safe' for auto-assigning certain extra permissions.

That is exactly what my patch provides, via the functions drupal_administrative_roles() and user_assign_administrative_permissions()....

All the patch needed was a little PDO help

No, there were also issues with using 0 as the defined constant or moving role IDs around, etc., as explained above.

and instead we've gone off the deep end into a meta-role area which is already covered by at least three other issues. I think the issue should steer back on track to the few posts before #52.

I honestly don't understand what you mean by this. The only difference between my patch and the original idea is that whereas before it was envisioned that every single Drupal site would be forced to have exactly one administrator role, my approach gives individual sites the freedom to choose the number of adminstrator roles -- 0, 1, or as many as they want.

Also, here is a rerolled version of the patch. I'm pretty sure the testbot failures reported above were spurious (since when I run those tests they seem to pass), but we will let the testbot decide again...

Resubmitting for testing, since the last failure was due to HEAD being broken.

Adding tags and subscribing.

Senpai wrote:

First of all, after applying this patch and then visiting admin/user/permissions, I cannot assign the 'administer comments' permission to anyone except anonymous or authenticated users.

David Rothstein wrote:

I can't reproduce this issue. When I try it, the admin/user/permissions page seems to work perfectly fine. Can you post step-by-step instructions for how to reproduce this? (I'm somewhat surprised that my patch could have anything to do with this, since the only change I made on the permissions page was a minor theming change, as I recall.)

Here's the step-by-step instructions on how to reproduce this. First, after applying the patch, go to the admin/user/permissions page with the intent of granting your staff members the ability to administer comments. There are exactly two roles right now, anonymous and authenticated. Next, allow your staff members to administer comments without allowing all logged-in users to administer comments.

There's no way to accomplish this simple and trivial task, and thus the patch in #58 fails to provide any sort of usability enhancement to Drupal core, which was the entire goal of this issue.

I'm going to take this issue back to it's basics, and simply add a third role to core, moving the anonymous user role to rid0 as i originally did. Once I reroll the original patch, I'll hopefully have a much better understanding of PDO as it allies to Drupal, cause the book I just finished reading covers the basics of PDO, but obviously not as they relate to Drupal databases specifically. :)

@Senpai: Go to admin/user/roles, create a new role, mark it as "administrative role". Afterwards, go to admin/user/permissions and see the difference.

Setting back to CNR to let the bot test again.

Daniel, the entire point of this issue is to get another role into core by default so that people don't *have* to understand how to create Roles, and don't have to do so before they can enjoy this extra level of security, and so they can see that there's a pre-planned difference in our Roles system that distinguishes logged-in users from people who might need to be granted "dangerous" permissions.

Once this issue is fixed, only then can we take on a proposal like what David Rothstein has come up with , and that would have to be in a different issue, since it would scope creep this issue too much.

@Senpai: Look at the last hunk of the patch. It creates a new "administrator" role upon installation of Drupal's "Default" profile. Previous follow-ups explain why this is the (only?) approach that has a chance to hit core.

According to #468768: Remove hardcoded anonymous and authenticated user roles it sounds like we have states and roles. Wouldn't it be better to have "states" of the roles. Where any role could be "anonymous" or "authenticated"?

According to the discussion going on over at d7ux.org: Admin Header and what seems to be needed from this discussion is a third "state": Administrator.

I have cross-posted this thinking at #468768.

The basic tenants of this feature have been coded by catch in #480660: Add an 'administrator' role to core and already committed, so there's no reason to continue this thread.

more accurate status