-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use RNSecureKeyStore for auth token persistent storage #3122
base: main
Are you sure you want to change the base?
Conversation
I quickly skim through the PR, first comments are:
|
General approach looks good! I think the logic is correct, and it will actually delete the token from local storage however the code may require some type changes and name changes for clarity. In the past the |
Thanks for the feedback @nicolasburtey and @UncleSamtoshi
I'll add the tests once the logic is approved
It is removed here:
The
I think the PersistentState is still a direct mapping to something that has been stored locally. The difference now is that we are using two storage methods, one secure and one non-secure. As I see, the data passed to migrateAndGetPersistentState is not being manipulated, just gathered from two different sources.
Combining the secure storage state and local storage state after Do you think that changing |
Previously the PersistentState was coupled directly to the local storage which is under a version control that is possible to be migrated. The secure storage state isn't under a version control which I think is okay because its so simple. However I think it is valuable to keep the integrity of the version control for local storage and be explicit about what is stored in each place. This could look like PersistentState staying the same type, but then renaming all the existing PersistentState stuff related to local storage. For example Perhaps this is clunky and there is a refactor that simplifies it all, but I think as long as the version controlled local storage state exists, it makes sense for us to explicitly know what is stored in local storage and what is stored in secure storage. Any thoughts @nicolasburtey |
that is fine, but tests are often the best way to make explicit what the logic aim to be
yup thanks I see that now. I missed the destructuring
At a minimum, Another thing that come to mind: there has been a frequent user request to be able to have several account on the mobile wallet at once. one for personal for professional for instance. in that case, we would need to store 2 tokens in the mobile app. how does the solution we would use here would make it possible or not an upgrade to a multi account solution? Maybe it's just a side point, not sure it's worth diving too much into it. |
* chore: tilt ci process is spinning up backend deps ci: fix android/mac setup to open sims, upload rec ci: test chore: try macos ci: test if everything works ci: upload recordings test try emulator creation manually export JAVA_HOME="/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home" ci: single e2e test export JAVA_HOME="/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home" * test(e2e): auth flow * test(e2e): intraledger flow ci: run e2e for all prs * test(e2e): conversion flow * test(e2e): send flow chore: better chore: better chore: external invoices chore: send onchain ci: update chore: fix-code chore: recv test: destroy backend save check code save chore: update quickstart which also fix funding issue fix: e2e change ios device more time tests save test test pls god save ci: separate test: fix * gha fix? builds * Update app/screens/send-bitcoin-screen/send-bitcoin-details-screen.tsx --------- Co-authored-by: nicolasburtey <[email protected]>
* chore: updating react native vision library to 3.9.1 * chore: fix codegen --------- Co-authored-by: Nicolas Burtey <[email protected]>
Co-authored-by: Nicolas Burtey <[email protected]>
Co-authored-by: Nicolas Burtey <[email protected]>
* chore: [skip CI] Translate en.json in el [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'el'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in hu [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'hu'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in es [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'es'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in tr [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'tr'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in sw [Manual Sync] 98% of minimum 98% translated source file: 'en.json' on 'sw'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in pt [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'pt'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format * chore: [skip CI] Translate en.json in ca [Manual Sync] 99% of minimum 98% translated source file: 'en.json' on 'ca'. Sync of partially translated files: untranslated content is included with an empty translation or source language content depending on file format --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Co-authored-by: Nicolas Burtey <[email protected]>
* chore: misc update * feat: conversation screen POC * chore: renaming conversation to chatbot --------- Co-authored-by: Nicolas Burtey <[email protected]>
Co-authored-by: CI Bot <[email protected]>
Signed-off-by: Prakhar Agarwal <[email protected]>
…ney#3145) * feat: Add link opening functionality in ScanningQRCodeScreen Signed-off-by: Prakhar Agarwal <[email protected]> * fix: add translations Signed-off-by: Prakhar Agarwal <[email protected]> * fix: handle different types of destinations Signed-off-by: Prakhar Agarwal <[email protected]> --------- Signed-off-by: Prakhar Agarwal <[email protected]>
* chore: [skip CI] Translate en.json in ca 100% translated source file: 'en.json' on 'ca'. * chore: [skip CI] Translate en.json in es 100% translated source file: 'en.json' on 'es'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
* feat: settings screen refactor feat: account banner on settings screen feat: blink address feat: account section ported chore: stuff feat: preferences chore: security and privacy feat: export tx feat: login from banner chore: empty right feat: need help fix: color profile for contact modal feat: community fix: tsx issue chore: small stuff feat: acc screen, auth email feat: auth flow working feat: tx limits feat: perfected delete flow chore: some more quirks feat: upgrade trial acc chore: upgrade for l1 feat: port complete fix: check-code fix: email issue feat: ln addr chore: addressing comments feat: totp feat: refetch totp and dont use caching fix: check-code fix: totp stuff Update app/components/modal-tooltip/modal-tooltip.tsx Co-authored-by: nicolasburtey <[email protected]> Update app/i18n/en/index.ts Co-authored-by: nicolasburtey <[email protected]> Update app/i18n/en/index.ts Co-authored-by: nicolasburtey <[email protected]> Update app/screens/settings-screen/account/settings/phone.tsx Co-authored-by: nicolasburtey <[email protected]> Update app/screens/settings-screen/settings/account-ln-address.tsx Co-authored-by: nicolasburtey <[email protected]> fix: comments test test(e2e): updated settings screen (GaloyMoney#2922) chore: adding storybook test chore: addressing comments fix: typo fix: typo fix: typo test: e2e test: e2e test: e2e test: e2e test: e2e fix: flickering issue save test: e2e chore: login method refresh in case test: group suffix removed * chore: LL utils - finding unused keys and removing Blink Address ones * chore: comments * chore: addressing most comments * chore: security screen * fix: rebased
* feat: show animations chore: initial draft chore: has the full animation flow chore: a little more optimization process the status text as before feat: all animations added done * send payment detail around * save * chore: getting transaction * feat: transaction detail linked * test: fix for sends * feat: added haptics * chore: translations * chore: fixes * chore: addressing comments and removing commented code
…oney#3156) * fix: no pulse at end * chore: animation delayed
* ci: upload recordings to gcs * test
* chore: restructuring data around settings screen * fix: e2e test
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
- | Generic High Entropy Secret | 06c91ed | dev/vendor/galoy-quickstart/dev/core-bundle/integration-env.json | View secret | |
- | Generic High Entropy Secret | 06c91ed | dev/vendor/galoy-quickstart/dev/core-bundle/serve-env.json | View secret | |
- | Generic High Entropy Secret | 06c91ed | dev/vendor/galoy-quickstart/dev/Tiltfile | View secret | |
- | Generic Private Key | 06c91ed | dev/vendor/galoy-quickstart/dev/config/notifications/fake_service_account.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
…24789/galoy-mobile into 848-use-react-native-keychain
Changes look good, thanks for making the state more explicit! Looks like check code and tests are still failing and need to be fixed however. You should be able to run these checks locally and get them working. |
PersistentState, | ||
} from "./state-migrations" | ||
|
||
const PERSISTENT_STATE_KEY = "persistentState" | ||
|
||
const loadPersistentState = async (): Promise<PersistentState> => { | ||
const data = await loadJson(PERSISTENT_STATE_KEY) | ||
return migrateAndGetPersistentState(data) | ||
const localStorageState = await migrateAndGetLocalStorageState(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as the code is written now, it will log people out of the app. I believe it will load the local storage state which no longer has the token and then will it will load the secure storage stage which should start as empty, as a result a user will be unexpectedly logged out. Not totally sure what the correct solution to this is. Maybe as part of the migrating the local storage state it should also save the token to the secure storage state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. The migration will remove the token. I think it makes sense to save the token in secure storage as part of the migration. I'll add that. Thanks!
@2624789 what's the state of this PR? Is there any way we can accelerate this? |
Related issue: #848
KeyStoreWrapper was extended to store/retrieve/remove secure state data.