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

Add a scope to allow resetting other users' passwords #2373

Merged
merged 10 commits into from
Jan 31, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Jan 25, 2023

Closes #2208
Closes #2007

Code Changes

  • Removes the old user:reset-password scope in favor of loosening up the endpoint to just always allow users to change their own passwords (as opposed to only allowing users to change their passwords in addition to needing this scope)
  • Adds an alembic migration to remove user:reset-password from every user
    • I'm not actually sure if this is the right place for it, as we can't really do a down revision for this. It's more a data migration than a db migration
  • Adds a new scope user:password-reset (I know, the name is confusing if the old scope still exists!! I had some trouble with naming in this PR. password-reset / reset-password just seems like the right name for this action!) which allows the user with this scope to reset passwords without knowing the old one (including other users'!)
  • Adds a new endpoint /user/{user_id}/force-reset-password to allow resetting a password without knowing the old one which is scoped on user:password-reset
    • The Forgot Password Cheat Sheet says to return a consistent message for both existent and non-existent accounts. I was going to do that for this endpoint, but thought it would cause confusion if an admin reset a user's password but the user no longer existed, so it should get a 404, but instead they get a success and then they try to tell that person to log in with the new password. Overall, I think this pattern of "admin making a password to reset a user's password" doesn't really follow best practices (as we've already discussed) and is temporary, but I could also make it always return a success instead if we want. Our other user/password related endpoints also do not give a consistent message, so maybe this is just something we need to do as a chunk once we have a proper password reset flow
  • Tests

Steps to Confirm

  • list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@allisonking allisonking marked this pull request as ready for review January 26, 2023 15:05
@allisonking allisonking mentioned this pull request Jan 26, 2023
10 tasks
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

Thanks @allisonking — just one minor comment on the docstring :)

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Base: 88.55% // Head: 88.56% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (8afc646) compared to base (502d689).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2373   +/-   ##
=======================================
  Coverage   88.55%   88.56%           
=======================================
  Files         327      327           
  Lines       15714    15726   +12     
  Branches     4348     4351    +3     
=======================================
+ Hits        13915    13927   +12     
  Misses       1644     1644           
  Partials      155      155           
Impacted Files Coverage Δ
...c/fides/api/ops/api/v1/endpoints/user_endpoints.py 91.42% <100.00%> (+1.10%) ⬆️
src/fides/api/ops/api/v1/scope_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/api/v1/urn_registry.py 100.00% <100.00%> (ø)
src/fides/lib/oauth/schemas/user.py 100.00% <100.00%> (ø)
src/fides/lib/oauth/scopes.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allisonking
Copy link
Contributor Author

Going to merge, as the failing pylint test is unrelated to this PR

@allisonking allisonking merged commit f02d445 into main Jan 31, 2023
@allisonking allisonking deleted the aking/2208/admin-reset-password branch January 31, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants