Comments

sanchi.girotra’s picture

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
neeravbm’s picture

I have made the changes as suggested. Please review.

suhani.jain’s picture

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

pnemes’s picture

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.

DmitriyMakeev’s picture

Status: Needs review » Needs work

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');
}
neeravbm’s picture

Thank you for reviewing the module. Here are the changes I have made:
1) When a user clicks on "No", he is redirected to "?q=user/register" rather than "user/register". This way we ensure that the link will work even without Clean URLs enabled.
2) Added javascript code so that when login page loads, "Yes" is checked by default.

neeravbm’s picture

Hi Suhani,

Thank you for reviewing the module. Here are responses to your queries:
1) !== is a valid operator. In fact, this is the one required here because I need to check boolean FALSE otherwise even 0 will match.
2) In javascript, I have changed redirection URL from "user/register" to "?q=user/register" to ensure that it works without clean URL enabled. In drupal_goto(), "user/register" will work.

Thank you.

Regards,
Neerav.

neeravbm’s picture

Hi pnemes,

After enabling the module, one will be able to log in using whatever username he or she used before. It does not change data of people who have already registered on the site. This way even the admin username will work since it is registered before the module is enabled.

Thank you.

Regards,
Neerav.

neeravbm’s picture

Hi DmitriyMakeev,

Thank you for reviewing the module. Here are the answers:
1) Automatic review only gives a few Warnings. The only errors it gives are errors in javascript where it asks to insert space between and after operators. But this is a minified javascript and removing the space is to reduce its size.
2) I changed amazon_like_login_valid_email() to valid_email_address().
3) To me, showing the message on install does not make sense since only the administrator will be able to see it when he installs the module. It should be up to the users if they want to update.

Thank you.

Regards,
Neerav.

neeravbm’s picture

Priority: Normal » Major
Status: Needs work » Needs review

Updating project priority since I haven't got any feedback since more than 2 weeks.

zymphonies.com’s picture

Status: Needs review » Needs work

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
Shanid kv

neeravbm’s picture

Priority: Major » Normal
Status: Needs work » Needs review

Fixed javascript coding format errors. Please review.

neeravbm’s picture

Priority: Normal » Major

Increasing the priority.

cubeinspire’s picture

Status: Needs review » Needs work

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.

klausi’s picture

Status: Needs work » Closed (won't fix)

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 :-)

neeravbm’s picture

Status: Closed (won't fix) » Needs review

I have fixed points 1, 2 and 3. Line 223 does not have a node query but a user query so I am not adding "node_access" tag. I could have added another tag but then somebody might accidentally break the functionality of the module. That's why I am not adding any tag to this query.

Homotechsual’s picture

Status: Needs review » Needs work

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'

PA robot’s picture

Status: Needs work » Closed (won't fix)

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.

PA robot’s picture

Issue summary: View changes

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

neeravbm’s picture

Issue summary: View changes
Status: Closed (won't fix) » Needs review
PA robot’s picture

Issue summary: View changes

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.

manish.upadhyay’s picture

Status: Needs review » Needs work
FileSize
514.88 KB

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,

neeravbm’s picture

Status: Needs work » Needs review

Hi manish.upadhyay,

The errors mentioned have been patched and fixed. Please see here

Thanks for reviewing!

neeravbm’s picture

Title: Amazon-like login » [D7] Amazon-like login

Updating The title.

joachim’s picture

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

neeravbm’s picture

Issue summary: View changes

Hi joachim,

Thanks for spotting that. I have removed the said link from the description.

It seems the domain has expired and been taken over as the description of this issue was updated long back.

neeravbm’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
manish_nagdevani’s picture

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).

gisle’s picture

Status: Needs review » Reviewed & tested by the community

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:

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.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -PAReview: review bonus +PAReview: security

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.

gisle’s picture

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.

neeravbm’s picture

Issue summary: View changes
FileSize
131.44 KB

Hi klausi, gisle

As per manual review I have made the following changes as per points noted here

1. Updated the Project Summary as per gisle's suggestion. see here
2. As gisle points out here I dont think the open callback is an issue.
Further the callback is open as it is a functionality wherein anonymous users can change the email id iff they have the previous email,password and login too in one step. see the screenshot
3. Fixed. see here
4. klausi mentioned

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.

I am not clear on this as I have not used any check_plain() inside amazon_like_login_form_user_login_validate().
5 - Fixed. see here
6 - @gisle Do you have recommendations/pointers about how this may be handled?

Also I have fixed the Drupal Practice Warning accordingly.

Thank you for reviewing the module.

gisle’s picture

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.

neeravbm’s picture

Status: Needs work » Needs review

Hi Gisle,

Thank you for providing your valuable feedback and suggestions to improve the module.

I have made the following changes to the module taking into consideration your suggestions:

1. The User is now asked for a desired username while registering.
2. The username now is not the email ID as this was the security issue pointed out by klausi

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

3. There is an option for the user to login with registered email-id or registered username. If someone created an account using a username before this module was installed (with or w/o an email id), it will still work.

The code diff is here

I am changing the status from `Needs work` to `Needs review`

ganesh_kumar’s picture

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.

neeravbm’s picture

Hi ganesh_kumar ,

A few Clarifications :

README.txt Does not follow the guidelines for in-project documentation

Can you please elaborate on what I am missing out?

(*) $_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

IMO $_SESSION['messages']['error'] is used just to set error message. why is that a major issue? Can you point me to any documentation which states we need to unset it?

Thanks for reviewing the module.

ganesh_kumar’s picture

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.

neeravbm’s picture

Issue summary: View changes
neeravbm’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus

Adding Review Bonus Tag after reviewing other projects see the Issue Changes here

neeravbm’s picture

Hi ganesh_kumar,

1. README.txt is fixed and now follows the guidelines see the diff here.

2. Added hook_help() as per your suggestion see here.

3. Regarding your comment

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.

I am not setting the value of this variable in my module. If you see here I am just removing the message to show a custom message. Further if you see Line 88, 92 They are just conditionals to check whether the variable has a value. Further I only Alter the message of the error in Line 98. So I dont think It is wise/justified why I should unset the variable as I am not setting it in my module. It might be set by drupal core and unsetting may not be wise.

Thank you!

ganesh_kumar’s picture

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

neeravbm’s picture

Hi ganesh_kumar

I have fixed the pareview.sh issues noted by you above.

Please see here

Thank you :)

klausi’s picture

Status: Needs review » Needs work

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.

neeravbm’s picture

Status: Needs work » Needs review

Hi klausi,

The issues mentioned above have now been fixed. here is the diff.

Thanks for reviewing the module :)

Changing the issue status to 'Needs review'

vishalkhialani’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed the module and its ready for approval.

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

Thank you,
Vishal

klausi’s picture

Status: Reviewed & tested by the community » Fixed

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:

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.