Skip to content
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

fix secure storage keys not being cleared on a fresh install #1732

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

fossephate
Copy link
Contributor

Issue Number (if Applicable): Fixes #

Description

Please include a summary of the changes and which issue is fixed / feature is added.

Pull Request - Checklist

  • Initial Manual Tests Passed
  • Double check modified code and verify it with the feature/task requirements
  • Format code
  • Look for code duplication
  • Clear naming for variables and methods

Copy link
Contributor

@OmarHatem28 OmarHatem28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please test this on an already existing app with some wallets and see if it will have any strange behavior

also please fix the build issue with linux

@OmarHatem28
Copy link
Contributor

ok so while reviewing this, I noticed that the main issue that this PR was made for, is not gonna be solved by this.

so the main issue is, on new installs, the app opens the pin screen and asks for pin, which from what I see can happen only if the shared prefs has a value for this key PreferencesKey.currentWalletName, or something else that is not very clear, but this doesn't seem to be a fix for it

@fossephate
Copy link
Contributor Author

IIRC, shared preferences are cleared on uninstalls, but secure storage is not (only on iOS)

This PR should fix this by deleting ALL secure storage entries when a fresh install is detected

If I'm understanding correctly you're saying we should delete the shared preference key: currentWalletName as well? I don't think it's necessary since uninstalls should clear the shared preferences but it probably can't hurt

@OmarHatem28
Copy link
Contributor

what I am saying is that the issue won't be solved with this change, because the app shows the PIN screen based on the shared prefs value, and since shared prefs value already gets cleared, then the issue is something else and not related to secure storage

@fossephate
Copy link
Contributor Author

🤔 that's weird because the issue did appear to be fixed in my testing; were you able to reproduce the issue / did the fix not work for you?

Also: this was an iOS specific bug right? Or did we have this issue pop up on android as well?

Because on iOS the shared_preferences package just wraps NSUSerDefaults and that should definitely be cleared on uninstall, the only exception I found being related to app groups:

https://stackoverflow.com/questions/24985825/nsuserdefaults-not-cleared-after-app-uninstall-on-simulator (see the 2nd reply about app groups)

@OmarHatem28
Copy link
Contributor

let's discuss it when you get back no problem, enjoy your vacation 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants