Page MenuHomePhabricator

Persist SIWE backup secrets to SQLite after succesfull registration process
ClosedPublic

Authored by marcin on Apr 11 2024, 9:35 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 9:55 PM
Unknown Object (File)
Wed, Dec 4, 11:21 AM
Unknown Object (File)
Wed, Dec 4, 11:21 AM
Unknown Object (File)
Wed, Dec 4, 11:15 AM
Unknown Object (File)
Wed, Dec 4, 11:01 AM
Unknown Object (File)
Wed, Dec 4, 11:00 AM
Unknown Object (File)
Wed, Dec 4, 10:57 AM
Unknown Object (File)
Wed, Dec 4, 10:56 AM
Subscribers

Details

Summary

This differential saves generated backup secrets in registration context which then persists it into SQLite when the regiatration is complete and database is
correctly re-created.

Test Plan
  1. Add the following gist: https://gist.github.com/marcinwasowicz/dfd0f0c213e662d9c91289a5ee083820. It creates a button that enables to print siwe backup secrets to the console.
  2. Execute registration flow.
  3. Tap the button. See siwe backup secrets in the console.
  4. Log out. Don't kill the app!
  5. Log in to the same SIWE account.
  6. Tap the button ensure that there is no backup secret in the db.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Clear backup secrets when user logs out.

