Development for a Drupal 6 port of Admin Role

• Why did you re-write .info?
• Don't t() in hook_menu() anymore.
• "access arguments" should be plural, not singular
• I created a role named administrator, then selected it at admin/user/adminrole, and when I loaded admin/user/permissions, anonymous had all the permissions.
• Uploading whole files makes it really hard to see what changes you made from what's in CVS without doing a diff myself. You should follow http://drupal.org/patch/create

I wrote it for drupal 6 not drupal 5. Your arguments does not stand under drupal 6..
Please test it with drupal6.

If it helps, to reproduce:

1. Fresh install of Drupal 6-rc3
2. Enable adminrole
3. Create administrator role
4. Create test user (uid 2) and assign to administrator role
5. Go to Administrator Role page and pick administrator
6. Go to Permissions page and anonymous and administrator have all permissions

In addition to the previous list, in .info

is added by project.module during packaging and shouldn't be added as part of the module's source in CVS.

You can find the plural vs singular docs at http://drupal.org/node/109157 for access arguments.

The standard capitalization of menu links means the link to the config page should be "Administrator role" not "Administrator Role".

Yes, you are right. I misunderstood you.

I think I addressed all your problems here, but I do not created a diff, sorry.
The main problem was the use of array_merge() function.

please test again

Much better.

I did some more in-depth testing this time. I put a bunch of modules in my install, then installed two that added more permissions to the permissions page. When I viewed the permissions page, only one of the new modules was auto-selected for the administrator role, the other was unchecked. I went back to enable more modules that added permission options and tried three more. On the permissions page, the one that was not enabled from before is now enabled, but the other new ones were not.

It appears, without looking at the code, that the permissions updater is only updating one at a time instead of fully looping through all the newly enabled modules. Whatever the cause, not all modules are being granted to the administrator role when I enable more than one at a time.

thanks for the review

please change the code on line 63
from

that should fix the problem

Here's a patch against HEAD (revision 1.3). I started to mark it ready to commit, but I think these lines need a bit more attention:

That only prevents anon and auth from showing in the form, not from being submitted. I can imagine an intranet allowing anon or auth users to have the admin role.

First I ask, is preventing anon and auth users from having this role something that should be enforced in the first place? If so, I think it also needs to be in adminrole_admin_settings_validate() as a check with error message result. Either way, the patch needs work.

Thoughts?

These roles are hard coded into drupal. They have some functionality associated to them. (Search for references in drupal source) Using any of them as an administrator role is not recommended.

Please explain how can you submit these values if they are not listed.

at least the anonymous role should be moved into the validation function to prevent random visitors receiving admin rights.

This adds validate hook to check for anonymous and authenticated user submission attempts (against HEAD revision 1.3).

but I don't understand how do you trigger this error message

if you need validation, then the adminrole_update_perms() function could do some validation..

Many of the functions of Drupal are self-documenting. adminrole_update_perms() is to update permissions so that's what it should do, hook_validate() is to validate form submissions. In the Drupal bootstrap process, setting an error at validate prevents hook_submit() from even being called. See also http://drupal.org/node/202756

I understand all this hook thing.
But tell me the reproduction steps to show the error message in your validation hook.

(I should not write short sentences, because people do not understand what I try to say..)

#209842 was marked a duplicate of this issue. In that issue I proposed changing the package for Admin Role from "User Access" to "Administration" so that it's listed with other modules. One objective reason to list Admin Role in "Administration" is simply because Admin Role one of the first modules I install on any site and find it more convenient to have it listed at the top.

That being said, I'm biased in favor of "Administration" since several of my modules list "Administration" in their info files, but other modules I know of that also do this are Admin RSS and Masquerade. In #209842 deekayen also lists Zimbra, Logwatcher, LDAP Integration, LDAP Provisioning, Front Page, Default filter and Abuse.

I don't need to reproduce it to know what it does and I know never to trust user input. For example, I didn't like any of the choices to show my relationship to http://advogato.org/proj/PHP-Nuke/, so I created a form of my own where the action was set to submit to advogato.org and it injected "Source" as my relationship instead of the standard form options at the bottom (None, Helper, Documenter, Contributor, Developer, and Lead Developer).

Granted, you'd already have to have administer permissions login to do the alternate post, but if you're going to block submission of anon and auth users, do it right.

#15, I removed the package in .info in my patch. I like to think the package definition is more suited for modules related to a larger module that accepts plugins like CCK, OG, or CAPTCHA.

Sounds good, thanks deekayen.

#16 ok, I understand. You created the validation for "bulletproof" blocking these roles. No more objections on using this validation hook, because I can't tell any example when this validation would cause a problem. But here is one example when something is over-validated and the validation hook is removed: #209584: Cannot set custom error pages to URL aliases.

really "bulletproof" validation would be changing

but as I said I have no objection on your patch, I am fine with it..

I disagree on #19. The point is to prevent the form submission. If you also do it in the update function, you block other modules from using the update function as part of extending functionality.

Hey guys,

Sorry to be an absentee maintainer, and I appreciate all the work and argument!

I'm going to change this back to CNR until I get a clear indication that the current patch is the real one.

Also, see http://drupal.org/node/221124#comment-729552

This patch may need to be included as well... To be honest, I haven't been following the 6.x thread here other than being happy someone is taking up the mantle here.

When you guys push it back to RTBC, I'll do a review and pass it around a bit and then commit it.

Thanks again!

Jacob

Jacob, I'm leaving this as CNR because I think it needs a maintainer's intervention in decision making if you're going to want http://drupal.org/node/221124 included. One of the biggest differences is strings and I like mine.

I researched and included in this patch a less hacky way of doing the alternate settings submit handler. That specifically meant diverging from the code difference at the bottom of the other issue's patch to form_alter().

Patch against HEAD.

Hi Guys,

Thanks a lot for this. My life has been pretty hectic the past few months, and I'm getting involved again. If any of you wants to take up maint of this mod, I'd be happy to include you.

I looked over the patch and tested superficially. It doesn't seem to work for me.

Here's what I did:

Installed the mod
went to settings page
saved as "none", nothing happened - Good!
Added a role called "admin" , selected and saved, all perms given - good!
Went to permissions, unchecked something and submitted, when I came back, was still off (not good :( ).

I think the module is supposed to give all perms to that role, so unchecking shouldn't do anything. It might be a nice addition to remove that role from the listing on the page somehow (menu override?) and just display some text like "all perms automatically provided" so people don't get confused.

Let me know if you see something similar.

Btw, this is against the patch in #22, didn't check earlier ones.

Making the name clearer so that the duplicates don't keep popping up.

Sorry, but I can't find the link for download the 6.x port in http://drupal.org/project/adminrole, where is it?

Thanks,

That's because there isn't one. It would at least be listed on http://drupal.org/node/128621/release if there was.

Yes I know that, thanks, I'm not new in the neighborhood, but I did imagine a dev version of this module if it's being ported, not a couple of txt files for copy and pasting.

Thanks anyway.

Hey Guys,

I made a few updates to the DRUPAL-5 branch (probably the last ones coming).

Can someone re-roll the patch against that? I promise, I will review again and commit ASAP. Also, please note my comment about unchecking from the module page.

Best,
J

subscribing

The above module files when enabled allow anonymous roll all admin functions instead of the role created for that purpose, not sure why

D6 release exists now.

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

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