-
Notifications
You must be signed in to change notification settings - Fork 886
fix: Add route for user to change own password #1812
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
Conversation
8ed6960
to
13e4b22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FE = ✔️
e828fb3
to
f1e430a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, but code LGTM!
// we want to require old_password field if the user is changing their | ||
// own password. This is to prevent a compromised session from being able | ||
// to change password and lock out the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this security through obscurity? A highjacked admin session could still reset everyone's passwords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly, it's to prevent a leaked admin token from being able to lock the admin out. They could reset other people's passwords, but not the admin themselves. It's a small scope but makes sense to do I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly reusing the same endpoint for reset & change. I think the security implications are not even a consideration at this stage. Admins have some super powers that we don't need to tighten for MVP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 nit
// we want to require old_password field if the user is changing their | ||
// own password. This is to prevent a compromised session from being able | ||
// to change password and lock out the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly reusing the same endpoint for reset & change. I think the security implications are not even a consideration at this stage. Admins have some super powers that we don't need to tighten for MVP
Backend for #1819