Subscription oauth v2 #1033

federicocappelli · 2024-10-25T09:52:19Z

Task/Issue URL: https://app.asana.com/0/1205842942115003/1207991044706235/f
iOS PR: duckduckgo/iOS#3480
macOS PR: duckduckgo/macos-browser#3580
What kind of version bump will this require?: Major
CC: @miasma13

iOS PR: duckduckgo/iOS#3480
macOS PR: duckduckgo/macos-browser#3580

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

Sources/MaliciousSiteProtection/API/APIClient.swift

Sources/Networking/v2/Extensions/HTTPURLResponse+Cookie.swift

Sources/Common/CodableHelper.swift

Sources/Subscription/Flows/Models/SubscriptionOptions.swift

Sources/Subscription/Managers/SubscriptionManager.swift

THISISDINOSAUR · 2024-12-13T16:06:37Z

There's basically no PIR stuff in BSK at the moment

diegoreymendez

Adding some comments. Still going through the code.

Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift

Sources/Common/KeychainType.swift

Sources/Common/UserDefaultsCache.swift

...lerErrorMessageObserver/ControllerErrorMesssageObserverThroughDistributedNotifications.swift

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez · 2024-12-20T16:45:05Z

Sources/NetworkProtection/PacketTunnelProvider.swift

-            try load(options: startupOptions)
-
-            if (try? tokenStore.fetchToken()) == nil {
-                throw TunnelError.startingTunnelWithoutAuthToken
-            }
+            try await load(options: startupOptions)
+            Logger.networkProtection.log("Startup options loaded correctly")


The error handling needs to be improved here. Previously we could only have startingTunnelWithoutAuthToken, but now we can get a number of different possible errors from tokenProvider.adopt and tokenProvider.getTokenContainer.

These errors should ideally implement CustomNSError and include underlying error information. See PacketTunnelProvider.TunnelError for reference.

The reason this is important is because it's what'll allow us to debug tunnel start issues coming from these changes, and understand what's going on.

Understood, atm getTokenContainer can fail for a variety of reasons, from storage to API, decoding and decryption errors, we don't have a single set Error so implementing this is quite difficult, should I wrap any possible error into a higher level TunnelError?

Yeah I'd do that.

We could wrap this in a (possibly new) TunnelError that contains an underlying error (by implementing CustomNSError) so we know where this is coming from at a high level, and what's the underlying problem.

That said, I'd encourage you to also consider implementing CustomNSError in the subscription errors we can get here too as that'll give us detailed error information in our pixels if we do. I know this can sound a bit annoying but the more detail we can get, the easier it will be to understand what's failing if something does fail, as we'll get detailed info in the pixels.

github-actions · 2025-01-01T00:23:17Z

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

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

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

diegoreymendez

Added some additional comments, still going through all this.

Sources/Common/KeychainType.swift

Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift

Sources/NetworkProtection/KeyManagement/NetworkProtectionKeychainStore.swift

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

Sources/Subscription/Managers/SubscriptionManager.swift

THISISDINOSAUR

There's almost nothing PIR related in BSK so I have no comments here

miasma13

First batch of comments

Tests/NetworkingTests/OAuth/.swift

Tests/BrowserServicesKitTests/Resources/privacy-reference-tests

miasma13 · 2025-01-15T10:01:52Z

Sources/Subscription/Storage/V2/SubscriptionTokenKeychainStorage+LegacyTokenStoring.swift

