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()