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

Revert recreating the encrypted token storage on initialisation error. #61

Closed
wants to merge 1 commit into from

Conversation

zamzterz
Copy link
Contributor

@zamzterz zamzterz commented Nov 28, 2022

If the EncryptedSharedPreferences can't be created, try to delete the existing file and delete the master key.
See tink-crypto/tink#535 for more info.

This might happen when a user uninstalls an app using this SDK, and then re-installs the app again.

Follow up of #50 and related to #48.

If the EncryptedSharedPreferences can't be created, try to delete
the existing file and delete the master key.
See tink-crypto/tink#535 for more info.
Copy link
Contributor

@xserxses xserxses left a comment

Choose a reason for hiding this comment

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

This mechanism caused random logouts during summer 2022 (SDK 2.4.3, 2.4.4) and huge drop of login rate in Schibsted publishers.

Bringing it back to SDK should be discussed internally.

@abdurahmanadilovic
Copy link
Contributor

This mechanism caused random logouts during summer 2022 (SDK 2.4.3, 2.4.4) and huge drop of login rate in Schibsted publishers.

Bringing it back to SDK should be discussed internally.

The alternative is to cause crashes every time the app starts, at least that's what's happening to Podme. So the app becomes unusable until users manually clear app data. Reinstalling the app will still cause the app to crash on start

@xserxses
Copy link
Contributor

xserxses commented Nov 29, 2022

If my understanding is correct, disabling backup of Shared Preferences in Android Manifest should mitigate this issue.

Proposed solution solving edge case will cause severe business impact on other users of the SDK.

GeneralSecurityException is to wide exception to always log user out. If we'll go with this solution is should be exception specyfic to this case.

@abdurahmanadilovic
Copy link
Contributor

abdurahmanadilovic commented Nov 29, 2022

If my understanding is correct, disabling backup of Shared Preferences in Android Manifest should mitigate this issue.

GeneralSecurityException is to wide exception to always log user out. If we'll go with this solution is should be exception specyfic to this case.

We tried to set allowBackup=false and it made no difference. We could still recreate the issue by reinstalling the app multiple times. Besides, what's the benefit of ignoring this, the encrypted storage can't be recovered anyways?

@abdurahmanadilovic
Copy link
Contributor

abdurahmanadilovic commented Nov 29, 2022

Another alternative is to stick with a nonproblematic version of the Tink library

@xserxses
Copy link
Contributor

Besides, what's the benefit of ignoring this, the encrypted storage can't be recovered anyways?

The problem with propsed soultion is it will log out not only in case You've described but others as well. And based on experience from summer - this is not always non recoverable cases - as GeneralSecurityException covers multiple scenarios. It should be narrowed down.

In general - we should aim to solve this specyfic case instead of logging people out as default solution...

@zamzterz
Copy link
Contributor Author

Closing this as work is ongoing to replace the storage backed by EncryptedSharedPreferences.

@zamzterz zamzterz closed this May 11, 2023
@zamzterz zamzterz deleted the revert-rebuild-encrypted-storage branch May 11, 2023 07:53
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.

None yet

3 participants