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

fix: Add route for user to change own password #1812

Merged
merged 10 commits into from
May 27, 2022
Merged

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented May 26, 2022

Backend for #1819

@f0ssel f0ssel force-pushed the f0ssel/add-user-pass-change branch from 8ed6960 to 13e4b22 Compare May 26, 2022 21:11
@f0ssel f0ssel marked this pull request as ready for review May 26, 2022 21:52
@f0ssel f0ssel requested a review from a team as a code owner May 26, 2022 21:52
@f0ssel f0ssel requested review from Emyrk and a team May 26, 2022 22:14
coderd/coderd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

FE = ✔️

@f0ssel f0ssel marked this pull request as draft May 27, 2022 15:25
@f0ssel f0ssel force-pushed the f0ssel/add-user-pass-change branch from e828fb3 to f1e430a Compare May 27, 2022 16:47
@f0ssel f0ssel marked this pull request as ready for review May 27, 2022 16:48
Copy link
Member

@kylecarbs kylecarbs left a 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!

Comment on lines +370 to +372
// 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.
Copy link
Member

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.

Copy link
Contributor Author

@f0ssel f0ssel May 27, 2022

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.

Copy link
Member

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

Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

1 nit

Comment on lines +370 to +372
// 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.
Copy link
Member

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

coderd/users.go Outdated Show resolved Hide resolved
@f0ssel f0ssel enabled auto-merge (squash) May 27, 2022 17:22
@f0ssel f0ssel merged commit 24d1a67 into main May 27, 2022
@f0ssel f0ssel deleted the f0ssel/add-user-pass-change branch May 27, 2022 17:29
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

4 participants