rikvd [rikvd]

I am changing the status as per previous comment.

  // Build initial URL.
  global $base_url;
  $base_url = str_replace('http://', 'https://', $base_url);
  $base_url = str_replace("www.", "", $base_url);

  if (isset($_GET['destination'])) {
    $options['query'] = $_GET['destination'];
  }
  $options['absolute'] = TRUE;
  $app_url = url('test/authenticate', $options);

The code is altering a global variable, which is not probably a good idea; if it just needs to use a different base URL, then it is possible to pass it in the options accepted by url().

    // Redirect user.
    header('Location: '. $redirecturl);
    exit();

The code should call drupal_set_header() instead of header(), and invoke module_invoke_all('exit') before exit().

    // Login was cancelled.
    watchdog('DigiD', t('DigiD login failed: '. _digid_error($result['result_code'])), array(), WATCHDOG_ERROR);

The second argument of the function is a not translated string, which is then translated from the function; use a placeholder, instead of concatenating strings.

  define('DIGID_HASHKEY', '<replace me>');

The constant doesn't have the correct value.

  define('DIGID_ERRORMESSAGE', 'Er is een fout opgetreden in de communicatie met DigiD. Probeert u het later nogmaals.');

Strings passed to t() or as second argument of watchdog() must be in American English, because the translation is done from American English to the language set as default language.

Every strings used in Drupal user interface needs to be translated. If they require you to report a sentence in Dutch, then you still need to provide the translation of that sentence; this means to pass the American English translation to t().

The following code still needs to be changed as per comment #7.

    watchdog('DigiD', t('DigiD login failed: ') . _digid_error($result['result_code']), array(), WATCHDOG_ERROR);
    drupal_set_message(DIGID_ERRORMESSAGE, 'error');

One function call has been changed, but not the others. The call to watchdog() is not correct (see watchdog()).

version = "6.x-1.0"

That line needs to be removed from digid.info.

  variable_set('bsn_'. $user->uid, $result['uid']);

See http://drupal.org/coding-standards for how the constants, Drupal variables, and functions should be named; that is called namespace respect, and it's a requirement for all the modules.

  t('DigiD account deleted.');
  t('DigiD login failed: !error');
  t('Login is canceled. you are not logged in with DigiD');

All the above sentences should use the present perfect (Login has been cancelled); the sentences should end with a period, and the words should be in sentence case (This is written in sentence case).

I'm interested in this module and wondering if this request is still allive, TIA!

This is a great module; and hosting this module on drupal.org will give a boost to Drupal use in the low countries.
Thanks @rikvd for your effort to release this code!

2 remarks, after I ran through the code from #16

1. Why:
   define('DIGID_HASHKEY', '<replace me>'); wouldn't it be better to use a parameter that is set in settings.php ? That way a separate hash can be used for separate installations (multisite).
2. Is it allowed to add an extra copyright statement to modules that are hosted on drupal.org (see the .install-file)

Instead of a setting in settings.php I'd suggest a variable_set() and a system settings form that enables you to configure the key, once it is set and tested, the user can't change it anymore (or instead of this approach, needs to confirm). Making it able to let the user configure the key lowers the barrier for less tech-savy users like many governemental IT staff to set the key, altering settings.php is a bit more complicated ;).

Good stuff!

niels

DigiD authentication is a prerequisite for using Drupal in any and all governmental settings in The Netherlands. This is an important addition for Dutch users.

A couple of remarks:

- the README and admin pages text has no information that this is a module specifically for the Netherlands: "cooperating governmental agencies" is a bit broad. I think you need to mention this fact as it is important for international users who probably have no use for this module.

- you're now setting the hashkey through the admin pages, you need to change the README line 26/27

- is it still required to show the DigiD logo on the webpage where you sign in? This used to be a requirement and I don't see the logo included in the module or being requested from an external url. This might be worth checking in the DigiD documentation.

I am changing the status as per previous comment.

I've checked the requirements of the latest version of the DigiD communications toolkit (v2.6, 29 jan 2010). Thankfully the quite hideous requirements have been toned down considerably.

Here's what you have to do now. On the page where you link to the DigiD login service:

