Subscription oauth api v2 #3580

federicocappelli · 2024-11-21T15:12:07Z

Task/Issue URL: https://app.asana.com/0/1205842942115003/1207991044706236/f
Tech Design URL: https://app.asana.com/0/1205842942115003/1207991044706232/f
CC: @miasma13

iOS PR: duckduckgo/iOS#3480
BSK PR: duckduckgo/BrowserServicesKit#1033

Description:

This PR introduces the use of OAuth V2 authentication in Privacy Pro Subscription.
The code changes are comprehensive due to the paradigm changes between the old access token lifecycle and the new JWT lifecycle.
The Subscription UI and UX should be unchanged.

Steps to test this PR:

Test all Privacy Pro Subscription features and UX, more details here

Internal references:

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

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

github-actions · 2024-11-26T12:30:44Z

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

Generated by 🚫 dangerJS against f3a5d24

# Conflicts: # DuckDuckGo/UnifiedFeedbackForm/UnifiedFeedbackFormViewController.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Application/AppDelegate.swift # DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift # DuckDuckGo/Subscription/SubscriptionManager+StandardConfiguration.swift # DuckDuckGo/UnifiedFeedbackForm/UnifiedFeedbackFormViewController.swift # DuckDuckGo/UnifiedFeedbackForm/UnifiedFeedbackFormViewModel.swift # LocalPackages/SubscriptionUI/Sources/SubscriptionUI/DebugMenu/SubscriptionDebugMenu.swift # LocalPackages/SubscriptionUI/Sources/SubscriptionUI/Preferences/PreferencesSubscriptionModel.swift # UnitTests/Freemium/DBP/Experiment/FreemiumDBPPixelExperimentManagingTests.swift # UnitTests/Freemium/DBP/FreemiumDBPFeatureTests.swift # UnitTests/Menus/MoreOptionsMenuTests.swift # UnitTests/Subscription/SubscriptionAppStoreRestorerTests.swift # UnitTests/Subscription/SubscriptionPagesUseSubscriptionFeatureForStripeTests.swift # UnitTests/Subscription/SubscriptionPagesUseSubscriptionFeatureTests.swift # UnitTests/UnifiedFeedbackForm/UnifiedFeedbackFormViewModelTests.swift

federicocappelli · 2025-01-13T12:34:16Z

I'm still seeing the same issue I reported Friday where the VPN stops on its own and shows me a subscription expired error.

Steps to reproduce:

Connect the VPN

Leave the computer unattended for ~1 hour.

Wake computer from sleep - VPN is disconnected and shows a subscription expired error.

I'm still testing to narrow down the precise steps but it seems to failing reliably on my end.

