-
Notifications
You must be signed in to change notification settings - Fork 50
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
Implement password update support #566
Conversation
No New Or Fixed Issues Found |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
==========================================
+ Coverage 51.65% 52.30% +0.65%
==========================================
Files 163 163
Lines 8202 8316 +114
==========================================
+ Hits 4237 4350 +113
- Misses 3965 3966 +1 ☔ View full report in Codecov by Sentry. |
UserLoginMethod::Username { email, kdf, .. } | ||
| UserLoginMethod::ApiKey { email, kdf, .. }, | ||
) => MasterKey::derive(new_password.as_bytes(), email.as_bytes(), kdf)?, | ||
_ => return Err(Error::NotAuthenticated), |
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.
We should probably have another error type for these scenarios. You technically are authenticated it's just not compatible with the current operation.
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.
Yeah I went with this one because it was used in other places too, but this should probably be Error::InvalidAuthentication
or something. We can change it but it's going to affect other places too, so I can leave it for another PR right after this one.
.await | ||
.unwrap(); | ||
|
||
let new_password_response = update_password(&mut client, "123412341234".into()).unwrap(); |
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.
Do we want to validate the new password hash too? We have a validate_password
that could be used.
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.
Hmm, validate_password
is validating a LocalAuthorization
hash, while we would be returning a ServerAuthorization
hash from the update_password
function, right?
Edit: Just used hash_password directly and compared the outputs.
Type of change
Objective
Support re-encrypting the user key with the new password.