[D7] ILS Authentication

Review of the 7.x-1.x branch:

Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/drivers/iii.inc:
+104: [minor] use lowercase html tags to comply with XHTML
+108: [minor] use lowercase html tags to comply with XHTML

Status Messages:
Coder found 7 projects, 7 files, 2 minor warnings, 0 warnings were flagged to be ignored

Source: http://ventral.org/pareview - PAReview.sh online service

No manual review given, and the minor coding style issues do not justify the "needs work" status.

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

Review of ILS Authentication:

Licensing:

- File /drivers/sip2_class.inc appears to be a 3rd party library. These should not be included in Drupal modules. Instead, you should add instructions in the modules installation instructions on how the users may download the library. See http://drupal.org/node/422996 .

Module duplication

- Surprisingly, I can't seem to find a module that does external authentication in a modular way. There are a lot of modules that do _one_ kind of external authentication. A modular approach would reduce the amount of duplicated code in the contrib module space. Have you considered making your module more generic? As it is, the module seems to be aimed only for ILS integration, but it looks to me like it would work with just about any external authentication source.

Drupal APIs

- In ilsauthen.module, you're including the driver file with a call to module_load_include() in the global context. You shouldn't do that.

- In function ilsauthen_savesettings_js, you may want to leave the div-markup outside the translated string. Instead of this:

  $output = t('<div class="messages warning">You must now click on "Save configuration" below to make the correct driver settings appear.</div>');

Do this:

  $output = '<div class="messages warning">' . t('You must now click on "Save configuration" below to make the correct driver settings appear.') . '</div>';

See http://drupal.org/node/322774 .

Security

- No immediate problems found, but I didn't go into that much detail. Being an authentication module, an in-depth security review should be done.

Drupal coding standards

- Some minor problems found with PAReview:

sites/all/modules/pareview_temp/./test_candidate/drivers/symphony.inc:
 +13: [minor] @see should always be at the beginning of a line, never inline in other comments.
 +22: [minor] @see should always be at the beginning of a line, never inline in other comments.
 +62: [minor] @see should always be at the beginning of a line, never inline in other comments.
 +68: [minor] @see should always be at the beginning of a line, never inline in other comments.
 +79: [minor] @see should always be at the beginning of a line, never inline in other comments.

sites/all/modules/pareview_temp/./test_candidate/drivers/iii.inc:
 +104: [minor] use lowercase html tags to comply with XHTML
 +108: [minor] use lowercase html tags to comply with XHTML

Code documentation

- The code is well documented.h2

Well, this reviewing thing is new to me, so don't take me as the final authority on the subject. But:

Licencing

"Download the SIP library from here http://php-sip2.googlecode.com/files/sip2.class.php and add it to your /sites/libraries/ directory ".

That's easily accessible in my book. Unless the version there isn't the right one? The file is listed as deprecated, so maybe the source in SVN is a more recent version?

Drupal coding standards

Not a blocker, no, but I see no reason to fix them anyway.

That sounds like valid reasoning for including the library, so I don't think that should be a blocker.

setting to critical since it's been waiting for a review for more than 5 weeks.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. ilsauthen_install(): remove the return at the end of the function, it has no effect.
2. ilsauthen.module: do not require the ilsauthen_driver globally, as this will be executed on every single page request. Require the file in functions if you actually need it.
3. ilsauthen_user_insert(): db_update() and db_insert() instead of manual queries.
4. 'Please add your email address to check_plain(!user).': what does the check_plain() mean here?
5. 6.x branch: "'#options' => $roles," this is vulnerable to XSS exploits as #options on checkboxes is not sanitized in the Drupal 6 Form API. Roles are user provided input and therefore need to be sanitized before printing. See http://drupal.org/node/28984
6. "'#default_value' => filter_xss_admin(variable_get($reset_password_message_element, "Please go to http://passwords.myexample.ca to change your password.\n\nThank you.")),": do not run filter_xss() on #default_value as the form API will run check_plain() on it anyway.
7. "$form['pass']['#title'] = 'Your name exactly as it appears on your library card';": all user facing text must run through t() for translation, please check all your strings, e.g. "$form['name']['#title'] = variable_get('ilsauthen_sip_username_title', 'Cardnumber');" and "'#default_value' => variable_get($reset_password_message_element, "Please go to https://koha.example.org/cgi-bin/koha/opac-user.pl to change your password."),".
8. "module_load_include('inc', 'ilsauthen', 'drivers/sip2.class');": do not load the class manually, add it to the info file for autoloading. See http://drupal.org/node/542202

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Can't test this, so this is a code walkthrough of 7.x-1.x

