Mirrored_Repos/yattee
1
0
Fork 0
mirror of https://github.com/yattee/yattee.git synced 2026-10-24 13:42:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

Stop adoption rollback from revoking a synced credential

Browse Source 
Two more issues from a second review pass.

Adoption verified its write by reading the key back and deleted it on any
mismatch. But a mismatch can also mean iCloud Keychain delivered a real
credential for that key between the add and the read - and since the delete
spans both synchronizable forms, it would propagate and revoke that
credential on every device. A readable value that is not ours is now left
strictly alone; only an unreadable key (the add reported success but
produced nothing usable) is rolled back so the offer can be retried.

An identity switch arriving while a resolution was in flight was dropped:
the observer's resolve() returned early on the isResolving guard, and the
in-flight one then bailed on its fingerprint check without starting a
lookup for the new identity, leaving credentials disabled until a
foreground transition or relaunch. Requests that arrive mid-resolution are
now replayed once the running one finishes.

Adds a fingerprint provider and an injectable backoff so identity
resolution is testable without a real Apple account or real seconds, plus
tests for retry, foreground recovery, mid-flight switching, signed-out
scoping and the cached-scope fast path.
This commit is contained in:
Arkadiusz Fal
2026-08-23 23:56:28 +02:00
parent c4053d5100
commit f5e88df403
3 changed files with 195 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
Yattee/Services/Credentials/AccountKeychainCredentials.swift
View File

