Page MenuHomePhabricator

Force already logged in ETH users to create and sign backup message
ClosedPublic

Authored by marcin on Apr 12 2024, 5:41 AM.
Tags
None
Referenced Files
F3355975: D11645.diff
Sat, Nov 23, 4:23 PM
Unknown Object (File)
Thu, Nov 21, 7:06 PM
Unknown Object (File)
Thu, Nov 21, 2:14 PM
Unknown Object (File)
Thu, Nov 21, 2:03 PM
Unknown Object (File)
Thu, Nov 21, 12:33 PM
Unknown Object (File)
Fri, Nov 15, 4:50 AM
Unknown Object (File)
Wed, Nov 13, 11:42 AM
Unknown Object (File)
Fri, Nov 8, 8:23 AM
Subscribers

Details

Summary

This differential introduces component that checks if backup message and its signature is in the database if the user is ETH user. In case it is not user is
navigated to screen with backup message creation.

Test Plan
  1. Log in to SIWE account.
  2. Ensure that screen with backup message creation is displayed.
  3. Create backup message.
  4. Ensure screen doesn't appear again.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-5267
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Can you share a screenshot of what this looks like?

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
15 ↗(On Diff #39076)

I like the generic name here... hopefully we can reuse this component for other parts of the registration flow where we might want to get data from existing users

tomek requested changes to this revision.Apr 15 2024, 1:08 AM
tomek added inline comments.
native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
30–33 ↗(On Diff #39076)

What is the relationship between siweBackupSecrets and nativeSIWEBackupSecrets? Why do we have to check nativeSIWEBackupSecrets? The async logic in this effect could cause a navigate to be called multiple times - if we can simply rely on siweBackupSecrets check, we can avoid this risk - can we do that?

native/account/registration/missing-registration-data/missing-siwe-backup-message.react.js
24 ↗(On Diff #39076)

Is this something we persist? Wondering how the app behaves after reopening.

27 ↗(On Diff #39076)

This doesn't need to be an async function

This revision now requires changes to proceed.Apr 15 2024, 1:08 AM
native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
30–33 ↗(On Diff #39076)

siweBackupSecrets - backup message and its signature that is created by CreateSIWEBackupMessage component (earlier in the stack). They are passed to RegistrationContext so that it can persist them in SQLite.

nativeSIEEBackupSecrets - backup message and its signature that are already persisted in SQLite.

Why do we have to check nativeSIWEBackupSecrets?

If this data is not present in SQLite we have to force user to create it by navigating to relevant screen. However there can be a case that user is in the process of registration. In such case siweBackupSecrets may be present but they haven't been persisted yet in SQLite but they are soon going to be. On the other hand I am starting to wonder if we actually need this check - wouldn't !loggedIn || accountHasPassword(currentUserInfo) be enough.

native/account/registration/missing-registration-data/missing-siwe-backup-message.react.js
24 ↗(On Diff #39076)

This data is not persisted in JS - it is only used to temporarily keep data before they are stored in SQLIte. It would be best to take a look at this diff: https://phab.comm.dev/D11638 since it introduces and handles this data,

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
30–33 ↗(On Diff #39076)

I think that !loggedIn || accountHasPassword(currentUserInfo) will suffice.

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
30–33 ↗(On Diff #39076)

Please ignore the comment above - it was my mistake. This condition is necessary to prevent this screen from showing after successful registration if loggedIn value happens changes before data becomes present in SQLite.

Bring back deleted condition that is actually necessary.

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
25 ↗(On Diff #39128)

Let me address this comment from @tomek again:

cachedSelections.siweBackupSecrets - during registration CreateSIWEBackupMessage component will create backup message and its signature. It will pass it to RegistrationContext (by setCachedSelections) which will put it to SQLite and set this data to undefined.

nativeSIWEBackupSecrets - backup message and its signature fetched from SQLite. If the user is ETH user we check if this data is present in SQLite. If it is not there we need to trigger its creation by navigating to the screen responsible for their creation. However we also need to check for cachedSelections.siweBackupSecrets. Imagine the case that during registration loggedIn becomes truth before this data is persisted to SQLite. In such case we need to check first in we aren't in the process of persisting siweBackupSecrets to SQLIte.

tomek added inline comments.
native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
25 ↗(On Diff #39128)

It's still not clear to me why we can't keep a check of siweBackupSecrets and avoid checking getSIWEBackupSecrets() - what would be the consequences?

This revision is now accepted and ready to land.Apr 16 2024, 3:27 AM
native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
25 ↗(On Diff #39128)

If the user closes the app and then reopens it then siweBackupSecrets are missing but nativeBackupSecrets will return data that is persisted in SQLite (in case it was persisted).

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
25 ↗(On Diff #39128)

siweBackupSecrets in temporarily present in the memory during registration. After registration is complete this data is deleted from memory and persisted to SQLite. Subsequent runs of the app won't have this data in the memory but will have it in SQLite. So the decision whether to force already logged in user to generate this data or not cannot be made based on siweBackupSecrets alone. We must look for this data in SQLite.

native/account/registration/missing-registration-data/missing-registration-data-handler.react.js
25 ↗(On Diff #39128)

Ok, makes sense, thanks!

It looks like this info is irrelevant, but just in case I wanted to share it: note that because of the nonce verification logic during login on the identity service, a signed ETH message for login is only valid for 120 seconds after the nonce was first delivered.

I'm not sure if the backup service has similar logic. We should be mindful of this behavior when thinking about how we persist signed ETH messages.

Persist directly to SQLite when forcing user to create backup message signature.