Refactor LocalData (EXPOSUREAPP-2850) #2569

jurajkusnier · 2021-03-10T14:21:22Z

Move encrypted shared preferences from LocalData object to other classes

Testing

Exhaustive Regression testing necessary
Test upgrade from the older version
Check if files are removed from the device
databases/coronawarnapp-db
shared_preferences/shared_preferences_cwa.xml

* Migrate Shared Preferences (SETTINGS) * Code refactoring * Fix tests * Migrate Shared Preferences (SETTINGS) * Cleaning code * Cleaning code

* EncryptedPreferencesMigration skeleton class * SecurityHelper return nullable SharedPreferences not an exception

# Conflicts: # Corona-Warn-App/src/androidTest/java/de/rki/coronawarnapp/util/security/DBPasswordTest.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/storage/legacy/RiskLevelResultMigrator.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/SecurityHelper.kt

…#2470 * Removed Onboarding related Properties from LocalData Added OnboardingData which provides the FlowPreferences that were defined in LocalData beforehand Currently no migration for these values is implemented Signed-off-by: Kolya Opahle <[email protected]> * Turns out MockFlowPreference and MockSharedPreferences were not usable in the device test flavour moved them into testShared Signed-off-by: Kolya Opahle <[email protected]> * OnboardingData -> OnboardingSettings OnboardingSettings: moved to get / set instead of FlowPreferences when subscribing is not needed isOnboarded is now based on onboardingCompletedTimestamp != null instead of set seperately Signed-off-by: Kolya Opahle <[email protected]> * Removed Onboarding Preference key strings from all languages Signed-off-by: Kolya Opahle <[email protected]> * Added Singleton annotation to OnboardingSettings Added migration code for OnboardingSettings to EncryptedPreferencesMigration Signed-off-by: Kolya Opahle <[email protected]>

# Conflicts: # Corona-Warn-App/src/androidTest/java/de/rki/coronawarnapp/ui/main/MainActivityTest.kt

… into feature/2850-refactor-localdata

* migrate LocalData tracing preferences to TracingSettings * update tests * Remove old code * Update tests

* Moved submission related settings from LocalData to SubmissionSettings Made BackgroundNoise injectable Refactored some of the SubmissionSettings Signed-off-by: Kolya Opahle <[email protected]> * Replaced isBeforeNow in BackgroundNoisePeriodicWorker with timeStamper.nowUTC to help with testing Signed-off-by: Kolya Opahle <[email protected]> * Fixed MainActivityTest by providing SubmissionSettings as a mockk Signed-off-by: Kolya Opahle <[email protected]> * Removed all Preference keys removed from LocalData Signed-off-by: Kolya Opahle <[email protected]> * Added migration code for SubmissionSettings to EncryptedPreferencesMigration Signed-off-by: Kolya Opahle <[email protected]> * Moved Instant conversion to extension function to satisfy detekt Signed-off-by: Kolya Opahle <[email protected]> * Removed lazy from dagger injection in BackgroundWorkSchedulerBase * Cleaned up TestResultDonorTest (Removed Instant.now() calls, fixed shouldBe case where Instant.now() would break the calculation) * Moved toInstantOrNull into TimeAndDateExtensions and used it in a few more places * Fixed a unit test that was added by the merge and fixed a merge diff that was resolved wrongly Signed-off-by: Kolya Opahle <[email protected]> * Format on commit got rid of these imports for some reason Signed-off-by: Kolya Opahle <[email protected]>

d4rken

Code looks good, still need to test on device. Few changes I'd like to discuss, see comments.

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/CoronaWarnApplication.kt

d4rken · 2021-03-11T09:35:47Z

...a-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatus.kt

            .addOnSuccessListener { cont.resume(it) }
            .addOnFailureListener { cont.resumeWithException(it) }
