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

feat(user): Adds logic to handle authentication. #258

Merged
merged 38 commits into from
Jun 25, 2023

Conversation

ERosendo
Copy link
Contributor

@ERosendo ERosendo commented Jun 1, 2023

This PR fixes #202 and also closes #203.

This PR adds a dropdown menu next to the Donate button in the header. This dropdown menu has a button to sign out and a link to the profile page.

Here's a screenshot of the new dropdown menu:

image

This PR also introduces the views and templates to register, reset the password and request the email confirmation link.

Here are screenshots of the new pages:

Signed out page:
image

Register form:

Successful Registration
image

Successful Email Confirmation
image

Invalid confirmation link
image

Expired confirmation link
image

Reset Your Password
image

@ERosendo ERosendo changed the title feat(user): Adds fields to handle authentication. feat(user): Adds logic to handle authentication. Jun 1, 2023
bc/core/utils/crypto.py Fixed Show fixed Hide fixed
@mlissner
Copy link
Member

mlissner commented Jun 7, 2023

Tests are failing and this is marked as a draft, but you're doing other work. Should I take a deeper look here or wait?

@ERosendo ERosendo force-pushed the feat-handle-user-authentication branch from 81e6b6e to cb1cc69 Compare June 9, 2023 13:49
@ERosendo ERosendo force-pushed the feat-handle-user-authentication branch from 562a9bd to 404a321 Compare June 9, 2023 17:00
@ERosendo ERosendo marked this pull request as ready for review June 9, 2023 17:10
@ERosendo ERosendo requested a review from mlissner June 9, 2023 17:10
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good, but there are some tricky changes that I'd suggest.

Thank you!

bc/assets/templates/includes/regular-anchor.html Outdated Show resolved Hide resolved
bc/core/utils/crypto.py Outdated Show resolved Hide resolved
bc/assets/templates/includes/header.html Show resolved Hide resolved
bc/core/utils/urls.py Outdated Show resolved Hide resolved
bc/users/forms.py Outdated Show resolved Hide resolved
bc/users/models.py Outdated Show resolved Hide resolved
bc/users/signals.py Outdated Show resolved Hide resolved
bc/users/templates/register/registration_complete.html Outdated Show resolved Hide resolved
bc/users/urls.py Outdated Show resolved Hide resolved
bc/users/views.py Outdated Show resolved Hide resolved
@mlissner
Copy link
Member

Oh, one other thing. All of our account-related views need to avoid the CDN, right? Or at least some of them do?

@ERosendo ERosendo force-pushed the feat-handle-user-authentication branch from 176cb13 to 906a1d1 Compare June 21, 2023 05:55
This commit introduces the following changes:
- Removes the crypto module.
- Removes the activation_key and the key_expires field from the User model.
- Adds a new method in the User class get the signed pk.
- Initialize a TimestampSigner object as a class attribute of the Users class.
- Updates the superuser_creation signal to remove the old fields of the user model.
- Tweak the register, request email confirmation and email confirmation views.
- Tweak the URL pattern that matches the email confirmation view.
This commit updates the script_src and the default_src directives to add hcaptcha as a valid source.
@ERosendo
Copy link
Contributor Author

ERosendo commented Jun 21, 2023

Thanks for your comments. I've applied all your suggestions.

I tweaked the registration page to use hcaptcha and also added a new ratelimit decorator to create a global counter to limit the POST request to the registration, forgot password, and request email confirmation pages.

OTOH, I reviewed the new account views and noticed the only one that needs to avoid the CDN is the Successful Registration page but that one should work fine because it uses the query string to get the data that renders on the page and we tweaked the cache policy to forward the query strings to the origin ( we did this to fix the add-a-case form).

Let me know what you think.

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple things. Thank you!

bc/settings/third_party/hcaptcha.py Outdated Show resolved Hide resolved
bc/users/views.py Outdated Show resolved Hide resolved
bc/users/views.py Show resolved Hide resolved
@ERosendo ERosendo requested a review from mlissner June 24, 2023 19:19
@mlissner mlissner merged commit e7bcb84 into main Jun 25, 2023
@mlissner mlissner deleted the feat-handle-user-authentication branch June 25, 2023 01:41
@mlissner
Copy link
Member

This is merged. Will you please create an issue in CL to update the way it does email verification URLs? Maybe put some notes or links to this in there so we can just do this same code over there?

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

Successfully merging this pull request may close these issues.

Add Login / Profile / Logout button/dropdown Handle user sign up / forgot passwords / etc
2 participants