Subscription oauth v2 #1033
Subscription oauth v2 #1033
Conversation
# Conflicts: # Sources/Networking/README.md # Sources/Networking/v2/APIRequestErrorV2.swift # Sources/Networking/v2/APIRequestV2.swift # Sources/Networking/v2/APIResponseConstraints.swift # Sources/Networking/v2/APIService.swift # Sources/TestUtils/MockAPIService.swift # Tests/NetworkingTests/v2/APIServiceTests.swift
# Conflicts: # Sources/Subscription/API/SubscriptionEndpointService.swift # Sources/Subscription/Managers/StorePurchaseManager/StorePurchaseManager.swift # Sources/Subscription/Managers/SubscriptionManager.swift # Tests/SubscriptionTests/Managers/SubscriptionManagerTests.swift
|
Note: I've edited the description to add links to the related PRs for easy of navigation. |
Sources/Common/KeychainType.swift
Outdated
| @@ -1,7 +1,7 @@ | |||
| // | |||
| // KeychainType.swift | |||
| // | |||
| // Copyright © 2023 DuckDuckGo. All rights reserved. | |||
| // Copyright © 2024 DuckDuckGo. All rights reserved. | |||
There was a problem hiding this comment.
This change is actually not right. The date in copyright notices is the date when the work was first published.
There was a problem hiding this comment.
KeychainType.swift existed in both bsk and mac app, I just moved it, reverting it back just to avoid noise
| @@ -123,7 +123,7 @@ final class NetworkProtectionConnectionTester { | |||
| } | |||
|
|
|||
| func stop() { | |||
| Logger.networkProtectionConnectionTester.log("🔴 Stopping connection tester") | |||
| Logger.networkProtectionConnectionTester.log("⚫️ Stopping connection tester") | |||
There was a problem hiding this comment.
Please, roll back the bullet color change.
There was a problem hiding this comment.
all the other stopping colours are black, this is the only one red and confusing, but I reverted it
| @@ -20,7 +20,7 @@ import Foundation | |||
| import Common | |||
| import os.log | |||
|
|
|||
| enum NetworkProtectionKeychainStoreError: Error, NetworkProtectionErrorConvertible { | |||
| public enum NetworkProtectionKeychainStoreError: Error, NetworkProtectionErrorConvertible { | |||
There was a problem hiding this comment.
Does this need to be public? I don't see it used anywhere outside the NetworkProtection module and for the most part I don't expect external code to be doing anything about these, other than just firing pixels with code and domain.
There was a problem hiding this comment.
correct, reverted
| @@ -189,7 +188,7 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement { | |||
| selectionMethod: NetworkProtectionServerSelectionMethod) async throws -> (server: NetworkProtectionServer, | |||
| newExpiration: Date?) { | |||
|
|
|||
| guard let token = try? tokenStore.fetchToken() else { throw NetworkProtectionError.noAuthTokenFound } | |||
| let token = try await VPNAuthTokenBuilder.getVPNAuthToken(from: tokenProvider, policy: .localValid) | |||
There was a problem hiding this comment.
Could you wrap this in a NetworkProtectionError with underlying error data?
It'll make it much easier to debug issues if we know where this error is coming from.
There was a problem hiding this comment.
I have modified Networking.OAuthClientError.missingTokens by adding an underlying error and used in here
| guard let token = try? tokenStore.fetchToken() else { | ||
| throw NetworkProtectionError.noAuthTokenFound | ||
| } | ||
| let token = try await VPNAuthTokenBuilder.getVPNAuthToken(from: tokenProvider, policy: .localValid) |
There was a problem hiding this comment.
Could you wrap this in a NetworkProtectionError with underlying error data?
It'll make it much easier to debug issues if we know where this error is coming from.
There was a problem hiding this comment.
I have modified Networking.OAuthClientError.missingTokens by adding an underlying error and used in here
| } | ||
| } catch { | ||
| Logger.subscription.error("Failed to migrate V1 token: \(error, privacy: .public)") |
There was a problem hiding this comment.
Shouldn't this error throw? The VPN calls this at startup: what happens if the migration fails?
There was a problem hiding this comment.
If the migration fails there's nothing we can do, we just don't have a token and we stop and just send a pixel (added now in the vpn too)
| } | ||
| } catch { | ||
| Logger.subscription.error("Failed to load initial subscription data: \(error, privacy: .public)") |
There was a problem hiding this comment.
Shouldn't this error throw? The VPN calls this at startup: what happens if the user is not authenticated? Can they still connect?
There was a problem hiding this comment.
Not authenticated = no auth token, so nothing done there would make sense
Not having a token is a valid state and nothing the vpn can do to recover
Tests/NetworkingTests/OAuth/.swift
Outdated
There was a problem hiding this comment.
Why do we have updates to this?
| 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)") |
There was a problem hiding this comment.
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.
| func purchaseSubscription(with subscriptionIdentifier: String, emailAccessToken: String?) async -> Result<AppStorePurchaseFlow.TransactionJWS, AppStorePurchaseFlowError> | ||
| @discardableResult | ||
| func completeSubscriptionPurchase(with transactionJWS: AppStorePurchaseFlow.TransactionJWS) async -> Result<PurchaseUpdate, AppStorePurchaseFlowError> | ||
| func purchaseSubscription(with subscriptionIdentifier: String) async -> Result<TransactionJWS, AppStorePurchaseFlowError> |
There was a problem hiding this comment.
Important: The API changed, you now omit the optional emailAccessToken which was passed to account creation. When creating a new account with that token we link the account with user's email protection address. Can we double check if and how this functionality is ported to Auth API v2?
| accountManager.signOut(skipNotification: true) | ||
| Logger.subscriptionAppStorePurchaseFlow.error("purchaseSubscription error: \(String(reflecting: error), privacy: .public)") | ||
|
|
||
| await subscriptionManager.signOut(notifyUI: true) |
There was a problem hiding this comment.
Wrong param, in the old implementation it was skipNotification: true but then notifyUI should be false.
There was a problem hiding this comment.
The old behaviour is very different from the current one, I re-tested this and notifying the Ui at logout here is needed in case the user has multiple windows and the subscription settings app is visible
| Logger.subscriptionAppStorePurchaseFlow.log("Recovering Subscription From Dead Token") | ||
|
|
||
| // Clear everything, the token is unrecoverable | ||
| await subscriptionManager.signOut(notifyUI: true) |
There was a problem hiding this comment.
Shouldn't notifyUI be false here?
There was a problem hiding this comment.
not really, we want to log out the user if every recovery attempt fails and we want to notify the UI accordingly
| do { | ||
| let transactionJWS = try await recoverSubscriptionFromDeadToken() | ||
| return .success(transactionJWS) | ||
| } catch { | ||
| return .failure(.purchaseFailed(OAuthClientError.deadToken)) | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
|
||
| do { | ||
| let subscription = try await subscriptionManager.confirmPurchase(signature: transactionJWS) | ||
| if subscription.isActive { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| subscription.platform != .apple { | ||
| return externalID | ||
| @discardableResult | ||
| private func recoverSubscriptionFromDeadToken() async throws -> String { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree, this is confusing, so I did the following:
- removed the recovery function call from every
AppStorePurchaseFlowcall 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
# Conflicts: # Package.swift # Sources/Subscription/API/SubscriptionEndpointService.swift # Sources/Subscription/Flows/AppStore/AppStorePurchaseFlow.swift # Sources/SubscriptionTestingUtilities/APIs/SubscriptionEndpointServiceMock.swift # Sources/SubscriptionTestingUtilities/Flows/AppStorePurchaseFlowMock.swift
| public final class SessionDelegate: NSObject, URLSessionTaskDelegate { | ||
|
|
||
| /// Disable automatic redirection, in our specific OAuth implementation we manage the redirection, not the user | ||
| public func urlSession(_ session: URLSession, task: URLSessionTask, willPerformHTTPRedirection response: HTTPURLResponse, newRequest request: URLRequest) async -> URLRequest? { | ||
| return nil |
There was a problem hiding this comment.
Seems like it is used only in one test. Please confirm if this is relevant and move accordingly.
There was a problem hiding this comment.
Do you mean the entire SessionDelegate? no, it's used in the OAuthClient init in the main apps
let urlSession = URLSession(configuration: configuration,
delegate: SessionDelegate(),
delegateQueue: nil)
| } | ||
|
|
||
| /// The sole entity responsible of obtaining, storing and refreshing an OAuth Token | ||
| public protocol SubscriptionTokenProvider { |
There was a problem hiding this comment.
Since this is a big file I would split out these additional protocols and enums not directly related to SubscriptionManager to separate files for improved readability.
There was a problem hiding this comment.
I moved SubscriptionFeature and SubscriptionTokenProvider to dedicated files
| public func isFeatureActive(_ entitlement: SubscriptionEntitlement) async -> Bool { | ||
| guard isUserAuthenticated else { return false } | ||
|
|
||
| let currentFeatures = await currentSubscriptionFeatures(forceRefresh: false) | ||
| return currentFeatures.contains { feature in | ||
| feature.entitlement == entitlement && feature.enabled | ||
| } |
There was a problem hiding this comment.
I find this naming confusing. Let's go one by one, currentSubscriptionFeatures above is clear and well defined, features that are provided by current subscription. But when you move to isFeatureActive and SubscriptionFeature.enabled it can be interpreted in multiple ways (e.g. isFeatureActive(.networkProtection), does it mean that VPN is running? etc.).
isFeatureActive- means if the feature is provided within the current subscription and if current user has entitlements to use it, previously we sticked to established vocabulary and checked if used has entitlements to a feature. If you don't like this maybe something along the lineisFeatureAvailableForUser?SubscriptionFeature.enabled- this is the underlying property storing Bool for the above so I would align them together
There was a problem hiding this comment.
I agree:
isFeatureActiverenamedisFeatureAvailableForUserSubscriptionFeature.enabledrenamedavailableForUser
| cacheSerialQueue.sync { | ||
| subscriptionCache.reset() | ||
| } | ||
| // NotificationCenter.default.post(name: .subscriptionDidChange, object: self, userInfo: nil) |
There was a problem hiding this comment.
Please double check if this is needed.
There was a problem hiding this comment.
Not needed for now, anything related to the subscription cancellation is handled by the signOut that sends the proper notification. I'm keeping this commented to remind me where it COULD be if I need it, not to be merged anyway.
# Conflicts: # Sources/PrivacyDashboard/PrivacyDashboardUserScript.swift
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