Check DBP Prerequisites in App and Disable Login Item If Necessary #2850

aataraxiaa · 2024-06-10T14:41:30Z

Task/Issue URL: https://app.asana.com/0/1206488453854252/1207501562611619/f

Description:
This PR introduces changes to check DBP prerequisites (user is authenticated and has valid entitlements) from the App when the app becomes active. When prerequisites are not satisfied, the app disables the login item. This is to prevent a scenario where the background agent checks prerequisites, exits due to failed prerequisites, and then is automatically re-started by the login item.

Notable Changes:

Renames DataBrokerProtectionFeatureVisibility to DataBrokerProtectionFeatureGatekeeper
Adds a arePrerequisitesSatisfied to DataBrokerProtectionFeatureGatekeeper
Adds tests and related mocks for arePrerequisitesSatisfied scenarios
Removes Waitlist-related code where necessary (i.e when changes would require subsequent changes to waitlist code, which is not needed now, we remove the waitlist code rather than updating it)

Steps to test this PR:

Test Setup:

Cause the background agent to fail checks by calling stopAgent immediately in the DefaultDataBrokerProtectionAgentStopper.validateRunPrerequisitesAndStopAgentIfnecessary method, e.g

    public func validateRunPrerequisitesAndStopAgentIfNecessary() async {
        stopAgent()
…

Cause the app to fail checks by failing the authentication check in DataBrokerProtectionFeatureGatekeeper.arePrerequisitesSatisfied method, e.g

//        let isAuthenticated = accountManager.accessToken != nil
        let isAuthenticated = false

Kill all background agents running on your machine
Open the console app and filter for DBP category, e.g category:Data Broker Protection
Ensure logging is enable for DBP via the browser debug menu
Configure Xcode to connect to the background agent on launch, see screenshot below

Tests:
Test 1: App Kills the Login Item

Launch the app from xcode after making the changes described above
Monitor the console app, and you should see something like:

Note: You may see the background agent also start and then stop. This is due to different threads performing the work. This is ok, as long as the background agent does not continuously restart.

Test 2: DPB Functions as expected

Undo all the temp changes made previously
Launch the browser
Open DBP
Initiate a scan
Ensure it completes successfully

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

github-actions · 2024-06-10T14:43:23Z

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

Generated by 🚫 dangerJS against c6a8b9b

aataraxiaa · 2024-06-11T09:39:59Z

DuckDuckGo/DBP/DataBrokerProtectionAppEvents.swift

            return
        }

