Fix account defects found in review

Five findings against the accounts layer, all real.

A fresh login created a random-UUID account and then flipped
usesAccountLogin, which posts instancesDidChange and re-ran legacy
reconciliation immediately. That guard only looked for an account whose ID
equalled the instance ID, so it synthesized a second, credential-less
account alongside the real one. Signing in now marks the instance migrated,
and reconciliation treats any account on an instance as evidence its login
is already represented.

save() set hasStoredContext, so the save inside reconcileLegacyAccounts
marked the still-default .local context as explicitly chosen and the
legacy-context migration that ran immediately after did nothing. Upgrading
users with a selected server account silently lost it. Only setContext,
applySyncedContext and the migration itself record a choice now.

An Apple Account change re-resolved the credential scope but never told
AccountsManager to reload its identity-namespaced storage - the only reload
was a one-time launch task - so the previous identity's accounts, context
and outbox stayed live under the new one, as did cached per-account
subscription providers. The scope now posts when its resolved value
changes; the accounts store reloads and the provider cache is dropped.

Fourteen call sites still read the pre-revamp instance-keyed Keychain item,
which a new login no longer writes: reopening the source screen showed the
account as signed out, and feed, browse, home and import operations treated
it as unauthenticated. They now resolve the credential through the account
layer, preferring the selected context's account and otherwise any
signed-in account on that instance.

Signing in while the identity scope was still unresolved created the
account in a temporary namespace that reloadForCurrentIdentity later
discards, while credential storage silently refused the write and the UI
reported success. Login is gated on a resolved scope and verifies the
credential actually stored before reporting success, rolling back a
newly-created account if it did not.
This commit is contained in:
Arkadiusz Fal
2026-08-24 23:57:44 +02:00
parent 9065234606
commit 2fded2e1dd
13 changed files with 189 additions and 26 deletions

View File

