[D7] Amazon-like login

Hi,

1. Please update the git link corresponding to non maintainer like "http://git.drupal.org/sandbox/neeravbm/1777308.git" in the issue summary.
2. Please review your module using Coder Module and see automated review here

Hi,
Mannual Review

In .module file

1)'!==' operator is there on line no 115.It should be '!='
2)In drupal_goto you have passed 'user/register' as an argument...But what if clean url in not on???..is it work

When i click on 'No, I am new to this site.' radio button under 'Do you have a localhost password?' its get redirected to 'http://localhost/user/register' which will throw 'Page not found'.Please use the drupal base path to get the base path of drupal

Please correct your git clone link it should be 'git clone http://git.drupal.org/sandbox/neeravbm/1777308.git amazon_like_login' and also correct all the issues mentioned in ventral.org

Thanks,
Suhani Jain

Hi
I installed your module and it works well.
You should check your coding here. there is a few error you should fix.
Also, you should change your git repository link described in comment #3, because it is hard to find this way.

After enabled the module, i cant login with my user's email address. I see you mentioned it in readme, but i found this a bit confusing.
At least, you should change the login block's title to e-mail/username.

Hi.
Nice module. Also module was checked with CAPTCHA and works fine.
1. Check automatic review http://ventral.org/pareview/httpgitdrupalorgsandboxneeravbm1777308git one more time.
2. Instead of amazon_like_login_valid_email() use valid_email_address().
3. Please add amazon_like_login.install file with:

function amazon_like_login_install() {
  drupal_set_message($t('Amazon Like Login module was installed. You need to !link.',
    array('!link' => l(t('update e-mail adresses'), 'email_update'))), 'status');
}

HI neeravbm,

You are getting few js error in automatic review
you should add space after and before of the +,=,== etc.

Please read drupal JavaScript coding standards.
http://drupal.org/node/172169

Thanks,

Welcome neeravbm !

Automatic review:

1. Please solve the remaining code standard (minor) issue: http://ventral.org/pareview/httpgitdrupalorgsandboxneeravbm1777308git

Manual review:

2. amazon_like_login.js:There is not enough to use the hostname, as some installations are not on the root url path (as mine is at localhost/drupal/mytestsited7/?q=[...]) You can access base path on the javascript side using Drupal.settings.basePath. window.location = window.location.protocol + '//' + window.location.hostname + '?q=user/register';

3. There is mispelling on line 114. You must enter a username. should be You must enter an username.

4. You should add a tag on the query at line 223 to allow other modules to modify this query. The query does not take node access grants into account, so node access modules might have no effect. Be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved, through the use of hook_query_TAG_alter(). See: http://api.drupal.org/api/drupal/modules!node!node.module/group/node_access/7

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hey neeravbm,

Grammar:

Firstly, please reverse the change made in #14, point 2.

The correct phrase is "a username" this is because username starts with a palatal glide (a 'y' sound). The use of a vs an relates to the sounds and not the letters used. You are sounding a consonant when you read username therefore you use a not an.

Automated Code Review:

No serious issues found. Please shorten the lines mentioned if it's at all possible.

Automated Code Review link.

Usability Points:

Personally I'd like to see the email_update url replaced with user/emailupdate for the sake of consistency. It'd be great to see user/password user/login user/register and user/emailupdate. However this is just a suggestion based on my observations.

My second usability point questions the validity of overriding Drupal's default username handling. In my opinion it would be better to use the existing email address field attached to users instead of overriding the functionality of username.

Manual Code Review:

.module file:
Line: 241form_set_error('name', t('You indicated that you are new to this site but an account with @username already exists.', array('@username' => $username)));

This doesn't quite read right. Suggest changing to: t('Sorry, an account with @username already exists.', array('@username' => $username)));

page.inc file:
Line: 27

Change to 'Current e-mail address'

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Checked with Drupal Coder module and Drupal Code Sniffer. Made changes for what makes sense. Also updated the git link in the project page.

Fixed the git clone URL in the issue summary for non-maintainer users.

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

Hi,

Please fix code formatting errors, i have found lots of error using automated code sniffer tool. You can check errors on pareview.sh or follow the following link to get all the errors :

http://pareview.sh/pareview/httpgitdrupalorgsandboxneeravbm1777308git

Thanks,

www.hobbylocal.com/user/login looks like a phishing site.

Manual Review
Hi Neerav,

I installed your module & its working fine. Apart from a few points listed below, its good to go.

1) Remove the commented code section in the amazon_like_login.js file.
2) You have a open menu callback in amazon_like_login.module : look out for it.
3) Do check for automated review for resolving code formatting issues(minor issues).

Automated Review

PAReview complains about missing automated test cases. Adding test cases are strongly recommended, but they are not mandatory. You should however add a comment before the line with the open pageback call and explain why there is no access restrictions. (I see no security problem with this open pageback call).

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not inform aboutd module duplication and/or fragmentation.

There already exists some modules that allows use of Email as username for Drupal. Here is a list with those I am aware of:

• Email registration
• LoginToboggan

Our users need to be informed about possible functional overlap. This should be made is section with the heading "Similar projects and how they are different" on the project's project page that:

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

(+) Your project page need to be updated with this information before creating a stable release, but not having this in place shall not block promotion to a full project.

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.

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

There are no remaining issues with the coding style and Drupal API usage.

The starred items (*) above are big issues and warrant the application going back to Needs Work. Items marked with a plus sign (+) should be addressed before a stable project release, but does not block the project from being promoted to a full project. The rest of the comments are only recommendations.

Conclusion

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

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.

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/amazon_like_login.module
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   20 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
  --------------------------------------------------------------------------
  
  Time: 51ms; Memory: 6Mb
  