ilsauthen_uninstall() does a wildcard delete from {variable}. The will cause problems if the module is ever sub-moduled in the same namespace.

You may want to consider using your own permission instead of 'administer users' for the admin tasks.

ilsauthen_login_validate(), ilsauthen_user_login(), etc, are using db_select() without using the return values???

Just a few minors:

• You could replace all you require_once DRUPAL_ROOT ... with module_load_include(). No drupal_get_path necessary then.
• In iii.inc in ilsauthen_driver_connect() you use file_get_contents() to do a HTTP request. Some (many) servers don't support that (allow_url_fopen). I suggest using drupal_http_request() or cURL.
• You could add admin/config/people/ilsauthen to you info file (configure).
• DBAL queries are usually formatted like this:

  $ils_authen_authname = db_select('authmap', 'a')
    ->fields('a', array('authname'))
    ->condition('uid', $account->uid)
    ->condition('module', 'ilsauthen')
    ->execute()
    ->fetchField();
  

  so with extra white-space for 'next lines', so it's more obvious where the command starts and ends.

• It might not be a good idea to log user/pass combo's into watchdog... (ilsauthen_login_validate()) If that was exactly your intention, sure, but it seems a little unsafe. Log tables usually don't contain the sensitive stuff.
• I think comments direct after Implements ...(). isn't 'allowed'. I'm talking about ilsauthen_form_alter(). Put it on the next line (with an empty between) to make the whole more readable. Details.
• Are all the old (?) db_query() statements really necessary? They're messy.

Looks very good overall! I didn't do a Pareview check, but I like to think I know what I'm talking about.

Really all I can think of that's imperfect now is the very, very long comment on line 251 for ilsauthen_query_driver().

Oh and 2 missing spaces before 'pass' => $_hashed_pass, but now I'm just trying.

All of it looks really, really good. I haven't actually tested it though, so I don't think I'm allowed to rtbc.

I don't get it... How does a user end up in {authmap} so it's actually validated through a driver..? I'm adding myself manually and creating a custom driver now. Let's testdrive this badboy!

But how does an external service know when to register a user? He can't register ALL users that don't have an account... Then how would the service know the user's mail?

$form['logging_warning'] should contain #markup, not #value and the message should probably be a warning, not an error.

The "Driver" dropdown in the admin form tries to load something with Ajax.... but doesn't. Why the message? Why not always load it and show it with Javascript?

You're using the driver's validation function only AFTER calling the connect function (so querying the API), which seems silly... The validation function checks user input, so it doesn't have to send it to the API, right?

In iii.inc's ilsauthen_driver_connect() you're assuming the result of drupal_http_request() is a string. It's not. Ever. It's an object where ->data is the response body, but you should also check ->code.

'Everything breaks' if I have Drupal create a user with a username that already exists in Drupal.

I'm Jaap and I have a My Service account (username "jaap"), but not a Drupal account. Another Jaap does have a drupal account (username "jaap_102"). Jaap logs in with My Service and Drupal wants to create an account for him with username "jaap", because that's his My Service username. Auch.

I'm not sure it's an edge case that you want to always handle, but the gross errors should be handled. My guess is somewhere in ilsauthen_authenticate().

Also, the watchdogs are very useful, but should be labeled "ilsauthen", not "user".

E-mail check in ilsauthen_user_login() doesn't work, because you're checking if a query->execute() is empty and it's never, because that returns a DatabaseStatementBase object. Check for the field value by adding ->fetchField() OR just check the $account argument...

Also, the message will be clearer (to translate) if you add all the text in one string (instead of 2, with an l() call): Please add your email address to <a href="@url">your profile</a>. @url should then be a fully qualified url (using url()).

And I don't think the comment above applies anymore..? "Show the message on every page except user/xx/edit, since ...."

Good thing I did testdrive it =)

A quick glance (it needs more reviewing):

* The $form agument in validation handlers isn't usually by-reference. It can be (apparently, I didn't know until just now), but usually isn't.
* } else { isn't according to the coding standards.
* Why all the absolute => true in url()?

I like it. Better than many existing modules out there.

Always keep in mind Drupal modules must be extendable, so add drupal_alters and module_invoke(_all)s wherever it's useful. (And don't forget to cache it.) In this case that might be using a _info hook to collect auth adapters/drivers instead of scanning a folder. Without it would be fine for a v1 though IMO.

Now it's up to the big dogs to review and approve your module/pro status.

Actually there's a few minors from PAReview:

