diff --git a/Yattee/Services/Credentials/AccountKeychainCredentials.swift b/Yattee/Services/Credentials/AccountKeychainCredentials.swift index ad822424..e78a1cb3 100644 --- a/Yattee/Services/Credentials/AccountKeychainCredentials.swift +++ b/Yattee/Services/Credentials/AccountKeychainCredentials.swift @@ -92,13 +92,22 @@ final class AccountKeychainCredentials { loginsByScope[scope, default: []].insert(account.id) - // Whatever was adopted here is gone, replaced by a login the user just - // proved they hold. Leaving the marker would keep this account out of - // the resync that follows the iCloud toggle, so the new credential - // would stay on this device and every other one would show an account - // it never receives a login for. It would also let a deletion retire an + // Whatever *this account* adopted is gone, replaced by a login the user + // just proved they hold. Leaving the marker would keep it out of the + // resync that follows the iCloud toggle, so the new credential would + // stay on this device and every other one would show an account it + // never receives a login for. It would also let a deletion retire an // unscoped item this account no longer has anything to do with. - markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope) + // + // A sibling's marker is left strictly alone: signing a second login + // into the same instance says nothing about the account that adopted + // that instance's pre-revamp secret. + markers.removeMarker( + service: store.service, + instanceID: account.instanceID, + accountID: account.id, + scope: scope + ) LoggingService.shared.info( "Stored \(credentialNoun)", @@ -412,7 +421,12 @@ final class AccountKeychainCredentials { } store.delete(key: legacyKey) - markers.removeMarker(service: store.service, instanceID: account.instanceID, scope: scope) + markers.removeMarker( + service: store.service, + instanceID: account.instanceID, + accountID: account.id, + scope: scope + ) LoggingService.shared.info( "Deleted the legacy \(credentialNoun) alongside its account", diff --git a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift index 0ce00fa8..7982e750 100644 --- a/Yattee/Services/Credentials/CredentialAdoptionMarker.swift +++ b/Yattee/Services/Credentials/CredentialAdoptionMarker.swift @@ -109,9 +109,20 @@ final class CredentialAdoptionMarkerStore { save(all) } - func removeMarker(service: String, instanceID: UUID, scope: String) { + /// Retires the marker for one account. + /// + /// Scoped to `accountID` as well as the instance: an instance can hold + /// several logins, and only the account that actually adopted the legacy + /// secret may retire its marker. Matching on the instance alone let a + /// *sibling* sign-in clear it, which silently withdrew the migrated + /// account's claim on the unscoped item — after that its deletion could + /// no longer retire the legacy secret, and it could re-adopt it. + func removeMarker(service: String, instanceID: UUID, accountID: UUID, scope: String) { save(markers().filter { - !($0.service == service && $0.instanceID == instanceID && $0.identityScope == scope) + !($0.service == service + && $0.instanceID == instanceID + && $0.accountID == accountID + && $0.identityScope == scope) }) } diff --git a/YatteeTests/AccountCredentialsTests.swift b/YatteeTests/AccountCredentialsTests.swift index ad11b1b2..5a1ab533 100644 --- a/YatteeTests/AccountCredentialsTests.swift +++ b/YatteeTests/AccountCredentialsTests.swift @@ -372,6 +372,33 @@ struct LegacyCredentialAdoptionTests { store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID)) } + @Test("A sibling sign-in does not retire the adopter's marker") + func siblingSignInLeavesTheAdoptionMarkerAlone() { + let service = makeService() + let markers = makeMarkers() + let instanceID = UUID() + let adopter = Account.migrated(from: instanceID) + // A second login the user adds on the same instance. + let sibling = Account(instanceID: instanceID) + seedLegacyItem(service: service, instanceID: instanceID, value: "legacy-secret") + + let credentials = makeCredentials(service: service, scope: "identity-A", markers: markers) + #expect(credentials.adoptLegacyCredential(for: adopter) == true) + + // Signing the sibling in says nothing about the account that adopted + // this instance's pre-revamp secret. + credentials.setCredential("sibling-secret", for: sibling) + #expect(markers.hasAdopted(service: service, instanceID: instanceID, scope: "identity-A") == true) + + // So the adopter can still retire the legacy item when it is deleted — + // matching on the instance alone silently withdrew that claim. + credentials.deleteLegacyCredentialIfOwned(for: adopter) + #expect(legacyValue(service: service, instanceID: instanceID) == nil) + + credentials.deleteCredential(for: adopter) + credentials.deleteCredential(for: sibling) + } + @Test("A login the user added themselves never inherits the legacy item") func onlyTheMigratedAccountAdopts() { let service = makeService()