Belgian eID IDP integration

Results from pareview:

Review of the 6.x-0.x branch:

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code Sniffer has found some code style issues (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. Go and review some other project applications, so we can get back to yours sooner.

FILE: ...l-7-pareview/sites/all/modules/pareview_temp/test_candidate/beididp.inc
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 12 LINE(S)
--------------------------------------------------------------------------------
  74 | ERROR | Missing parameter type at position 1
  76 | ERROR | Missing parameter type at position 2
 130 | ERROR | Missing parameter type at position 1
 133 | ERROR | Data type of return value is missing
 213 | ERROR | Missing parameter type at position 1
 216 | ERROR | Data type of return value is missing
 268 | ERROR | Missing parameter type at position 1
 271 | ERROR | Data type of return value is missing
 319 | ERROR | Missing parameter type at position 1
 400 | ERROR | Missing parameter type at position 1
 402 | ERROR | Missing parameter type at position 2
 405 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------

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

I did some testing, but without https, and it works:

1. I created a new account using my eid card
2. I logged out and used my eid card to login again

Some remarks:

• You're using hook_form_alter (ex beididp_form_user_login_block_alter), but the comment does not mention it: Implements hook_form_FORM_ID_alter().
• beididp_cron: don't log a message if $rows = 0?
• beididp_hash: the comments says 'change this', why not make it a setting?
• beididp_admin_validate: there's a space between ! and (
• beididp_checkrole_login_validate: exclude UID 1?
• beididp_profile_admin: uses for ($i = 1; $record = db_fetch_object($result); $i++), why not a while loop?

you missed one in beididp_admin_validate: if (! valid_url($idp)) {

About the "change this": now the values are in code, and people can not change the value inside the module, because if they upgrade they loose their values and all registered accounts stop working. If people don't want to store the value inside the database they can add it to their settings.php like this:

  global $conf;
  $conf['beididp_hash_prefix'] = 'myprefix';
  $conf['beididp_hash_suffix'] = 'mysuffix';

This looks good to me, marking his as RTBC so someone else can have a look, also adding a tag for security review.

@attiks: we use the security tag only for applications where we find security issues, so I'm removing it from this one.

manual review:

• beididp_perm(): doc block is wrong, see http://drupal.org/node/1354#hookimpl . Also on other hook implementing functions.
• _beididp_form_alter(): doc block does not tell anything.
• "@db_fetch_array(...": why do you need the "@"?
• "define('OPENID_NS_AX', 'http://openid.net/srv/ax/1.0');": all constants should be prefixed with ypur module's name to avoid name colissions with others. Same for OPENID_NS_PAPE and possibly others.
• "drupal_set_message($result->error, 'error');": the result is untrusted input from the outside, so you should sanitize it.

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

manual review:

• beididp_install(): no need to set variables, you can always use defaults in variable_get() anyway. Same in beididp_profile_install().
• still found several "@" brefore db_query() in your code.
• beididp_user_add_form(): the closing ")" of an array should be on a separate line, see http://drupal.org/node/318#array
• "db_query("INSERT INTO {beididp_association} ...": why can't you use drupal_write_record() here? Applis also for other insert/update queries.
• beididp_checkrole_install(): empty function, so remove it.
• beididp_checkrole_admin(): the role name is user provided data, so it needs to be sanitized before printing in #title. See also http://drupal.org/node/28984
• "Implements beidip_auth_request()." should be "Implements hook_beidip_auth_request().
• beididp_profile_admin(): sanitization again: #title and #options need to be sanitized to avoid XSS vulnerabilities.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please don't remove the security tag, we use that for statistics and to show examples of security problems.

Can you run your code through coder again, there are still some warnings:

• replace all tabs with spaces
• indentation problems
• leading white spaces
• return($name . $prefix); can be changed to return $name . $prefix;

There a strange sentence inside the README.txt: The IDP applet can work with our without eID middleware, so there is no need to install it on the client.

Inside beididp_help: if the string inside t() contains single quotes, you better use double quotes around the string, this will make it easier for translators because they don't have to escape the single quotes.

Review of the 6.x-0.x branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• 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:

• "$form['account']['name']['#default_value'] = check_plain($nickname);": #default_value will be sanitized for you, no need to use check_plain() here. See http://drupal.org/node/28984 . Also in other places.
• beididp_profile_admin(): no need to use check_plain() for #options on 'select' boxes.
• "return db_result(db_query("SELECT COUNT(*) FROM {users} WHERE name = '%s';", $name));": the ";" at the end of the query is not needed.

Sorry to bump this back to "needs work", but you need to know where to use check_plain() and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Great, looks RTBC to me now.

Assigning to tim.plunkett, as he said he might have time to review this.

This looks great, and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.