Details

Reviewers

Commits

rCOMMa6a35763be95: [lib][native][web] add new changeIdentityUserPassword action

Summary

users should be able to change their passwords on the identity service. this diff introduces the action that will be dispatched from native and web. it also includes the changes to the grpc-web client.

Test Plan

successfully dispatched the new action from native and web. then successfully logged in to identity with the new password.

Diff Detail

Repository

rCOMM Comm

Branch

changepassword (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Jan 11 2024, 12:50 PM

Herald added a subscriber: tomek. · View Herald TranscriptJan 11 2024, 12:50 PM

Requesting review to get help resolving the issue described in the inline comment

web/grpc/identity-service-client-wrapper.js
107 ↗	(On Diff #35563)	this is giving me a flow error because the Error constructor only expects one argument. I'm trying to follow this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error#rethrowing_an_error_with_a_cause

varun added inline comments.Jan 11 2024, 12:57 PM

web/grpc/identity-service-client-wrapper.js
79–81 ↗	(On Diff #35563)	i'm not sure why, but flow didn't like this, so i had to declare a new variable `client` in `changePassword`

Harbormaster completed remote builds in B25718: Diff 35563.Jan 11 2024, 1:15 PM

varun planned changes to this revision.Jan 11 2024, 2:00 PM

fix error handling

Harbormaster completed remote builds in B25719: Diff 35567.Jan 11 2024, 3:39 PM

JS looks great!! Didn't review the specifics of the OPAQUE stuff but trust you have that covered

This revision is now accepted and ready to land.Jan 11 2024, 8:03 PM

tomek added inline comments.Jan 12 2024, 2:36 AM

native/identity-service/identity-service-context-provider.react.js
67	This await doesn't change the logic: If it is present, the promise is resolved to a value, which is then automatically wrapped in a promise by the async function If it isn't present, the promise is directly returned, without unwrapping and wrapping So it might be unnoticeably more efficient to not have the await. But I guess it might cause some confusion, so up to you. (I think eslint should warn us about a promise that is left without being awaited or returned, but I might be wrong)

ashoat added inline comments.Jan 12 2024, 6:39 AM

native/identity-service/identity-service-context-provider.react.js
67	Thanks for explaining this... didn't realize that's how it worked. @varun can probably leave the logic as-is and undo these changes. Personally I think either way is fine, but if perf is better in your variant then that's probably best I think eslint should warn us about a promise that is left without being awaited or returned, but I might be wrong It's actually Flow that will warn us about that

varun added a child revision: D10623: [native] use useChangeIdentityUserPassword.Jan 12 2024, 11:31 PM

ashoat requested changes to this revision.Jan 13 2024, 6:48 PM

ashoat added inline comments.

lib/actions/user-actions.js
508	The latest release of the whitepaper (viewable on Overleaf or GitHub) defines a protocol for password updates in Section 3.2.1.4. As part of that protocol, the user's current password is supposed to be verified. The whitepaper update happened earlier this month... sorry for not noticing earlier. I think this will require some identity service changes, and the client will need to make at least two gRPC calls...