Page MenuHomePhabricator

[lib][native][web] Introduce usePasswordLogIn hook
ClosedPublic

Authored by ashoat on Apr 24 2024, 12:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 3 2024, 5:29 AM
Unknown Object (File)
Oct 15 2024, 10:26 AM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Unknown Object (File)
Oct 12 2024, 2:55 PM
Subscribers
None

Details

Summary

This addresses ENG-7681, ENG-7671, and ENG-7673.

This furthers the goal of the parent task, which is to move authoritative keyserver auth from KeyserverConnectionHandler to the individual components responsible for login.

Depends on D11763

Test Plan

This whole stack was tested with the following steps:

  1. On native, attempting to log in with an ETH account that hasn’t been registered yet
  2. On native, attemping to register with an ETH account that has already been registered
  3. On native, log in with an ETH account that has already been registered
  4. On native, register with an ETH account that hasn’t already been registered
  5. On native, register with a password account
  6. On native, log in with a password account
  7. On web, attempting to log in with an ETH account that hasn’t been registered yet
  8. On web, log in with an ETH account that has already been registered
  9. On web, log in with a password account

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/hooks/login-hooks.js
31–33 ↗(On Diff #39451)

In the next diff (D11766) I end up removing this in favor of just calling saveCredentials at the end (after keyserver auth), but I decided to leave it in here in case that diff needs to get reverted for some reason

lib/hooks/login-hooks.js
57–59 ↗(On Diff #39451)

In D11763 we check if step is inactive. Do we need to do that here?

63 ↗(On Diff #39451)

This probably shouldn't be named "registration". And, same as in D11763, I think this shouldn't be named "waiting for", when later it represents the state after registration has succeeded.
Also because we don't have a third step to represent the keyserver auth, I think we actually care about two states - "no login in progress" and "identity login concluded, keyserver login in progress"

lib/hooks/login-hooks.js
57–59 ↗(On Diff #39451)

Good call! Yes, I'll make that change

63 ↗(On Diff #39451)

This probably shouldn't be named "registration"

Good point, I'll make this change

And, same as in D11763, I think this shouldn't be named "waiting for", when later it represents the state after registration has succeeded.

As described there, we're really waiting for data to appear in Redux, but I thought that name was a bit of a mouthful. Open to other suggestions, but opposed to identity_registration here because we're really doing keyserver auth during this step.

  1. waiting_for_identity_registration -> waiting_for_identity_login_in_redux
  2. Add a check that step is inactive in order to avoid triggering auth multiple times
This revision is now accepted and ready to land.Apr 26 2024, 5:57 AM

waiting_for_identity_login_in_redux -> identity_login_dispatched

This revision was landed with ongoing or failed builds.Apr 26 2024, 8:18 AM
This revision was automatically updated to reflect the committed changes.