AuthV2 / Adding v2 classes #1194

federicocappelli · 2025-01-27T12:40:41Z

Task/Issue URL: https://app.asana.com/0/1205842942115003/1209170372758737/f
iOS PR: duckduckgo/iOS#3882
macOS PR: duckduckgo/macos-browser#3789
What kind of version bump will this require?: Patch

Description:

Subscription renamed PrivacyProSubscription
APIService renamed SubscriptionAPIService
Subscription V2 classes added but not used
If a struct or an enum has been removed (e.g GetProductsItem) that code is shared between V1 and V2 and has been moved in the V2 version of the class

Internal references:

Software Engineering Expectations
Technical Design Template

federicocappelli · 2025-01-27T13:34:04Z

Sources/Subscription/Storage/V1/SubscriptionTokenKeychainStorage.swift

        return attributes
    }
 }
-
-public enum KeychainType {


moved to BSK/Common

federicocappelli · 2025-01-27T14:38:38Z

Sources/Subscription/SubscriptionCookie/SubscriptionCookieManager.swift

@@ -21,7 +21,6 @@ import Common
 import os.log

 public protocol SubscriptionCookieManaging {
-    init(subscriptionManager: SubscriptionManager, currentCookieStore: @MainActor @escaping () -> HTTPCookieStore?, eventMapping: EventMapping<SubscriptionCookieManagerEvent>)


the init doesn't belong to the protocol, removing this the entire manager can be easily mocked

Tests/MaliciousSiteProtectionTests/MaliciousSiteProtectionUpdateManagerTests.swift

miasma13

I have reviewed the code, please find some comments and let me follow with some smoke testing of the apps tomorrow.

miasma13 · 2025-01-30T13:27:05Z

Sources/Subscription/API/Model/PrivacyProSubscription.swift

+    /// Not parsed from 
+    public var features: [SubscriptionEntitlement]?
+


What is the purpose for this var if it is not a part of parsed model?

The SubscriptionEntitlement are created with a combination of (A) user entitlements and (B) subscription entitlements, both A and B are fetched after the subscription is fetched and saved here, and then the entire subscription is saved in the cache. So this is a way of saving the features without overcomplicating with separate models and handling logic.

miasma13 · 2025-01-30T14:20:22Z

Sources/Subscription/API/SubscriptionEndpointServiceV2.swift

+        cacheSerialQueue.sync {
+            subscriptionCache.reset()
+        }
+//        NotificationCenter.default.post(name: .subscriptionDidChange, object: self, userInfo: nil)


Are we able to determine if this notification is necessary or not? If we are not sure at this point let's have a TODO to verify it later.

Agree, todo added

miasma13 · 2025-01-30T14:29:54Z

Sources/Subscription/Flows/AppStore/AppStorePurchaseFlowV2.swift

+        case .failure(let error):
+            Logger.subscriptionAppStorePurchaseFlow.error("purchaseSubscription error: \(String(reflecting: error), privacy: .public)")
+
+            await subscriptionManager.signOut(notifyUI: true)


The old implementation had an inverse and skipped the notification here. IIRC this was connected with a trigger to reload/close Privacy Pro related tabs and on mac this would hit the very tab you were making the purchase on.

I'm sure I tested this specific case but I can't remember now, so I'm reverting this to false and add a todo for later

miasma13 · 2025-01-30T14:59:46Z

Sources/Subscription/Flows/AppStore/AppStoreRestoreFlowV2.swift

+                Logger.subscriptionAppStoreRestoreFlow.error("Subscription expired")
+                // Removing all traces of the subscription and the account
+                await subscriptionManager.signOut(notifyUI: false)
+                return .failure(.subscriptionExpired)


Handling for situation when restore from past purchase gives us expired subscription is not the same as in the old API. Please refer to

BrowserServicesKit/Sources/Subscription/Flows/AppStore/AppStoreRestoreFlow.swift

Line 121 in 20b2408

let details = RestoredAccountDetails(authToken: authToken, accessToken: accessToken, externalID: externalID, email: email)

we did not signed out user immediately, but returned wrapped account details info to be handled later on. The flow was to present the user with an alert "Your subscription had expired" -> View Plans. This allowed us to enter the purchase flow having the IDs of old account already in place - the user was not creating a new account but was effectively repurchasing a subscription to their old account.

miasma13 · 2025-01-30T15:20:45Z

Sources/Subscription/SubscriptionFeatureV2.swift

+/// A `SubscriptionFeature` is **availableForUser** if the logged in user has the required entitlements.
+public struct SubscriptionFeatureV2: Equatable, CustomDebugStringConvertible {
+    public var entitlement: SubscriptionEntitlement
+    public var availableForUser: Bool


availableForUser -> isAvailableForUser

miasma13 · 2025-01-30T15:28:20Z

Sources/Subscription/Storage/V2/SubscriptionTokenKeychainStorageV2.swift

+import Networking
+import Common
+
+public final class SubscriptionTokenKeychainStorageV2: AuthTokenStoring {


Maybe a better name for SubscriptionTokenKeychainStorageV2 would be AuthTokenKeychainStorage / AuthTokenContainerKeychainStorage / AccountTokenContainerKeychainStorage, WDYT?

The name comes from auth v1 SubscriptionTokenKeychainStorage, I would like to keep the same name for now for highlighting that they are the same class.

miasma13 · 2025-01-30T15:32:50Z

Sources/Subscription/Storage/V2/SubscriptionTokenKeychainStorageV2.swift

+     multiple accounts/tokens at the same time
+     */
+    enum SubscriptionKeychainField: String, CaseIterable {
+        case tokens = "subscription.v2.tokens"


tokens -> tokenContainer

miasma13 · 2025-01-30T16:52:50Z

Sources/Subscription/Managers/SubscriptionManagerV2.swift

+
+            if referenceCachedTokenContainer == nil { // new login
+                Logger.subscription.debug("New login detected")
+                NotificationCenter.default.post(name: .accountDidSignIn, object: self, userInfo: nil)


Not sure if this is a good approach, I understand where it came from with translating v1 behaviour to v2 but conceptually the old implementation for the accountDidSignIn/Out events was triggered by storing/clearing authentication data. Here we have it implemented as a side-effect of the getTokenContainer getter.

That point in the code is the only place where we know if a login has changed, and it is a function that is used 100% of the time when a token is retrieved.
Removing this notification from here would force us to implement a check manually every time we get a token, I wouldn't know how to do that.
I wouldn't categorise a notification as a side effect; it's simply notifying that something has changed from the only function responsible for retrieving the tokens.
If you find a safer and nicer place where to check, I'm open to suggestions; I haven't found one.

miasma13 · 2025-01-30T17:00:55Z

Sources/Subscription/Managers/SubscriptionManagerV2.swift

+            let subscription = try await subscriptionEndpointService.getSubscription(accessToken: "", // Token is unused
+                                                                                     cachePolicy: .returnCacheDataDontLoad)
+            switch subscription.platform {


I suspect it is highly unlikely this logic to succeed, if the token is dead it means that it is long time since the app interacted with BE, hence the subscription cache will be stale (currently it is set to 20 minutes).

I agree. The dead token handling needs to be reviewed completely as you pointed out in your first PR review, and I created a task dedicated to that https://app.asana.com/0/72649045549333/1209252725803000

miasma13 · 2025-01-30T17:01:28Z

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 {


What is the real purpose of this function?

#1194 (comment)

miasma13

👍 LGTM

federicocappelli added 6 commits January 27, 2025 11:47

re-adding v1 classes and renaming new classes with v2

337a9c0

porting other changes

9c48278

v1 suffix removed and v2 suffix added

ace2294

V1 Testing utilities re-added

36b2690

func documentation, renaming

ac64aa3

commented out code removed

91d64d0

federicocappelli commented Jan 27, 2025

View reviewed changes

lint

2dfe234

federicocappelli commented Jan 27, 2025

View reviewed changes

federicocappelli added 6 commits January 27, 2025 14:48

unit tests renamed v2

b5f5759

v1 tests restored

e17b52c

lint

fc31514

tests fixed

f77e067

Merge branch 'main' into fcappelli/authv2/adding_v2_classes

fecc9d9

tests timeouts increased

d2d6852

federicocappelli commented Jan 27, 2025

View reviewed changes

Tests/MaliciousSiteProtectionTests/MaliciousSiteProtectionUpdateManagerTests.swift Show resolved Hide resolved

federicocappelli requested a review from miasma13 January 28, 2025 09:30

mallexxx and others added 2 commits January 28, 2025 19:20

fix MaliciousSiteProtectionUpdateManagerTests

c81c5f8

Merge branch 'main' into fcappelli/authv2/adding_v2_classes

38dd027

miasma13 reviewed Jan 30, 2025

View reviewed changes

duckduckgo deleted a comment from miasma13 Jan 30, 2025

federicocappelli added 4 commits January 30, 2025 17:52

todo added

fe63581

availableForUser renamed

2aa5875

pr suggestions

abf0681

lint

8679e29

miasma13 approved these changes Jan 31, 2025

View reviewed changes

federicocappelli merged commit eee15cd into main Jan 31, 2025
7 checks passed

federicocappelli deleted the fcappelli/authv2/adding_v2_classes branch January 31, 2025 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthV2 / Adding v2 classes #1194

AuthV2 / Adding v2 classes #1194

federicocappelli commented Jan 27, 2025 •

edited

Loading

federicocappelli Jan 27, 2025

federicocappelli Jan 27, 2025

miasma13 left a comment

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025 •

edited

Loading

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 Jan 30, 2025

federicocappelli Jan 30, 2025

miasma13 left a comment

		/// Not parsed from
		public var features: [SubscriptionEntitlement]?

AuthV2 / Adding v2 classes #1194

AuthV2 / Adding v2 classes #1194

Conversation

federicocappelli commented Jan 27, 2025 • edited Loading

Internal references:

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

federicocappelli Jan 30, 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

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

miasma13 left a comment

Choose a reason for hiding this comment

federicocappelli commented Jan 27, 2025 •

edited

Loading

federicocappelli Jan 30, 2025 •

edited

Loading