Subscription oauth v2 #3480

federicocappelli · 2024-10-25T09:51:27Z

Task/Issue URL: https://app.asana.com/0/1205842942115003/1207991044706235/f
CC: @miasma13

macOS PR: duckduckgo/macos-browser#3580
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:

Software Engineering Expectations
Technical Design Template

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

# Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppDependencyProvider.swift # DuckDuckGo/NetworkProtectionRootView.swift # DuckDuckGo/NetworkProtectionTunnelController.swift # DuckDuckGo/Subscription/DefaultSubscriptionManager+AccountManagerKeychainAccessDelegate.swift # DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift # DuckDuckGo/Subscription/ViewModel/SubscriptionSettingsViewModel.swift # DuckDuckGo/Subscription/Views/SubscriptionContainerViewFactory.swift # PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/AppDependencyProvider.swift

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

github-actions · 2024-10-25T13:30:08Z

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

Generated by 🚫 dangerJS against 5f574f3

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

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

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

github-actions · 2024-12-21T00:19:16Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

github-actions · 2024-12-31T00:19:40Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppDelegate.swift # DuckDuckGo/SettingsViewModel.swift # DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppDependencyProvider.swift # DuckDuckGo/SettingsState.swift # DuckDuckGo/SettingsSubscriptionView.swift # DuckDuckGoTests/Subscription/SubscriptionPagesUseSubscriptionFeatureTests.swift

diegoreymendez · 2025-01-10T08:12:06Z

Note: I've edited the description to add links to the related PRs for easy of navigation.

diegoreymendez · 2025-01-10T12:32:32Z

I'm seeing trouble in iOS to get the subscription enabled. After recovering my subscription I see this message.

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

@diegoreymendez same issue than macOS, fixing now

diegoreymendez · 2025-01-13T19:37:55Z

I messaged you directly, but I'll still add a message here for history. Just tested the latest test build on iOS and the subscription expired after about 1 hour of usage (the subscription isn't expired as I'm using my own long-living account).

miasma13 · 2025-01-14T12:59:39Z

DuckDuckGo/Subscription/DefaultSubscriptionManager+AccountManagerKeychainAccessDelegate.swift