Which environment and purchase platform are you using? App store subscriptions are renewing every 5 minutes until a max of (can't remember) times, so finding the VPN disconnected seems correct, are the settings showing an expired subscription?

diegoreymendez · 2025-01-13T12:35:02Z

I'm using my pre-existing account from stripe + staging.

federicocappelli · 2025-01-13T12:35:45Z

I'm using my pre-existing account from stripe + staging.

Then it's not right, I'm investigating

diegoreymendez · 2025-01-13T12:57:51Z

Did some additional testing - it's not always disconnecting, but when it doesn't disconnect I often see this. The subscription was added correctly and the VPN was enabled with a working subscription (my own pre-existing subscription in staging).

...Sources/DataBrokerProtection/Authentication/DataBrokerProtectionAuthenticationManaging.swift

DuckDuckGo/DBP/DataBrokerProtectionSubscriptionEventHandler.swift

miasma13 · 2025-01-13T16:02:46Z

DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift

+        let features = await subscriptionManager.currentSubscriptionFeatures(forceRefresh: false)
+        let vpnFeature = features.first { $0.entitlement == .networkProtection }
+        let dbpFeature = features.first { $0.entitlement == .dataBrokerProtection }
+        let itrFeature = features.first { $0.entitlement == .identityTheftRestoration }
+        let itrgFeature = features.first { $0.entitlement == .identityTheftRestorationGlobal }
+
+        Task { @MainActor in
+            self.networkProtectionItem.isEnabled = vpnFeature?.enabled ?? false
+            self.dataBrokerProtectionItem.isEnabled = dbpFeature?.enabled ?? false
+            let hasIdentityTheftRestoration = itrFeature?.enabled ?? false
+            let hasIdentityTheftRestorationGlobal = itrgFeature?.enabled ?? false
+            self.identityTheftRestorationItem.isEnabled = hasIdentityTheftRestoration || hasIdentityTheftRestorationGlobal


Maybe we can migrate the whole block to the new simpler entitlement check with subscriptionManager.isFeatureActive(...)?

subscriptionManager.isFeatureActive(...) is a bit too heavy for checking all entitlements because it is fetching (local) auth token and subscription every time, this is the same but just faster. We have two places where we use this approach, the code duplication is not super pretty but it's difficult to parametrise.

DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift

DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift

DuckDuckGo/Subscription/DefaultSubscriptionManager+StandardConfiguration.swift

DuckDuckGo/Tab/UserScripts/Subscription/SubscriptionPagesUseSubscriptionFeature.swift

DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift

diegoreymendez · 2025-01-14T16:28:17Z

...workProtection/NetworkExtensionTargets/NetworkExtensionTargets/MacPacketTunnelProvider.swift

@@ -397,13 +385,14 @@ final class MacPacketTunnelProvider: PacketTunnelProvider {
 #if NETP_SYSTEM_EXTENSION
        "\(Bundle.main.bundleIdentifier!).authToken"
 #else
-        NetworkProtectionKeychainTokenStore.Defaults.tokenStoreService
+        "com.duckduckgo.networkprotection.authToken"


Consider adding a constant here.

I transformed tokenServiceName in a let so that's now a constant, there was no need to be a var calculated at runtime.
I didn't move the string somewhere else (like in the previous implementation) because is only used there.

diegoreymendez · 2025-01-14T16:29:33Z

...workProtection/NetworkExtensionTargets/NetworkExtensionTargets/MacPacketTunnelProvider.swift

@@ -436,16 +417,60 @@ final class MacPacketTunnelProvider: PacketTunnelProvider {
        case .staging:
            subscriptionEnvironment.serviceEnvironment = .staging
        }
+        subscriptionEnvironment.purchasePlatform = .stripe // we don't care about the purchasePlatform


I've no idea why we don't care about the purchase platform. Could we document what this means, maybe providing a link with reference or a clearer explanation?

Comment added: The SysExt doesn't care about the purchase platform because the only operations executed here are about the Auth token. No purchase or platforms-related operations are performed.

diegoreymendez · 2025-01-14T16:31:05Z

...workProtection/NetworkExtensionTargets/NetworkExtensionTargets/MacPacketTunnelProvider.swift

+                                                          serviceName: Self.tokenServiceName,
+                                                          keychainType: Bundle.keychainType)
+        let authClient = DefaultOAuthClient(tokensStorage: tokenStorage,
+                                            legacyTokenStorage: nil, // Note: The VPN SysExt will stop at the first transition from auth v1 to v2, is up to the user to re-enable it


Can you explain this to me in a bit more detail? It's not clear to me what the "transition" is... is it the first time they manually start the VPN?

I've confirmed with Thomas that it is actually possible to exchange a token v1 multiple times so this comment is not relevant anymore, I've updated the code so the SysExt can now perform a full migration from auth v1 to v2 and never stop

diegoreymendez

Still looking but added several comments.

THISISDINOSAUR

Testing and PIR seems to work now. I've just left one minor comment on the code

THISISDINOSAUR · 2025-01-14T18:13:27Z

DuckDuckGo/DBP/DataBrokerProtectionAppEvents.swift

@@ -52,6 +53,7 @@ struct DataBrokerProtectionAppEvents {
            // In this case, let's disable the agent and delete any left-over data because there's nothing for it to do
            if let profileQueriesCount = try? DataBrokerProtectionManager.shared.dataManager.profileQueriesCount(),
               profileQueriesCount > 0 {
+                Logger.dataBrokerProtection.log("Found \(profileQueriesCount) profile queries in DB. Disabling agent.")


I think this comment is wrong (confused by the comment above no longer being accurate)
If there are profile queries, we don't disable it, we want the agent to be running. We restart it rather than disable it (to help with the flakeyness of login items)

Thanks, modified with "Restarting agent"

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Common/Extensions/WKWebViewExtension.swift # LocalPackages/NewTabPage/Sources/NewTabPage/Configuration/NewTabPageConfigurationClient.swift # LocalPackages/NewTabPage/Sources/NewTabPage/Favorites/NewTabPageFavoritesClient.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

federicocappelli added 3 commits November 19, 2024 11:18

Initial auth v2 adoption

88549ff

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

9b2e7ba

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

DI done

4fa2b0a

federicocappelli added includes BSK changes DO NOT MERGE labels Nov 21, 2024

token v1/v2 exchange fixed, token providing improved

0d333af

federicocappelli changed the title ~~Fcappelli/subscription oauth api v2~~ Subscription oauth api v2 Nov 22, 2024

federicocappelli added 8 commits November 22, 2024 14:16

new log

b79e8eb

tokencontainer codable

18b8fe6

vpn token propagation

e6cc664

more logs

00c4cbc

keychain type unification

4bcc286

keychain fix

875e880

init improvements

92014b4

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

8dc4078

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

federicocappelli added 14 commits November 26, 2024 13:51

codablehelper

09eee50

it works!

ffb9ecd

fixing unit tests

dcccf44

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

6ddece0

# Conflicts: # DuckDuckGo/UnifiedFeedbackForm/UnifiedFeedbackFormViewController.swift

unit tests fixes

62ddb64

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

975d8ac

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

unit tests fixed

a121c06

BSK > branch

d3e657c

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

5ccd25b

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

lint

08bf3f0

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

1ef216e

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

unit tests improvments

b5a3f43

varius

6dc2e1f

subscription cache bug fix

9eaf062

miasma13 reviewed Jan 14, 2025

View reviewed changes

federicocappelli added 3 commits January 14, 2025 14:53

bug fixing

c77e374

some changes requested in the PR

d321469

pr changes

a2470eb

diegoreymendez reviewed Jan 14, 2025

View reviewed changes

federicocappelli added 3 commits January 14, 2025 16:32

pixels added

d0ca705

cleanup

39b30fc

settings subscription status logic improved

de72420

THISISDINOSAUR reviewed Jan 14, 2025

View reviewed changes

samsymons removed their request for review January 15, 2025 05:36

federicocappelli added 12 commits January 15, 2025 10:43

legacy token support implemented in VPN, migration improved

c29989b

pr comments addressed

ca8f251

bsk updated

f737f2d

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

4d2d595

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift

Subscription stuff renamed

7284019

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

e5ec1bd

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

e98529a

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

BSK updated

bbde6d6

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

f11debc

BSK update

f3a5d24

Tunnel controller now refreshes the token after passing it to the VPN

445fe07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription oauth api v2 #3580

Subscription oauth api v2 #3580

federicocappelli commented Nov 21, 2024 •

edited by diegoreymendez

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

federicocappelli commented Jan 13, 2025

diegoreymendez commented Jan 13, 2025

federicocappelli commented Jan 13, 2025

diegoreymendez commented Jan 13, 2025 •

edited

Loading

miasma13 Jan 13, 2025

federicocappelli Jan 14, 2025

diegoreymendez Jan 14, 2025

federicocappelli Jan 15, 2025

diegoreymendez Jan 14, 2025

federicocappelli Jan 15, 2025

diegoreymendez Jan 14, 2025

federicocappelli Jan 15, 2025

diegoreymendez left a comment

THISISDINOSAUR left a comment

THISISDINOSAUR Jan 14, 2025

federicocappelli Jan 15, 2025

Subscription oauth api v2 #3580

Are you sure you want to change the base?

Subscription oauth api v2 #3580

Conversation

federicocappelli commented Nov 21, 2024 • edited by diegoreymendez Loading

Internal references:

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

federicocappelli commented Jan 13, 2025

diegoreymendez commented Jan 13, 2025

federicocappelli commented Jan 13, 2025

diegoreymendez commented Jan 13, 2025 • 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

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

federicocappelli commented Nov 21, 2024 •

edited by diegoreymendez

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading

diegoreymendez commented Jan 13, 2025 •

edited

Loading