@@ -219,13 +219,31 @@ final class AccountKeychainCredentials {
// The marker is only written after the scoped copy reads back: a marker
// for a credential that never landed would suppress the adoption offer
// and strand the user with no way to sign in short of retyping.
guard store.value(forKey: key) == legacy else {
let readback = store.value(forKey: key)
guard readback == legacy else {
// A *different* readable value means iCloud Keychain delivered a
// credential for this key between the add and the read. It is newer
// than the legacy secret and belongs to a live login, so it is left
// strictly alone — deleting here would propagate through iCloud
// Keychain and revoke it on every device.
if readback != nil {
LoggingService.shared.info(
"Legacy \(credentialNoun) adoption superseded by a credential that arrived mid-write",
category: .keychain,
details: "accountID=\(account.id)"
)
return false
}
// Nothing readable is there: the add reported success but produced
// no usable item, so rolling it back destroys nothing and lets the
// adoption offer be retried.
LoggingService.shared.error(
"Legacy \(credentialNoun) adoption failed verification",
category: .keychain,
details: "accountID=\(account.id)"
)
// Safe to remove: this write is the item that just landed.
store.delete(key: key)
return false
}

42
Yattee/Services/Credentials/CredentialIdentityScope.swift
View File

@@ -58,6 +58,9 @@ final class CredentialIdentityScope {
private let defaults: UserDefaults
private let resolveUserRecordID: @Sendable () async -> String?
private let fingerprintProvider: @Sendable () -> String?
/// 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
/// last reference is gone and so cannot race with it.
private nonisolated(unsafe) var identityChangeObserver: NSObjectProtocol?
@@ -68,6 +71,10 @@ final class CredentialIdentityScope {
/// whenever the identity changes.
private var resolutionAttempts = 0
private var isResolving = false
/// Set when a resolution is requested while one is already running — the
/// 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.
private var pendingResolve = false
private static let maxResolutionAttempts = 4
/// The resolved scope, or `nil` while unresolved.
@@ -80,10 +87,16 @@ final class CredentialIdentityScope {
init(
defaults: UserDefaults = .standard,
resolveUserRecordID: @escaping @Sendable () async -> String? = { await CloudKitIdentity.userRecordID() }
fingerprintProvider: @escaping @Sendable () -> String? = { CloudKitIdentity.ubiquityFingerprint },
resolveUserRecordID: @escaping @Sendable () async -> String? = { await CloudKitIdentity.userRecordID() },
retryDelay: @escaping @Sendable (Int) async -> Void = { attempt in
try? await Task.sleep(for: .seconds(pow(2.0, Double(attempt))))
}
) {
self.defaults = defaults
self.fingerprintProvider = fingerprintProvider
self.resolveUserRecordID = resolveUserRecordID
self.retryDelay = retryDelay
adoptCurrentIdentity()
observeIdentityChanges()
}
@@ -122,7 +135,9 @@ final class CredentialIdentityScope {
/// `nil` pins the unresolved state.
init(fixedScope: String?, defaults: UserDefaults = .standard) {
self.defaults = defaults
fingerprintProvider = { nil }
resolveUserRecordID = { nil }
retryDelay = { _ in }
fingerprint = nil
state = fixedScope.map { State.resolved($0) } ?? .unresolved
}
@@ -135,7 +150,7 @@ final class CredentialIdentityScope {
/// Call again on `.accountChange`: a switch that happened while the app was
/// closed shows up here, before any CloudKit request could report it.
func adoptCurrentIdentity() {
let current = CloudKitIdentity.ubiquityFingerprint
let current = fingerprintProvider()
if current != fingerprint {
resolutionAttempts = 0
}
@@ -170,11 +185,28 @@ final class CredentialIdentityScope {
/// rather than "not signed in" — callers must not treat that as a missing
/// credential.
func resolve() async {
guard !isResolved, !isResolving else { return }
guard !isResolved else { return }
// A resolution already running is for whatever identity was current
// when it started. If that is no longer this one it will bail on its
// fingerprint check, so the request has to be replayed afterwards
// rather than dropped — otherwise a switch mid-flight leaves the new
// identity unresolved until a foreground transition or relaunch.
guard !isResolving else {
pendingResolve = true
return
}
isResolving = true
defer { isResolving = false }
repeat {
pendingResolve = false
await performResolution()
} while pendingResolve && !isResolved
}
private func performResolution() async {
// Captured before the first await so a switch mid-flight can be
// detected: a record ID fetched under account A must never be cached
// for B.
@@ -184,7 +216,7 @@ final class CredentialIdentityScope {
resolutionAttempts += 1
if let recordID = await resolveUserRecordID() {
guard CloudKitIdentity.ubiquityFingerprint == requested, fingerprint == requested else {
guard fingerprintProvider() == requested, fingerprint == requested else {
LoggingService.shared.info(
"Discarding credential scope resolved for a previous Apple account",
category: .keychain
@@ -207,7 +239,7 @@ final class CredentialIdentityScope {
guard resolutionAttempts < Self.maxResolutionAttempts else { break }
try? await Task.sleep(for: .seconds(pow(2.0, Double(resolutionAttempts))))
await retryDelay(resolutionAttempts)
guard fingerprint == requested else { return }
}

138
YatteeTests/AccountCredentialsTests.swift
View File

@@ -343,6 +343,144 @@ struct LegacyCredentialAdoptionTests {
}
}
@Suite("Credential identity scope resolution")
@MainActor
struct CredentialIdentityScopeTests {
private func makeDefaults() -> UserDefaults {
UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")!
}
/// Mutable identity + resolver so a switch can be staged mid-resolution.
private final class Harness: @unchecked Sendable {
var fingerprint: String? = "identity-A"
var resultsByFingerprint: [String: String] = [:]
var failuresRemaining = 0
var onResolveStart: (() -> Void)?
var switchTask: Task<Void, Never>?
func resolve() async -> String? {
onResolveStart?()
if failuresRemaining > 0 {
failuresRemaining -= 1
return nil
}
guard let fingerprint else { return nil }
return resultsByFingerprint[fingerprint]
}
}
@Test("A transient failure is retried rather than disabling credentials forever")
func transientFailureIsRetried() async {
let harness = Harness()
harness.resultsByFingerprint = ["identity-A": "_recordA"]
harness.failuresRemaining = 2
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
retryDelay: { _ in }
)
#expect(scope.isResolved == false)
await scope.resolve()
#expect(scope.current == "_recordA")
}
@Test("Exhausted attempts are retried on foreground activation")
func exhaustedAttemptsRetryOnForeground() async {
let harness = Harness()
harness.resultsByFingerprint = ["identity-A": "_recordA"]
harness.failuresRemaining = 99 // every attempt fails
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
retryDelay: { _ in }
)
await scope.resolve()
#expect(scope.isResolved == false)
// Connectivity returns; the app comes back to the foreground.
harness.failuresRemaining = 0
await scope.resolveIfNeeded()
#expect(scope.current == "_recordA")
}
@Test("An identity switch mid-resolution resolves the new identity")
func switchDuringResolutionResolvesNewIdentity() async {
let harness = Harness()
harness.resultsByFingerprint = ["identity-A": "_recordA", "identity-B": "_recordB"]
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
retryDelay: { _ in }
)
// The Apple account switches while A's lookup is in flight — exactly
// what the notification observer does, only deterministically.
harness.onResolveStart = { [weak scope] in
harness.onResolveStart = nil
harness.fingerprint = "identity-B"
MainActor.assumeIsolated {
scope?.adoptCurrentIdentity()
harness.switchTask = Task { await scope?.resolve() }
}
}
await scope.resolve()
await harness.switchTask?.value
// A's result must be discarded, and B must not be left unresolved.
#expect(scope.current != "_recordA")
#expect(scope.current == "_recordB")
}
@Test("Signing out scopes to local without a lookup")
func signedOutUsesLocalScope() {
let harness = Harness()
harness.fingerprint = nil
let scope = CredentialIdentityScope(
defaults: makeDefaults(),
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
retryDelay: { _ in }
)
#expect(scope.current == CredentialIdentityScope.localScope)
}
@Test("A known Apple account resolves synchronously on relaunch")
func cachedScopeResolvesSynchronously() async {
let defaults = makeDefaults()
let harness = Harness()
harness.resultsByFingerprint = ["identity-A": "_recordA"]
let first = CredentialIdentityScope(
defaults: defaults,
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { await harness.resolve() },
retryDelay: { _ in }
)
await first.resolve()
#expect(first.current == "_recordA")
// Relaunch: no network involved this time.
let second = CredentialIdentityScope(
defaults: defaults,
fingerprintProvider: { harness.fingerprint },
resolveUserRecordID: { nil },
retryDelay: { _ in }
)
#expect(second.current == "_recordA")
}
}
@Suite("Migrated account identity")
@MainActor
struct MigratedAccountTests {