diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 10ed8555..74fbefdd 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -9,6 +9,12 @@ import Foundation /// Why an account cannot currently be used. enum AccountStatus: Hashable, Sendable { + /// Nothing is known yet — the Apple account this device's credentials are + /// keyed by has not been established, so the Keychain cannot be read under + /// the right scope. Distinct from `credentialMissing`, which is an answer: + /// this one is the absence of one, and must never be presented as "signed + /// out" or acted on. + case checking /// Signed in and usable. case signedIn /// No credential on this device. On tvOS this is permanent until the user @@ -499,8 +505,14 @@ final class AccountsManager { NotificationCenter.default.post(name: .accountsDidChange, object: nil) } + /// An account with no recorded status is *unknown*, not signed out. While + /// the identity scope is unresolved nothing can read the Keychain under the + /// right scope, and reporting a missing credential then would tell a user + /// with a perfectly good login to sign in again — and drop reads to local + /// data — for as long as iCloud stayed unreachable. func status(for accountID: UUID) -> AccountStatus { - accountStatuses[accountID] ?? .credentialMissing + if let status = accountStatuses[accountID] { return status } + return identityScope.isResolved ? .credentialMissing : .checking } // MARK: - Legacy migration diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 61845d7f..9aad5949 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -805,14 +805,17 @@ final class AppEnvironment { continue } - // A credential the server has rejected is still a credential: only - // the server can say it works again. Reporting "signed in" here - // because the Keychain item exists would erase the rejection the - // moment it was recorded — the API layer sets it, which posts the - // notification that runs this — and the user would never be asked - // to sign in again. Storing a new credential resets the status - // explicitly. - guard accountsManager.status(for: account.id) != .credentialRejected else { continue } + // A credential the server rejected — or a server that could not be + // reached — is still a credential, and only the server can say + // otherwise. Reporting "signed in" here because the Keychain item + // exists would erase those the moment they were recorded: the API + // layer sets them, and that posts the notification that runs this. + // The next successful call clears them; storing a new credential + // resets the status explicitly. + switch accountsManager.status(for: account.id) { + case .credentialRejected, .unreachable: continue + case .checking, .signedIn, .credentialMissing: break + } accountsManager.setStatus(.signedIn, for: account.id) } diff --git a/Yattee/Localizable.xcstrings b/Yattee/Localizable.xcstrings index 92bac8ec..02c91241 100644 --- a/Yattee/Localizable.xcstrings +++ b/Yattee/Localizable.xcstrings @@ -183,6 +183,16 @@ } } }, + "accounts.status.checking" : { + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "Checking\u2026" + } + } + } + }, "accounts.status.credentialMissing" : { "localizations" : { "en" : { diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index 95248efb..626b30b1 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -178,6 +178,8 @@ final class SubscriptionFeedCache { /// Updates the cache with new videos and persists to disk. func update(videos: [Video]) { + guard isCurrentAccountsResult() else { return } + let oldCount = self.videos.count let oldDate = self.lastUpdated self.videos = videos @@ -193,6 +195,8 @@ final class SubscriptionFeedCache { /// Appends videos to the existing cache (for pagination). private func appendVideos(_ newVideos: [Video]) { + guard isCurrentAccountsResult() else { return } + let existingIDs = Set(videos.map { $0.id }) let uniqueNewVideos = newVideos.filter { !existingIDs.contains($0.id) } videos.append(contentsOf: uniqueNewVideos) @@ -210,10 +214,40 @@ final class SubscriptionFeedCache { /// Call this when the user switches between local and Invidious accounts. func handleAccountChange() { LoggingService.shared.info("Subscription account changed, clearing feed cache", category: .general) + // Anything already in flight was authenticated as the previous account. + // Clearing does not cancel it, and signing in again keeps the same + // account ID and context — so without this fence the old request lands + // afterwards and writes the previous user's private feed straight back + // into the cache, and onto disk. + accountGeneration += 1 clear() feedLoadState = .idle } + /// Which account's data the running request belongs to. + /// + /// Only one refresh runs at a time (every entry point guards on + /// `isLoading`), so a single pair of counters is enough to tell "this result + /// is for the account we are showing" from "this result is a leftover". + private var accountGeneration = 0 + private var requestGeneration = 0 + + /// Marks the start of a request, capturing the account it is for. + private func beginRequest() { + requestGeneration = accountGeneration + } + + private func isCurrentAccountsResult() -> Bool { + guard requestGeneration == accountGeneration else { + LoggingService.shared.info( + "Discarding a feed result that belongs to the previous login", + category: .general + ) + return false + } + return true + } + // MARK: - Refresh /// Refreshes the feed from network. @@ -229,6 +263,8 @@ final class SubscriptionFeedCache { return } + beginRequest() + // The *effective* context, so an account still waiting on its instance // or credential falls back to local rather than reporting // notAuthenticated while the subscriptions list already shows local data. @@ -437,6 +473,7 @@ final class SubscriptionFeedCache { return } + beginRequest() isLoading = true feedLoadState = .loadingMore diff --git a/Yattee/Services/Subscriptions/DataContextValidator.swift b/Yattee/Services/Subscriptions/DataContextValidator.swift index 001f3b37..8529d993 100644 --- a/Yattee/Services/Subscriptions/DataContextValidator.swift +++ b/Yattee/Services/Subscriptions/DataContextValidator.swift @@ -9,6 +9,10 @@ import Foundation /// Why the selected context cannot be used at the moment. enum DataContextUnavailability: Hashable, Sendable { + /// Nothing is known yet: the Apple account the credentials are keyed by is + /// still being established, so the Keychain cannot be read under the right + /// scope. Never render this as "sign in required". + case checking /// The account exists but its instance hasn't arrived from iCloud yet. case instancePending /// The account's instance has been switched off. The selection is kept — @@ -105,6 +109,7 @@ final class DataContextValidator { switch accountsManager.status(for: id) { case .signedIn: return .valid + case .checking: return .temporarilyUnavailable(.checking) case .credentialMissing: return .temporarilyUnavailable(.credentialMissing) case .credentialRejected: return .temporarilyUnavailable(.credentialRejected) case .unreachable: return .temporarilyUnavailable(.unreachable) diff --git a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift index c6d25d0a..a35ac358 100644 --- a/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/InvidiousSubscriptionProvider.swift @@ -38,7 +38,22 @@ final class InvidiousSubscriptionProvider: SubscriptionProvider { return } - guard case APIError.unauthorized = error else { return } + // A server that cannot be reached says nothing about the credential. + // Recorded all the same: the account has to stop reporting itself as + // usable, or reads never fall back and the user is shown failures + // instead of local data. + switch error { + case APIError.timeout, APIError.noConnection: + accountsManager.setStatus(.unreachable, for: account.id) + return + case APIError.cancelled: + return + case APIError.unauthorized: + break + default: + return + } + guard accountsManager.instance(for: account)?.usesBasicAuth == false else { return } accountsManager.setStatus(.credentialRejected, for: account.id) diff --git a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift index 5ac93231..ee46a7c9 100644 --- a/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift +++ b/Yattee/Services/Subscriptions/PipedSubscriptionProvider.swift @@ -38,7 +38,22 @@ final class PipedSubscriptionProvider: SubscriptionProvider { return } - guard case APIError.unauthorized = error else { return } + // A server that cannot be reached says nothing about the credential. + // Recorded all the same: the account has to stop reporting itself as + // usable, or reads never fall back and the user is shown failures + // instead of local data. + switch error { + case APIError.timeout, APIError.noConnection: + accountsManager.setStatus(.unreachable, for: account.id) + return + case APIError.cancelled: + return + case APIError.unauthorized: + break + default: + return + } + guard accountsManager.instance(for: account)?.usesBasicAuth == false else { return } accountsManager.setStatus(.credentialRejected, for: account.id) diff --git a/Yattee/Views/Settings/InstanceAccountsSection.swift b/Yattee/Views/Settings/InstanceAccountsSection.swift index fdb2dc50..0b943225 100644 --- a/Yattee/Views/Settings/InstanceAccountsSection.swift +++ b/Yattee/Views/Settings/InstanceAccountsSection.swift @@ -153,11 +153,12 @@ struct InstanceAccountsSection: View { } private func status(for account: Account) -> AccountStatus { - appEnvironment?.accountsManager.status(for: account.id) ?? .credentialMissing + appEnvironment?.accountsManager.status(for: account.id) ?? .checking } private func statusText(for account: Account) -> String { switch status(for: account) { + case .checking: String(localized: "accounts.status.checking") case .signedIn: String(localized: "accounts.status.signedIn") case .credentialMissing: String(localized: "accounts.status.credentialMissing") case .credentialRejected: String(localized: "accounts.status.credentialRejected") @@ -171,6 +172,7 @@ struct InstanceAccountsSection: View { private func statusColor(for account: Account) -> Color { switch status(for: account) { + case .checking: .secondary case .signedIn: .secondary case .credentialMissing: .secondary case .credentialRejected: .red diff --git a/Yattee/Views/Settings/iCloudSettingsView.swift b/Yattee/Views/Settings/iCloudSettingsView.swift index 6f812964..a62c0ef3 100644 --- a/Yattee/Views/Settings/iCloudSettingsView.swift +++ b/Yattee/Views/Settings/iCloudSettingsView.swift @@ -532,11 +532,19 @@ struct iCloudSettingsView: View { resyncStoredLogins() } - /// Rewrites stored logins so they carry the sync setting that has just - /// changed. Whether a Keychain item syncs is fixed when it is written, so a - /// login stored while sync was off would stay on this device while its + /// Rewrites stored logins so they carry the sync setting that has just been + /// turned **on**. Whether a Keychain item syncs is fixed when it is written, + /// so a login stored while sync was off would stay on this device while its /// account record travelled — every other device showing an account whose /// credential never arrives. + /// + /// Deliberately not called when sync is switched off. Making a + /// synchronizable item local means deleting and rewriting it, and deleting + /// a synchronizable item propagates to every device (Apple-documented, and + /// measured for this project) — so "stop syncing this login" would sign the + /// user out on their other devices, which is not what the toggle says. + /// Turning sync off stops new writes from syncing and stops account records + /// from uploading; what is already in iCloud Keychain stays. private func resyncStoredLogins() { appEnvironment?.invidiousCredentialsManager.resyncCredentialsWithCurrentSetting() appEnvironment?.pipedCredentialsManager.resyncCredentialsWithCurrentSetting() diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index ce1f0c3e..10682f4e 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -186,6 +186,24 @@ struct AccountsManagerTests { #expect(manager.context(forLegacyInstanceID: instanceID) == .account(second.id)) } + @Test("An unresolved identity is not a missing credential") + func unknownStatusIsNotSignedOut() { + let defaults = makeDefaults() + let unresolved = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: nil), + defaults: defaults + ) + let account = unresolved.addAccount(instanceID: UUID()) + + // Nothing can read the Keychain under the right scope yet, so "no + // credential" is not an answer the app is entitled to give. + #expect(unresolved.status(for: account.id) == .checking) + + let resolved = makeManager(defaults: defaults, scope: "identity-A") + let other = resolved.addAccount(instanceID: UUID()) + #expect(resolved.status(for: other.id) == .credentialMissing) + } + @Test("A fresh device adopts the synced context") func firstSyncedContextIsAdopted() { let manager = makeManager()