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

Admin should be able to reset password without the knowledge of old password #2208

Closed
saiatethyca opened this issue Jan 11, 2023 · 6 comments · Fixed by #2373
Closed

Admin should be able to reset password without the knowledge of old password #2208

saiatethyca opened this issue Jan 11, 2023 · 6 comments · Fixed by #2373
Assignees
Labels
bug Something isn't working customer-request A feature or bug-fix requested by a customer

Comments

@saiatethyca
Copy link

saiatethyca commented Jan 11, 2023

Description

Currently, in the user management tab. Admin can change a user's password only if they are aware of the previous password.
This is not a good experience for users as the trigger for a password change is generally when they forget their password. We need to support this use case.

Use case

  • User forgot their password, and they requested their admin to change it.

Acceptance Criteria

  • Admin can change the password without adding an older password.
  • Admin can view the status of the change request. Whether it's successful or Failed.

Mockups

Current
Screen Shot 2023-01-11 at 5 36 09 PM

Expected
Screen Shot 2023-01-20 at 12 36 22 PM

@Kelsey-Ethyca Kelsey-Ethyca added bug Something isn't working enhancement and removed enhancement bug Something isn't working labels Jan 17, 2023
@NevilleS NevilleS added the bug Something isn't working label Jan 18, 2023
@allisonking
Copy link
Contributor

Current state

  • Users can only change their own passwords. Furthermore, they must have the user:reset-password scope in order to do so. This is non-obvious as the frontend still renders the Update password form even though the backend will just 403 if the user attempts to update their own password without that scope. This is a bug, but also it seems like the frontend does very little in terms of rendering things based on scopes, so that might be a large task.
  • Admins have no special password abilities

Proposed solution

  • We would need a new scope for a user being able to assign a password without any auth. I think it should be named user:reset-password but that already exists (arguably the one that already exists should be user:change-password), so instead maybe this new one is something like user:overwrite-password. Only the root user should get this scope!
  • We'd probably want a new endpoint for overwriting passwords. It would have the scope user:overwrite-password and would take a user ID of the user whose password needs to be changed, and also the new password the admin user is assigning them.
    • This is quite a powerful scope, so I'm not sure if we'd even want to expose it as a scope users can assign to other users...
  • The front end would, if a user has the user:overwrite-password scope, render a modal like the one Sai pasted above

Ideally, the next time the user logs in they'd be prompted to change their password, but that might be out of the scope of this ticket. Even better would be if we had a "forgot password" flow, but that's a big chunk of work...

Anyway, wanted to get this proposal down since I want to make sure we get things like passwords right, and I haven't touched scopes before. Maybe @seanpreston can give some feedback?

@allisonking
Copy link
Contributor

allisonking commented Jan 20, 2023

After some slack discussion, how about:

  1. Remove the current code that checks for the scope user:reset-password. We will no longer use a scope to check for if a user can update their own password—they should always be allowed to
  2. Create a migration that removes the user:reset-password scope from all users' scopes except for the root user's
  3. Add an endpoint which takes a user_id and a password which is scoped to user:reset-password. This is the endpoint a root user can hit if they need to reset someone else's password
  4. Add a button the user's page that only renders if the user has the user:reset-password scope that says something like "Reset this user's password" which brings up a modal with two form fields: new password, and confirm new password. This will hit the endpoint in bullet point 3

Does that sound ok to everyone? The only thing I worry about is if there's any danger in reusing user:reset-password like this (i.e. is it possible we could accidentally give permission to someone to reset other user's passwords who wouldn't have it?)

@saiatethyca
Copy link
Author

Thanks @allisonking, what you've outlined is clear to me!

One other concern I have is if we can restrict adding reset-password scope to a non-root user.

@seanpreston
Copy link
Contributor

One other concern I have is if we can restrict adding reset-password scope to a non-root user.

I don't think this is necessary, since the scope has to be explicitly given.

@daveqnet
Copy link
Contributor

For my own reference, the specific use case in this issue is for an authenticated “fides admin” (although we don’t have roles yet) to reset a password for any other fides user.

Presuming we do eventually have roles rather than a long, granular list of scopes, this is a power that only an authenticated admin should have. In the meantime we'll have to settle for an authenticated user with this password reset scope.

When we do introduce flows for users to reset their own passwords, take a look at this when refining the issue: https://cheatsheetseries.owasp.org/cheatsheets/Forgot_Password_Cheat_Sheet.html

The problems and solutions for this are pretty universal, hence the cheatsheet.

@saiatethyca
Copy link
Author

Following is what we concluded after the call:

  1. Create a new scope, which is added to the root user by default.
  2. Root User can reset a user's password without knowing the old password.
  3. Reset password modal has two input fields, one to enter the new password and the other to confirm the same.

CC: @allisonking @seanpreston

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working customer-request A feature or bug-fix requested by a customer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants