support local storage fireproofing #3612

brindy · 2024-11-22T19:32:45Z

Task/Issue URL: https://app.asana.com/0/392891325557410/1204888209490842/f
Tech Design URL:
CC: @samsymons, @bwaresiak

Description:

Revert to using default persistence container for website data storage.

Also various refactoring to make future singleton slaying quests a bit easier.

Steps to test this PR:

Using an older version of the app (any before this PR should be fine). Open some tabs, sign into to some websites, and fire proof them. Check the fire proofing is working by using the fire button and leave the tabs open to simulate users receiving this update. Then update the app to this branch.
Use the fire button and ensure previously signed in sites still work as before.
Use the Fireproofing UI in settings to remove fireproofed sites and use the fire button to clear them, ensuring their data has been cleared.
Use the Cookies UI in debug to check there's no cookies.
Navigate to https://privacy-test-pages.site/features/local-storage.html and start the counter.
Use the fire button and navigate back to https://privacy-test-pages.site/features/local-storage.html - previous values should be removed.
Fireproof the site and start the counter. Wait for a while, then use the fire button. Navigate back to https://privacy-test-pages.site/features/local-storage.html and the previous values should be set.
Remove fireproofing for the site and use the fire button. Navigate to it once more and ensure the values are again unset.
Do similar smoke testing using the fire button on the tab switcher
Do so similar smoke testing using auto-clear setting on
Run the release_tests/local-storage UI test and validate it works

Definition of Done (Internal Only):

Does this PR satisfy our Definition of Done?

Copy Testing:

Use of correct apostrophes in new copy, ie ’ rather than '

Device Testing:

iPhone
iPad (smoke testing)

OS Testing:

iOS 15
iOS 16 (option)
iOS 17 (option)
iOS 18

Internal references:

Software Engineering Expectations
Technical Design Template

github-actions · 2024-11-22T19:34:21Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against ffe3448

jaceklyp · 2024-11-29T09:09:22Z

Core/WebCacheManager.swift

