Page MenuHomePhabricator

[lib][native] Reset login state if authoritative keyserver fails
ClosedPublic

Authored by ashoat on Apr 30 2024, 11:36 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 21, 2:56 PM
Unknown Object (File)
Sat, Dec 21, 2:56 PM
Unknown Object (File)
Sat, Dec 21, 2:56 PM
Unknown Object (File)
Sat, Dec 21, 2:55 PM
Unknown Object (File)
Sat, Dec 21, 2:54 PM
Unknown Object (File)
Nov 24 2024, 10:49 AM
Unknown Object (File)
Nov 22 2024, 9:27 PM
Unknown Object (File)
Nov 13 2024, 7:56 PM
Subscribers

Details

Summary

We have a two-part login and registration process: first the identity service, then the authoritative keyserver.

Ideally, if the first part succeeds but the second part fails, we could cache the success of the first part, and recompute it if inputs change. But I scoped that work and determined we didn't have the cycles for it right now.

An easier, more immediate solution is to simply reset the state between attempts. We'll attempt to reset the state in two places:

  1. On the identity service we'll delete the CSAT, and in the case of registration delete the newly-created user entirely.
  2. On the client we'll unset the currentUserInfo through either an identityLogOut or deleteDiscardedIdentityAccount action, which will result in clearSensitiveData being called due to the user ID no longer matching the one stamped on the SQLite DB.

Note – would normally put @tomek and @inka on this review, but since they're out for the rest of this week, I'm asking @varun to review.

Test Plan
  1. Force keyserver auth to fail with this patch
  2. Test new registration flow and confirm that it fails
  3. Check the logs and confirm that the SQLite database was deleted
  4. Stash the patch above
  5. Try registration again and confirm that it works, and doesn't error due to identity already having an account with that username

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/actions/user-actions.js
334 ↗(On Diff #39710)

I initially tried using the existing logOut and deleteAccount, but deleteAccount didn't work because the keyserver account deletion failed, which would result in dispatching DELETE_ACCOUNT_FAILED instead of DELETE_ACCOUNT_SUCCESS. As described in the comment above, we need DELETE_ACCOUNT_SUCCESS in order for the reducers to clear the currentUserInfo, which is what leads to the SQLite database being cleared.

To get around this I decided to implement new functions. useIdentityLogOut wasn't strictly necessary since useLogOut always succeeds already, but I decided it would be better to skip calling the keyserver logOut endpoint here since it's not necessary.

varun requested changes to this revision.May 1 2024, 10:14 AM

what happens if identity account deletion fails after the failed keyserver auth? should we notify the user that their account was registered successfully?

lib/actions/user-actions.js
346 ↗(On Diff #39710)

deleteDiscardedIdentityAccount

This revision now requires changes to proceed.May 1 2024, 10:14 AM

Addressed feedback. Added an alert when deleting the identity account fails

This revision is now accepted and ready to land.May 1 2024, 7:01 PM