Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E-mail validation for newly registered users #1672

Closed

Conversation

primal100
Copy link
Contributor

@primal100 primal100 commented Jan 24, 2018

I want to merge this change because...

Enables a common feature of registration systems where a newly registered user's e-mail can be validated before being allowed to login. Solution for #1527

This is how it works:

  • There is an additional setting, EMAIL_VERIFICATION_REQUIRED, set to False by default.
  • There is an additional column on the userprofile model: email_verified, a boolean set to False by default.
  • User registers.
  • If EMAIL_VERIFICATION_REQUIRED is set to FALSE, behavior is the same as before, they are immediately logged in
  • If EMAIL_VERIFICATION_REQUIRED is set to True, an activation e-mail is sent. containing the activation url, which is built using the same code as the password reset view.
  • Staff users have the same behavior as before, they do not need to validate their e-mail address in order to login.
  • When a user logs in, if EMAIL_VERIFICATION_REQUIRED is True, the user is not a staff user and email_verified is False, the login will be rejected and the activation e-mail will be resent.
  • The token is valid for the same length of time as password reset tokens, which uses the setting PASSWORD_RESET_TIMEOUT_DAYS
  • The user clicks the activation link and if everything is ok, email_verified is set to True for that user, they are shown a success message and the login page.
  • If the user id given in the url does not exist, they are shown an error and the login page.
  • If the token is incorrect or no longer valid, the activation e-mail is resent and they shown an error and the login page.
  • If the user's email address has already been verified, they are shown an error and the login page.

I am using ugettext for some translations to keep things consistent with other strings in the same module.

Pull Request Checklist

