mirror of
https://github.com/yattee/yattee.git
synced 2026-10-28 15:37:17 +00:00
Revalidate a tokenless credential scope on account change
Fourth review pass, on a hole the previous commit opened. Making a nil ubiquity token mean "unresolved" fixed the shared-local-scope collision, but left a scope resolved from CloudKit alone with nothing local to watch: with iCloud Drive off the token stays nil across a switch from account A to B, so NSUbiquityIdentityDidChange never fires and the scope kept pointing at A - and its retained credentials with it - until relaunch. Two additions. CKAccountChanged is now observed alongside the ubiquity notification, and a tokenless scope is dropped outright when either fires because there is no fingerprint to re-derive it from. Foreground activation also revalidates a tokenless scope against CloudKit, covering a switch whose notification never arrived; it compares before replacing so the common case never blanks out credentials, and an unreachable account leaves the scope standing rather than flapping on every transient failure. A token-derived scope is untouched by all of this - it changes only via the notification, and a test pins that down.
This commit is contained in:
@@ -5,6 +5,7 @@
|
|||||||
// Resolves the Apple-account scope that Keychain credentials are keyed by.
|
// Resolves the Apple-account scope that Keychain credentials are keyed by.
|
||||||
//
|
//
|
||||||
|
|
||||||
|
import CloudKit
|
||||||
import Foundation
|
import Foundation
|
||||||
|
|
||||||
/// Resolves the identity scope that account credentials are namespaced under.
|
/// Resolves the identity scope that account credentials are namespaced under.
|
||||||
@@ -65,6 +66,7 @@ final class CredentialIdentityScope {
|
|||||||
/// Written only on the main actor; read in `deinit`, which runs after the
|
/// Written only on the main actor; read in `deinit`, which runs after the
|
||||||
/// last reference is gone and so cannot race with it.
|
/// last reference is gone and so cannot race with it.
|
||||||
private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol?
|
private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol?
|
||||||
|
private nonisolated(unsafe) var cloudKitAccountObserver: NSObjectProtocol?
|
||||||
|
|
||||||
private static let cacheKey = "credentialIdentityScopeCache"
|
private static let cacheKey = "credentialIdentityScopeCache"
|
||||||
|
|
||||||
@@ -76,6 +78,12 @@ final class CredentialIdentityScope {
|
|||||||
/// running one is for a now-stale identity and will bail on the fingerprint
|
/// running one is for a now-stale identity and will bail on the fingerprint
|
||||||
/// check, so the request has to be replayed rather than dropped.
|
/// check, so the request has to be replayed rather than dropped.
|
||||||
private var pendingResolve = false
|
private var pendingResolve = false
|
||||||
|
/// Whether the current scope was resolved with no ubiquity token behind it.
|
||||||
|
///
|
||||||
|
/// Such a scope has no fingerprint to compare against, so an Apple-account
|
||||||
|
/// switch produces no token change to observe — it has to be revalidated
|
||||||
|
/// against CloudKit instead.
|
||||||
|
private var resolvedWithoutToken = false
|
||||||
private static let maxResolutionAttempts = 4
|
private static let maxResolutionAttempts = 4
|
||||||
|
|
||||||
/// The resolved scope, or `nil` while unresolved.
|
/// The resolved scope, or `nil` while unresolved.
|
||||||
@@ -110,6 +118,9 @@ final class CredentialIdentityScope {
|
|||||||
if let identityChangeObserver {
|
if let identityChangeObserver {
|
||||||
NotificationCenter.default.removeObserver(identityChangeObserver)
|
NotificationCenter.default.removeObserver(identityChangeObserver)
|
||||||
}
|
}
|
||||||
|
if let cloudKitAccountObserver {
|
||||||
|
NotificationCenter.default.removeObserver(cloudKitAccountObserver)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Re-scopes as soon as the Apple account changes under a running app.
|
/// Re-scopes as soon as the Apple account changes under a running app.
|
||||||
@@ -119,21 +130,44 @@ final class CredentialIdentityScope {
|
|||||||
/// reads and writes reach the previous account's retained credentials —
|
/// reads and writes reach the previous account's retained credentials —
|
||||||
/// the exact leak scoping exists to prevent.
|
/// the exact leak scoping exists to prevent.
|
||||||
private func observeIdentityChanges() {
|
private func observeIdentityChanges() {
|
||||||
|
// Two signals, because neither covers the other. The ubiquity token
|
||||||
|
// changes only when iCloud Drive is available; with it switched off the
|
||||||
|
// token stays nil across a switch and only CloudKit notices.
|
||||||
identityChangeObserver = NotificationCenter.default.addObserver(
|
identityChangeObserver = NotificationCenter.default.addObserver(
|
||||||
forName: .NSUbiquityIdentityDidChange,
|
forName: .NSUbiquityIdentityDidChange,
|
||||||
object: nil,
|
object: nil,
|
||||||
queue: .main
|
queue: .main
|
||||||
) { [weak self] _ in
|
) { [weak self] _ in
|
||||||
MainActor.assumeIsolated {
|
MainActor.assumeIsolated { self?.identityDidChange(source: "ubiquity token") }
|
||||||
guard let self else { return }
|
|
||||||
LoggingService.shared.info(
|
|
||||||
"Apple account changed — re-resolving the credential identity scope",
|
|
||||||
category: .keychain
|
|
||||||
)
|
|
||||||
self.adoptCurrentIdentity()
|
|
||||||
Task { await self.resolve() }
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
cloudKitAccountObserver = NotificationCenter.default.addObserver(
|
||||||
|
forName: .CKAccountChanged,
|
||||||
|
object: nil,
|
||||||
|
queue: .main
|
||||||
|
) { [weak self] _ in
|
||||||
|
MainActor.assumeIsolated { self?.identityDidChange(source: "CloudKit account") }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private func identityDidChange(source: String) {
|
||||||
|
LoggingService.shared.info(
|
||||||
|
"Apple account changed (\(source)) — re-resolving the credential identity scope",
|
||||||
|
category: .keychain
|
||||||
|
)
|
||||||
|
|
||||||
|
// 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
|
||||||
|
// one.
|
||||||
|
if resolvedWithoutToken {
|
||||||
|
state = .unresolved
|
||||||
|
resolvedWithoutToken = false
|
||||||
|
resolutionAttempts = 0
|
||||||
|
}
|
||||||
|
|
||||||
|
adoptCurrentIdentity()
|
||||||
|
Task { await resolve() }
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Test seam: a scope pinned up front, never consulting CloudKit.
|
/// Test seam: a scope pinned up front, never consulting CloudKit.
|
||||||
@@ -165,6 +199,7 @@ final class CredentialIdentityScope {
|
|||||||
guard CloudKitAvailability.isAvailable else {
|
guard CloudKitAvailability.isAvailable else {
|
||||||
// No iCloud entitlement: nothing can sync, and there is no ambient
|
// No iCloud entitlement: nothing can sync, and there is no ambient
|
||||||
// identity for a retained credential to leak to.
|
// identity for a retained credential to leak to.
|
||||||
|
resolvedWithoutToken = false
|
||||||
state = .resolved(Self.localScope)
|
state = .resolved(Self.localScope)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -181,6 +216,7 @@ final class CredentialIdentityScope {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if let cached = cachedScope(for: current) {
|
if let cached = cachedScope(for: current) {
|
||||||
|
resolvedWithoutToken = false
|
||||||
state = .resolved(cached)
|
state = .resolved(cached)
|
||||||
} else {
|
} else {
|
||||||
state = .unresolved
|
state = .unresolved
|
||||||
@@ -235,6 +271,7 @@ final class CredentialIdentityScope {
|
|||||||
switch await accountAvailabilityProvider() {
|
switch await accountAvailabilityProvider() {
|
||||||
case .noAccount:
|
case .noAccount:
|
||||||
guard fingerprintProvider() == nil, fingerprint == nil else { return }
|
guard fingerprintProvider() == nil, fingerprint == nil else { return }
|
||||||
|
resolvedWithoutToken = true
|
||||||
state = .resolved(Self.localScope)
|
state = .resolved(Self.localScope)
|
||||||
LoggingService.shared.info(
|
LoggingService.shared.info(
|
||||||
"No Apple account — credentials scoped locally",
|
"No Apple account — credentials scoped locally",
|
||||||
@@ -266,6 +303,7 @@ final class CredentialIdentityScope {
|
|||||||
if let requested {
|
if let requested {
|
||||||
cache(recordID, for: requested)
|
cache(recordID, for: requested)
|
||||||
}
|
}
|
||||||
|
resolvedWithoutToken = requested == nil
|
||||||
state = .resolved(recordID)
|
state = .resolved(recordID)
|
||||||
|
|
||||||
LoggingService.shared.info(
|
LoggingService.shared.info(
|
||||||
@@ -297,11 +335,52 @@ final class CredentialIdentityScope {
|
|||||||
/// offline would otherwise keep credentials unavailable for the entire
|
/// offline would otherwise keep credentials unavailable for the entire
|
||||||
/// process lifetime, long after connectivity returned.
|
/// process lifetime, long after connectivity returned.
|
||||||
func resolveIfNeeded() async {
|
func resolveIfNeeded() async {
|
||||||
guard !isResolved else { return }
|
guard !isResolved else {
|
||||||
|
await revalidateTokenlessScope()
|
||||||
|
return
|
||||||
|
}
|
||||||
resolutionAttempts = 0
|
resolutionAttempts = 0
|
||||||
await resolve()
|
await resolve()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Confirms that a tokenless scope still names the signed-in account.
|
||||||
|
///
|
||||||
|
/// With iCloud Drive switched off the ubiquity token stays `nil` across an
|
||||||
|
/// Apple-account switch, so nothing local changes and the notification may
|
||||||
|
/// 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.
|
||||||
|
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)
|
||||||
|
|
||||||
|
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
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// MARK: - Cache
|
// MARK: - Cache
|
||||||
|
|
||||||
private func cachedScope(for fingerprint: String) -> String? {
|
private func cachedScope(for fingerprint: String) -> String? {
|
||||||
|
|||||||
@@ -392,6 +392,8 @@ struct CredentialIdentityScopeTests {
|
|||||||
var resultsByFingerprint: [String: String] = [:]
|
var resultsByFingerprint: [String: String] = [:]
|
||||||
var failuresRemaining = 0
|
var failuresRemaining = 0
|
||||||
var availability: CloudKitIdentity.AccountAvailability = .available
|
var availability: CloudKitIdentity.AccountAvailability = .available
|
||||||
|
/// User record returned when there is no ubiquity token to key on.
|
||||||
|
var tokenlessRecord: String?
|
||||||
var onResolveStart: (() -> Void)?
|
var onResolveStart: (() -> Void)?
|
||||||
var switchTask: Task<Void, Never>?
|
var switchTask: Task<Void, Never>?
|
||||||
|
|
||||||
@@ -525,6 +527,77 @@ struct CredentialIdentityScopeTests {
|
|||||||
#expect(scope.current == CredentialIdentityScope.localScope)
|
#expect(scope.current == CredentialIdentityScope.localScope)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test("A tokenless scope is re-pointed when the account changes silently")
|
||||||
|
func tokenlessScopeIsRevalidated() async {
|
||||||
|
// iCloud Drive off: the token stays nil across the switch, so nothing
|
||||||
|
// local changes and no ubiquity notification fires.
|
||||||
|
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")
|
||||||
|
|
||||||
|
// The user switches to account B and returns to the app.
|
||||||
|
harness.tokenlessRecord = "_recordB"
|
||||||
|
await scope.resolveIfNeeded()
|
||||||
|
#expect(scope.current == "_recordB")
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test("A tokenless scope falls back to local once the account is gone")
|
||||||
|
func tokenlessScopeFallsBackOnSignOut() async {
|
||||||
|
let harness = Harness()
|
||||||
|
harness.fingerprint = nil
|
||||||
|
harness.availability = .available
|
||||||
|
|
||||||
|
let scope = CredentialIdentityScope(
|
||||||
|
defaults: makeDefaults(),
|
||||||
|
fingerprintProvider: { harness.fingerprint },
|
||||||
|
resolveUserRecordID: { "_recordA" },
|
||||||
|
accountAvailabilityProvider: { harness.availability },
|
||||||
|
retryDelay: { _ in }
|
||||||
|
)
|
||||||
|
|
||||||
|
await scope.resolve()
|
||||||
|
#expect(scope.current == "_recordA")
|
||||||
|
|
||||||
|
harness.availability = .noAccount
|
||||||
|
await scope.resolveIfNeeded()
|
||||||
|
#expect(scope.current == CredentialIdentityScope.localScope)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test("A token-derived scope is not re-checked on every activation")
|
||||||
|
func tokenDerivedScopeIsStable() async {
|
||||||
|
let harness = Harness()
|
||||||
|
harness.resultsByFingerprint = ["identity-A": "_recordA"]
|
||||||
|
|
||||||
|
let scope = CredentialIdentityScope(
|
||||||
|
defaults: makeDefaults(),
|
||||||
|
fingerprintProvider: { harness.fingerprint },
|
||||||
|
resolveUserRecordID: { await harness.resolve() },
|
||||||
|
accountAvailabilityProvider: { harness.availability },
|
||||||
|
retryDelay: { _ in }
|
||||||
|
)
|
||||||
|
|
||||||
|
await scope.resolve()
|
||||||
|
#expect(scope.current == "_recordA")
|
||||||
|
|
||||||
|
// A token-backed scope changes only via the notification, so a
|
||||||
|
// foreground pass must leave it exactly as it was.
|
||||||
|
harness.resultsByFingerprint = ["identity-A": "_somethingElse"]
|
||||||
|
await scope.resolveIfNeeded()
|
||||||
|
#expect(scope.current == "_recordA")
|
||||||
|
}
|
||||||
|
|
||||||
@Test("An undeterminable account status leaves the scope unresolved")
|
@Test("An undeterminable account status leaves the scope unresolved")
|
||||||
func indeterminateStatusStaysUnresolved() async {
|
func indeterminateStatusStaysUnresolved() async {
|
||||||
let harness = Harness()
|
let harness = Harness()
|
||||||
|
|||||||
Reference in New Issue
Block a user