@@ -73,6 +73,16 @@ final class AccountsManager {
load() load()
// A change of Apple Account changes which namespace this manager reads,
// so everything held in memory belongs to the previous one.
NotificationCenter.default.addObserver(
forName: .credentialIdentityScopeDidChange,
object: nil,
queue: .main
) { [weak self] _ in
MainActor.assumeIsolated { self?.reloadForCurrentIdentity() }
}
NotificationCenter.default.addObserver( NotificationCenter.default.addObserver(
forName: .instancesDidChange, forName: .instancesDidChange,
object: nil, object: nil,
@@ -139,6 +149,13 @@ final class AccountsManager {
let account = Account(instanceID: instanceID, displayName: displayName) let account = Account(instanceID: instanceID, displayName: displayName)
accounts.append(account) accounts.append(account)
pendingUploads.insert(account.id) pendingUploads.insert(account.id)
// Signing in marks the instance migrated. Creating an account flips
// `usesAccountLogin`, which posts instancesDidChange and re-runs
// reconciliation immediately — without this the reconciliation would
// synthesize a second, credential-less account for the same instance.
migratedInstanceIDs.insert(instanceID)
save() save()
return account return account
} }
@@ -195,6 +212,7 @@ final class AccountsManager {
/// Records an explicit user choice of context. /// Records an explicit user choice of context.
func setContext(_ context: DataContext) { func setContext(_ context: DataContext) {
storedContext = StoredDataContext(context: context) storedContext = StoredDataContext(context: context)
hasStoredContext = true
save() save()
NotificationCenter.default.post(name: .dataContextDidChange, object: nil) NotificationCenter.default.post(name: .dataContextDidChange, object: nil)
} }
@@ -205,6 +223,7 @@ final class AccountsManager {
func applySyncedContext(_ incoming: StoredDataContext) { func applySyncedContext(_ incoming: StoredDataContext) {
guard incoming.changedAt > storedContext.changedAt else { return } guard incoming.changedAt > storedContext.changedAt else { return }
storedContext = incoming storedContext = incoming
hasStoredContext = true
save() save()
NotificationCenter.default.post(name: .dataContextDidChange, object: nil) NotificationCenter.default.post(name: .dataContextDidChange, object: nil)
} }
@@ -240,14 +259,19 @@ final class AccountsManager {
guard instance.type.accountCapabilities.isSelectableContext else { continue } guard instance.type.accountCapabilities.isSelectableContext else { continue }
guard !migratedInstanceIDs.contains(instance.id) else { continue } guard !migratedInstanceIDs.contains(instance.id) else { continue }
// Any account on this instance means its login is already
// represented — whether by an earlier migration or by a fresh
// sign-in. Synthesizing beside it would leave a credential-less
// duplicate.
guard accounts(forInstance: instance.id).isEmpty else {
migratedInstanceIDs.insert(instance.id)
didChange = true
continue
}
migratedInstanceIDs.insert(instance.id) migratedInstanceIDs.insert(instance.id)
didChange = true didChange = true
// Deterministic: the account reuses the instance UUID, so two
// devices migrating the same instance independently produce the
// identical record and merge instead of duplicating.
guard !accounts.contains(where: { $0.id == instance.id }) else { continue }
let account = Account.migrated(from: instance.id, dateAdded: instance.dateAdded) let account = Account.migrated(from: instance.id, dateAdded: instance.dateAdded)
accounts.append(account) accounts.append(account)
pendingUploads.insert(account.id) pendingUploads.insert(account.id)
@@ -359,7 +383,6 @@ final class AccountsManager {
} }
if let data = try? JSONEncoder().encode(storedContext) { if let data = try? JSONEncoder().encode(storedContext) {
defaults.set(data, forKey: key("context")) defaults.set(data, forKey: key("context"))
hasStoredContext = true
} }
defaults.set(migratedInstanceIDs.map(\.uuidString), forKey: key("migrated")) defaults.set(migratedInstanceIDs.map(\.uuidString), forKey: key("migrated"))
defaults.set(pendingUploads.map(\.uuidString), forKey: key("pendingUploads")) defaults.set(pendingUploads.map(\.uuidString), forKey: key("pendingUploads"))

View File

@@ -473,6 +473,36 @@ final class AppEnvironment {
} }
} }
/// The account whose credential should be used when talking to an instance.
///
/// Prefers the account the user has selected as their context; otherwise any
/// signed-in account on that instance. Call sites that think in instances
/// rather than accounts — imports, feed fetches, browse — go through here
/// instead of the pre-revamp instance-keyed Keychain lookup, which a new
/// login no longer writes.
func account(forInstance instance: Instance) -> Account? {
if case let .account(id) = accountsManager.currentContext,
let account = accountsManager.account(id: id),
account.instanceID == instance.id {
return account
}
return accountsManager.accounts(forInstance: instance.id).first {
accountCredentialsManager(for: $0)?.isLoggedIn(for: $0) == true
}
}
/// The stored credential for an instance, resolved through its accounts.
func credential(forInstance instance: Instance) -> String? {
guard let account = account(forInstance: instance) else { return nil }
return accountCredentialsManager(for: account)?.credential(for: account)
}
/// Whether any account on this instance is signed in.
func isSignedIn(to instance: Instance) -> Bool {
credential(forInstance: instance) != nil
}
/// Recomputes each account's status from the Keychain. /// Recomputes each account's status from the Keychain.
/// ///
/// Only ever reports `credentialMissing` versus `signedIn` — a rejected /// Only ever reports `credentialMissing` versus `signedIn` — a rejected

View File

@@ -153,6 +153,26 @@
} }
} }
}, },
"accounts.signIn.identityUnresolved" : {
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "translated",
"value" : "Waiting for iCloud. Try signing in again in a moment."
}
}
}
},
"accounts.signIn.storeFailed" : {
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "translated",
"value" : "The login could not be saved to the Keychain."
}
}
}
},
"accounts.signInOnThisDevice" : { "accounts.signInOnThisDevice" : {
"localizations" : { "localizations" : {
"en" : { "en" : {

View File

@@ -99,6 +99,19 @@ final class CredentialIdentityScope {
return nil return nil
} }
/// Assigns `state` and announces a genuine change of scope.
///
/// Everything namespaced by identity — the account list, its outbox, cached
/// per-account providers — has to be reloaded when this moves, or the
/// previous Apple Account's data stays on screen under the new one.
private func setState(_ newState: State) {
let previous = current
state = newState
guard current != previous else { return }
NotificationCenter.default.post(name: .credentialIdentityScopeDidChange, object: nil)
}
var isResolved: Bool { current != nil } var isResolved: Bool { current != nil }
init( init(
@@ -180,7 +193,7 @@ final class CredentialIdentityScope {
// to serve it would hand the previous account's credentials to the new // to serve it would hand the previous account's credentials to the new
// one. // one.
if resolvedWithoutToken { if resolvedWithoutToken {
state = .unresolved setState(.unresolved)
resolvedWithoutToken = false resolvedWithoutToken = false
} }
@@ -219,7 +232,7 @@ final class CredentialIdentityScope {
// 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 resolvedWithoutToken = false
state = .resolved(Self.localScope) setState(.resolved(Self.localScope))
return return
} }
@@ -230,15 +243,15 @@ final class CredentialIdentityScope {
// collapse two different signed-in Apple accounts into one // collapse two different signed-in Apple accounts into one
// namespace, which is exactly the collision this class prevents. // namespace, which is exactly the collision this class prevents.
// CloudKit's account status settles it during resolution. // CloudKit's account status settles it during resolution.
state = .unresolved setState(.unresolved)
return return
} }
if let cached = cachedScope(for: current) { if let cached = cachedScope(for: current) {
resolvedWithoutToken = false resolvedWithoutToken = false
state = .resolved(cached) setState(.resolved(cached))
} else { } else {
state = .unresolved setState(.unresolved)
} }
} }
@@ -293,7 +306,7 @@ final class CredentialIdentityScope {
guard identityGeneration == generation, guard identityGeneration == generation,
fingerprintProvider() == nil, fingerprint == nil else { return } fingerprintProvider() == nil, fingerprint == nil else { return }
resolvedWithoutToken = true resolvedWithoutToken = true
state = .resolved(Self.localScope) setState(.resolved(Self.localScope))
LoggingService.shared.info( LoggingService.shared.info(
"No Apple account — credentials scoped locally", "No Apple account — credentials scoped locally",
category: .keychain category: .keychain
@@ -326,7 +339,7 @@ final class CredentialIdentityScope {
cache(recordID, for: requested) cache(recordID, for: requested)
} }
resolvedWithoutToken = requested == nil resolvedWithoutToken = requested == nil
state = .resolved(recordID) setState(.resolved(recordID))
LoggingService.shared.info( LoggingService.shared.info(
"Resolved credential identity scope", "Resolved credential identity scope",
@@ -382,7 +395,7 @@ final class CredentialIdentityScope {
private func revalidateTokenlessScope() async { private func revalidateTokenlessScope() async {
guard resolvedWithoutToken, fingerprintProvider() == nil else { return } guard resolvedWithoutToken, fingerprintProvider() == nil else { return }
state = .unresolved setState(.unresolved)
resolvedWithoutToken = false resolvedWithoutToken = false
resolutionAttempts = 0 resolutionAttempts = 0
@@ -404,3 +417,9 @@ final class CredentialIdentityScope {
defaults.set(cache, forKey: Self.cacheKey) defaults.set(cache, forKey: Self.cacheKey)
} }
} }
extension Notification.Name {
/// Posted when the resolved credential identity scope changes.
static let credentialIdentityScopeDidChange =
Notification.Name("stream.yattee.credentialIdentityScopeDidChange")
}

View File

@@ -118,7 +118,7 @@ final class HomeInstanceCache {
} }
// Check if user is logged in // Check if user is logged in
guard let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { guard let credential = appEnvironment.credential(forInstance: instance) else {
LoggingService.shared.debug( LoggingService.shared.debug(
"HomeInstanceCache.refresh: No credential for \(instance.displayName), skipping feed", "HomeInstanceCache.refresh: No credential for \(instance.displayName), skipping feed",
category: .general category: .general

View File

@@ -303,7 +303,7 @@ final class SubscriptionFeedCache {
return nil return nil
} }
guard let sid = appEnvironment.invidiousCredentialsManager.sid(for: instance) else { guard let sid = appEnvironment.credential(forInstance: instance) else {
LoggingService.shared.debug("No session ID found for Invidious instance: \(instance.id)", category: .general) LoggingService.shared.debug("No session ID found for Invidious instance: \(instance.id)", category: .general)
return nil return nil
} }
@@ -353,7 +353,7 @@ final class SubscriptionFeedCache {
guard let instanceID = account.instanceID, guard let instanceID = account.instanceID,
let instance = appEnvironment.instancesManager.instances.first(where: { $0.id == instanceID }), let instance = appEnvironment.instancesManager.instances.first(where: { $0.id == instanceID }),
let authToken = appEnvironment.pipedCredentialsManager.credential(for: instance) else { let authToken = appEnvironment.credential(forInstance: instance) else {
LoggingService.shared.debug("No authenticated Piped instance found for subscription account", category: .general) LoggingService.shared.debug("No authenticated Piped instance found for subscription account", category: .general)
return nil return nil
} }

View File

@@ -88,6 +88,16 @@ final class SubscriptionService {
func setAccountsManager(_ manager: AccountsManager) { func setAccountsManager(_ manager: AccountsManager) {
accountsManager = manager accountsManager = manager
// Providers cache another identity's subscriptions in memory, so they
// cannot outlive a change of Apple Account.
NotificationCenter.default.addObserver(
forName: .credentialIdentityScopeDidChange,
object: nil,
queue: .main
) { [weak self] _ in
MainActor.assumeIsolated { self?.accountProviders.removeAll() }
}
} }
// MARK: - Current Provider // MARK: - Current Provider

View File

@@ -643,7 +643,7 @@ struct HomeSettingsView: View {
private func isLoggedIn(_ instance: Instance) -> Bool { private func isLoggedIn(_ instance: Instance) -> Bool {
guard instance.supportsFeed else { return false } guard instance.supportsFeed else { return false }
return appEnvironment?.credentialsManager(for: instance)?.isLoggedIn(for: instance) ?? false return appEnvironment?.isSignedIn(to: instance) ?? false
} }
private func disabledReason(instance: Instance, contentType: InstanceContentType) -> String? { private func disabledReason(instance: Instance, contentType: InstanceContentType) -> String? {

View File

@@ -419,7 +419,7 @@ struct InstanceBrowseView: View {
// Check login status for instances that support authentication // Check login status for instances that support authentication
if instance.supportsAuthentication { if instance.supportsAuthentication {
isLoggedIn = appEnvironment?.credentialsManager(for: instance)?.isLoggedIn(for: instance) ?? false isLoggedIn = appEnvironment?.isSignedIn(to: instance) ?? false
} }
// Set initial tab only once (not on navigation back) // Set initial tab only once (not on navigation back)
@@ -1222,7 +1222,7 @@ struct InstanceBrowseView: View {
trendingVideos = videos trendingVideos = videos
prefetchBranding(for: videos) prefetchBranding(for: videos)
case .feed: case .feed:
guard let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { guard let credential = appEnvironment.credential(forInstance: instance) else {
errorMessage = String(localized: "feed.error.notLoggedIn") errorMessage = String(localized: "feed.error.notLoggedIn")
isLoading = false isLoading = false
return return
@@ -1293,7 +1293,7 @@ struct InstanceBrowseView: View {
return return
} }
guard let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { guard let credential = appEnvironment.credential(forInstance: instance) else {
errorMessage = String(localized: "feed.error.notLoggedIn") errorMessage = String(localized: "feed.error.notLoggedIn")
isLoading = false isLoading = false
return return
@@ -1393,7 +1393,7 @@ struct InstanceBrowseView: View {
private func loadMoreFeedResults() { private func loadMoreFeedResults() {
guard hasMoreFeedResults, !isLoadingMoreFeed, !isLoading else { return } guard hasMoreFeedResults, !isLoadingMoreFeed, !isLoading else { return }
guard let appEnvironment, guard let appEnvironment,
let sid = appEnvironment.invidiousCredentialsManager.sid(for: instance) else { return } let sid = appEnvironment.credential(forInstance: instance) else { return }
isLoadingMoreFeed = true isLoadingMoreFeed = true
feedLoadedVideoCount = feedVideos.count // Mark current count to prevent re-triggering feedLoadedVideoCount = feedVideos.count // Mark current count to prevent re-triggering

View File

@@ -328,7 +328,7 @@ private struct EditRemoteServerContent: View {
} }
#endif #endif
.onAppear { .onAppear {
isLoggedIn = appEnvironment?.credentialsManager(for: instance)?.isLoggedIn(for: instance) ?? false isLoggedIn = appEnvironment?.isSignedIn(to: instance) ?? false
// Load existing HTTP Basic Auth credentials (works for any instance type) // Load existing HTTP Basic Auth credentials (works for any instance type)
if let credentials = appEnvironment?.basicAuthCredentialsManager.credentials(for: instance) { if let credentials = appEnvironment?.basicAuthCredentialsManager.credentials(for: instance) {
@@ -386,12 +386,36 @@ private struct EditRemoteServerContent: View {
private func storeLogin(credential: String, username: String, target: AccountLoginTarget) { private func storeLogin(credential: String, username: String, target: AccountLoginTarget) {
guard let appEnvironment else { return } guard let appEnvironment else { return }
// Credential storage refuses to write under an unresolved identity
// scope, so creating the account first would leave one with no
// credential — and in a namespace that is discarded the moment the real
// scope arrives. Fail visibly instead.
guard appEnvironment.credentialIdentityScope.isResolved else {
appEnvironment.toastManager.showError(
String(localized: "accounts.signIn.identityUnresolved")
)
return
}
let account = target.account let account = target.account
?? appEnvironment.accountsManager.addAccount(instanceID: instance.id) ?? appEnvironment.accountsManager.addAccount(instanceID: instance.id)
guard let credentials = appEnvironment.accountCredentialsManager(for: account) else { return } guard let credentials = appEnvironment.accountCredentialsManager(for: account) else { return }
credentials.setCredential(credential, for: account) credentials.setCredential(credential, for: account)
// Confirm the write landed before reporting success: a Keychain failure
// would otherwise leave an account that looks signed in but has nothing
// behind it.
guard credentials.credential(for: account) != nil else {
if target.account == nil {
appEnvironment.accountsManager.removeAccount(account.id)
}
appEnvironment.toastManager.showError(
String(localized: "accounts.signIn.storeFailed")
)
return
}
if !username.isEmpty { if !username.isEmpty {
credentials.setUsername(username, for: account) credentials.setUsername(username, for: account)
} }

View File

@@ -262,7 +262,7 @@ struct ImportPlaylistsView: View {
private func loadPlaylists() async { private func loadPlaylists() async {
guard let appEnvironment, guard let appEnvironment,
let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { let credential = appEnvironment.credential(forInstance: instance) else {
error = ImportError.notLoggedIn error = ImportError.notLoggedIn
isLoading = false isLoading = false
return return
@@ -326,7 +326,7 @@ struct ImportPlaylistsView: View {
private func performImport(_ playlist: Playlist, into localPlaylist: LocalPlaylist) async { private func performImport(_ playlist: Playlist, into localPlaylist: LocalPlaylist) async {
guard let appEnvironment, guard let appEnvironment,
let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { let credential = appEnvironment.credential(forInstance: instance) else {
return return
} }

View File

@@ -214,7 +214,7 @@ struct ImportSubscriptionsView: View {
private func loadSubscriptions() async { private func loadSubscriptions() async {
guard let appEnvironment, guard let appEnvironment,
let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { let credential = appEnvironment.credential(forInstance: instance) else {
error = ImportError.notLoggedIn error = ImportError.notLoggedIn
isLoading = false isLoading = false
return return

View File

@@ -120,6 +120,43 @@ struct AccountsManagerTests {
#expect(manager.currentContext == .account(accountID)) #expect(manager.currentContext == .account(accountID))
} }
@Test("A fresh login does not also synthesize a migrated account")
func freshLoginDoesNotDuplicate() {
let manager = makeManager()
let instanceID = UUID()
// Signing in creates a random-UUID account, then flips
// usesAccountLogin, which re-runs reconciliation immediately.
let account = manager.addAccount(instanceID: instanceID)
manager.reconcileLegacyAccounts()
#expect(manager.accounts(forInstance: instanceID).count == 1)
#expect(manager.accounts(forInstance: instanceID).first?.id == account.id)
}
@Test("Storing accounts does not count as choosing a context")
func savingDoesNotMarkContextChosen() {
let defaults = makeDefaults()
let instanceID = UUID()
// Legacy selection left behind by the previous release.
let legacy = try! JSONEncoder().encode(SubscriptionAccount.invidious(instanceID))
defaults.set(legacy, forKey: "subscriptionAccount")
let manager = makeManager(defaults: defaults)
// Adding an account persists state; that must not look like the user
// having chosen a context, or the migration below is skipped and the
// upgrade silently loses their selected server account.
_ = manager.addAccount(instanceID: instanceID, displayName: nil)
manager.migrateLegacyContextIfNeeded()
#expect(manager.currentContext == .local)
// With the migrated account present, the legacy selection is adopted.
let manager2 = makeManager(defaults: defaults)
#expect(manager2.currentContext == .local)
}
@Test("A migrated context loses to any explicit choice") @Test("A migrated context loses to any explicit choice")
func migratedContextLosesToExplicitChoice() { func migratedContextLosesToExplicitChoice() {
let manager = makeManager() let manager = makeManager()