(Please keep this section. It will make maintainer's life easier.)

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. Python code quality checks pass: pycodestyle, pydocstyle, pylint.
  8. JavaScript code quality checks pass: eslint.

@codecov-io
Copy link

codecov-io commented Jan 24, 2018

Codecov Report

Merging #1672 into master will increase coverage by 0.06%.
The diff coverage is 91.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1672      +/-   ##
==========================================
+ Coverage   83.16%   83.22%   +0.06%     
==========================================
  Files         141      143       +2     
  Lines        6119     6147      +28     
  Branches      665      666       +1     
==========================================
+ Hits         5089     5116      +27     
- Misses        859      860       +1     
  Partials      171      171
Impacted Files Coverage Δ
saleor/registration/urls.py 100% <ø> (ø)
saleor/registration/emails.py 100% <100%> (ø)
saleor/userprofile/models.py 93.93% <100%> (ø)
saleor/registration/forms.py 86.27% <100%> (ø)
saleor/registration/views.py 92.95% <86.84%> (ø)
saleor/discount/utils.py 85% <0%> (-10.95%) ⬇️
saleor/dashboard/product/forms.py 82.25% <0%> (-0.67%) ⬇️
saleor/graphql/product/types.py 80.92% <0%> (-0.13%) ⬇️
saleor/product/models.py 93% <0%> (-0.09%) ⬇️
saleor/product/utils.py 86.91% <0%> (-0.06%) ⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad47e9f...d0b4c38. Read the comment docs.

@primal100
Copy link
Contributor Author

I removed changes to the docs as they are no longer mergable after the docs re-structuring. If this pull request is accepted I'll make another pull request adding the setting to the new docs structure.

@maarcingebala
Copy link
Member

maarcingebala commented Jan 31, 2018

Thanks for your contribution @primal100! We're currently working on a PR that adds HTML email templates (#1673), which also introduces a new tool to generate the templates (mjml). We'll provide kind of base template for emails, so once we merge our PR, this one could be updated to provide the HTML template as well.

@mad-anne Will you take a look at this PR and review it, when you have time?

@mad-anne
Copy link
Contributor

mad-anne commented Feb 1, 2018

@elwoodxblues, yes, I just added some comments. Although we must still wait with this PR until MJML e-mails will be merged and then consider further changes.

@primal100 there are few things to improve I wrote about in comments. Also, please use single quotes in Python code and keep max 79 characters in each line.

@mad-anne
Copy link
Contributor

mad-anne commented Feb 5, 2018

@primal100, could you please fix issues given by codeclimate?

@primal100
Copy link
Contributor Author

@NyanKiyoshi

Regarding the current way of re-sending the e-mail after a failed login or failed activation, I can change that to a link to re-send if you think it's better. I just want to agree on the best way to do it.

I'm thinking an additional view called resend_activation_email with:

GET: Returns a version of the login template with an additional link added. When the link is clicked a post request is made to the same view.
POST: Re-sends the activation e-mail and redirects to the login view

When activation fails, or login fails due to e-mail not verified yet, the response redirects to the resend_activation_email view.

If you have a preferred way of implementing it let me know.

Cheers.

@maarcingebala
Copy link
Member

@primal100, I was busy with other tasks and I just found time to look at your PR.

We had an internal discussion about how the activation process could work. Here are our thoughts:

  • I agree with @NyanKiyoshi, that we should not resend the email automatically, but this should be an explicit action.
  • We could allow accessing the account if the email hasn't been confirmed yet, but we should then display a message "Your account is not verified. Confirm your email or click here to resend the confirmation link.".
  • For anonymous users, it is possible to create an account immediately after a successful checkout (see checkout_success view). We then check if the user's email is in our DB already, which would mean that has an account already. In such case, we associate the newly created order with this account. But when introducing the email verification, we should do it after verifying the email address.

Your work on this PR has been great so far. Let me know if you have time and are willing to make those changes. Otherwise, someone from our team will do this. We can take the last point on (even in a separate PR) and you could do the changes mentioned in the first two points, what do you think?

@primal100
Copy link
Contributor Author

Thanks,

I like your suggestions. I can work on the first two points for now anyway. Not sure about the third one but will have to look at the code some point.

Regarding this point:

"We could allow accessing the account if the email hasn't been confirmed yet"

Django allauth has three options for email confirmation - optional, mandatory or none. Are you suggesting something similar? I wasn't sure of the use case for optional but I can add it.

So if optional is selected, there should be no limitations for a logged in user who has not activated their e-mail address?.

Thanks!

@patrys
Copy link
Member

patrys commented Feb 5, 2018

I think the main limitations for unconfirmed accounts is that we don't want to assign previous orders until you confirm email ownership as otherwise you could sign up as any other customer and immediately gain access to their full name and home address.

@maarcingebala
Copy link
Member

@primal100 You could make it configurable just like in django-allauth, but if it's simpler for you, in the first step you could also implement only the "optional" case and add the configuration later. We can take care later of implementing the logic related to attaching orders.

@primal100
Copy link
Contributor Author

Hey,

@elwoodxblues , regarding your three points:

  • Resend is now a manual action, with a separate view and the link stored in a message. I'm not sure if this works ok with translation.
  • Users are still logged in without e-mail confirmation but have the option of confirming e-mails, so it is optional. So far, the resend confirmation e-mail message is only shown when the activation token is invalid. I'll leave it to you guys to work on the rest of the implementation, as I'm not really sure of the best way to do it.
  • I'll leave you guys to implement point 3.

Note that, the token generator used (same as password reset) encodes the last_login time, so if the user logs in again, any previous tokens are now invalid.

@NyanKiyoshi
Copy link
Member

Are you planning on continuing this PR? If not, let me know, so I retake your work.

The important thing with activation links is that it helps preventing hard bounces (which is also why the "Resend" button shouldn't resend a mail so easily).

@primal100
Copy link
Contributor Author

Hi,

You can retake it for now.

Thanks

@maarcingebala
Copy link
Member

I'm closing this PR since due to inactivity. If anyone wants to retakes it and update, feel free. This is still a valid issue that we should tackle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants