Stop a missing ubiquity token from meaning "signed out"

Third review pass.

adoptCurrentIdentity() treated a nil ubiquityIdentityToken as sign-out and
scoped credentials to "local". But Foundation also returns nil when iCloud
Drive is unavailable or switched off, and Yattee declares only the CloudKit
service with no ubiquity container, so the token tracks iCloud Drive state
rather than the account. In that configuration two different signed-in
Apple accounts both resolved to "local" and shared one credential
namespace - since Keychain items survive an account switch, account B could
then read account A's retained credential. That is the precise failure this
whole change exists to prevent.

A nil token now means unresolved, and CloudKit's account status settles it
during resolution: noAccount scopes to local, available proceeds to the
user-record lookup, anything else stays unresolved and retries. A scope
resolved without a token is not cached, because there is no key that could
be trusted to mean this account.

Adoption markers are now namespaced by Keychain service. Both credential
managers share one marker store and an instance UUID can outlive a change
of backend, so adopting an Invidious credential could suppress the Piped
offer for the same instance and point deletion at the wrong secret.

Writing the account-status branch also surfaced a Swift trap: an
unqualified `break` inside the switch left only the switch and fell through
to the user-record lookup, resolving a scope on an undetermined account
status. The loop is labelled now, and the test that caught it is included.
This commit is contained in:
Arkadiusz Fal
2026-08-23 23:56:31 +02:00
parent f5e88df403
commit 313403cc1e
5 changed files with 205 additions and 19 deletions

View File

@@ -42,6 +42,44 @@ enum CloudKitIdentity {
return shortDigest(of: data)
}
/// Whether an Apple account is signed in, as distinct from unreachable.
///
/// `ubiquityIdentityToken` alone cannot answer this: Foundation returns
/// `nil` both when the user is signed out *and* when iCloud Drive is
/// unavailable or switched off, and Yattee declares only the CloudKit
/// service (no ubiquity container), so the token's availability tracks
/// system iCloud Drive state rather than the account. Treating a nil token
/// as "signed out" would collapse two different Apple accounts into the
/// same `local` credential scope.
enum AccountAvailability {
/// No Apple account — the `local` scope is genuinely correct.
case noAccount
/// Signed in; the user record can be fetched.
case available
/// Unreachable, restricted, or undetermined. Never assume either way.
case indeterminate
}
static func accountAvailability() async -> AccountAvailability {
guard CloudKitAvailability.isAvailable else { return .noAccount }
do {
let container = CKContainer(identifier: AppIdentifiers.iCloudContainer)
switch try await container.accountStatus() {
case .noAccount: return .noAccount
case .available: return .available
default: return .indeterminate
}
} catch {
LoggingService.shared.debug(
"Could not determine CloudKit account status",
category: .cloudKit,
details: "\(error)"
)
return .indeterminate
}
}
/// The CloudKit user record name for the current identity.
///
/// Returns `nil` when CloudKit is unavailable (sideloaded build), when no

View File