• 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. +1 to what gisle said about module duplication. Please update your project page.
2. amazon_like_login_menu(): changing the email address should be restricted to logged in users, right? Then the access callback should be user_is_logged_in().
3. "$_SESSION['messages']['error'][$key] = 'You must enter an e-mail address.';": all user facing text must run through t() for translation.
4. amazon_like_login_form_user_login_validate(): the check_plain() is wrong here since your are not printing $username diretly no HTML and the '@' placeholder will already sanitize it in the form error message.
5. amazon_like_login_form_user_login_validate(): drupal_goto() is bad during form validation because then all other form validation handlers will be skipped. Can you use $form_state['redirect'] instead or does that not work?
6. You are using the user's email address as user name. That is a big security problem for Drupal sites because user names are practically public information on a Drupal site. An attacker could enumerate all user names of a site and get valuable email addresses for spamming. Here is the page explaining our policy: https://www.drupal.org/node/1004778 . That means user names must never be email addresses. The email_registration module solves this by cutting off the domain part of the address and use that as user name. See the code of that module to get inspiration. We had several security advisories already about emails as user names, see for example https://www.drupal.org/node/2336357 and https://www.drupal.org/SA-CORE-2016-001 . 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.

klausi wrote:

amazon_like_login_menu(): changing the email address should be restricted to logged in users, right? Then the access callback should be user_is_logged_in().

To change the email address, one has to supply a valid password for the account. This means that IMHO there is no need to restrict use of this callback to user_is_logged_in().

However, I agree with the security concerns of using the full email address as screen name.

The author says this in the project's README:

Internally this module works by using email address as the username. So any time format_username() command is used by any module, the email id of the user shows up. If you need the email id to be private, then use hook_username_alter() to change the username to what you like.

After reading the review by klausi, I realize this is not enough of a precaution, and that it should be resolved in a more robust manner.

6: [how to handle user names] @gisle Do you have recommendations/pointers about how this may be handled?

As pointed out by klausi, user names and user ids are not considered private information in Drupal, but a user's email address is.

This means that an email address cannot be a username. Your module may take steps to protect the username/email-address, but your module cannot control what other modules do with this field in the user's profile.

The main idea behind Amazon.com's login is that users these days are members of dozens of websites, and that usernames differ between sites, so keeping track of those puts a strain on the user's memory. One's own e-mail or mobile number is something that most people never forget, so using those as login credentials are attractive.

However, there is no requirement in Drupal to use the username as the (sole) means of identification of the account to authenticate during login. The login process just authenticates the user, and the identification part can be done by a number of alternate methods, including one that asks the user to supply a unique email-address and/or mobile number. The only constraint Drupal imposes on the username is that it must exist and be unique.

The Email registration module solves this by automatically generating a unique user name behind the scenes by cutting off the domain part of the email-address and appending the Drupal uid. Since drupal uids are unique, this simple algorithm authomatically produces usernames that are guaranteed to be unique across the site.

The LoginToboggan module solves this using slightly different approach. The registration form requires the user to register with a regular user name and email address (i.e. standard Drupal). However, when asking for credentials, the user can use one or the other (i.e. both will work equally well to identify the account to authenticate).

I prefer the way LoginToboggan handles this over Email registration, but both methods will work. One of the advantages over the LoginToboggan method is that all usernames created before the module is installed will continue to work as before.

To further emulate Amazon.com's login, you should also allow the user to register a mobile phone number, and allow that field to be used as well to identify the account to authenticate.

One of the nifty features of Amazon.com is that you're allowed to pick your own "Public Name" and you may even change it at any time. To emulate this part of Amazon's functionality, you could generate a unique user name behind the scenes during registration (Email registration method) or let the user pick an initial Drupal username (LoginToboggan method). You should the refer to the Drupal username as the user's "Public Name". The ability for users to change their "Public Name" is already in core: Simply set the "Change own username" permission for the role.

Hi neeravbm ,

Few tweaks are here README.txt does not followed guidelines for in-project documentation and It would be good, if you implement hook_help in your module and check below Coding style & Drupal API usage content.

Automated Review

All Pareview issues fixed

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

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

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. (*) $_SESSION['messages'] used in your module its there any dependency of module/file are using for unset if not $_SESSION['messages'] in your module still its not unset of $_SESSION['messages'] variable kindly check and unset this variable
2. (+) It would be good, if you implement hook_help in your module.
3. Just a recommendation - nothing

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.

Hi neeravbm,

You should follow the template for "README.txt" file . Please refer the below url
https://www.drupal.org/node/2181737

if the session is not destroy properly variable will be stored in the session until the user logout, Better to clear the session variable after it is use its not highly critical but its safe to do so.

Hi neeravbm,

For the session issue its not a blocker.

Automated Review

Fix all the Pareview issue listed in the given url http://pareview.sh/pareview/httpsgitdrupalorgsandboxneeravbm1777308git
Its not a blocker but its recommendation it would be fixed

manual review:

1. amazon_like_login_form_user_login_validate(): the check_plain() is wrong here since your are not printing $username diretly no HTML and the '@' placeholder will already sanitize it in the form error message. Yes, you are using check_plain() in this function, please remove both calls.
2. When a user edits their account then the email address will be saved as user name. So the email address will again be leaked through the username. Looks like amazon_like_login_form_user_profile_form_validate() is wrong?

The second point is the same security issue again and a blocker right now.

I have reviewed the module and its ready for approval.

All the security issues seem to be fixed according to me.

Thank you,
Vishal

Looks good to me now.

Thanks for your contribution, neeravbm!

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.