Optional Implicit Verification of accounts when successfully resetting password#499
Conversation
…r password successfully reset.
…unt on successful password reset.
|
Thank you for submitting a PR. I agree in principle that a valid password reset should implicitly verify. The whole point of account verification is to ensure the email is valid, and if the user can prove receipt of a password reset email, I think that's sufficient for account verification. I'll do a deeper review later today, and consider whether this should be a separate feature or just part of an existing feature. |
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
This looks good, but after review, I think it's more appropriate as a separate feature. If you want to work on changing this PR to use a separate feature, that works for me. Otherwise, I'm happy to merge this PR and handle those changes.
| verify_account | ||
| remove_verify_account_key |
There was a problem hiding this comment.
The main issue here is you can set reset_password_implicitly_verifies? to true without loading the verify_account feature, in which case you end up with a NoMethodError here. It's possible to check that, but I think it's simpler to add a new feature that depends on both reset_password and verify_account, and has these changes (as you mentioned when submitting).
| account_from_key(token, reset_password_account_status_value){|id| get_password_reset_key(id)} | ||
| end | ||
|
|
||
| def reset_password_account_status_value |
There was a problem hiding this comment.
You can keep this method inside the reset_password feature (and the call inside _account_from_reset_password_key) and have it return account_open_status_value, and extend it in the reset_password_verifies_accounts feature to do [super, account_unverified_status_value].flatten.
|
I merged this and then moved the code to a new feature. Thank you for your contribution! |
2 participants
In the common user flow of
user signs up -> user forgets to ever verify -> user leaves for a while and comes back -> user has forgotten their password so tries resetting
the system as works now will prevent them from resetting their password or ever logging in unless the implementing system is conscious of catching the error and routing the user through a verification flow. However, even in the case of the implementing system doing this, the flow is nightmare ux. The user will have to enter their email to verify, go to their email and click the link to verify, then once that's done, request a password reset, go back to their email once again, click the link, and then enter their new password.
It's my belief that a user resetting their password is implicitly verifying their account email, as the very act of resetting a password requires the email to be real.
This feature adds an optional and defaulted to
falsereset_password_implicitly_verifies?which handles this preferred user path.I tried to keep it as simple and as bound to the style/convention of the codebase as possible. I did have a doubt as to whether this should be a toggle on :ResetPassword or its own feature definition (as it also depends on the verification feature which reset password does not automatically do). I opted for the former since it's basically a toggle check, but if you think it's more appropriate as a feature let me know and I can redo it.