-        Task {
-            try? await DataBrokerProtectionWaitlist().redeemDataBrokerProtectionInviteCodeIfAvailable()
+        let loginItemsManager = LoginItemsManager()


Moving the creation of these later in the method, after guard checks, when we know they are needed.

aataraxiaa · 2024-06-11T10:37:21Z

UnitTests/DBP/Tests/DataBrokerProtectionFeatureGatekeeperTests.swift

+
+@testable import DuckDuckGo_Privacy_Browser
+
+final class DataBrokerProtectionFeatureGatekeeperTests: XCTestCase {


This test was renamed, and so this is not all new code. The newly added tests start at line 127.

THISISDINOSAUR

LGTM!

THISISDINOSAUR · 2024-06-12T11:23:56Z

DuckDuckGo.xcodeproj/project.pbxproj

@@ -9825,7 +9845,7 @@
 				3706FB4C293F65D500E42796 /* SharingMenu.swift in Sources */,
 				3706FB4D293F65D500E42796 /* GrammarFeaturesManager.swift in Sources */,
 				3706FB50293F65D500E42796 /* SafariFaviconsReader.swift in Sources */,
-				31267C692B640C4200FEF811 /* DataBrokerProtectionFeatureVisibility.swift in Sources */,
+				31267C692B640C4200FEF811 /* DataBrokerProtectionFeatureGatekeeper.swift in Sources */,


What name do we use for this sort of thing generally? I didn't like visibility, but it would be good if we can be consistent across the codebase

Agree, Visibility is inaccurate IMO, as it’s availability rather than visibility. Good point, we have NetworkProtectionFeatureVisibility and NetworkProtectionBouncer. They do different things so if we were to rename I’d prob only rename NetworkProtectionFeatureVisibility at this point. Happy to do that now if you think it makes sense @THISISDINOSAUR .

I don't mind either way, but either way I'd mention it to the O-N people
I'm also not a fan of bouncer, IMO it could mean too many different things

Cool, I’ll mention to O-N people and rename …Visibility if no objections. Agree on the Bouncer re: meanings.

diegoreymendez · 2024-06-12T12:32:03Z

DuckDuckGo/DBP/DataBrokerProtectionAppEvents.swift


+        Task {


One thing we do for the VPN that I'm not sure we're doing for DBP is restart the agent on launch, if there's been an app update. This basically tries to ensure the agent we're running is updated too.

Yeah, @jotaemepereira has mentioned this as something we should do at some point. Thanks for highlighting @diegoreymendez .

diegoreymendez · 2024-06-12T12:37:00Z

DuckDuckGo/DBP/DataBrokerProtectionFeatureGatekeeper.swift

+    func arePrerequisitesSatisfied() async -> Bool {
+        let entitlements = await accountManager.hasEntitlement(for: .dataBrokerProtection,
+                                                               cachePolicy: .reloadIgnoringLocalCacheData)
+        var hasEntitlements: Bool
+        switch entitlements {
+        case .success(let value):
+            hasEntitlements = value
+        case .failure:
+            hasEntitlements = false
+        }
+
+        let isAuthenticated = accountManager.accessToken != nil
+
+        firePrerequisitePixelsAndLogIfNecessary(hasEntitlements: hasEntitlements, isAuthenticatedResult: isAuthenticated)
+
+        return hasEntitlements && isAuthenticated
+    }
+}


Just adding a though here for you, but I think this can be addressed incrementally as I don't want to blow up the scope of your PR.

I think this logic is likely to be the same across PPro products, and that we might want to push this up to the subscription library.

Agree. I’ll refrain from actioning here to avoid scope increase as you say.

diegoreymendez · 2024-06-12T12:38:47Z

Added some comments looking mostly at the code changes. None of the two comments are blocking though - they're more thoughts I wanted to share.

aataraxiaa · 2024-06-12T13:07:41Z

Added some comments looking mostly at the code changes. None of the two comments are blocking though - they're more thoughts I wanted to share.

Thanks @diegoreymendez , I’m implemented the VPN renaming changes I suggested on MM. Can you take a quick look please?

DuckDuckGo/Waitlist/VPNFeatureGatekeeper.swift

diegoreymendez

Nice work @aataraxiaa , thanks for doing this!

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Preferences/Model/PreferencesSidebarModel.swift

# By Sam Symons (8) and others # Via GitHub * main: (35 commits) Check DBP Prerequisites in App and Disable Login Item If Necessary (#2850) Fixes an issue with my last merge Implement VPN control through UDS (#2767) Add VPN reddit cookie workaround (#2851) Bump version to 1.92.0 (202) Update Send Feedback icon (#2852) Make passwords easier to discover (#2847) Bump version to 1.92.0 (201) Update BSK for RMF survey changes (#2846) DBP: Update people-wizard.com (#2849) Remove VPN launch pixels (#2845) Prevent showing multiple VPN uninstalled popovers (#2844) Bump version to 1.92.0 (200) Set marketing version to 1.92.0 Update embedded files DBP: Implement stats pixels (#2812) DBP: Add support for noResultsSelector (#2840) Fire compilation failed pixel if needed (#1626) Update autoconsent to v10.10.0 (#2842) Fix running Autofill-related UI tests in CI (#2843) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Jun 10, 2024

aataraxiaa force-pushed the pete/prevent-background-agent-restart branch from 0a2b3d6 to 7aa0d56 Compare June 11, 2024 09:28

github-actions bot removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Jun 11, 2024

aataraxiaa commented Jun 11, 2024

View reviewed changes

aataraxiaa added 5 commits June 11, 2024 11:45

Implement & call prerequisitesAreSatisfied when app becomes active

f9a3904

Rename & move existing tests, add new tests & mocks

3fbc605

Update callsites, remove impacted waitlist code

bf8b8aa

Tidy up, add pixels

2244c26

Minor improvements

27c5277

aataraxiaa force-pushed the pete/prevent-background-agent-restart branch from 7aa0d56 to 27c5277 Compare June 11, 2024 10:49

aataraxiaa marked this pull request as ready for review June 11, 2024 11:04

aataraxiaa requested a review from THISISDINOSAUR June 11, 2024 11:05

THISISDINOSAUR approved these changes Jun 12, 2024

View reviewed changes

diegoreymendez reviewed Jun 12, 2024

View reviewed changes

aataraxiaa added 2 commits June 12, 2024 13:50

Rename VPN Visibility type to Gatekeeper to be consistent with DBP

0a0122e

Fix file header

18b725e

diegoreymendez reviewed Jun 12, 2024

View reviewed changes

DuckDuckGo/Waitlist/VPNFeatureGatekeeper.swift Outdated Show resolved Hide resolved

diegoreymendez self-requested a review June 12, 2024 13:36

diegoreymendez approved these changes Jun 12, 2024

View reviewed changes

aataraxiaa added 2 commits June 12, 2024 16:00

Merge branch 'main' into pete/prevent-background-agent-restart

f7fc74d

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Preferences/Model/PreferencesSidebarModel.swift

Merge branch 'main' into pete/prevent-background-agent-restart

c6a8b9b

aataraxiaa merged commit 75a5c9a into main Jun 12, 2024
19 checks passed

aataraxiaa deleted the pete/prevent-background-agent-restart branch June 12, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check DBP Prerequisites in App and Disable Login Item If Necessary #2850

Check DBP Prerequisites in App and Disable Login Item If Necessary #2850

aataraxiaa commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

aataraxiaa Jun 11, 2024

aataraxiaa Jun 11, 2024

THISISDINOSAUR left a comment

THISISDINOSAUR Jun 12, 2024

aataraxiaa Jun 12, 2024

THISISDINOSAUR Jun 12, 2024

aataraxiaa Jun 12, 2024

diegoreymendez Jun 12, 2024

aataraxiaa Jun 12, 2024

diegoreymendez Jun 12, 2024

aataraxiaa Jun 12, 2024

diegoreymendez commented Jun 12, 2024

aataraxiaa commented Jun 12, 2024

diegoreymendez left a comment


		@testable import DuckDuckGo_Privacy_Browser

		final class DataBrokerProtectionFeatureGatekeeperTests: XCTestCase {

Check DBP Prerequisites in App and Disable Login Item If Necessary #2850

Check DBP Prerequisites in App and Disable Login Item If Necessary #2850

Conversation

aataraxiaa commented Jun 10, 2024 • edited Loading

Internal references:

github-actions bot commented Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THISISDINOSAUR left a 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

Choose a reason for hiding this comment

diegoreymendez commented Jun 12, 2024

aataraxiaa commented Jun 12, 2024

diegoreymendez left a comment

Choose a reason for hiding this comment

aataraxiaa commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading