[D7]PasswordResetTabs

Manual Review

I have just install your module and verify i have some points:

• I don not know how to use it. You do not implement help_hook : https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
• I can not set permission for your module which can do with permission_hook https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

Hello Saurabh,
The "core" key is used twice in password_reset_tabs.info, please correct.
core = 7.x
core = "7.x"

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.

Hi Saurab,

module_load_include('inc', 'user', 'user.pages'); is used at both header of file and in function definition of password_reset_tabs_pass_reset_page();

Review :-

1. Don't know why you have

      module_load_include('inc', 'user', 'user.pages');
    

in module starting ?

2. Few methods returning string / message without using t(). Example - password_reset_tabs_validation().
3. No documentation for $option array in password_reset_tabs_pass_reset_page().

Also I checked the reviews done by you and most of them looked to me same as pareview bot. So I am removing the PAreview bonus tag. Do review 3 more applications.

Thanks

Hi! saurabh.tripathi.cs

in: password_reset_tabs_menu()

EDIT: titles in hook_menu() must not be translated with t(), so this is fine.

Automated Review

	FILE: /var/www/drupal-7-pareview/pareview_temp/password_reset_tabs.module
	---------------------------------------------------------------
	FOUND 2 ERRORS AFFECTING 2 LINES
	---------------------------------------------------------------
	 141 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
	 150 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
	---------------------------------------------------------------
	

Manual Review

Individual user account

Yes: Follows the 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 / No: Does not follow 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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Please add spacing between functions as per the Drupal standards.
2. You can allow to change password of the all existing users on the site for users with certain roles (admin roles).

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

@saurabh.tripathi.cs, I'm just mentioning that "admin" role can have privilege to change other user's passwords too.

On the other hand, it's your design. You can pick up my suggestion as a feature request.

manual review:

1. password_reset_tabs.module: why do you module_load_include() in the global scope? That means the file is loaded on every single page request, which is not necessary. You should only include it when you actually need it in function bodies.
2. password_reset_tabs_new_password: no need to call drupal_render() here, just return the $form render array. Drupal core will render it later for you.
3. This module has a severe security vulnerability. As part of our git admin training I'm assigning this to joachim so that he can take a look. If he does not have time I will post details about the security vulnerability in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

OK, posting the details about the vulnerability now:

password_reset_tabs_pass_reset_page() is vulnerable to access bypass problems. Any anonymous attacker can login as arbitrary user by just going to the page /user/reset/1/f/a/i/l and BOOM! They will be logged in as user 1. You need to make sure that users can only login if their password reset link is valid.

Manual Review:
In password_reset_tabs.module
Line: 138:
$timestamp <= $current && $account = reset($users)

We should place out of if statement, and make sure $account not empty before you use it: $account->login
like this:

$account = reset($users)
if($timestamp <= $current && !empty($account)){
//
}

Line 93,175:
Becarefull with arg(2) and make sure it not empty = if(isset(arg(2))...

$uid = arg(2);
$user = user_load($uid);

You are using user_load https://api.drupal.org/api/drupal/modules!user!user.module/function/user_load/7, It may be return FALSE,
$user->uid will cause error.

Please check $user is not empty. 

In password_reset_tabs.module
Line: 138:
$timestamp <= $current && $account = reset($users)
you should check the !empty for $account before use otherwise it's gives error while accessing the $account object.
So add validation for $account variable.
like kandy-io said,
$account = reset($users)
if($timestamp <= $current && !empty($account)){

Hi saurabh.tripathi.cs,

In "password_reset_tabs.info" file has script:

scripts[] = js/password_reset_tabs.js

Please add this file into git repos, or if it's not use, please remove this line.

Removing review bonus tag, you have not added 3 more reviews to the issue summary? Please only add it back when there are at least 6 manual review comments in the issue summary.

yes, please list all your manual reviews in the issue summary. Thanks!

Removed automated reviews with no manual source code review.

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

• ESLint has found some issues with your code (please check the JavaScript coding standards).

js/password_reset_tabs.js: line 6, col 1, Error - Missing "use strict" statement. (strict)
js/password_reset_tabs.js: line 10, col 68, Error - Missing space before function parentheses. (space-before-function-paren)
js/password_reset_tabs.js: line 10, col 70, Error - Missing space before opening brace. (space-before-blocks)
js/password_reset_tabs.js: line 10, col 83, Error - Missing whitespace after semicolon. (semi-spacing)

4 problems

./README.txt:32: acces  ==> access

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. "user_pass_rehash($account->pass, $timestamp, $account->login)": this function call misses the 4th parameter, which is important for security reasons. This was fixed in one of the more recent core releases. It is important that you get the security APIs right for critical functionality like this. Make sure to test your module with the latest version of Drupal core, you should get PHP warnings when you miss the 4th parameter on that function.
2. password_reset_tabs_pass_reset_page(): why do you use arg() here for the $uid when it gets passed in as parameter to the function? Should not be necessary?
3. password_reset_tabs_done(): why do you destroy the session here? Please add a comment.
4. If I go to /password_reset/new_password as anonymous user I see a user registration form and can create new user accounts. That works even if I have set my Drupal site set to "only admins can create accounts", so this is a security issue. I think you should deny access or redirect anonymous users on that page.

Please make sure that you test your module properly against all scenarios, since this very security sensitive functionality.

Hi Saurabh,
I installed, tested and ran through your code. Everything looks great! This can be used as a new UI for changing Drupal password.

Reports

Online Automated testing report
http://pareview.sh/pareview/httpgitdrupalorgsandboxsaurabhtripathics2403787git

Code is clean.

Manual review result:
Individual user account
Yes
No duplication
Yes
Master Branch
Yes
Licensing
Yes
3rd party assets/code
Yes
README.txt/README.md
Yes
Code long/complex enough for review
Yes : Code is long enough for review.
Secure code
Yes : Looks secured
Coding style & Drupal API usage
Yes : Looks good and follows drupal coding standards.

Thanks for your contribution, Saurabh.

scripts[] = js/password_reset_tabs.js

This will load your js file on every page whether it is required or not.
It should be loaded on required pages only.

Thanks for your contribution!

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.