+                assertionFailure("Failed to retrieve auth token: \(error)")
+            }
+            return nil
+        }
+        set(newValue) {
+            do {
+                guard let newValue else {
+                    try removeAccessToken()
+                    return
+                }
+                try store(accessToken: newValue)
+            } catch {
+                assertionFailure("Failed set token: \(error)")


I think we also need error handling for the legacy token store. Having pixel for failures to read/write at point where we for example attempt to migrate the token would be an important signal.

Sources/Subscription/API/Model/PrivacyProSubscription.swift

Sources/Subscription/Flows/AppStore/AppStorePurchaseFlow.swift

miasma13 · 2025-01-15T10:47:54Z

Sources/Subscription/Flows/AppStore/AppStorePurchaseFlow.swift

+                    do {
+                        let transactionJWS = try await recoverSubscriptionFromDeadToken()
+                        return .success(transactionJWS)
+                    } catch {
+                        return .failure(.purchaseFailed(OAuthClientError.deadToken))
                    }


This handling seems to be incorrect:

in L:117 you attempt to recoverSubscriptionFromDeadToken()

inside this function you call appStoreRestoreFlow.restoreAccountFromPastPurchase()

this does not makes sense as this call already failed in L:107 (scenario for no subscription to be recovered via past App Store purchases)

restoreAccountFromPastPurchase fails because the token is dead, the first action of recoverSubscriptionFromDeadToken is to sign out the user (aka deleting everything, including the dead token) and re-run restoreAccountFromPastPurchase. The assumption is that the user had a token (dead) so a subscription was present, so the same subscription is recoverable with a new token.

miasma13 · 2025-01-15T11:06:04Z

Sources/Subscription/Flows/AppStore/AppStorePurchaseFlow.swift

+
+        do {
+            let subscription = try await subscriptionManager.confirmPurchase(signature: transactionJWS)
+            if subscription.isActive {


Not sure about this check. If the subscription after completing the purchase ends up in wrong state, it is a BE issue and there is little we can do about it, cannot recover from it nor we don't special handle it.

I would only keep the refetch and check if entitlements were granted to the account.

I'm not sure how to handle this but:

The request has a 3 max retries with a 4s delay between retries APIRequestV2.RetryPolicy(maxRetries: 3, delay: 4.0) but the call itself doesn't fail if the subscription is inactive

If the subscription is inactive we log it and return a failure to the script page with the correct error, so I would like to detect it still. I have never seen a failure here, I'm just trying to fail gracefully in case of BE errors.

I moved the token refresh out of the if so it is done no matter what.

miasma13 · 2025-01-15T11:16:35Z

Sources/Subscription/Flows/AppStore/AppStorePurchaseFlow.swift

-            subscription.platform != .apple {
-            return externalID
+    @discardableResult
+    private func recoverSubscriptionFromDeadToken() async throws -> String {


I see this is being called 3x from different scenarios but none of them seems for me to be likely to encounter dead token as the account was either just restored or created. In my opinion that while dead token is possible option during the purchase flow it should be treated as other "default" errors but is not recoverable here. We should just fail at given step.

I agree, this is confusing, so I did the following:

removed the recovery function call from every AppStorePurchaseFlow call and returned the dead token error normally

made recoverSubscriptionFromDeadToken() public and generic, so can be used from averyone receiving a dead token error, I'll update the clients accordingly

miasma13

Second part of the review

Sources/Networking/OAuth/OAuthClient.swift

Sources/Networking/OAuth/SessionDelegate.swift

Sources/Subscription/Managers/SubscriptionManager.swift

Sources/Subscription/API/SubscriptionEndpointService.swift

github-actions · 2025-02-07T00:20:55Z

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

federicocappelli added the DO NOT MERGE label Oct 25, 2024

federicocappelli commented Dec 12, 2024

View reviewed changes

Sources/MaliciousSiteProtection/API/APIClient.swift Outdated Show resolved Hide resolved

federicocappelli commented Dec 12, 2024

View reviewed changes

Sources/Networking/v2/Extensions/HTTPURLResponse+Cookie.swift Outdated Show resolved Hide resolved

federicocappelli marked this pull request as ready for review December 12, 2024 17:42

federicocappelli requested review from miasma13, samsymons, diegoreymendez and THISISDINOSAUR December 13, 2024 10:52

miasma13 reviewed Dec 13, 2024

View reviewed changes

Sources/Common/CodableHelper.swift Outdated Show resolved Hide resolved

miasma13 reviewed Dec 13, 2024

View reviewed changes

Sources/Subscription/Flows/Models/SubscriptionOptions.swift Outdated Show resolved Hide resolved

miasma13 reviewed Dec 13, 2024

View reviewed changes

Sources/Subscription/Managers/SubscriptionManager.swift Outdated Show resolved Hide resolved

miasma13 reviewed Dec 13, 2024

View reviewed changes

Sources/Subscription/Managers/SubscriptionManager.swift Outdated Show resolved Hide resolved

diegoreymendez reviewed Dec 20, 2024

View reviewed changes

github-actions bot added the stale label Jan 1, 2025

federicocappelli removed the stale label Jan 3, 2025

This was referenced Jan 10, 2025

Subscription oauth v2 duckduckgo/iOS#3480

Open

Subscription oauth api v2 duckduckgo/macos-browser#3580

Open

diegoreymendez self-requested a review January 14, 2025 15:55

diegoreymendez reviewed Jan 14, 2025

View reviewed changes

THISISDINOSAUR reviewed Jan 14, 2025

View reviewed changes

miasma13 reviewed Jan 15, 2025

View reviewed changes

miasma13 reviewed Jan 17, 2025

View reviewed changes

samsymons removed their request for review January 23, 2025 23:43

federicocappelli force-pushed the fcappelli/subscription_oauth_api_v2 branch from 1349661 to 402a26b Compare January 30, 2025 13:53

reset and removed some useless things

cad6354

federicocappelli force-pushed the fcappelli/subscription_oauth_api_v2 branch from 402a26b to cad6354 Compare January 30, 2025 14:08

github-actions bot added the stale label Feb 7, 2025

federicocappelli removed the stale label Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription oauth v2 #1033

Subscription oauth v2 #1033

federicocappelli commented Oct 25, 2024 •

edited by diegoreymendez

Loading

THISISDINOSAUR commented Dec 13, 2024

diegoreymendez left a comment

diegoreymendez Dec 20, 2024

federicocappelli Dec 23, 2024

diegoreymendez Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 1, 2025

diegoreymendez commented Jan 10, 2025

diegoreymendez left a comment

THISISDINOSAUR left a comment

miasma13 left a comment

miasma13 Jan 15, 2025

miasma13 Jan 15, 2025

federicocappelli Jan 21, 2025 •

edited

Loading

miasma13 Jan 15, 2025

federicocappelli Jan 21, 2025

miasma13 Jan 15, 2025

federicocappelli Jan 21, 2025

miasma13 left a comment

github-actions bot commented Feb 7, 2025

Subscription oauth v2 #1033

Are you sure you want to change the base?

Subscription oauth v2 #1033

Conversation

federicocappelli commented Oct 25, 2024 • edited by diegoreymendez Loading

Internal references:

THISISDINOSAUR commented Dec 13, 2024

diegoreymendez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2025

diegoreymendez commented Jan 10, 2025

diegoreymendez left a comment

Choose a reason for hiding this comment

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

miasma13 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 Jan 21, 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

miasma13 left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2025

federicocappelli commented Oct 25, 2024 •

edited by diegoreymendez

Loading

diegoreymendez Jan 14, 2025 •

edited

Loading

federicocappelli Jan 21, 2025 •

edited

Loading