Dead token recovery #1216

federicocappelli · 2025-02-05T13:34:36Z

Task/Issue URL: https://app.asana.com/0/1205842942115003/1209252725803000
iOS PR: no
macOS PR: no
What kind of version bump will this require?: none

Description:

This PR introduces improvements in the "dead token" detection and recovery for Auth V2.
A deat token happens when the refresh has expired.
The only thing we can do is log out the user and try to recover the subscription, if it was purchased via the Apple store, or show an error to the user for Stripe.

Steps to test this PR:
The code is not used yet, check the unit tests

Internal references:

Software Engineering Expectations
Technical Design Template

federicocappelli · 2025-02-05T13:36:52Z

Sources/Networking/Auth/OAuthClient.swift

@@ -204,26 +207,45 @@ final public class DefaultOAuthClient: OAuthClient {
    public func getTokens(policy: AuthTokensCachePolicy) async throws -> TokenContainer {
        let localTokenContainer = tokenStorage.tokenContainer

+        if policy != .local,


we now check the refresh token validity every time we return a token

federicocappelli · 2025-02-05T14:11:14Z

Sources/Networking/Auth/OAuthClient.swift

+#if DEBUG
+            let expiresSoon = false
+#else
+            // Expires in less than 10 minutes, check only in release, the staging token expires every 4 minutes


I've added a local check for the token expiry date, if it's less than 10 minutes we treat it as expired

What do you think about extracting the "10" into a constant. Also for the DEBUG you could use a different value (2?) there? Logic here would be cleaner and the flow would be dependant on a value.

federicocappelli · 2025-02-05T14:12:54Z

Sources/Networking/Auth/OAuthClient.swift

@@ -237,11 +259,12 @@ final public class DefaultOAuthClient: OAuthClient {
                return refreshedTokens
            } catch OAuthServiceError.authAPIError(let code) where code == OAuthRequest.BodyErrorCode.invalidTokenRequest {
                Logger.OAuthClient.error("Failed to refresh token: invalidTokenRequest")
-                throw OAuthClientError.refreshTokenExpired
+                throw OAuthClientError.invalidTokenRequest


Here I maintain a similar error to the one sent by the API. .invalidTokenRequest can happen for reasons other than a dead token, for example when we try to use a staging token in production.

federicocappelli · 2025-02-05T14:24:51Z

Sources/Subscription/DeadTokenRecoverer.swift

+@available(macOS 12.0, *)
+public struct DeadTokenRecoverer {
+
+    private static var recoveryAttempted: Int = 0


In case the BE has a problem and always returns a OAuthClientError.invalidTokenRequest every time we request a token, we don't want to enter an infinite loop, so we must be sure that we are not already in a recovery state.
This state helps with that and it's not a bool because multiple, parallel API requests could use this utility and we want to be sure we don't have concurrency issues.

federicocappelli · 2025-02-05T14:26:18Z

Sources/Subscription/DeadTokenRecoverer.swift

+import os.log
+
+@available(macOS 12.0, *)
+public struct DeadTokenRecoverer {


This utility must be outside the subscription manager because requires Apple recovery flow, but we don't want to re-implement it every time, so the dead token recovery business logic is all here and can be used by the main apps.
This utility's main function is used in the SubscriptionManagerV2 autoRecoveryHandler block

federicocappelli · 2025-02-05T14:30:21Z

Tests/SubscriptionTests/Managers/SubscriptionManagerV2Tests.swift


        subscriptionManager = DefaultSubscriptionManagerV2(
            storePurchaseManager: mockStorePurchaseManager,
            oAuthClient: mockOAuthClient,
            subscriptionEndpointService: mockSubscriptionEndpointService,
            subscriptionEnvironment: SubscriptionEnvironment(serviceEnvironment: .staging, purchasePlatform: .stripe),
-            pixelHandler: { _ in }
+            pixelHandler: { _ in },
+            autoRecoveryHandler: {


this is an example of the new handler implementation, ignore the overrideTokenResponse that is used only for testing purposes

federicocappelli · 2025-02-05T14:31:04Z

Sources/Subscription/Managers/SubscriptionManagerV2.swift

-    /// If the client succeeds in making a refresh request but does not get the response, then the second refresh request will fail with `invalidTokenRequest` and the stored token will become unusable and un-refreshable.
-    private func throwAppropriateDeadTokenError() async throws -> TokenContainer {
-        Logger.subscription.fault("Dead token detected")
+    func attemptTokenRecovery() async throws -> TokenContainer {


this is where the autoRecoveryHandler is used

miasma13

Overall LGTM, just couple of comments

miasma13 · 2025-02-06T10:59:23Z

Sources/Networking/Auth/OAuthClient.swift

+#if DEBUG
+            let expiresSoon = false
+#else
+            // Expires in less than 10 minutes, check only in release, the staging token expires every 4 minutes


What do you think about extracting the "10" into a constant. Also for the DEBUG you could use a different value (2?) there? Logic here would be cleaner and the flow would be dependant on a value.

Sources/Subscription/DeadTokenRecoverer.swift

Sources/Subscription/Managers/SubscriptionManagerV2.swift

miasma13 · 2025-02-06T11:25:07Z

Sources/Subscription/DeadTokenRecoverer.swift

+
+    private static var recoveryAttempted: Int = 0
+
+    public static func recoverDeadToken(endpointService: any SubscriptionEndpointServiceV2,


Two things:

I am thinking about more descriptive name for it as it does very specific thing. I am missing that this is an attempt (and may fail) and how the recovery is made. Suggestions:

attemptDeadTokenRecoveryFromPastPurchase(endpointService: any SubscriptionEndpointServiceV2, restoreFlow: any AppStoreRestoreFlowV2)

attemptDeadTokenRecovery(using restoreFlow: any AppStoreRestoreFlowV2, endpointService: any SubscriptionEndpointServiceV2)

To be wary for the "real" implementation if the function can potentially be invoked multiple times in parallel without synchronisation.

What about DeadTokenRecoverer.attemptRecoveryFromPastPurchase(...)?

In theory, recoveryAttemptCount in the struct is the safeguard for that. Only one recovery operation at the time will be executed, the other ones will throw a .tokenUnRefreshable

dead token recovery + tests

a8c7d27

federicocappelli commented Feb 5, 2025

View reviewed changes

federicocappelli added 2 commits February 5, 2025 22:14

lint

8135891

date extension comments improved

cbafa53

federicocappelli commented Feb 5, 2025

View reviewed changes

federicocappelli requested a review from miasma13 February 5, 2025 14:33

Merge branch 'main' into fcappelli/Refresh_Token_Expired

cdee6e9

federicocappelli requested review from miasma13 and removed request for miasma13 February 5, 2025 14:42

federicocappelli added 2 commits February 6, 2025 13:57

subscription caching back-ported

c24b61b

lint

8b7f37f

miasma13 approved these changes Feb 6, 2025

View reviewed changes

federicocappelli added 3 commits February 6, 2025 22:03

PR comments

c096b71

pr suggestions

57eb623

lint

d11ab40

federicocappelli merged commit 7ceea51 into main Feb 7, 2025
7 checks passed

federicocappelli deleted the fcappelli/Refresh_Token_Expired branch February 7, 2025 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dead token recovery #1216

Dead token recovery #1216

federicocappelli commented Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025

federicocappelli Feb 5, 2025

miasma13 Feb 6, 2025

federicocappelli Feb 6, 2025

federicocappelli Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025

federicocappelli Feb 5, 2025

miasma13 left a comment

miasma13 Feb 6, 2025

miasma13 Feb 6, 2025

federicocappelli Feb 6, 2025


		private static var recoveryAttempted: Int = 0

		public static func recoverDeadToken(endpointService: any SubscriptionEndpointServiceV2,

Dead token recovery #1216

Dead token recovery #1216

Conversation

federicocappelli commented Feb 5, 2025 • edited Loading

Internal references:

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 Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

federicocappelli Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

federicocappelli Feb 5, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

federicocappelli commented Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025 •

edited

Loading

federicocappelli Feb 5, 2025 •

edited

Loading