mirror of
https://github.com/yattee/yattee.git
synced 2026-11-03 10:27:19 +00:00
Let only the adopting account retire its marker
Found on the iPad: after signing a second login into an instance, its migrated account could no longer retire the unscoped legacy item when it was deleted, and the instance was left still claiming a pre-revamp login. Storing a credential retires the adoption marker, but it matched on service, instance and identity only. An instance holds several logins, so a sibling sign-in silently withdrew the migrated account's claim on a secret it had nothing to do with. After that the legacy item outlived the account that adopted it, and could be adopted a second time. Erring that way costs nothing — an item that is not deleted is an item nobody is logged out of — but the marker already carries the account it was written for, and deletion authority already checks it. Retiring it now matches on the same thing.
This commit is contained in:
@@ -92,13 +92,22 @@ final class AccountKeychainCredentials {
|
|||||||
|
|
||||||
loginsByScope[scope, default: []].insert(account.id)
|
loginsByScope[scope, default: []].insert(account.id)
|
||||||
|
|
||||||
// Whatever was adopted here is gone, replaced by a login the user just
|
// Whatever *this account* adopted is gone, replaced by a login the user
|
||||||
// proved they hold. Leaving the marker would keep this account out of
|
// just proved they hold. Leaving the marker would keep it out of the
|
||||||
// the resync that follows the iCloud toggle, so the new credential
|
// resync that follows the iCloud toggle, so the new credential would
|
||||||
// would stay on this device and every other one would show an account
|
// stay on this device and every other one would show an account it
|
||||||
// it never receives a login for. It would also let a deletion retire an
|
// never receives a login for. It would also let a deletion retire an
|
||||||
// unscoped item this account no longer has anything to do with.
|
// 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(
|
LoggingService.shared.info(
|
||||||
"Stored \(credentialNoun)",
|
"Stored \(credentialNoun)",
|
||||||
@@ -412,7 +421,12 @@ final class AccountKeychainCredentials {
|
|||||||
}
|
}
|
||||||
|
|
||||||
store.delete(key: legacyKey)
|
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(
|
LoggingService.shared.info(
|
||||||
"Deleted the legacy \(credentialNoun) alongside its account",
|
"Deleted the legacy \(credentialNoun) alongside its account",
|
||||||
|
|||||||
@@ -109,9 +109,20 @@ final class CredentialAdoptionMarkerStore {
|
|||||||
save(all)
|
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 {
|
save(markers().filter {
|
||||||
!($0.service == service && $0.instanceID == instanceID && $0.identityScope == scope)
|
!($0.service == service
|
||||||
|
&& $0.instanceID == instanceID
|
||||||
|
&& $0.accountID == accountID
|
||||||
|
&& $0.identityScope == scope)
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -372,6 +372,33 @@ struct LegacyCredentialAdoptionTests {
|
|||||||
store.delete(key: KeychainCredentialStore.legacyKey(instanceID: instanceID))
|
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")
|
@Test("A login the user added themselves never inherits the legacy item")
|
||||||
func onlyTheMigratedAccountAdopts() {
|
func onlyTheMigratedAccountAdopts() {
|
||||||
let service = makeService()
|
let service = makeService()
|
||||||
|
|||||||
Reference in New Issue
Block a user