- you must show this line of text: "Bij [naam_organisatie] kunt u inloggen met uw DigiD."

- you must show the DigiD logo

- the DigiD logo must be clickable and must link to www.digid.nl

The whole line of text in line 157 of the module can be replaced by the above line of text and the logo.

I have attached a zip with the logos in several formats.

Most government websites also provide a link to the DigiD signup page, www.digid.nl/aanvragen/ and I think this is a common courtesy that might be implemented as well. It's not a requirement though.

Other technical requirements (that have no impact on the current module code, just for later reference) are that:

- you may not show the DigiD login pages inside an iframe

- you may not exchange login details with the DigiD authentication service in a way invisible to the user: the user must be redirected to the DigiD login pages on the DigiD servers

I've glanced at the code upon bertboerlands instigation:

Heine: bertboerland: while [certainly] not crappy, there are two issues on a glance: variable_set('digid_bsn_'. [for every] uid
Heine: bertboerland: and the second one is that after user_external_login_register a call should be made to session_regenerate_id to protect against fixation attacks (IMO; as said, just glancing at the code).

I took a look through the code (without testing) and it looks like a nice drupal module. Code standards are fine so this looks good.
Maybe somebody with the digid card can mark this as reviewed and tested but for a CVS account I'm not sure if this is required.

Release blockers

• Is the licence under which the DigiD logos are released compatible with the GPLv2?
• Add an administrative permission for this particular module. Access administration pages may be granted to users that should not have access to sensitive DigiD settings.

Other comments

• I suggest making it easier for site admins to let users log in without DigiD as well. According to README.txt this is now only possible by creating an extra links manually. Also, a GET variable called drupal is rubbish. Something along the lines of digid=0 would be better.
• Change The Digid module requires the <a href="@link">hashkey</a> has been set to The Digid module requires a <a href="@digid_settings">hashkey</a>. This is better English and the placeholder is self explanatory.
• The PHP constant FALSE should be written in capital letters only. See digid.module at line 125.
• Once set the hashkey cannot be changed. I suggest you show the form element at all times, so it can be changed or checked in the future.
• The admin page contains redundant form element descriptions. There is no reason to use descriptions that are basically identical to the elements' titles. If you still think you need those, take a minute to revise the titles instead.
• What does l(t('Request DigiD'), 'aanvragen-digid', array('attributes' => array('title' => t('Request DigiD')))) do? It points to /aanvragen-digid, which is not only a non-existing URL path, but the path is also Dutch instead of American English.
• You may want to use hook_form_FORM_ID_alter() instead of hook_form_alter() for cleaner code.
• Code comments should end with a period. I also suggest putting comments on a separate line instead of at the same line as a piece of working code. See digid.module lines 239-242 for instance.
• digid_authentication_page() sets a message if authentication was cancelled. This is however set as a confirmation/ok message instead of a warning.
• _digid_encode_bsn() looks a little useless. It is just a wrapper for sha1().
• Your doxygen comments could use some work. They do not always comply with the coding standards and you should use parameter names when describing parameters with @param. Example: @param $result_code string instead of @param string.
• Try to keep everything in English. 'burgerservicenummer' is an incorrect parameter description. Be consistent and use the abbreviation BSN everywhere, except in help texts, where you can explain what it means and what it is.
• Change digid_complete() to digid_validate(). This is more self explanatory.

I am changing status as per previous comments.

Sorry, why?

I am sorry; the status I meant to select was needs work.

A lot of contributed modules provide administrative permissions. Access administration apges is a very general permission for pages with often only a few settings. Changing such settings often does not break things. DigiD however has settings that if changed could break your website. They also look pretty sensitive to me, so a separate permission that is to be given to trusted users only looks like a requirement to me.

There is no reason to use form element descriptions that are for 80% identical to the elements' titles. I still recommend you remove those descriptions. The UX team gives the same advice to Drupal developers. If the official documentation encourages such titles and description, then IMO it is 'broken'.

I will review the code tomorrow morning my time (which means between 16 hours).
I apologize for the delay in approving CVS applications; it's not our intent to discourage users from contributing on Drupal.org.

Thank you for your contribution! I am going to update your account.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.