From 00a298b09fcf2da72c881ce39784b71d6d58b0b8 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Mon, 24 Aug 2026 19:59:25 +0200 Subject: [PATCH] Order tokenless scope resolution with an identity generation Sixth review pass, both findings in the tokenless path the fourth pass introduced. The staleness guard compared fingerprints, which is useless when there is no ubiquity token: nil before the switch and nil after it, so a user-record lookup started under account A was accepted as current after CKAccountChanged had already announced B. The scope then read as resolved, the pending-resolve replay was skipped because it only runs while unresolved, and B was left pointed at A's credential namespace. Every async path now captures a generation counter that every observed account change bumps, and refuses to apply a result across it. Foreground revalidation also kept serving the old tokenless scope across both awaits, and indefinitely when the account status came back undetermined - so a switch that happened while the app was suspended could hand A's retained credentials to B. The scope is now dropped before the confirmation rather than compared after it, which also let the bespoke revalidation logic collapse into resolve(), where the signed-out, signed-in and undetermined cases are already handled. A tokenless setup briefly reports "can't tell yet" on each activation; the tri-state UI covers that, and the alternative traded a real cross-account disclosure for the convenience. --- .../Credentials/CredentialIdentityScope.swift | 66 ++++++++++-------- YatteeTests/AccountCredentialsTests.swift | 68 ++++++++++++++++++- 2 files changed, 103 insertions(+), 31 deletions(-) diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index 5d36ad81..866fd203 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -84,6 +84,13 @@ final class CredentialIdentityScope { /// switch produces no token change to observe — it has to be revalidated /// against CloudKit instead. private var resolvedWithoutToken = false + /// Bumped on every observed Apple-account change. + /// + /// The fingerprint alone cannot order async results: with no ubiquity token + /// it is `nil` before *and* after a switch, so comparing it would accept + /// the previous account's in-flight lookup as current. Every async + /// operation captures this and refuses to apply its result if it moved. + private var identityGeneration = 0 private static let maxResolutionAttempts = 4 /// The resolved scope, or `nil` while unresolved. @@ -150,12 +157,18 @@ final class CredentialIdentityScope { } } - private func identityDidChange(source: String) { + /// Handles an observed Apple-account change. Internal so tests can drive it + /// deterministically instead of racing a notification. + func identityDidChange(source: String) { LoggingService.shared.info( "Apple account changed (\(source)) — re-resolving the credential identity scope", category: .keychain ) + // Invalidates every in-flight lookup: results captured under the old + // generation are refused even when the fingerprint looks unchanged. + identityGeneration += 1 + // A tokenless scope cannot be re-derived by `adoptCurrentIdentity()` // (there is no fingerprint to compare), so drop it outright: continuing // to serve it would hand the previous account's credentials to the new @@ -193,6 +206,7 @@ final class CredentialIdentityScope { let current = fingerprintProvider() if current != fingerprint { resolutionAttempts = 0 + identityGeneration += 1 } fingerprint = current @@ -257,6 +271,7 @@ final class CredentialIdentityScope { // detected: a record ID fetched under account A must never be cached // for B. let requested = fingerprint + let generation = identityGeneration // Labelled: an unqualified `break` inside the switch below would leave // only the switch and fall through to the user-record lookup, which @@ -270,7 +285,8 @@ final class CredentialIdentityScope { if requested == nil { switch await accountAvailabilityProvider() { case .noAccount: - guard fingerprintProvider() == nil, fingerprint == nil else { return } + guard identityGeneration == generation, + fingerprintProvider() == nil, fingerprint == nil else { return } resolvedWithoutToken = true state = .resolved(Self.localScope) LoggingService.shared.info( @@ -283,13 +299,14 @@ final class CredentialIdentityScope { case .indeterminate: guard resolutionAttempts < Self.maxResolutionAttempts else { break attempts } await retryDelay(resolutionAttempts) - guard fingerprint == requested else { return } + guard identityGeneration == generation, fingerprint == requested else { return } continue attempts } } if let recordID = await resolveUserRecordID() { - guard fingerprintProvider() == requested, fingerprint == requested else { + guard identityGeneration == generation, + fingerprintProvider() == requested, fingerprint == requested else { LoggingService.shared.info( "Discarding credential scope resolved for a previous Apple account", category: .keychain @@ -317,7 +334,7 @@ final class CredentialIdentityScope { guard resolutionAttempts < Self.maxResolutionAttempts else { break attempts } await retryDelay(resolutionAttempts) - guard fingerprint == requested else { return } + guard identityGeneration == generation, fingerprint == requested else { return } } // Every credential read is disabled until this succeeds, so failing @@ -350,35 +367,24 @@ final class CredentialIdentityScope { /// never arrive. Without this check the scope would keep pointing at the /// previous account until relaunch — and its retained credentials with it. /// - /// Revalidation compares before replacing rather than invalidating first, - /// so the common case (nothing changed) never blanks out credentials. + /// The scope is dropped **before** the confirmation, not compared after it. + /// Holding it live across the awaits would serve the previous account's + /// credentials for the length of a CloudKit round trip — and indefinitely + /// when the account status comes back undetermined. The cost is that a + /// tokenless setup briefly reports "can't tell yet" on each activation, + /// which the tri-state UI already handles; the alternative trades a real + /// cross-account disclosure for that convenience. private func revalidateTokenlessScope() async { guard resolvedWithoutToken, fingerprintProvider() == nil else { return } - switch await accountAvailabilityProvider() { - case .noAccount: - guard current != Self.localScope else { return } - LoggingService.shared.info( - "Apple account signed out — credential scope falling back to local", - category: .keychain - ) - resolvedWithoutToken = true - state = .resolved(Self.localScope) + state = .unresolved + resolvedWithoutToken = false + resolutionAttempts = 0 - case .available: - guard let recordID = await resolveUserRecordID(), recordID != current else { return } - resolvedWithoutToken = true - LoggingService.shared.info( - "Apple account changed without a token change — credential scope re-pointed", - category: .keychain - ) - state = .resolved(recordID) - - case .indeterminate: - // Unreachable proves nothing, and invalidating on every transient - // network failure would make credentials flap. The scope stands. - break - } + // `resolve()` already encodes the whole decision: signed out scopes to + // local, signed in resolves the user record, undetermined stays + // unresolved and retries. + await resolve() } // MARK: - Cache diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index dd69dc08..b793cea5 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -398,12 +398,15 @@ struct CredentialIdentityScopeTests { var switchTask: Task? func resolve() async -> String? { + // Snapshotted before the hook runs, so a switch staged inside it + // yields a genuinely stale result rather than the new value. + let tokenless = tokenlessRecord onResolveStart?() if failuresRemaining > 0 { failuresRemaining -= 1 return nil } - guard let fingerprint else { return nil } + guard let fingerprint else { return tokenless } return resultsByFingerprint[fingerprint] } } @@ -598,6 +601,69 @@ struct CredentialIdentityScopeTests { #expect(scope.current == "_recordA") } + @Test("A tokenless scope is dropped, not served, while it is unconfirmed") + func tokenlessScopeIsNotServedWhileUnconfirmed() async { + let harness = Harness() + harness.fingerprint = nil + harness.availability = .available + harness.tokenlessRecord = "_recordA" + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { harness.tokenlessRecord }, + accountAvailabilityProvider: { harness.availability }, + retryDelay: { _ in } + ) + + await scope.resolve() + #expect(scope.current == "_recordA") + + // Foregrounding with CloudKit unreachable must not keep serving A — + // the account may have changed while the app was suspended. + harness.availability = .indeterminate + await scope.resolveIfNeeded() + #expect(scope.current == nil) + } + + @Test("A stale tokenless lookup is refused after an account change") + func staleTokenlessLookupIsRefused() async { + // Both before and after the switch the fingerprint is nil, so only a + // generation counter can tell the two lookups apart. + let harness = Harness() + harness.fingerprint = nil + harness.availability = .available + harness.tokenlessRecord = "_recordA" + + let scope = CredentialIdentityScope( + defaults: makeDefaults(), + fingerprintProvider: { harness.fingerprint }, + resolveUserRecordID: { await harness.resolve() }, + accountAvailabilityProvider: { harness.availability }, + retryDelay: { _ in } + ) + + // CKAccountChanged lands while A's lookup is in flight. + harness.onResolveStart = { [weak scope] in + harness.onResolveStart = nil + MainActor.assumeIsolated { + harness.tokenlessRecord = "_recordB" + scope?.identityDidChange(source: "test") + } + } + + await scope.resolve() + + // The decisive assertion: A's result must not be accepted just because + // nil == nil on both sides of the switch. + #expect(scope.current != "_recordA") + + // Then the replay lands on B (driven here rather than awaiting the + // observer's detached task). + await scope.resolveIfNeeded() + #expect(scope.current == "_recordB") + } + @Test("An undeterminable account status leaves the scope unresolved") func indeterminateStatusStaysUnresolved() async { let harness = Harness()