Encrypt seed #192
Merged
Encrypt seed #192
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
sasho-axellero
requested review from
abizjak,
ali-concordium and
bisgardo
and removed request for
abizjak
bisgardo
reviewed
Feb 6, 2023
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
…references.kt simplify delete and save Co-authored-by: Michael Bisgaard Olesen <michaelbisgaardo@gmail.com>
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
bisgardo
reviewed
Feb 8, 2023
There was a problem hiding this comment.
I don't really understand the flow of where the seed phrase is supposed to be migrated; it seems like it's happening all over the place.
We have tryToSetEncryptedSeedPhrase, setSeedPhraseEncrypted, and checkAndTryToEncryptSeed that all write the encrypted phrase. And same with getSeedPhrase and getSeedPhraseDecrypted for reading it. I can't really see any logic in which variant is used where, and some of them additionally check and delete the unencrypted one.
I feel like there should just be a migration function somewhere in the initial code path that does the following:
- Check if the encrypted seed is stored. Return if it is.
- Otherwise, encrypt the unencrypted seed and store it.
- Decrypt the stored encrypted seed and compare it against the unencrypted one. Delete the latter if they match.
After this, we just pretend like the unencrypted one never existed. No need for fallbacks to the unencrypted seed.
You can also choose to handle it gracefully if storing the encrypted seed failed. Then you would probably do item 3 above just before returning in item 1. And keep the fallbacks.
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
|
There are quite a few comments. Let's have a live session tomorrow, where we talk instead of type :-) |
…entication Screen until the old seed is encrypted and deleted. Added error status and error explanation for failed seed encryption on the Authentication Screen
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/passphrase/recover/PassPhraseRecoverViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/auth/login/AuthLoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Show resolved
Hide resolved
ali-concordium
approved these changes
Feb 13, 2023
app/src/main/java/com/concordium/wallet/ui/passphrase/recover/RecoverWalletActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/passphrase/recover/PassPhraseRecoverInputFragment.kt
Show resolved
Hide resolved
bisgardo
approved these changes
Feb 21, 2023
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/data/preferences/AuthPreferences.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/auth/login/AuthLoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/auth/login/AuthLoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/auth/login/AuthLoginViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/passphrase/recover/RecoverWalletActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/passphrase/setup/SetupWalletActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/concordium/wallet/ui/auth/login/AuthLoginActivity.kt
Outdated
Show resolved
Hide resolved
…references.kt Removed encrypted seed check Co-authored-by: Michael Bisgaard Olesen <michaelbisgaardo@gmail.com>
…nActivity.kt Co-authored-by: Michael Bisgaard Olesen <michaelbisgaardo@gmail.com>
Added comment to checkAndTryToEncryptSeed. Named parameter in saveSeed lambda.
bisgardo
approved these changes
Feb 21, 2023
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.
Purpose
Encrypt the seed and save it securely.
Changes
Encrypt seed before saving.
Encrypt already existing seed.
Re-encrypt seed with new password when changing passwords.
Checklist
hard-to-understand areas.