atul added inline comments.
native/account/registration/registration-context-provider.react.js
51 ↗(On Diff #39065)

Think this is redundant since the isLoggedIn selector below includes state.storeLoaded:

8c0f7f.png (292×1 px, 55 KB)

This revision is now accepted and ready to land.Apr 11 2024, 6:53 PM
ashoat requested changes to this revision.Apr 12 2024, 8:50 AM

Just a question

native/account/registration/registration-context.js
15–16 ↗(On Diff #39077)

All of the other data from registration screens is cached in CachedUserSelections. Why are we handling siweBackupSecrets differently?

This revision now requires changes to proceed.Apr 12 2024, 8:50 AM
native/account/registration/registration-context.js
15–16 ↗(On Diff #39077)

We can go with CachedSelections.

Use cachedSelections to keep siweBackupSecrets

ashoat requested changes to this revision.Apr 15 2024, 10:10 AM
ashoat added inline comments.
native/account/registration/registration-context-provider.react.js
72 ↗(On Diff #39125)

This is weird... why would we clear the user selection here?

The intention of userSelections is to make sure that the user isn't forced to re-enter this data later. Are we able to cache it, such that if the user goes back to before this screen, and then advances to this screen again, the SIWE flow does not need to be re-entered?

79–86 ↗(On Diff #39125)

Why does`siweBackupSecrets` need to be handled separately here? It seems like we should handle clearing all of the cached selections together. This is tracked in ENG-7389, some perhaps this can just be removed here in favor of it being handled in a more generic way later. Alternately, we could consider just setCachedSelections({})?

This revision now requires changes to proceed.Apr 15 2024, 10:10 AM
native/account/registration/registration-context-provider.react.js
72 ↗(On Diff #39125)

why would we clear the user selection here?

We need to somehow clear siweBackupSecrets after persisting them to SQLite. Imagine the case when user registers, siweBackupSecrets is persisted to SQLite. Then the user logs out and logs in to another siwe account without closing the app in the process. In such case we would be keeping secret data for previous user in memory which sounds like a risk to me. Additionally it must be noted that siweBackupSecrets are the only data that we query for if the user is already logged in but they are missing. That said, keeping this data in memory between log out and log ins will make it harder to correctly detect if we should be forcing user to create the data or not.

Are we able to cache it, such that if the user goes back to before this screen, and then advances to this screen again, the SIWE flow does not need to be re-entered?

This code will not clear this data until they are persisted in SQLite, which will not happen until user becomes logged in (registration is complete). That said we can use this data in a way you described.

The effect below is somehow redundant so we can potentially remove it but its intention is the same - to prevent siweBackupSecrets from being kept in memory if the user logs out. We can either remove the effect below or remove setting to undefined above.

79–86 ↗(On Diff #39125)

I think that my previous comment explains why this data receives special treatment. Agree that doing ENG-7389 would solve the problem. However we would need to solve it in this diff since unlike for other data in cachedSelections keeping siweBackupSecrets in memory could produce regressions.

Alternately, we could consider just setCachedSelections({})?

Open to doing it in this diff.

  1. Clear all cached selections at once after user becomes logged in. Persist siweBackupSecrets in SQLite beforehand.

Are we able to cache it, such that if the user goes back to before this screen, and then advances to this screen again, the SIWE flow does not need to be re-entered?

Thanks for pointing this out. I haven't thought about this earlier. Could you share some additional info about expected user experience in such case? For instance - if the user creates backup message, goes back to change the avatar should we then just skip create backup message screen or potentially display different UI?

ashoat requested changes to this revision.EditedApr 16 2024, 7:07 AM

Are we able to cache it, such that if the user goes back to before this screen, and then advances to this screen again, the SIWE flow does not need to be re-entered?

Thanks for pointing this out. I haven't thought about this earlier. Could you share some additional info about expected user experience in such case? For instance - if the user creates backup message, goes back to change the avatar should we then just skip create backup message screen or potentially display different UI?

We should handle it the same way we handle the SIWE screen currently. It would show two options: use existing backup message or create new one

native/account/registration/registration-context-provider.react.js
50–79

Instead of handling this here, should it be handled inside useRegistrationServerCall?

Reasons:

  1. Unifies with the rest of the logic
  2. Avoids persisting to SQLite database which may be cleared prior to registration (see here)
  3. Avoids need to separate clear cached selections
This revision now requires changes to proceed.Apr 16 2024, 7:07 AM
native/account/registration/registration-context-provider.react.js
50–79

I will look into useRegistrationServerCall but two notes on this:

Avoids persisting to SQLite database which may be cleared prior to registration

  1. This code prevents from persisting to SQLite before registration is completed and SQLite becomes stable.
  2. We will eventually need some form of logic above for D11645. Potentially we could move it entirely to MissingRegistrationDataHandler but we will need something similar anyway.

Move siwe secrets persistence and cached selections clearing to registration server call.

native/account/registration/registration-server-call.js
289 ↗(On Diff #39209)

I opted to extend this step instead of creating new one since both siwe backup secrets persistence and avatar upload require the same state of redux returned by hasCurrentUserInfo selector.

ashoat requested changes to this revision.Apr 18 2024, 11:42 AM

Getting close!

native/account/registration/registration-server-call.js
289 ↗(On Diff #39209)

The hasCurrentUserInfo stuff is broken actually for avatar uploads. We need the client to be auth'd with the authoritative keyserver in order to set the avatar, since the avatar setting is a keyserver call.

I have a diff in my local copy to change this to instead check if the user has a user cookie (is authed) with the authoritative keyserver. I think your stuff will need to be a separate step, since it needs to be checking hasCurrentUserInfo. Can you separate it out into its own step?

native/account/registration/registration-terms.react.js
57–64 ↗(On Diff #39209)

Why is this being "drilled" through so many layers? Can't we access RegistrationContext directly in useRegistrationServerCall?

This revision now requires changes to proceed.Apr 18 2024, 11:42 AM
native/account/registration/registration-server-call.js
289 ↗(On Diff #39209)

Here's the diff I referenced above: D11693

native/account/registration/registration-terms.react.js
57–64 ↗(On Diff #39209)

This was actually my first thought but then I noticed that we could probably access RegistrationContext directly to get other variables as well so I decided to introduce this additional layer of abstraction to meet the convention for other variables.

native/account/registration/registration-terms.react.js
57–64 ↗(On Diff #39209)

Ahhh, we probably can't since RegistrationContext exposes register property which is bound to useRegisterServerCall therefore we can't access it directly in useRegisterServerCall. Am I missing something?

native/account/registration/registration-terms.react.js
57–64 ↗(On Diff #39209)

Ah yes, you're right

Introduce two new steps: SQLite data persistence and clearing cached selections

It wasn't easy to introduce new step here. If this solution is not accepted I will just introduce new step that clears cached Selections in the same promise as sqlite persistence and leave avatar upload unchanged.

Add finally statements to set gurd refs to false eventually.

ashoat requested changes to this revision.Apr 19 2024, 8:40 AM

I'm rethinking whether this should have been a separate step, and now considering whether it should be combined with step 1. My apologies for not considering this earlier. I've tried to leave some more detailed comments in this review, and in particular suggest you read the code comment pointing out by my first inline comment below – it gives some context on the "step" mechanism we're using here

native/account/registration/registration-server-call.js
49–52 ↗(On Diff #39278)

The reason for the "steps" is explained here. If we don't have this problem for one of the steps you introduced, we should avoid introducing a new step for it, and instead just include the work in the prior step

312 ↗(On Diff #39278)

Seems like a typo here... did you mean sqlitePersistenceBeingHandledRef?

333–335 ↗(On Diff #39278)

Question for you: is there a reason we need to wait for hasCurrentUserInfo before calling this? What would happen if we called it on line 284?

415 ↗(On Diff #39278)

I don't understand why this needs to be a separate step. What would occur if just did all of step 4 on line 399?

This revision now requires changes to proceed.Apr 19 2024, 8:40 AM
native/account/registration/registration-server-call.js
333–335 ↗(On Diff #39278)

Initially I was under the impression that this condition will protect us from SQLite database being deleted after siwe secrets are persisted. Now after looking at sqlite-data-handler.js code it looks like deletion is only triggered when database previously contained non-anonymous user id. So I think it is safe to put persistence call directly in returnFunc. I tested it and it works.

415 ↗(On Diff #39278)

My initial idea was that it feels natural that clearing cached selections should be the last operation done here. If someone introduces new step after avatar upload that relies on cached selections they might forget about moving it. With separate step for clearing cached selections it is harder to overlook.

  1. Put SIWE backup secrets persistence to step 1.
  2. Put clearing cached selections to step 2.

Thanks, and sorry for all the iteration here!

This revision is now accepted and ready to land.Apr 19 2024, 12:26 PM