+            .also {
+                tracingSettings.isConsentGiven = true


Where was this set before?

It was there all the time. It was called initialTracingActivationTimestamp() before, but as we always used it only as a boolean value in TracingPermissionHelper.isConsentGiven() I rename it.
It was removed by mistake in previous PR 1b61142#diff-429f8ee04350d96dcf9c0b0176310a0cf1db823097983b0ae865463833b267dc
So you can't see the proper diff here.

...-App/src/main/java/de/rki/coronawarnapp/storage/preferences/EncryptedPreferencesMigration.kt

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/storage/preferences/PreferencesModule.kt

d4rken · 2021-03-11T10:14:29Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/SecurityHelper.kt


    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal val encryptedPreferencesProvider: (ApplicationComponent) -> SharedPreferences = {
+    internal val encryptedPreferencesProvider: (ApplicationComponent) -> SharedPreferences? = {


Please make the access & retry mechanism part of EncryptedPreferencesHelper, I think the whole object SecurityHelper can then be deleted. I think EncryptionErrorResetToolcan then also be almost deleted,EncryptedPreferencesFactoryshould then likely also be simplified and made part ofEncryptedPreferencesHelper`.

I moved SecurityHelper to EncryptedPreferencesHelper and EncryptionErrorResetTool was updated as well

.../src/test/java/de/rki/coronawarnapp/worker/DiagnosisTestResultRetrievalPeriodicWorkerTest.kt

…0-refactor-localdata

harambasicluka · 2021-03-11T14:41:00Z

@jurajkusnier test on an OnePlus 8 (Android 11). The app closed (no crash) after scanning a pending test (WRU-XD). Negative, invalid and positive work as expected. Can't reproduce it.
I send you the logs of the corresponding device via email to not expose any internal urls.

harambasicluka · 2021-03-11T14:51:18Z

I have this flickering in the submission flow: teletan & qr code.
In the video you can see it twice in the end. I'll send you the logs via email.

flickering_dialog_fragment_stuff.mp4

… into feature/2850-refactor-localdata

harambasicluka · 2021-03-11T15:15:27Z

Tested an update scenario on a Xiamoi MI9 (Android 9, WRU-XD), had a tested registered and changed some default settings and everything was like it was before the update 👍 But I also have here the flickering, will now test release/1.15.x to check if this PR is the cause.

harambasicluka · 2021-03-11T15:21:21Z

The flickering is already in release/1.15.x but not in release/1.14.x
#2569 (comment)

Fixed in: #2583

d4rken

We will follow up with another PR tomorrow.

sonarqubecloud · 2021-03-11T17:14:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

21.4% Coverage
0.0% Duplication

jurajkusnier and others added 4 commits March 4, 2021 22:15

Shared preferences migration: SETTINGS DATA (EXPOSUREAPP-5378) (#2468)

85cd4e1

* Migrate Shared Preferences (SETTINGS) * Code refactoring * Fix tests * Migrate Shared Preferences (SETTINGS) * Cleaning code * Cleaning code

LocalData Migration skeleton (EXPOSUREAPP-5295) (#2509)

adfce6a

* EncryptedPreferencesMigration skeleton class * SecurityHelper return nullable SharedPreferences not an exception

jurajkusnier added maintainers Tag pull requests created by maintainers do not merge labels Mar 10, 2021

jurajkusnier added this to the 1.15.0 milestone Mar 10, 2021

jurajkusnier and others added 7 commits March 10, 2021 15:23

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

2e07327

# Conflicts: # Corona-Warn-App/src/androidTest/java/de/rki/coronawarnapp/ui/main/MainActivityTest.kt

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

658d436

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

649f9d5

Migrate Interoperability settings

fc2f7cd

Merge remote-tracking branch 'origin/feature/2850-refactor-localdata'…

2aa9a4b

… into feature/2850-refactor-localdata

Remove tracing local data (EXPOSUREAPP-5377) (#2568)

5227363

* migrate LocalData tracing preferences to TracingSettings * update tests * Remove old code * Update tests

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

5be0a36

harambasicluka added the prio PRs to review first. label Mar 10, 2021

BMItr self-assigned this Mar 10, 2021

jurajkusnier added 4 commits March 10, 2021 20:06

EncryptedPreferencesMigration refactoring

139a0a0

DataReset implementation

b294077

Drop storage of active tracing time

cabd96c

Cleaning code

f11885e

jurajkusnier marked this pull request as ready for review March 10, 2021 20:16

jurajkusnier requested review from a team March 10, 2021 20:16

jurajkusnier added 2 commits March 10, 2021 21:25

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

41c5ac7

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

7f3194f

jurajkusnier removed the do not merge label Mar 11, 2021

d4rken self-assigned this Mar 11, 2021

d4rken self-requested a review March 11, 2021 09:10

d4rken suggested changes Mar 11, 2021

View reviewed changes

jurajkusnier and others added 9 commits March 11, 2021 12:20

Remove TODO

eb10be9

Call migration earlier

36713b4

Update tests

da285bb

Delete PreferencesModule

88672f0

Merge remote-tracking branch 'origin/release/1.15.x' into feature/285…

ce0efca

…0-refactor-localdata

Update EncryptedPreferencesMigration

44ae17c

Fix lint issue

6da9d22

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

49db47c

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

32023ba

jurajkusnier and others added 3 commits March 11, 2021 15:51

Update EncryptedPreferencesHelper

d65bd7b

Merge remote-tracking branch 'origin/feature/2850-refactor-localdata'…

4ae9975

… into feature/2850-refactor-localdata

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

9d6f364

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

d447e47

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

c914f42

d4rken approved these changes Mar 11, 2021

View reviewed changes

Merge branch 'release/1.15.x' into feature/2850-refactor-localdata

fff5005

harambasicluka approved these changes Mar 11, 2021

View reviewed changes

BMItr approved these changes Mar 11, 2021

View reviewed changes

harambasicluka merged commit f9f22f3 into release/1.15.x Mar 11, 2021

harambasicluka deleted the feature/2850-refactor-localdata branch March 11, 2021 17:19

d4rken mentioned this pull request Mar 12, 2021

Encryption migration edge case handling (DEV) #2596

Merged

Refactor LocalData (EXPOSUREAPP-2850) #2569

Refactor LocalData (EXPOSUREAPP-2850) #2569

Uh oh!

Conversation

jurajkusnier commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

d4rken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d4rken Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

jurajkusnier Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

d4rken Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

jurajkusnier Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harambasicluka commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harambasicluka commented Mar 11, 2021

Uh oh!

harambasicluka commented Mar 11, 2021

Uh oh!

harambasicluka commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d4rken left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jurajkusnier commented Mar 10, 2021 •

edited

Loading

harambasicluka commented Mar 11, 2021 •

edited

Loading

harambasicluka commented Mar 11, 2021 •

edited

Loading