FILE: /var/www/drupal-7-pareview/pareview_temp/ilsauthen.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AND 8 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
88 | ERROR | Function comment short description must be on a single line
132 | WARNING | Line exceeds 80 characters; contains 118 characters
135 | ERROR | Line indented incorrectly; expected 6 spaces, found 7
139 | ERROR | Array closing indentation error, expected 10 spaces but found 12
195 | ERROR | Inline comments must start with a capital letter
208 | WARNING | Line exceeds 80 characters; contains 104 characters
208 | ERROR | Inline comments must end in full-stops, exclamation marks, or question marks
219 | WARNING | Line exceeds 80 characters; contains 114 characters
220 | WARNING | Line exceeds 80 characters; contains 97 characters
240 | WARNING | Line exceeds 80 characters; contains 94 characters
240 | ERROR | Line indented incorrectly; expected 8 spaces, found 6
240 | ERROR | No space before comment text; expected "// If this is a new
| | mail, update the current account to reflect the
| | driver-provided mail." but found "//If this is a new mail,
| | update the current account to reflect the driver-provided
| | mail."
276 | ERROR | Line indented incorrectly; expected 4 spaces, found 5
322 | WARNING | Line exceeds 80 characters; contains 274 characters
344 | WARNING | Line exceeds 80 characters; contains 82 characters
361 | WARNING | Line exceeds 80 characters; contains 244 characters
--------------------------------------------------------------------------------

Only the one about "If this is a new mail" is important IMO. The comment is on the wrong line I think. It should be inside the next else block, a few lines later.

You can use PAReview yourself online: http://ventral.org/node/562/repeat

jsherman,

Apologies for bringing this full circle back to where it started ... but I've asked a few people, and so far, the general consensus is that inclusion of the sip2_class.inc file inside your module would be in contravention of the licensing guidelines. The motivation behind this opinion is that all code hosted on Drupal.org is carried under a GPLv2 license; and the sip2_class.inc was originally released as GPLv3. I'm told that moving from GPLv3 to GPLv2 is considered a license incompatibility (though the other way would be acceptable).

The next step therefore become i) contacting the author and asking if they would release the code as GPLv2, or ii) leave the code out, and give end users instructions as to where to obtain it, as firebird suggested in comment #9.

Otherwise, the excellent reviews by rudiedirkx look to have addressed any other potential concerns, and a quick scan of the code looked okay, other than the message on line 375 of the main module (appears to have a check_plain() in the string body ... and see http://drupal.org/node/322774 for the recommended way to include links in translatable strings) and a generic uneasy feeling about any code that updates user password entries in the db, just on principle. ;)

Setting back to 'needs work' on the licensing issue.

The code might meet the requirements for an exception, but those exceptions are handled at a higher level than you'll find within the project applications queue ... the exceptions need to be explicitly granted (by the webmasters team, perhaps?).

I think (and I'm speculating here) that the "GPLv2+" in the documentation refers to code that is explicitly released as "GPL version 2 or greater" ... The subtle difference is that this wording includes the GPLv2 *and* GPLv3 licenses, and is thus compatible with the drupal.org licensing, whereas an explicit "GPLv3" license is v3 only (and thus not compatible with the drupal.org licensing).

There's been some fluctuation in this policy over the last couple months, but the latest revision of the documentation includes a comment that the research could not find actual evidence that the policy had changed yet ... I suspect that this item will be put forward to one of the Drupal.org governance working groups for an 'official' decision, once they are established later this year.

On the password change, user_save() would be the preferred way to modify the password. Generally, we nudge people to use the available APIs at every opportunity ... which ensures that your module is protected against underlying database changes if they occur (as the API would be updated at the same time). I probably should have stressed that in the review ... but in this case, I missed one. :)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

• You have a couple of unused global variables reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxjsherman1418010git.
• Great project page!
• I'm not really a fan of the bare PHP in sip2.inc, but I guess it's ok. The user has to choose that method of connection. But it does mean Drupal will execute that code on every page request if that's the chosen method. Maybe you could implement a hook_requirements() that in turn checked for a requirements function in the chosen authentication method?
• Can you add a ilsauthen.api.php file to explain what functions must be implemented by a new driver? See the "hook definition" section of https://drupal.org/coding-standards/docs#functions.

Setting to needs work for the last 2 points. We only need to review one branch, so just 7.x is fine. Nice module!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Seems reasonable, thanks. What kind of feedback are you looking for on the api?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/ilsauthen.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   11 | ERROR | ilsauthen_requirements() is an installation hook and must be
      |       | declared in an install file
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. ilsauthen_menu(): not sue "administer users" is a good permission here, I would rather implement a separate permission to make the distinction clear.
2. ilsauthen_form_alter(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.

But otherwise looks good!

Thanks for your contribution, jsherman!

I updated your account so you can 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.