[D7] miniOrange Active Directory/LDAP Module for Cloud Service Providers

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxgauravsood912556275git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Hello.

I reviewed you module. I found few warning there. Is there any specific reason to use raw posted inputs instead of values?

Everything else looks great.

FILE: /var/www/drupal-7-pareview/pareview_temp/miniorange_ldap.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------
84 | WARNING | Do not use the raw $form_state['input'], use
| | $form_state['values'] instead where possible
85 | WARNING | Do not use the raw $form_state['input'], use
| | $form_state['values'] instead where possible
86 | WARNING | Do not use the raw $form_state['input'], use
| | $form_state['values'] instead where possible
---------------------------------------------------------------------

Hey,

I reviewed the module.I ran the automated test and apart from using $form['input'] instead of $form['values'], it seems to be fine.

Thanks.

Hi,

I reviewed your module manually.Below are some of the points which your module follows.

Individual user account
Yes: Follows guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements
Coding style & Drupal API usage
Yes: It follows code and API guidelines.

Thanks.

Review of the 7.x-1.x branch (commit d44e735):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. "variable_get('miniorange_ldap_customer_id', '');": variables defined by your module need to be deleted in hook_uninstall().
2. miniorange_ldap_user_login(): drupal_set_message() is used wrong, the second parameter should be 'status', right?
3. "if ($ldap_login_enabled != 0 and $ldap_login_enabled != FALSE) {": you could use "!empty()" instead.
4. miniorange_ldap_form_user_login_alter(): do not completely replace all the validation callbacks in #validate. That means no additional login module that wants to do some validation will run. You shoud just insert your callback in the #validate list and remove the one from core.
5. miniorange_ldap_login_validate(): doc block is wrong, this is not a hook. See https://www.drupal.org/coding-standards/docs#forms
6. miniorange_ldap_login_validate(): why do you call user_load_by_name() twice here, you can just assign a variable the first time?
7. miniorange_ldap_login_validate(): flood protection is missing to prevent brute force attacks where an attacker tries an infinite number of name/password combinations. See user_login_authenticate_validate() in core on how to use the flood protection system. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
8. class MiniorangeLdapConfig: why is this class no registered in the info file for auto loading? See https://www.drupal.org/node/542202#files
9. MiniorangeLdapConfig: why do you call exit(0) here? That will completely break the workflow for users and show a white screen of death? And you are leaking curl error information to the user here by printing it, use watchdog() instead for logging errors.
10. miniorange_ldap_login_validate(): error handling is missing here, what if you don't get valid JSON here? Then "$response->statusCode == 'SUCCESS'" will throw PHP warnings?

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

Hello,

I have done manual review for your project and I can see all issues pointed by klausi have been fixed.

Looks good to me. Moving it to RTBC

No automated test cases were found, good.

Individual user account
Yes: Follows guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements

Coding style & Drupal API usage
Yes: It follows code and API guidelines.

manual review:

1. miniorange_ldap_login_validate(): why are there 3 identical calls to user_load_by_name() when the result is already in the $account variable?
2. miniorange_ldap_login_validate(): why did you change the code from core with the flood protection? I think the comments were helpful and also the filter for blocked users in the query seems like a good idea?
3. miniorange_ldap_login_validate(): why the hard-coded administrator role here? That role will not exist on every Drupal site? Please add a comment why this is necessary.
4. includes/customer_ldap_config.php: why do you call module_load_include() here just to include files containing classes? Just add the files to the info file, as already said. See https://www.drupal.org/node/542202#files . Then remove the module_laod_include() calls everywhere.
5. includes/customer_setup.php: do not echo errors directly to the page as that can leak private information to the end user. watchdog() should be enough, right? Make sure that all echo and print calls for errors are removed from your code base and turned into useful watchdog() messages instead.
6. "watchdog('miniorange_ldap_curl_error', 'cURL Request Not Sent');": that is not helpful at all. You should pass the exact error message from curl to the log, so that the admin knows what went wrong. Also, the first parameter should be the module name, i.e. 'miniorange_ldap'.

The leaking of error information directly to HTML with print statements is a blocker right now.

I have done manual review for the project and also tested for the issues pointed by klausi and I can see all of them are fixed.

Everything looks good. Moving it to RTBC

manual review:
"watchdog('miniorange_ldap', curl_error($ch));": the first parameter of watchdog() should be a translatable message with placeholders for dynamic variables. It is important that those dynamic variables get sanitized with the correct placeholder, make sure to read https://www.drupal.org/node/28984 again.

I have reviewed change for watchdog function and it looks good . Moving it to RTBC

manual review:

1. "watchdog('miniorange_ldap', "cURL Error: %error", array('%error' => curl_error($curl)));": you are always using the same log message, so it is hard to track down where the actual problem occurred. A better message would be "User authentication failed: %error" or "configuration settings could not be retrieved: %error".
2. '#markup' => '<h3>Enter Gateway URL</h3>',: all user facing text must run through t() for translation. Make sure to check all your strings, for example also in miniorange_ldap_gateway_setup().
3. miniorange_ldap_customer_setup(): doc block is wrong, this is not a hook but a form constructor callback. See https://www.drupal.org/coding-standards/docs#forms
4. miniorange_ldap_customer_setup(): do not print the variables unsanitized to HTML. They come from a third party source, so they should be sanitized before printing. Make sure to read https://www.drupal.org/node/28984 again. I think this is not a security blocker because you are already trusting the third party miniOrange service with your user logins. You probably trust them to not send you XSS content, but this is still an attack vector that should be eliminated.

Although you should definitely fix those issues they are not critical application blockers, otherwise looks RTBC to me.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

OK, since @kalpeshhiran confirmed the RTBC already ...

Thanks for your contribution, @gauravsood91!

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.