+    ///
+    /// The migration code removes the key that is used to check for the isConsumed flag so will only be
+    ///  true if the data needs to be migrated.
+    public func consumeCookies(intoHTTPCookieStore httpCookieStore: WKHTTPCookieStore) async {


httpCookieStore is strongly typed, so I believe it should be consumeCookies(into httpCookieStore: WKHTTPCookieStore) async {

jaceklyp · 2024-11-29T09:13:34Z

Core/DataStoreIdManager.swift

+
+/// Supports an existing ID set in previous versions of the app, but moving forward does not allocate an ID.  We have gone back to using the default
+///  peristence for the webview storage so that we can fireproof types that don't have an API for accessing their data. (e.g. localStorage)
+public protocol DataStoreIdManaging {


nitpick; I think we should be using ID (not Id) just like Apple uses it in their libs, e.g. UUID or https://developer.apple.com/documentation/swiftui/scrollposition/viewid(type:)

jaceklyp · 2024-11-29T09:28:10Z

Core/Fireproofing.swift

@@ -51,15 +54,19 @@ public class UserDefaultsFireproofing: Fireproofing {
        }
    }

+    var allowedDomainsIncludingDuckDuckGo: [String] {


jaceklyp · 2024-12-02T12:42:52Z

Core/HTTPCookieExtension.swift

+    static func cookieDomain(_ cookieDomain: String, matchesTestDomain testDomain: String) -> Bool {
+        return testDomain == cookieDomain
+            || ".\(testDomain)" == cookieDomain
+            || (cookieDomain.hasPrefix(".") && testDomain.hasSuffix(cookieDomain))
+    }


could you provide a piece of documentation what do we test here for and possible e.g of what matches means for us

added comment and note to look at some unit tests

jaceklyp · 2024-12-02T14:48:15Z

Core/MigratableCookieStorage.swift

+    /// Called after consuming cookies but migration has not happened.
+    /// This can happen in the following flow
+    /// * User presses fire button on container based app and then backgrounds it/switches to another app
+    /// * Installs update to this version of the app without using the browser
+    /// * Starts browsing
+    func setConsumed() {
+        resetStore()
+    }


im a bit confused, should anything else happen here besides "resetStore"?

No, just wanted an API that made sense from where it was used. This is "behinds the scenes" behaviour related to migration.

jaceklyp · 2024-12-02T14:53:45Z

Core/WebCacheManager.swift


-        var uuids = await WKWebsiteDataStore.allDataStoreIdentifiers
+        // Check version here rather than on function so that we don't need complicated logic related to verison in the calling function
+        guard #available(iOS 17, *) else { return }


why do we need this check?

Minor optimisation - < iOS 17 no migration is needed. Will add comment.

jaceklyp

LGTM :)

* release/7.148.0: Release 7.148.0-1 (#3661) Make a simple state machine to identify incorrect transitions (#3660) support local storage fireproofing (#3612) Make NTP Grid layout adaptive for various screen sizes (#3657) Release 7.148.0-0 (#3659) Bump BSK, add Free Trials Feature Flag (#3655)

* main: Release 7.148.0-2 (#3676) Merge hotfix changes from 7.147.1 (#3674) Add `install` pixel (#3656) AI Chat browsing menu (#3635) Release 7.147.1-0 (#3672) Use actor isolation for `SubscriptionFeatureMappingCache` (#3670) cherry picked - already approved in #3666 fix suggestion scrolling (#3654) Update autoconsent to v12.1.0 (#3650) Malware protection: bump BSK (#3617) Release 7.148.0-1 (#3661) Make a simple state machine to identify incorrect transitions (#3660) support local storage fireproofing (#3612) Make NTP Grid layout adaptive for various screen sizes (#3657) Release 7.148.0-0 (#3659) Bump BSK, add Free Trials Feature Flag (#3655)

brindy added 3 commits November 22, 2024 13:48

initial refactoring and migration code

1e73bca

migration logic and refactoring

fcbdf72

update unit tests

f80bf7b

brindy added 12 commits November 22, 2024 19:41

swiftlint

c52d9a5

re-use macos implementation

1f9458e

added tests but need an abstraction for the data store container

327a12f

Merge branch 'main' into brindy/support-local-storage-fireproofing

1660429

abstract datastore removal to make it testable

7ec9cde

mock the observations data

5de2087

re-use domain cookie matching code to fix long-standing bug

7ea99d1

merge main

62d96a0

merge main

4d88a1e

containers need to be removed on the main actor

b3e58e1

remove print statements

0454552

update maestro and add local storage test

f54fb19

brindy marked this pull request as ready for review November 29, 2024 01:08

brindy requested a review from jaceklyp November 29, 2024 01:09

Merge branch 'main' into brindy/support-local-storage-fireproofing

5874ffd

jaceklyp requested changes Dec 2, 2024

View reviewed changes

address feedback comments

42b7063

brindy requested a review from jaceklyp December 2, 2024 16:47

fix api in mock

ffe3448

jaceklyp approved these changes Dec 2, 2024

View reviewed changes

brindy changed the base branch from main to release/7.148.0 December 2, 2024 20:13

brindy merged commit d3a068a into release/7.148.0 Dec 3, 2024
18 checks passed

brindy deleted the brindy/support-local-storage-fireproofing branch December 3, 2024 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support local storage fireproofing #3612

support local storage fireproofing #3612

brindy commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

jaceklyp Nov 29, 2024

jaceklyp Nov 29, 2024

jaceklyp Nov 29, 2024

jaceklyp Dec 2, 2024

brindy Dec 2, 2024

jaceklyp Dec 2, 2024

brindy Dec 2, 2024

jaceklyp Dec 2, 2024

brindy Dec 2, 2024

jaceklyp left a comment

support local storage fireproofing #3612

support local storage fireproofing #3612

Conversation

brindy commented Nov 22, 2024 • edited Loading

Internal references:

github-actions bot commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaceklyp left a comment

Choose a reason for hiding this comment

brindy commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading