diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index e215a32a..5ddaf951 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -73,6 +73,16 @@ final class AccountsManager { 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( forName: .instancesDidChange, object: nil, @@ -139,6 +149,13 @@ final class AccountsManager { let account = Account(instanceID: instanceID, displayName: displayName) accounts.append(account) 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() return account } @@ -195,6 +212,7 @@ final class AccountsManager { /// Records an explicit user choice of context. func setContext(_ context: DataContext) { storedContext = StoredDataContext(context: context) + hasStoredContext = true save() NotificationCenter.default.post(name: .dataContextDidChange, object: nil) } @@ -205,6 +223,7 @@ final class AccountsManager { func applySyncedContext(_ incoming: StoredDataContext) { guard incoming.changedAt > storedContext.changedAt else { return } storedContext = incoming + hasStoredContext = true save() NotificationCenter.default.post(name: .dataContextDidChange, object: nil) } @@ -240,14 +259,19 @@ final class AccountsManager { guard instance.type.accountCapabilities.isSelectableContext 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) 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) accounts.append(account) pendingUploads.insert(account.id) @@ -359,7 +383,6 @@ final class AccountsManager { } if let data = try? JSONEncoder().encode(storedContext) { defaults.set(data, forKey: key("context")) - hasStoredContext = true } defaults.set(migratedInstanceIDs.map(\.uuidString), forKey: key("migrated")) defaults.set(pendingUploads.map(\.uuidString), forKey: key("pendingUploads")) diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 3f0b4c43..9dd79ef4 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -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. /// /// Only ever reports `credentialMissing` versus `signedIn` — a rejected diff --git a/Yattee/Localizable.xcstrings b/Yattee/Localizable.xcstrings index f6c51c0a..8b80b619 100644 --- a/Yattee/Localizable.xcstrings +++ b/Yattee/Localizable.xcstrings @@ -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" : { "localizations" : { "en" : { diff --git a/Yattee/Services/Credentials/CredentialIdentityScope.swift b/Yattee/Services/Credentials/CredentialIdentityScope.swift index faf00982..6dbb02da 100644 --- a/Yattee/Services/Credentials/CredentialIdentityScope.swift +++ b/Yattee/Services/Credentials/CredentialIdentityScope.swift @@ -99,6 +99,19 @@ final class CredentialIdentityScope { 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 } init( @@ -180,7 +193,7 @@ final class CredentialIdentityScope { // to serve it would hand the previous account's credentials to the new // one. if resolvedWithoutToken { - state = .unresolved + setState(.unresolved) resolvedWithoutToken = false } @@ -219,7 +232,7 @@ final class CredentialIdentityScope { // No iCloud entitlement: nothing can sync, and there is no ambient // identity for a retained credential to leak to. resolvedWithoutToken = false - state = .resolved(Self.localScope) + setState(.resolved(Self.localScope)) return } @@ -230,15 +243,15 @@ final class CredentialIdentityScope { // 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 + setState(.unresolved) return } if let cached = cachedScope(for: current) { resolvedWithoutToken = false - state = .resolved(cached) + setState(.resolved(cached)) } else { - state = .unresolved + setState(.unresolved) } } @@ -293,7 +306,7 @@ final class CredentialIdentityScope { guard identityGeneration == generation, fingerprintProvider() == nil, fingerprint == nil else { return } resolvedWithoutToken = true - state = .resolved(Self.localScope) + setState(.resolved(Self.localScope)) LoggingService.shared.info( "No Apple account — credentials scoped locally", category: .keychain @@ -326,7 +339,7 @@ final class CredentialIdentityScope { cache(recordID, for: requested) } resolvedWithoutToken = requested == nil - state = .resolved(recordID) + setState(.resolved(recordID)) LoggingService.shared.info( "Resolved credential identity scope", @@ -382,7 +395,7 @@ final class CredentialIdentityScope { private func revalidateTokenlessScope() async { guard resolvedWithoutToken, fingerprintProvider() == nil else { return } - state = .unresolved + setState(.unresolved) resolvedWithoutToken = false resolutionAttempts = 0 @@ -404,3 +417,9 @@ final class CredentialIdentityScope { 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") +} diff --git a/Yattee/Services/HomeInstanceCache.swift b/Yattee/Services/HomeInstanceCache.swift index 3174727b..404b9de5 100644 --- a/Yattee/Services/HomeInstanceCache.swift +++ b/Yattee/Services/HomeInstanceCache.swift @@ -118,7 +118,7 @@ final class HomeInstanceCache { } // 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( "HomeInstanceCache.refresh: No credential for \(instance.displayName), skipping feed", category: .general diff --git a/Yattee/Services/SubscriptionFeedCache.swift b/Yattee/Services/SubscriptionFeedCache.swift index ea0ed3e4..1826ea36 100644 --- a/Yattee/Services/SubscriptionFeedCache.swift +++ b/Yattee/Services/SubscriptionFeedCache.swift @@ -303,7 +303,7 @@ final class SubscriptionFeedCache { 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) return nil } @@ -353,7 +353,7 @@ final class SubscriptionFeedCache { guard let instanceID = account.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) return nil } diff --git a/Yattee/Services/SubscriptionService.swift b/Yattee/Services/SubscriptionService.swift index ffb35548..66f9829a 100644 --- a/Yattee/Services/SubscriptionService.swift +++ b/Yattee/Services/SubscriptionService.swift @@ -88,6 +88,16 @@ final class SubscriptionService { func setAccountsManager(_ manager: AccountsManager) { 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 diff --git a/Yattee/Views/Home/HomeSettingsView.swift b/Yattee/Views/Home/HomeSettingsView.swift index 23da1bba..590b1794 100644 --- a/Yattee/Views/Home/HomeSettingsView.swift +++ b/Yattee/Views/Home/HomeSettingsView.swift @@ -643,7 +643,7 @@ struct HomeSettingsView: View { private func isLoggedIn(_ instance: Instance) -> Bool { 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? { diff --git a/Yattee/Views/Instances/InstanceBrowseView.swift b/Yattee/Views/Instances/InstanceBrowseView.swift index ebed4432..65134b06 100644 --- a/Yattee/Views/Instances/InstanceBrowseView.swift +++ b/Yattee/Views/Instances/InstanceBrowseView.swift @@ -419,7 +419,7 @@ struct InstanceBrowseView: View { // Check login status for instances that support authentication 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) @@ -1222,7 +1222,7 @@ struct InstanceBrowseView: View { trendingVideos = videos prefetchBranding(for: videos) 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") isLoading = false return @@ -1293,7 +1293,7 @@ struct InstanceBrowseView: View { 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") isLoading = false return @@ -1393,7 +1393,7 @@ struct InstanceBrowseView: View { private func loadMoreFeedResults() { guard hasMoreFeedResults, !isLoadingMoreFeed, !isLoading else { return } guard let appEnvironment, - let sid = appEnvironment.invidiousCredentialsManager.sid(for: instance) else { return } + let sid = appEnvironment.credential(forInstance: instance) else { return } isLoadingMoreFeed = true feedLoadedVideoCount = feedVideos.count // Mark current count to prevent re-triggering diff --git a/Yattee/Views/Settings/EditSourceView.swift b/Yattee/Views/Settings/EditSourceView.swift index 7bd4693b..9795a298 100644 --- a/Yattee/Views/Settings/EditSourceView.swift +++ b/Yattee/Views/Settings/EditSourceView.swift @@ -328,7 +328,7 @@ private struct EditRemoteServerContent: View { } #endif .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) 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) { 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 ?? appEnvironment.accountsManager.addAccount(instanceID: instance.id) guard let credentials = appEnvironment.accountCredentialsManager(for: account) else { return } 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 { credentials.setUsername(username, for: account) } diff --git a/Yattee/Views/Settings/ImportPlaylistsView.swift b/Yattee/Views/Settings/ImportPlaylistsView.swift index 2c8acd6b..0f01ca42 100644 --- a/Yattee/Views/Settings/ImportPlaylistsView.swift +++ b/Yattee/Views/Settings/ImportPlaylistsView.swift @@ -262,7 +262,7 @@ struct ImportPlaylistsView: View { private func loadPlaylists() async { guard let appEnvironment, - let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { + let credential = appEnvironment.credential(forInstance: instance) else { error = ImportError.notLoggedIn isLoading = false return @@ -326,7 +326,7 @@ struct ImportPlaylistsView: View { private func performImport(_ playlist: Playlist, into localPlaylist: LocalPlaylist) async { guard let appEnvironment, - let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { + let credential = appEnvironment.credential(forInstance: instance) else { return } diff --git a/Yattee/Views/Settings/ImportSubscriptionsView.swift b/Yattee/Views/Settings/ImportSubscriptionsView.swift index 1a58148c..d5a529fd 100644 --- a/Yattee/Views/Settings/ImportSubscriptionsView.swift +++ b/Yattee/Views/Settings/ImportSubscriptionsView.swift @@ -214,7 +214,7 @@ struct ImportSubscriptionsView: View { private func loadSubscriptions() async { guard let appEnvironment, - let credential = appEnvironment.credentialsManager(for: instance)?.credential(for: instance) else { + let credential = appEnvironment.credential(forInstance: instance) else { error = ImportError.notLoggedIn isLoading = false return diff --git a/YatteeTests/AccountsManagerTests.swift b/YatteeTests/AccountsManagerTests.swift index 4765a8c2..89b440a5 100644 --- a/YatteeTests/AccountsManagerTests.swift +++ b/YatteeTests/AccountsManagerTests.swift @@ -120,6 +120,43 @@ struct AccountsManagerTests { #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") func migratedContextLosesToExplicitChoice() { let manager = makeManager()