Unilogin

Please follow the guidelines here while creating a new project application - http://drupal.org/node/1011698
Adhere to point 6.

Manual Review

install File
Line 48 - What is "$t" ?
Line 51 - Typo - "check_makrkup"

hook_unistall queries - please use %s for strings and 5d for integer values.
Capital "FROM" in first query.

.module file
Gives me error at line 218
is this "unilogin_form_user_profile_form_alter" form alter, if so, please recheck
Use hook_form_alter or hook_form_FORM_ID_alter.

Rectify these and will review it back again...

And your link to GIT repo is still incorrect.
It should be - http://git.drupal.org/sandbox/miksan/1674092.git
please update.

Correcting language

Git repos path updated

The system requirements for Drupal 7 state PHP 5.2.5 or higher (5.3 recommended) and PHP 4.4.0 or higher (5.2 recommended) for Drupal 6. If your module depends on PHP 5.3, I would add a note to that effect in the README and project summary.

[updated comment to include Drupal 6 requirements]

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

• 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. You have to get a review bonus to get a review from me.

manual review:

1. Project page is too short, see http://drupal.org/node/997024
2. unilogin_install(): no need to use check_plain() here as no user provided input is involved. Same for check_markup().
3. unilogin_menu(): no need to use get_t() here, this will not run during the installation, so t() is fine.
4. unilogin_init(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
5. _unilogin_authenticate(): if you depend on PHP SOAP you should check for that in hook_requirements(). Example (D7): http://drupalcode.org/project/wsclient.git/blob/refs/heads/7.x-1.x:/wscl...

Running out of time right now, will followup later.

Oh of course, t() should not be used in hook_menu() at all, I overlooked that.

Hi Mikkel,

These are some remarks after a manual review of your code:

file unilogin.db.inc in function _unilogin_db_save_unilogin_id:

22  if (empty($uid) || empty($unilogin_id)) {
23    return;
24  }
25  if (empty($unilogin_id)) {
26    db_query("DELETE FROM {unilogin_users} WHERE uid = %d", $uid);
27    return;
28  }

On line 22 you check whether $uid or $unilogin_id is empty, if so then you return. However on line 25 you check again if $unilogin_id is empty, and if so you execute a DELETE query in the database. It seems to me like the code on lines 26 and 27 will never be executed since if $unilogin_id is empty, you will already exit the function before the line 25 will be evaluated.

file unilogin.db.inc in function _unilogin_db_load_ticket:

90  return db_result(
91    db_query("SELECT * FROM {unilogin_tickets} WHERE name = '%s'", $name)
92  );

According to the db_result function specification, you should only use this function if exactly one field is being selected. You select however the entire row from the table.

file unilogin.module in function unilogin_init:

In order to avoid potential security issues, you should do some filtering on the parameters user, timestamp and auth. You use these parameters without checking whether they contain any non-authorised characters. I would recommend to do some checking here by means of regular expressions. For example: check whether the timestamp really is a timestamp, etc. Strictly speaking this is not a Drupal requirement, but since you pass these variables on to the SoapClient without checking them, there might be a potential character injection vulnerability here...

file unilogin.module in function _unilogin_authenticate:

173 drupal_goto($_GET['q'] != 'node' ? $_GET['q'] : '');

You probably have a security issue here. drupal_goto also accepts external URL's. In order to avoid phishing attacks or insecure redirect vulnerabilities, I would recommend to do some checking on the value $_GET['q'] such as checking whether it really is an internal URL.

file unilogin.module in function _unilogin_user_login_validate:

353 _unilogin_authenticate($values['name']);

You call the _unilogin_authenticate with only 1 parameter, whereas this function actually requires 3 parameters.

As a last remark: perhaps you should mention to potential users that in order to effectively use the unilogin module, that they must enable the block Unilogin to be shown together with the login form. This could be added on the project website or in the README.txt file.

http://drupalcode.org/sandbox/miksan/1674092.git/blob/refs/heads/6.x-1.x...

I would probably have a bit of text there in case the README.txt was removed on the production site, with a file_exists() test.

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

Added another link to a manual review

Nice module! I've created a SSO module for a specific case myself, and know how hard all the validations and stuff can be. I've found a couple of things that I think could be better though.

Manual review:
Unilogin.module
require_once 'unilogin.db.inc'; makes Drupal load this file every page-load. Isn't it possible to only load it on certain page loads (like when an anon-user loads the site and the $_GET parameters are given)?

hook_help() should return a localized string, so you need to add a t() there.

On line 199 you have this code:

drupal_goto($current_uri != 'node' ? $current_uri : '');

If this is meant to redirect users to '' instead of 'node' (because they are the same by default) you should use
drupal_is_front_page(), because other pages can also be the frontpage.

You send the secret key ($vars['secret']) all the way down to the template file. This itself isn't a security risk, but I'd try to keep those variables as much in one place as possible (like one function to create the auth string).

unilogin_form_user_profile_form_alter() on line 319 should have the comment Implements hook_form_FORM_ID_alter()

On line 326 you wrote this:

'#title' => 'UNI Login id',

The title should have a t() around it.

On line 623 the code could be simpler

return strtotime($timestamp)
  <= strtotime(gmdate('M d Y H:i:s') . " - $minutes minutes");

return $timestamp  <= (time() - ($minutes * 60)));

unilogin.install
For the description of the uid, you should make a reference to the users table (eg. The unique identifier for a user as defined in {users}.uid). This will help users of the schema module.

    'primary key' => array('uid'),
    'unique keys' => array(
      'uid' => array('uid'),

A primary key is always unique, there is no need to create a second index on that field.

That was all I could find.
Good luck with the module!

Thanks Yaron for you valuable input! Those points should definitely been looked at, but I don't see a major application blocker here, so I would say this is still RTBC. Revert the status if you disagree.

@klausi I was thinking the same, but thought that maybe those points would be lost if the project is accepted. I think the module looks better than a lot of other modules already on d.o.

@miksan Ah yeah missed the timezone.
About the inclusion, wouldn't it be possible to only put the require_once just before where those functions are called?

Rather than poking around in the cache table's I'd recommend you to use cache_clear_all('variables', 'cache'); to reset the variables cache.

t(file_get_contents($readme_file))
         : t($reduced_readme)

Never ever put variables into t(), the translation system at localize.drupal.org is not able to detect translatable strings this way and they'll be untranslatable. Always use static strings within t()!

The maximum colums per line of 80 rule is ONLY for comments! Don't break lines just to make them shorter than 80 columns if your code looks ugly because of it!

In unilogin_preprocess_unilogin_link() your building a query string manually, what about using drupal_query_string_encode()?

I highly recommend fixing these issues but these are no blockers for me.

Thanks for your contribution, miksan!

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:

• 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 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.