[D7] Password Reset Form Tweaks

Your git clone command contains your username and is for you only. It will not allow others to clone your project.

Had it worked it would have created a clone named "2298161" instead of a clone bearing your project's name.

Please read the: Project application checklist before requesting a review.

For instance:

2.1 Ensure the repository actually contains code.

On drupal.org we use GIT repositories to share code. If you haven't yet pushed code to your project's repository because you don't know how, please have a look at the documentation about using GIT on drupal.org.

Your repo is currently non-existing: http://cgit.drupalcode.org/sandbox-EthanT-2298305/commit/?id=24afd04

Well, it is available now, but it was not when I checked last time. Reviewers should be able to check out the project on the web as well as via git.

Btw, you need to remove the license blurb fro README.txt. Drupal will add the appropriate version automatically during packaging so your repository should not include it. See also: licensing requirements.

1.1 Ensure your application contains a repository and project page link.
Yes.
1.2 Ensure your project is not a duplication.
There might be, but I haven't seen any.
1.3 Ensure you don't have multiple applications.
Yes
2.1 Ensure the repository actually contains code.
Yes
2.2 Ensure you are working in a version specific branch.
Yes
3.1 Ensure the project does not contain any security issues.
I don't see any obvious security problems except for wrapping HTML in t(), but since it calls check_plain, they should be stripped.
4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.
It doesn't.
4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.
It doesn't.
5.1 Ensure the project page contains detailed information.
The information isn't that advanced, but the use-case seems pretty straightforward.
5.2 Ensure the repository contains a detailed README.txt.
The information isn't that advanced, but the use-case seems pretty straightforward.
5.3 Ensure the code contains a well-balanced amount of inline-comments.
Contains inline comments, but some of these could be expanded to help reviewers understand what is being done.
6.1 Run an automated review and ensure there are no major issues.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
21 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

The above is pretty trivial.

7.1 Ensure you are using Drupals API correctly.
Aside from the specific issues I've included below, everything seems in order.

Specific issues:

I don't really understand why you are using variable_del('new_user_load') and variable_set('new_user_load') in password_reset_form_tweaks_form_alter. variable_del and variable_set should be used for site_wide variables, but it looks like you're using them to store data on an instance-by-instance basis.

HTML should not be used in t() as you do in password_reset_form_tweaks_form() on line 131. https://www.drupal.org/node/2117055#comment-8838743

In password_reset_form_tweaks_form(), you have an array of elements, which could be laid out one line per element, as is required by the drupal coding standards.

You could also alter your description of password_reset_form_tweaks_message to use a replacement like "@allowed_tags" and then pass '@allowed_tags' => implode(', ', $allowed_tags)

@EthanT I'm afraid I have to agree with @gwprod on the variable_get()/variable_set() issue. In reviewing the code, it looks to me like you have a potential race condition. If two (or more) users hit the password reset form at the same time, isn't it possible that user A will cause the variable to be set to their data, but, before the code gets to the token substitution, user B has set THEIR data in that variable, resulting in user A seeing user B's data. I know the code execution looks atomic, but you need to consider the multi process/multi threaded nature of most web servers.

]I'm not marking this as "Needs Work" because I'm certainly willing to be proven wrong. Besides, I'm not positive this is an actual blocker, though it certainly IS something I'd like to see resolved before a production version was released.

I'll review your code again, it may be that there is no need for a global variable at all.

I tested this module manually, and so far this is what I've gotten:

Notice: Trying to get property of non-object in password_reset_form_tweaks_tokens() (line 87 of

I think this problem lies in the fact that you are doing

// Grab our user acct info for the name token.
  global $_user_load;
  $user = $_user_load;
  $user_name = $user->name;

Before testing to see that

$type == 'password_reset_form_tweaks_custom'

Also, its unclear if this message should be generated on the confirmation dialogue from the reset link or on the login form after confirmation, or both.

The global user object is available on the second step, but not on the confirmation dialogue.

Regarding using the global variable $_user_load, you can pass data to the token_replace command, so you can do something like this:

$form_text = token_replace(variable_get('password_reset_form_tweaks_message'), array('user' => $_user_load);

And in password_reset_form_tweaks_tokens(), you can retrieve it by doing

$_user_load = $data['user'];

No globals required.

I don't see any potential for a race condition in the method I specified, since the request executes through and there is no sharing of data between requests in the way caused by the variable_set/get

By using the variable_set/get method, you were opening up to a potential situation where 1 user was setting the variable, which was then being read by another user, which is a security issue.

There is no good reason to use a global variable here. But that is my simple opinion.

Looks good!

@EthanT, your module looks good to me as well. As I reviewed your code and also installed on my local machine found, it throwing following warning after message submission on admin/config/password-reset-form-tweaks page.

Theme hook token_tree_link not found.
Severity: warning

You can also check the same on admin/reports/dblog page.

Thankyou!

Good catch, er.pushpinderrana. This module should have token as a dependency

I tested this module and working properly, good work =)

Maybe you could add the hook_help.

@EthanT, Thanks for your contribution.

I did manual review of your module and here are my notes:

1. At line 18 and 21, you are using user_load function, I think you can use this function single time instead twice.
2. I would recommend you, you must write a hook_help() function for your module.
3. I think in following code, you must check whether user have uploaded any picture.

$admin_greeting_thumb_path = array(
      'path' => image_style_url('thumbnail',
              $admin_pic_load->field_user_picture[LANGUAGE_NONE][0]['uri']),
    );

If yes then following two line would be inside that if statement:

$admin_greeting_thumb = theme('image', $admin_greeting_thumb_path);
variable_set('admin_greeting_thumb', $admin_greeting_thumb);

Rest looks good to me. Once you will do these change, I'll move to this RTBC :-)

I am not moving this to "Need Work" because these issues are not blocker, so you please do these changes.

Thanks again!

@er.pushpinderrana:

His module is loading two separate users, so two calls to user_load makes sense.

@EthanT:

I can see no good reason to create a variable for the admin thumb, then retrieve it in password_reset_form_tweaks_tokens. Either pass it as a replacement to token, or generate it inside password_reset_form_tweaks_tokens.

Is 'field_user_picture' a guaranteed user field? Is thumbnail a guaranteed style?
Look at https://api.drupal.org/api/drupal/modules%21user%21user.module/function/...

His module is loading two separate users, so two calls to user_load makes sense.

@gwprod, thanks for correcting me, I missed that.

Reviewers, please remember to set the issue status when you are done with a review. Set it back to Needs Work if you find a major issue that needs to be fixed before the application get approved, or Reviewed and Tested By Community if you don't see any show stoppers. The Drupal.org project application workflow page has more information on this.

@EthanT

I don't think so, personally.

manual review:

1. password_reset_form_tweaks_form_alter(): if you are only targeting one form you should use hook_form_FORM_ID_alter() instead.
2. "Set the value for the acct that will be passed to the token.": you mean account instead of acct?
3. "$admin_pic_load->field_user_picture[LANGUAGE_NONE][0]['uri']": this will throw PHP notices if no admin picture is set on the admin user account, right?
4. password_reset_form_tweaks_form_alter(): why do you perform a variable_set('admin_greeting_thumb', $admin_greeting_thumb); here? Please add a comment.
5. "variable_set('admin_greeting_thumb', $admin_greeting_thumb);": All variables defined by your module need to be removed in hook_uninstall().
6. variable_set('admin_greeting_thumb', $admin_greeting_thumb);: all variables used by your module need to be prefixed with your module's name to avoid name clashes with other modules.
7. password_reset_form_tweaks_form(): doc block is wrong, this is not a hook. See http://drupal.org/node/1354#forms
8. "'access arguments' => array('access site configuration'),": that permission is not defined in drupal core. Did you mean "administer site configuration?"
9. "'#default_value' => filter_xss(...),": filter_xss() is wrong here, #default_value will be automatically escaped by the form API. Make sure to read https://www.drupal.org/node/28984 again. password_reset_form_tweaks_form_alter() is the place where you must apply the sanitization (when user provided content is printed to HTML). Currently you don't sanitize there, so this would be an XSS security issue, but since you use a permission right now that does not exist only user 1 is able to input malicious stuff - and that user has all site-owning permissions anyway.

Hi EthanT,

Automated code check OK.
Not a duplication.
Repository OK.
Security OK.
Licensing OK.
Documentation OK.
Code style OK.
API use OK.

So I will proceed to do a manual testing of the functionality:

When the admin account doesn't have a picture, and I use tokens for the reset message, when the tokens are displayed, I get:
Fatal error: Unsupported operand types in includes/theme.inc on line 1087

Why do you use such a restricted set of tags? I can't use paragraph. You should get the tags from filter_xss_admin() function instead.

Edit: forgot to mention before, I didn't see the configuration page "Password Reset Form Tweaks Settings" in admin/config, while using the default admin theme. I think you should move the menu callback from "admin/config/password-reset-form-tweaks" to "admin/config/people/password-reset-form-tweaks".

Also a suggestion:
Rather than using the administrator account picture, it would be more useful to upload a picture for this specific purpose.

Looks OK to me.

Assigning to myself for next review.

Automated Review

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template. It's short, but I think approprite for the current state of the module.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. Explicitly tested XSS on the forms. Security issues in #41 addressed w/ proper use of sanitization functions.

Coding style & Drupal API usage

(+) The module name / short name is pretty bad :) I would think about a better one. I think you could generalize this to some of the other user forms that anon users see
so, maybe User Forms Help (user_forms_help) would be better?

A hook_help() would be nice.

I think a separate permission for this would also be handy. With the organizations I work with, this type of text wouldn't necesarilly be handled by someone
with full admin rights on the site.

In password_reset_form_tweaks_form(), your variable get needs a default value.

(+) You need a hook_uninstall() to delete the variable that you set in password_reset_form_tweaks_form().

You can move password_reset_form_tweaks_form() into a separate include, and then set the 'file' value on the menu $item. This helps with memory usage on live sites.

The docblock is wrong for password_reset_form_tweaks_form_user_pass_reset_alter(). It implements hook_form_FORM_ID_alter().

In password_reset_form_tweaks_tokens(), use REQUEST_TIME instead of time().

(+) password_reset_form_tweaks_tokens() should really use the Field API to get at values. See https://api.drupal.org/api/drupal/modules!field!field.module/group/field/7, and field_get_items and field_view_value.

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.

Blocking issues from #41 have been addressed. Been sitting at RTBC for a while now, so...

Thanks for your contribution, EthanT!

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.

Congrats on Fixed, @EthanT

Congrats @EthanT , Great thanks for your contribution!