@@ -170,7 +170,7 @@ final class AccountKeychainCredentials {
func hasAdoptableLegacyCredential(for account: Account) -> Bool {
guard let scope = currentScope() else { return false }
guard !markers.hasAdopted(instanceID: account.instanceID, scope: scope) else { return false }
guard !markers.hasAdopted(service: store.service, instanceID: account.instanceID, scope: scope) else { return false }
guard credential(for: account) == nil else { return false }
return legacyCredential(for: account) != nil
@@ -186,7 +186,7 @@ final class AccountKeychainCredentials {
@discardableResult
func adoptLegacyCredential(for account: Account) -> Bool {
guard let scope = currentScope() else { return false }
guard !markers.hasAdopted(instanceID: account.instanceID, scope: scope) else { return false }
guard !markers.hasAdopted(service: store.service, instanceID: account.instanceID, scope: scope) else { return false }
guard let legacy = legacyCredential(for: account) else { return false }
// Eligibility is re-checked here, not just when the offer was rendered:
@@ -249,6 +249,7 @@ final class AccountKeychainCredentials {
}
markers.record(
service: store.service,
instanceID: account.instanceID,
accountID: account.id,
scope: scope,
@@ -272,6 +273,7 @@ final class AccountKeychainCredentials {
let currentValue = store.value(forKey: legacyKey)
guard markers.canDeleteLegacyItem(
service: store.service,
instanceID: account.instanceID,
accountID: account.id,
scope: scope,
@@ -286,7 +288,7 @@ final class AccountKeychainCredentials {
}
store.delete(key: legacyKey)
markers.removeMarker(instanceID: account.instanceID, scope: scope)
markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope)
LoggingService.shared.info(
"Deleted the legacy \(credentialNoun) alongside its account",

View File

@@ -20,6 +20,12 @@ struct CredentialAdoptionMarker: Codable, Hashable, Sendable {
/// The identity scope that adopted the item. Apple account A's marker must
/// never authorize anything under account B.
let identityScope: String
/// Keychain service the adopted item lives in. The Invidious and Piped
/// managers keep separate services but share this store, and an instance
/// UUID can outlive a change of backend — without this, adopting one
/// backend's credential would suppress the other's offer and point
/// deletion at the wrong secret.
let service: String
let instanceID: UUID
let accountID: UUID
/// Salted, truncated digest of the adopted credential — non-secret, and
@@ -56,22 +62,30 @@ final class CredentialAdoptionMarkerStore {
///
/// Once true, the app stops offering adoption and stops reading the legacy
/// key entirely for that account.
func hasAdopted(instanceID: UUID, scope: String) -> Bool {
markers().contains { $0.instanceID == instanceID && $0.identityScope == scope }
func hasAdopted(service: String, instanceID: UUID, scope: String) -> Bool {
marker(service: service, instanceID: instanceID, scope: scope) != nil
}
private func marker(service: String, instanceID: UUID, scope: String) -> CredentialAdoptionMarker? {
markers().first {
$0.service == service && $0.instanceID == instanceID && $0.identityScope == scope
}
}
func record(
service: String,
instanceID: UUID,
accountID: UUID,
scope: String,
secret: String
) {
var all = markers().filter {
!($0.instanceID == instanceID && $0.identityScope == scope)
!($0.service == service && $0.instanceID == instanceID && $0.identityScope == scope)
}
all.append(
CredentialAdoptionMarker(
identityScope: scope,
service: service,
instanceID: instanceID,
accountID: accountID,
secretFingerprint: fingerprint(of: secret),
@@ -81,8 +95,10 @@ final class CredentialAdoptionMarkerStore {
save(all)
}
func removeMarker(instanceID: UUID, scope: String) {
save(markers().filter { !($0.instanceID == instanceID && $0.identityScope == scope) })
func removeMarker(service: String, instanceID: UUID, scope: String) {
save(markers().filter {
!($0.service == service && $0.instanceID == instanceID && $0.identityScope == scope)
})
}
// MARK: - Legacy deletion authority
@@ -107,14 +123,15 @@ final class CredentialAdoptionMarkerStore {
/// Deleting a synchronizable item propagates everywhere, so a wrong answer
/// here logs other devices out.
func canDeleteLegacyItem(
service: String,
instanceID: UUID,
accountID: UUID,
scope: String,
currentLegacyValue: String?
) -> Bool {
guard let marker = markers().first(where: {
$0.instanceID == instanceID && $0.identityScope == scope
}) else { return false }
guard let marker = marker(service: service, instanceID: instanceID, scope: scope) else {
return false
}
guard marker.accountID == accountID else { return false }
guard let currentLegacyValue else { return false }

View File

@@ -59,6 +59,7 @@ final class CredentialIdentityScope {
private let defaults: UserDefaults
private let resolveUserRecordID: @Sendable () async -> String?
private let fingerprintProvider: @Sendable () -> String?
private let accountAvailabilityProvider: @Sendable () async -> CloudKitIdentity.AccountAvailability
/// Backoff between attempts. Injectable so tests don't pay real seconds.
private let retryDelay: @Sendable (Int) async -> Void
/// Written only on the main actor; read in `deinit`, which runs after the
@@ -89,6 +90,9 @@ final class CredentialIdentityScope {
defaults: UserDefaults = .standard,
fingerprintProvider: @escaping @Sendable () -> String? = { CloudKitIdentity.ubiquityFingerprint },
resolveUserRecordID: @escaping @Sendable () async -> String? = { await CloudKitIdentity.userRecordID() },
accountAvailabilityProvider: @escaping @Sendable () async -> CloudKitIdentity.AccountAvailability = {
await CloudKitIdentity.accountAvailability()
},
retryDelay: @escaping @Sendable (Int) async -> Void = { attempt in
try? await Task.sleep(for: .seconds(pow(2.0, Double(attempt))))
}
@@ -96,6 +100,7 @@ final class CredentialIdentityScope {
self.defaults = defaults
self.fingerprintProvider = fingerprintProvider
self.resolveUserRecordID = resolveUserRecordID
self.accountAvailabilityProvider = accountAvailabilityProvider
self.retryDelay = retryDelay
adoptCurrentIdentity()
observeIdentityChanges()
@@ -137,6 +142,7 @@ final class CredentialIdentityScope {
self.defaults = defaults
fingerprintProvider = { nil }
resolveUserRecordID = { nil }
accountAvailabilityProvider = { .indeterminate }
retryDelay = { _ in }
fingerprint = nil
state = fixedScope.map { State.resolved($0) } ?? .unresolved
@@ -164,9 +170,13 @@ final class CredentialIdentityScope {
}
guard let current else {
// Signed out. Local scope is correct *and* isolating: items written
// here never collide with a signed-in account's keys.
state = .resolved(Self.localScope)
// A nil token does NOT establish sign-out — Foundation also returns
// nil when iCloud Drive is unavailable or switched off, and Yattee
// declares only the CloudKit service. Scoping to `local` here would
// collapse two different signed-in Apple accounts into one
// namespace, which is exactly the collision this class prevents.
// CloudKit's account status settles it during resolution.
state = .unresolved
return
}
@@ -212,9 +222,35 @@ final class CredentialIdentityScope {
// for B.
let requested = fingerprint
while resolutionAttempts < Self.maxResolutionAttempts {
// Labelled: an unqualified `break` inside the switch below would leave
// only the switch and fall through to the user-record lookup, which
// must not run when the account status is undetermined.
attempts: while resolutionAttempts < Self.maxResolutionAttempts {
resolutionAttempts += 1
// With no ubiquity token, only CloudKit can distinguish "signed
// out" (where `local` is correct) from "signed in but iCloud Drive
// is off" (where `local` would be a cross-account collision).
if requested == nil {
switch await accountAvailabilityProvider() {
case .noAccount:
guard fingerprintProvider() == nil, fingerprint == nil else { return }
state = .resolved(Self.localScope)
LoggingService.shared.info(
"No Apple account — credentials scoped locally",
category: .keychain
)
return
case .available:
break // fall through to the user-record lookup below
case .indeterminate:
guard resolutionAttempts < Self.maxResolutionAttempts else { break attempts }
await retryDelay(resolutionAttempts)
guard fingerprint == requested else { return }
continue attempts
}
}
if let recordID = await resolveUserRecordID() {
guard fingerprintProvider() == requested, fingerprint == requested else {
LoggingService.shared.info(
@@ -224,6 +260,9 @@ final class CredentialIdentityScope {
return
}
// Only cacheable against a token. Without one there is no key
// to cache under that could be trusted to mean this account,
// so this lookup repeats each launch.
if let requested {
cache(recordID, for: requested)
}
@@ -232,12 +271,12 @@ final class CredentialIdentityScope {
LoggingService.shared.info(
"Resolved credential identity scope",
category: .keychain,
details: "fingerprint=\(requested ?? "none"), attempt=\(resolutionAttempts)"
details: "fingerprint=\(requested ?? "none (no ubiquity token)"), attempt=\(resolutionAttempts)"
)
return
}
guard resolutionAttempts < Self.maxResolutionAttempts else { break }
guard resolutionAttempts < Self.maxResolutionAttempts else { break attempts }
await retryDelay(resolutionAttempts)
guard fingerprint == requested else { return }

View File

@@ -321,6 +321,42 @@ struct LegacyCredentialAdoptionTests {
.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID))
}
@Test("Adoption in one backend does not suppress the other's offer")
func markersAreNamespacedByService() {
// Both credential managers share one marker store, and an instance UUID
// can outlive a change of backend.
let invidiousService = makeService()
let pipedService = makeService()
let markers = makeMarkers()
let instanceID = UUID()
let account = Account.migrated(from: instanceID)
seedLegacyItem(service: invidiousService, instanceID: instanceID, value: "invidious-sid")
seedLegacyItem(service: pipedService, instanceID: instanceID, value: "piped-token")
let invidious = makeCredentials(service: invidiousService, scope: "identity-A", markers: markers)
let piped = makeCredentials(service: pipedService, scope: "identity-A", markers: markers)
#expect(invidious.adoptLegacyCredential(for: account) == true)
// Piped's own legacy item is untouched and still offered.
#expect(piped.hasAdoptableLegacyCredential(for: account) == true)
#expect(piped.adoptLegacyCredential(for: account) == true)
#expect(piped.credential(for: account) == "piped-token")
#expect(invidious.credential(for: account) == "invidious-sid")
// Each deletion consults its own marker and its own secret.
invidious.deleteLegacyCredentialIfOwned(for: account)
#expect(legacyValue(service: invidiousService, instanceID: instanceID) == nil)
#expect(legacyValue(service: pipedService, instanceID: instanceID) == "piped-token")
piped.deleteLegacyCredentialIfOwned(for: account)
#expect(legacyValue(service: pipedService, instanceID: instanceID) == nil)
invidious.deleteCredential(for: account)
piped.deleteCredential(for: account)
}
@Test("Adoption is refused while the identity scope is unresolved")
func adoptionRefusedWhenUnresolved() {
let service = makeService()
@@ -355,6 +391,7 @@ struct CredentialIdentityScopeTests {
var fingerprint: String? = "identity-A"
var resultsByFingerprint: [String: String] = [:]
var failuresRemaining = 0
var availability: CloudKitIdentity.AccountAvailability = .available
var onResolveStart: (() -> Void)?
var switchTask: Task<Void, Never>?
@@ -379,6 +416,7 @@ struct CredentialIdentityScopeTests {
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
@@ -397,6 +435,7 @@ struct CredentialIdentityScopeTests {
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
@@ -418,6 +457,7 @@ struct CredentialIdentityScopeTests {
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
@@ -440,21 +480,69 @@ struct CredentialIdentityScopeTests {
#expect(scope.current == "_recordB")
}
@Test("Signing out scopes to local without a lookup")
func signedOutUsesLocalScope() {
@Test("A missing ubiquity token alone never scopes to local")
func missingTokenDoesNotImplySignedOut() async {
// iCloud Drive off while still signed in: Foundation returns no token,
// but the account is real. Scoping to local here would put two
// different Apple accounts in one namespace.
let harness = Harness()
harness.fingerprint = nil
harness.availability = .available
harness.resultsByFingerprint = [:]
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { "_recordA" },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
#expect(scope.current == nil)
await scope.resolve()
#expect(scope.current == "_recordA")
#expect(scope.current != CredentialIdentityScope.localScope)
}
@Test("Signing out scopes to local, confirmed by CloudKit")
func signedOutUsesLocalScope() async {
let harness = Harness()
harness.fingerprint = nil
harness.availability = .noAccount
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
// Not assumed up front — only CloudKit can tell "signed out" from
// "token unavailable".
#expect(scope.current == nil)
await scope.resolve()
#expect(scope.current == CredentialIdentityScope.localScope)
}
@Test("An undeterminable account status leaves the scope unresolved")
func indeterminateStatusStaysUnresolved() async {
let harness = Harness()
harness.fingerprint = nil
harness.availability = .indeterminate
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { "_shouldNotBeUsed" },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
await scope.resolve()
#expect(scope.current == nil)
}
@Test("A known Apple account resolves synchronously on relaunch")
func cachedScopeResolvesSynchronously() async {
let defaults = makeDefaults()
@@ -465,6 +553,7 @@ struct CredentialIdentityScopeTests {
defaults: defaults,
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
await first.resolve()
@@ -475,6 +564,7 @@ struct CredentialIdentityScopeTests {
defaults: defaults,
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { nil },
accountAvailabilityProvider: { harness.availability },
retryDelay: { _ in }
)
#expect(second.current == "_recordA")