simpleSAMLphp Authentication

This project application came from discussions on the SimpleSAML user group. A release is needed to further the development of the project.

After a code and security review -- this is an authentication module -- we should quickly approve.

@geekwisdom have you run the module through Coder yet? http://drupal.org/project/coder

Remove // $Id$ and similar tags, no longer needed with git.

Removing version and packaging script info from .info file, that is added by git.

Copyright information needs to be removed. Licensing will be added by git under GPLv3 and copyright information should not be found in your module file.

Author credits should not be given in the module file.

Please review drupal coding standards at http://drupal.org/coding-standards

Indentation is two spaces, no tabs

Inline comments should be of the // variety, beginning with a space, capital letter, and ending with a period.

Files need an @file comment at the top.

Strings in drupal_set_message() should be run through t()

Booleans need to be in all caps FALSE

411 drupal_set_message('Please upgrade SimpleSAMLphp. You are using '.$saml_version,'warning');
you should be using placeholders here.

And your commit should not have commented out debugging code.

There are too many coding standards issues to point them all out, but most should be caught by coder. Just run it through coder on minor (most) and set this issue back to needs review once complete.

> A release is needed to further the development of the project.

Just curious, why is this? We don't normally prioritize applications unless you get lucky, fall through the cracks, and end up waiting for several months at our own fault.

Subscribing.

Having a stable release on d.o. will bring the project out from behind the Google Group (of roughly a dozen active folks) and give it more real-world testing.

We've also been pushing @geekwisdom to make a proper release, so I don't think our process should be a barrier.

subscribe.

@agentrickard got it! Sounds good.

That sounds fine. Some Coder warnings can be ignored for those reasons, of course. As long as there is no real security threat. I'll take a look at this again shortly.

What are the simplesamlphp_auth.module~ and similar files in the repository for?

In hook_perm() you are declaring the same permission twice.

Variables are removed in hook_uninstall() good.

Implements hook_help() always a plus!

$failure = 'SimpleSAMLphp could not be found at ' . $basedir . '....
You wrap the variable in t(), but this is not correct, the string itself needs to be ran through t(), it should not be done in a variable. You can do

$failure = t('SimpleSAMLphp...'); and $basedir should be inserted via placeholders in the function like so:

$failure = t('SimpleSAMLphp could not be found at %basedir...', array('%basedir' => $basedir));

$output .= 'I am drupal, here me roar';
Other strings are nut run through t() at all, which needs to be done.

With all the messages being passed for use in watchdog like this:
607 _simplesaml_auth_debug('Evaluate rolemap: ' . $rolemap);
These can't be translated? You can pass variables for translating instead of concatenating if you want it to be translatable http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/watchd...

Hook_exit() should probably be removed in a final version if it is doing nothing.

Also, all functions need to be documented. Doxygen comments are missing from everything except hook implementations.

Database queries look secure and coding standards are followed well now.

Files have @file docblocks : )

README file looks good as well. So I guess the most prominent issue I see is a lack of proper use of translation capabilities.

Let's get this released, then.

I agree that this module is ready for a release, even though it has some coding standards issues.
Attached is a patch that will handle most of the problems for you, although I added at least two @TODOs.

I've granted you full project creation rights. Use this ability carefully!
Thank you for your patience.