[D7] User External Invite

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.

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

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

Hello afinnarn,

Here is what I found looking at your module.

File - user_external_invite.module
1) The submit function for user_external_invite_settings_form() is not used, so better remove it from the module.

2) Its good practice to follow every drupal_got() statement with 'exit'.

3) In _user_external_invite_pending_invites() line 232 you can do away with the "->fields('i');" in your query, apparently you are returning all columns of the record, in which case you can take off the code in line 232.

4) Write function to delete the variables defined in your module on uninstalling the module.

SUGGESTION:
1) The module looks too long in terms of the lines of code for the functionality, you can think about moving all the non hook functions to other files.

2) What will happen if say after two days I change the 'roles' that are to be assigned to the invited users in which case the users who already are invited into the system has to be updated manually (forget if the number of user's are too many).

3) You can also think of adding a permanent log of all the users who are invited and eventually accepted the invite.

@afinnarn

I observe some points while reviewing your module.

1. Remove following statement from info file
   package = People
2. Use t() function in drupal_set_title at line no. 275 .module file
   drupal_set_title('Invite New User');

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

@afinnarn,

I was trying to know what this module does and how it is used but didn't found "hook_help()". Please add "hook_help()".

Thanks

Hi afinnarn,

1. It would be nice to have a drush alias for the command user-external-invite-send.
2. You should move the admin setting forms/page callbacks to a separate user_external_invite.admin.inc file.
3. There is no need for a separate user_external_invite_form_access access callback. Just do it like you did for the first menu item 'admin/config/people/invite'

Would look like:

  $items['admin/people/invite'] = array(
    'title' => 'Invite user',
    'page callback' => 'user_external_invite_page',
    'page arguments' => array('user_external_invite_form'),
    'access arguments' => array('invite new user'),
    'type' => MENU_LOCAL_ACTION,
  );
  $items['admin/people/invite/cancel/%'] = array(
    'title' => 'Cancel user invite',
    'page callback' => 'user_external_invite_cancel_invite',
    'access arguments' => array('invite new user'),
    'page arguments' => array(4),
    'type' => MENU_NORMAL_ITEM,
  );

Best regards

Automated Review

Looks good for me

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] 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. / No: List of security issues identified.]

Coding style & Drupal API usage

1. (*) After successful installation of this module, i went people invite form there am facing issue like roles field is required.(roles radios buttons are not pre-populating.)
2. Am getting list of warning in people invite page
   1.Warning: Invalid argument supplied for foreach() in user_external_invite_form() (line 279 of sites\all\modules\user_external_invite\user_external_invite.module).
   2. Notice: Undefined variable: role_options in user_external_invite_form() (line 286 of \sites\all\modules\user_external_invite\user_external_invite.module).
3. ...]

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.

Sounds like the PHP warnings should be fixed before we continue reviewing here, can you check @afinnarn?

A few general thoughts that definitely aren't blockers:

• The table in _user_external_invite_pending_invites_table does not have a limit on it, so a site that uses this feature a lot will ultimately have problems with this page responding
• Similarly, the query in user_external_invite_cron may run into issues when there are lots of records. You may want to add an index on the expire column so that query can run faster
• The commit messages you've used recently don't match the standard format so as you do more work with issues, please be sure to follow that

I noticed 2 security issues:

• The url admin/people/invite/cancel/behat%40example.com is vulnerable to a CSRF vulnerability. It's possible for a malicious user to trick an admin into deleting an invite for a user by causing a logged-in user to visit the url for many emails (e.g. using img tags)
• A much lower priority, but important principle to understand: there is XSS in the table created by _user_external_invite_pending_invites_table. An attacker who can get javascript into a role name can get it to execute. Attached is a screenshot where I created a role with "><plaintext>. This issue isn't a huge problem as modifying the role to contain malicious characters requires advanced permissions.

I've added the security tag to this review which we use for statistical purposes. Please leave it even after you fix the security issues.

Edited to clarify: first points definitely aren't blockers

Hi afinnarn,

I have a few suggestions for your project:

1) Mention about passing and return parameters in your functions.

2) Use space between a line and comment for another line to understand code easily.

For Example:

//This is for $x
$x = 5

//This is for $y
$y = 10;

3) Do not need to take extra variable.
For example:
$role_name = $user_role->name;
$role_name = ucwords(str_replace('_', ' ', $role_name));

This can be done in a single line: $role_name = ucwords(str_replace('_', ' ', $user_role->name));

4) You can include mail.inc and write mail related functions in this file instead of module file.

Thanks, afinnarn, any chance you'd review 3 project applications to get the review bonus?

I was concerned that user_external_invite_pending_invites_form_submit() calls a function that's passed in from the admin form - it could have opened the door to someone with access to that page to change the form value to "user_delete_multiple" and then change the submitted values to "1" and then delete the main admin account, but Form API checks the value against what is defined for the select field's #options list and blocks the submission if the value doesn't exist. So, you *almost* had a major security problem, but yay for Form API :)

A few quick things:

• You should pass in a portion of the function name in user_external_invite_pending_invites_form() and not the whole function, just so you stop scaring people who read the code ;-) In general you should never simply call call_user_func() on a string passed in from a form.
• There are some minor errors that show when the module's Invite page is first loaded:
  • Notice: Undefined variable: rows in user_external_invite_pending_invites_form() (line 155 of user_external_invite.module).
  • Notice: Undefined offset: 0 in user_external_invite_pending_invites_form() (line 116 of user_external_invite.module).
• There are a few minor things in the syntax, e.g. the README.txt has a few lines over 80 characters, some other unnecessary space, some function args that are unnecessary, but this are minor quibbles.

@afinnarn: If you make the first change I'd be ok marking it RTBC, the others could be dealt with later.

but Form API checks the value against what is defined for the select field's #options list and blocks the submission if the value doesn't exist

FWIW, yes, Form API protects against some kinds of "semantic forgery" vulnerabilities like that.

I think another solution for point #1 is to validate them against a list of known good values (if that's possible).

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/user_external_invite.module
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  --------------------------------------------------------------------------
   179 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   203 | WARNING | Messages are user facing text and must run through t()
       |         | for translation
  --------------------------------------------------------------------------
  

• 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 reivew:

1. user_external_invite_schema(): foreign key to the user table for uid is missing, same for the role ID. See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
2. user_external_invite_entity_info(): an entity type name is a machine name, so it must not contain dashes.
3. user_external_invite_cancel_invite(): seems to be unused and should be removed for now?
4. "'#suffix' => empty($rows) ? 'No pending invites.' : '',": all user facing text must run through t() for translation. Please check all your strings.
5. user_external_invite_cancel_invites(): the ":" prefix for variable to t() does not exist in Drupal 7. Use "@" instead.
6. user_external_invite_cancel_invites(): do not translate singular/plural with a dynamic variable. Use format_plural() instead.
7. _user_external_invite_pending_invites(): the "string" type hint does only exist on PHP 7, so your module will break on every PHP 5 site. Either remove that primitive string type hint or add PHP 7 as dependency to your info file. See https://www.drupal.org/node/542202#php
8. _user_external_invite_calculate_hash(): the second parameter to drupal_hmac_base64() should only be secret information, no need to duplicate other data there again. For example drupal_get_token() adds the private key and the session ID. $expire is not secret information, so should be part of the first parameter.

Although you should definitely fix those issues they don't seem to be absolutely critical application blockers, otherwise RTBC. Assigning to greggles as he might have time to take a final look at this.

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

Thanks for your contribution, afinnarn! You are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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. We know it's broken and are trying to fix it.