[lib][native] Defer saving user credentials until successful auth
ClosedPublic
Actions

Authored by ashoat on Apr 24 2024, 12:42 PM.

Details

Reviewers

tomek
inka

Commits

rCOMM424ea6f92e58: [lib][native] Defer saving user credentials until successful auth

Summary

We're currently saving the user credentials after a successful auth with identity. This diff makes it so we wait until the whole auth concludes before we save the user credentials. This allows us to simplify some code, and is more consistent with how we're handling other side effects of auth: treating the auth as a single cohesive flow, and not considering it successful unless all parts succeed.

Depends on D11765

Test Plan

This whole stack was tested with the following steps:

On native, attempting to log in with an ETH account that hasn’t been registered yet
On native, attemping to register with an ETH account that has already been registered
On native, log in with an ETH account that has already been registered
On native, register with an ETH account that hasn’t already been registered
On native, register with a password account
On native, log in with a password account
On web, attempting to log in with an ETH account that hasn’t been registered yet
On web, log in with an ETH account that has already been registered
On web, log in with a password account

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Apr 24 2024, 12:42 PM

ashoat added a child revision: D11767: [native] Use useRegistrationServerCall in FullscreenSIWEPanel for identity registration.

ashoat mentioned this in D11765: [lib][native][web] Introduce usePasswordLogIn hook.Apr 24 2024, 12:49 PM

Harbormaster completed remote builds in B28442: Diff 39452.Apr 24 2024, 1:12 PM

ashoat requested review of this revision.Apr 24 2024, 1:12 PM

inka added inline comments.Apr 25 2024, 3:57 AM

lib/hooks/login-hooks.js
66 ↗	(On Diff #39452)	It feels inconsistent that we set the `currentUserInfo` between identity and keyserver login, but wait to set user credentials. Why do we want to do that? The code in this diff doesn't seem to be simplified very much, but seems to introduce this inconsistency

ashoat added inline comments.Apr 25 2024, 5:59 AM

lib/hooks/login-hooks.js
66 ↗	(On Diff #39452)	We set `currentUserInfo` immediately after identity auth because on web, it's the only place where we can store the `userID` in order to allow it to be used for Comm services auth. I had initially explored deferring setting `currentUserInfo` until after keyserver auth, but cancelled that task when I realized this. See this comment for more details. As for why we want to defer setting user credentials, this is described in the diff description: is more consistent with how we're handling other side effects of auth: treating the auth as a single cohesive flow, and not considering it successful unless all parts succeed Where `currentUserInfo` being set is an implementation detail not visible to the user, saving the credentials is something that is visible to the user. The user does not understand the distinction between identity auth and keyserver auth; in their perception, if keyserver auth fails then that just means auth failed. It feels weird to have the credentials saved sometimes when auth fails, but not always.