[Wallet] Fix firebase initialization error on iOS after reinstalling the app #1423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ror after reinstalling the app Here's what happened: - on the very first run, redeeming an invite code worked successfully and firebase could initialize correctly - after the app is deleted and reinstalled, redeeming an invite code led to a firebase initialization error (permission error in the db) - the reason for this is the firebase auth user for both session was the same since Firebase stores the auth credentials in the keychain which is not cleared when the app is uninstalled - this had the consequence of the app referencing the old address in the /users/$uid path in the db, and hence causing subsequent permission errors as the rules for changing /registrations/$address or reading /pendingRequests/$address could not be satisfied (you can only read/write them) if the address matches the address set in /users/$uid The fix here makes sure the keychain is cleared after a reinstall of the app and before we configure firebase (that's when it reads the persisted auth info from the keychain). Note that react-native-secure-key-store is already ensuring the keychain is cleared on app reinstall but it's run too late (after firebase configure has been called).
…ress This is to ensure that there's not a logic error that breaks our current assumptions in the future
jmrossy
reviewed
Oct 22, 2019
| } else if (userData.address !== undefined && userData.address !== address) { | ||
| // This shouldn't happen! If this is thrown it means the firebase user is reused | ||
| // with different addresses (which we don't want) or the db was incorrectly changed remotely! | ||
| throw new Error("User address in the db doesn't match persisted address") |
There was a problem hiding this comment.
Just want to confirm that it was intentional to throw here instead of returning undefined? This callback is what will run on the db to modify the data. I don't think the error will propagate to the app, but I could be wrong. I'm uncertain what happens if we throw here.
There was a problem hiding this comment.
Good question!
It triggers a red screen in dev, and it's not captured by the saga promise flow.
It's logged by Sentry and crashes the app. Which is what I intended as I see it like a logic error which shouldn't happen.
Codecov Report
@@ Coverage Diff @@
## master #1423 +/- ##
=========================================
Coverage ? 67.25%
=========================================
Files ? 267
Lines ? 7884
Branches ? 531
=========================================
Hits ? 5302
Misses ? 2476
Partials ? 106
Continue to review full report at Codecov.
|
jmrossy
approved these changes
Oct 23, 2019
tkporter
pushed a commit
that referenced
this pull request
Oct 24, 2019
aaronmgdr
added a commit
that referenced
this pull request
Oct 24, 2019
* master: [Wallet] Wallet can switch between hosted and local node (#1419) [Wallet] Prevent error from Avatar when name is missing (#1454) [Wallet] Show splash screen until JS is ready on iOS (#1453) Use new segment api keys used by both iOS and Android (#1452) [Wallet] Don't log all props, which includes i18n (#1445) [Helm] Updated the helm package to deploy the upgraded blockscout version (#1129) Tiny copy change (#1429) [contractkit] SortedOraclesWrapper + tests (#1405) [wallet] Refactor leftover thunk to sagas (#1388) [Wallet] Fix repeated QR code scanning and related navigation issues (#1439) [Wallet] Show the currency values with correct rounding. (#1435) [Wallet] Fix firebase initialization error on iOS after reinstalling the app (#1423) [Wallet] Use exit on iOS since we can't restart like Android (#1424) [Wallet] Update local currency styles and layout (#1325) Reset pincode cache if unlock fails (#1430)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reset iOS keychain on first app run to fix firebase initialization error after reinstalling the app.
Here's what happened:
firebase could initialize correctly
/users/$uidpath in the db, and hence causing subsequent permission errors as the rules for changing/registrations/$addressor reading/pendingRequests/$addresscould not be satisfied (you can only read/write them if the address matches the address set in/users/$uid)The fix here makes sure the keychain is cleared after a reinstall of the app and before we configure firebase (that's when it reads the persisted auth info from the keychain).
Note that react-native-secure-key-store is already ensuring the keychain is cleared on app reinstall but it's run too late (after firebase configure has been called).
Tested
Other changes
N/A
Related issues
Backwards compatibility
Yes