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

Chore/98 invalidate jwt on reset password #459

Merged
merged 11 commits into from
Dec 7, 2023

Conversation

hahn-kev
Copy link
Collaborator

@hahn-kev hahn-kev commented Dec 5, 2023

closes #98

sends the user to the login page with an error when they click an expired jwt link where the jwt was expired because the user has been updated since it was generated.

@myieye I can't get the test to work, it's kinda hard to follow and figure out what I need to do to click on the link again.

@hahn-kev hahn-kev requested a review from myieye December 5, 2023 10:16
Copy link

github-actions bot commented Dec 5, 2023

UI unit Tests

1 tests   1 ✔️  0s ⏱️
1 suites  0 💤
1 files    0

Results for commit 7c4b7d7.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 5, 2023

C# Unit Tests

37 tests  ±0   37 ✔️ ±0   5s ⏱️ ±0s
  7 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 7c4b7d7. ± Comparison against base commit 58eecf4.

♻️ This comment has been updated with latest results.

@myieye
Copy link
Contributor

myieye commented Dec 5, 2023

@hahn-kev

  • I fixed backend/Testing/Browser/EmailWorkflowTests.cs
  • Testing.Browser.UserPageTest.CanResetPassword is failing, because if a user goes to their settings and tries to change their password they get a 403. It seems that it's become exclusive to the reset-password audience?
  • Most links seems to currently fail, because there's some imprecision somewhere. When trying to verify my email it says the link is expired, because 2023-12-05T15:30:51.8288550+00:00 != 2023-12-05T15:30:51.8288552+00:00

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

See my previous comment

@hahn-kev
Copy link
Collaborator Author

hahn-kev commented Dec 6, 2023

Thanks, I'm seeing this date issue as well. I also noticed that the test has a weird failure mode.

When the test does a submit of the reset form, the reset fails (due to a bug), however it then just sends the user to the home page, without any indication that the password change failed. This is also a bug in the front end, if a password reset fails (for any reason) we need to tell the user, not just send them home. Create #465 to track this

@hahn-kev
Copy link
Collaborator Author

hahn-kev commented Dec 6, 2023

@myieye test's are working again, please take another look.

For the date issue I just truncated it to unix seconds and used that for the comparison instead.

@hahn-kev hahn-kev requested a review from myieye December 6, 2023 04:34
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

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

Looks like things are working well.
This does introduce some strange behaviour, because unrelated properties all depend on the same date: E.g. user display name, email, password etc.
This will only matter in edge cases, but I expect there will be a few weird situations:
E.g.

  • A user registers, fixes their display name and then tries to verify their email address.
  • An admin resets a user's password (shouldn't happen I know, but it will 😆), the user logs in, sees: "oh, I should verify my email address", looks for the email => 💥

We probably just need to wait and see what happens.
We could potentially change the message to something like "The link is no longer valid due to changes to your account."
I'll leave that up to you.

@myieye
Copy link
Contributor

myieye commented Dec 6, 2023

Oh, I know what we should change. We should only validate the date if the user is changing their email; not if they're verifying the email address that we already have in the db

@hahn-kev hahn-kev merged commit da1915f into develop Dec 7, 2023
11 checks passed
@myieye myieye deleted the chore/98-invalidate-jwt-on-reset-password branch December 18, 2023 10:33
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.

reset password jwt should be unique and expire on password change
2 participants