The module allows an developer to log in as user #1 with user accounts from a separate authentication source, such as LDAP. Can also be used without LDAP to login with a config-set (eg. settings.php) password.

Useful for administering a large cluster of multisite sites that share the all/modules directory. Just configure the module and enable it on all sites; no need to remember the user #1 password any more.

Project page: http://drupal.org/sandbox/ZeiP/1407262
Git: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/ZeiP/1407262.git developer_authentication

CommentFileSizeAuthor
#9 _developer_authentication_clean_ldap_string.patch1.09 KBAnonym

Comments

saitanay’s picture

saitanay commented
Status: Needs review » Needs work

Hi Zeip,

Looks like good idea,.

Below are the comments from Ventral Automated review:
FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
24 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

FILE: ...ll/modules/pareview_temp/test_candidate/developer_authentication.module
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
97 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
97 | ERROR | Comment indentation error, expected only 1 spaces
98 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
99 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
99 | ERROR | There must be no blank line following an inline comment
--------------------------------------------------------------------------------

@ http://ventral.org/pareview/httpgitdrupalorgsandboxzeip1407262git

Once fixed you could re-run the review to check the status.

This reviews just the formatting and other Drupal coding standards. Someone would review the code manually once its is clean.

zeip’s picture

zeip commented
Status: Needs work » Needs review

I fixed major style issues previously, but now all the problems reported by the autoreview should be fixed. Thanks for the review!

chintan.vyas’s picture

chintan.vyas commented

There is still a master branch, remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732.
I haven't played much with this module but some quick manual review.

Manual Review

  1. It would be great if you could mention some example incorrect usage of this module which can realy harm site security, in Project description or somewhere in README.txt file.
  2. .info file description and Project description in sandbox is different. In Project description it is : "Allows an developer to log in as user #1 with user accounts from a separate authentication source, such as LDAP. Can also be used without LDAP to login" and in .info file its : "Allows developers to login via LDAP as user #1". I think in .info file should have description similar to this text "Allows developers to login via separate authentication source such as LDAP or other authentication as user #1"
  3. In .install file line no 14 looks overhead. Get rid of $t = get_t(); line.
  4. In .module file, Line no 12 : Remove $items = array(); no need of defining $items. $items will be deined and initialized in next line.
  5. You may need to add validate function for developer_authentication_form in .module file

Regards,
Chintan Vyas.

chintan.vyas’s picture

chintan.vyas commented
Status: Needs review » Needs work

Forgot to change status :P

patrickd’s picture

patrickd commented
Status: Needs work » Needs review

I'm afraid you did not point out a single issue which justifies setting 'needs work'.

chintan.vyas’s picture

chintan.vyas commented

I mentioned 5 points in #3 Manual review.

zeip’s picture

zeip commented

Thanks for the review. I did the changes chintan.vyas suggested, even though I think most of them are optional (at least 1 and 5). I did not add a validation function since I don't think it's necessary.

zeip’s picture

zeip commented

I reverted the fix for point 3, since the automatic code review complains if not using get_t() to find the t() function.

Anonym’s picture

Anonym commented
StatusFileSize
new _developer_authentication_clean_ldap_string.patch1.09 KB

Hello. I tested the module with LDAP:

  1. function _developer_authentication_clean_ldap_string($str) must return value or use pointer.
    Patch to fix
  2. I have a warning if not set settings: Warning: ldap_bind() [function.ldap-bind]: Unable to bind to server: Can't contact LDAP server in function _developer_authentication_ldaplogin() (line 104 in file /.../sites/all/modules/developer_authentication/developer_authentication.module).
  3. Why module configuration saves into settings.php? Why module does not have a configuration page?
  4. How i can select users from LDAP who can access to login as developer?
zeip’s picture

zeip commented

1. Weird, sorry for not noticing that! I prefer return values, so I added such.
2. I added a check for the only required setting (base DN) to the form function – it outputs an error message if that's not set.
3. That's because I've made it for clusters of multiple sites, and I'm using it via a setting file for all the sites in /etc – so if the config changes I don't need to change the setting to each site, and on the other hand I can move the sites between servers and have different settings on each server (production servers can have the LDAP part set up and local environment can have the SHA1 password set for login with user "admin"). So it's really a matter of the meant usage – of course it can also be used by setting the settings per-site.
4. The current code only accommodates for all users under the specified DN being able to login.

zeip’s picture

zeip commented
Priority: Normal » Critical

Elevating priority per guideline (last status change was July 25th.)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

bircher’s picture

bircher
Location 🇨🇿
commented
Priority: Critical » Normal

Hello
I could not test it with LDAP but from looking at the code and the variable authentication I can say it looks pretty good.
The only thing I would classify as a bug at this point is the fact that you allow the log in with a master password but you still get the error:

You haven't set the LDAP log in settings.

So either make it as a warning or better blend it out if developer_authentication_admin_pwd is set.

Although I do it myself on most of my sites it would be better to call the username not admin. That could also be a setting or defined in a variable. After all it is a second field to get right when hacking the site.

I am not the most experience reviewer but I would give it a rtbc if you fix the LDAP warning.
see you

zeip’s picture

zeip commented

I changed the message from error to warning, thanks for the suggestion! It's also not shown if the admin pwd is set any more.

IMO setting a fixed password for an admin account isn't that good an idea as-is, so I think the added security of changing the admin name isn't too useful. It's a good idea to make it a variable, though; I'll add it to my list. Often hacking is also tried on the /user page; the hacker might not try /devlogin at all.

Thanks for the suggestions, hope the application's now ok!

bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs review » Reviewed & tested by the community

Hello

Yes we are absolutely on the same page about the password and the security then. And I agree with you that the fixed name is not a show stopper.
Then again the password for user 1 also needs to be taken care of and I have the feeling that that also doesn't happen often... And I guess if you enable this module you are well aware of the risk. After all, you mentioned it.

So in my opinion you are good to go.
Good luck!

zeip’s picture

zeip commented
Priority: Normal » Major

Issue has been in RTBC for three weeks.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

zeip’s picture

zeip commented
Priority: Major » Critical
jthorson’s picture

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

I would probably raise the visibility of the usage warning on your front page, as this does have the potential to severely compromise the security of a site if misused. I'd also suggest that having an option that logs the user in as a specific user who is NOT user #1, but a secondary admin account with full permissions, would be a bit safer ... as you would not be exposing UID #1, and the admins setting up the site would have to consciously consider what permissions they were making available before the module could be used (and those who do not know the security risks couldn't inadvertantly expose them).

That said, the code is clean and it uses the APIs properly ... I haven't tested as I don't have an LDAP server readily available, but I don't see any immediate red flags. Thus ...

Thanks for your contribution, Zeip!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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