-            PixelParameters.source: "browser"
-        ]
-
-        DailyPixel.fireDailyAndCount(pixel: .privacyProKeychainAccessError,


Same comment as for macOS, in new code we retained the 'privacyProKeychainAccessError' pixel but there are no places where it is fired. Additionally we lack feedback from v2 keychain store regarding access errors.

miasma13 · 2025-01-14T13:06:49Z

DuckDuckGo/NetworkProtectionStatusView.swift

@@ -283,7 +283,7 @@ struct NetworkProtectionStatusView: View {

    @ViewBuilder
    private func about() -> some View {
-        let viewModel = UnifiedFeedbackFormViewModel(subscriptionManager: statusModel.subscriptionManager,
+        let viewModel = UnifiedFeedbackFormViewModel(subscriptionManager: AppDependencyProvider.shared.subscriptionManager,


Not sure if this is wrongly resolved merge conflict, we can obtain the subsc. manager from the model. Left line seems to be correct.

miasma13 · 2025-01-14T13:12:20Z

DuckDuckGo/MainViewController.swift

-            guard case .success(false) = await accountManager.hasEntitlement(forProductName: .networkProtection) else { return }
+            let subscriptionManager = AppDependencyProvider.shared.subscriptionManager
+
+            guard let tokenContainer = try? await subscriptionManager.getTokenContainer(policy: .local),


Should't we use 'guard !subscriptionManager.isFeatureActive(.networkProtection)' here?

miasma13 · 2025-01-14T13:13:04Z

DuckDuckGo/Feedback/VPNMetadataCollector.swift

    private let settings: VPNSettings
    private let defaults: UserDefaults

    init(statusObserver: ConnectionStatusObserver,
-         serverInfoObserver: ConnectionServerInfoObserver,
-         accountManager: AccountManager = AppDependencyProvider.shared.subscriptionManager.accountManager,
+         serverInfoObserver: ConnectionServerInfoObserver,// ConnectionServerInfoObserverThroughSession(),


miasma13 · 2025-01-14T13:16:24Z

DuckDuckGo/DefaultNetworkProtectionVisibility.swift


 struct DefaultNetworkProtectionVisibility: NetworkProtectionFeatureVisibility {
    private let userDefaults: UserDefaults
-    private let accountManager: AccountManager
+    private let oAuthClient: any OAuthClient


Shouldn't we use 'any SubscriptionManager' here?

miasma13 · 2025-01-14T16:20:26Z

DuckDuckGo/Subscription/TokenBackgroundRefreshTask.swift

+            do {
+                try await self.subscriptionManager.getTokenContainer(policy: .localForceRefresh)
+                Logger.subscription.log("Token background refresh task completed successfully in \(Date().timeIntervalSince(refreshStartDate)) seconds")
+                task.setTaskCompleted(success: true)


Shouldn't we schedule another task after completing one?

miasma13 · 2025-01-14T16:25:13Z

PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift

+            Logger.networkProtection.log("Subscription Entitlements check...")
+            let isNetworkProtectionEnabled = await subscriptionManager.isFeatureActive(.networkProtection)
+            Logger.networkProtection.log("NetworkProtectionEnabled if: \( isNetworkProtectionEnabled ? "Enabled" : "Disabled", privacy: .public)")
+            return .success(isNetworkProtectionEnabled)


Not sure if valid, but earlier the closure returned .failure when fetching entitlement failed. Is this not possible for the new implementation of entitlements?

miasma13 · 2025-01-14T16:30:08Z

DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

        let purchaseTransactionJWS: String

-        switch await appStorePurchaseFlow.purchaseSubscription(with: subscriptionSelection.id,
-                                                               emailAccessToken: emailAccessToken) {
+        switch await appStorePurchaseFlow.purchaseSubscription(with: subscriptionSelection.id) {


Will look closer at it in BSK but I see you dropped the emailAccessToken: emailAccessToken) param from the purchase call.
This was the access token from Email Protection feature. When we were creating a new account during the purchase and we passed this token as param the user account on BE was linked with user's Duck address from Email Protection. Not sure if this is still supported in v2.

miasma13 · 2025-01-14T16:31:53Z

DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

+            _ = try await subscriptionManager.exchange(tokenV1: authToken)
+            Logger.subscription.log("v1 token exchanged for v2")
+
            onSetSubscription?()
-
-        } else {
-            Logger.subscription.error("Failed to obtain subscription options")
+        } catch {
+            Logger.subscription.error("Failed to exchange v1 token for v2")


Maybe worthy adding a pixel for (also on macOS)?

miasma13 · 2025-01-14T16:33:55Z

DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

+//        guard let accessToken = accountManager.accessToken else {
+//            Logger.subscription.error("Missing access token")
+//            return nil
+//        }
+//
+//        switch await accountManager.fetchAccountDetails(with: accessToken) {
+//        case .success(let accountDetails):
+//            switch await subscriptionManager.subscriptionEndpointService.getSubscription(accessToken: accessToken) {
+//            case .success:
+//                accountManager.storeAccount(token: accessToken,
+//                                            email: accountDetails.email,
+//                                            externalID: accountDetails.externalID)
+//                onBackToSettings?()
+//            case .failure(let error):
+//                Logger.subscription.error("Error retrieving subscription details: \(error.localizedDescription)")
+//            }
+//        case .failure(let error):
+//            Logger.subscription.error("Could not get account Details: \(error.localizedDescription)")
+//            setTransactionError(.generalError)
+//        }
+//        return nil


Probably commented out code can be removed.

What about handling the error scenarios like in original code e.g. L:373?

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppLifecycle/AppStates/Inactive.swift # DuckDuckGo/OldAppDelegate.swift

# Conflicts: # DuckDuckGo/DuckPlayer/DuckPlayer.swift

federicocappelli added 10 commits October 16, 2024 13:13

builds

cc16889

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

8b07ea5

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

auth and first purchase improved

79faa6b

purchase flow fixed

c6c2bef

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

c640a91

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/AppDependencyProvider.swift

purchase fixed

018751a

token fixes

de81314

loggers improved and restore fixed

b4c661e

subscription refresh improved

d123038

federicocappelli added includes-BSK-changes Do Not Merge labels Oct 25, 2024

federicocappelli mentioned this pull request Oct 25, 2024

Subscription oauth v2 duckduckgo/BrowserServicesKit#1033

Open

federicocappelli added 2 commits October 25, 2024 14:26

v1 to v2 auth migration

9d90ccf

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

457922c

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

federicocappelli added 14 commits October 25, 2024 16:41

lint ad restore commented code

60d4012

review suggestions applied

07e2cc0

tests and logs

bd5bb1b

vpn fixed

e92d0db

debug menu improved

1406315

unit tests and lint

eebbc48

unit tests and improvements

8ea3254

signout as async

c78a9c2

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

c021b95

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

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

0f426a1

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

lint

f06370b

BSK points to branch now

5f69e54

bsk updated

929e12d

Merge branch 'main' of https://github.com/duckduckgo/iOS into fcappel…

386fd70

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

federicocappelli requested review from THISISDINOSAUR and samsymons and removed request for THISISDINOSAUR December 13, 2024 10:51

github-actions bot added the stale label Dec 21, 2024

federicocappelli removed the stale label Dec 23, 2024

github-actions bot added the stale label Dec 31, 2024

federicocappelli removed the stale label Jan 3, 2025

federicocappelli added 4 commits January 6, 2025 09:53

testing comments fixed

88cd2d9

BSK updated

7b93366

diegoreymendez mentioned this pull request Jan 10, 2025

Subscription oauth api v2 duckduckgo/macos-browser#3580

Open

federicocappelli added 3 commits January 10, 2025 16:49

BSK updated, some flows updated

62bd2da

subscription manager load initial data improved, pixels added

b72a122

BSK update with subscription cache bug fix

4e887d4

miasma13 reviewed Jan 14, 2025

View reviewed changes

federicocappelli added 8 commits January 16, 2025 15:39

settings and init improvements

6f3a7ad

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

7a0e848

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift

Subscription stuff renamed

ef81d09

BSK updated

b4196db

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

bc33fcf

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppLifecycle/AppStates/Inactive.swift # DuckDuckGo/OldAppDelegate.swift

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

5660c74

Merge branch 'main' into fcappelli/subscription_oauth_api_v2

5f574f3

# Conflicts: # DuckDuckGo/DuckPlayer/DuckPlayer.swift

pr suggestions

d045cce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription oauth v2 #3480

Subscription oauth v2 #3480

federicocappelli commented Oct 25, 2024 •

edited by diegoreymendez

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Dec 21, 2024

github-actions bot commented Dec 31, 2024

diegoreymendez commented Jan 10, 2025

diegoreymendez commented Jan 10, 2025

federicocappelli commented Jan 10, 2025

diegoreymendez commented Jan 13, 2025 •

edited

Loading

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

miasma13 Jan 14, 2025

Subscription oauth v2 #3480

Are you sure you want to change the base?

Subscription oauth v2 #3480

Conversation

federicocappelli commented Oct 25, 2024 • edited by diegoreymendez Loading

Internal references:

github-actions bot commented Oct 25, 2024 • edited Loading

github-actions bot commented Dec 21, 2024

github-actions bot commented Dec 31, 2024

diegoreymendez commented Jan 10, 2025

diegoreymendez commented Jan 10, 2025

federicocappelli commented Jan 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

federicocappelli commented Oct 25, 2024 •

edited by diegoreymendez

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

diegoreymendez commented Jan 13, 2025 •

edited

Loading