Ignore un-deserializable tokens in allPersistentTokens() #179
Conversation
| @@ -281,7 +284,8 @@ class KeychainTests: XCTestCase { | |||
| let persistentRef = try addKeychainItem(withAttributes: keychainAttributes) | |||
|
|
|||
| XCTAssertThrowsError(try keychain.persistentToken(withIdentifier: persistentRef)) | |||
| XCTAssertThrowsError(try keychain.allPersistentTokens()) | |||
| // TODO: Restore deserialization error handling in allPersistentTokens() | |||
There was a problem hiding this comment.
Todo Violation: TODOs should be resolved (Restore deserialization error ...). (todo)
| @@ -264,7 +266,8 @@ class KeychainTests: XCTestCase { | |||
| let persistentRef = try addKeychainItem(withAttributes: keychainAttributes) | |||
|
|
|||
| XCTAssertThrowsError(try keychain.persistentToken(withIdentifier: persistentRef)) | |||
| XCTAssertThrowsError(try keychain.allPersistentTokens()) | |||
| // TODO: Restore deserialization error handling in allPersistentTokens() | |||
There was a problem hiding this comment.
Todo Violation: TODOs should be resolved (Restore deserialization error ...). (todo)
| @@ -247,7 +248,8 @@ class KeychainTests: XCTestCase { | |||
| let persistentRef = try addKeychainItem(withAttributes: keychainAttributes) | |||
|
|
|||
| XCTAssertThrowsError(try keychain.persistentToken(withIdentifier: persistentRef)) | |||
| XCTAssertThrowsError(try keychain.allPersistentTokens()) | |||
| // TODO: Restore deserialization error handling in allPersistentTokens() | |||
There was a problem hiding this comment.
Todo Violation: TODOs should be resolved (Restore deserialization error ...). (todo)
| @@ -231,7 +231,8 @@ class KeychainTests: XCTestCase { | |||
| let persistentRef = try addKeychainItem(withAttributes: keychainAttributes) | |||
|
|
|||
| XCTAssertThrowsError(try keychain.persistentToken(withIdentifier: persistentRef)) | |||
| XCTAssertThrowsError(try keychain.allPersistentTokens()) | |||
| // TODO: Restore deserialization error handling in allPersistentTokens() | |||
There was a problem hiding this comment.
Todo Violation: TODOs should be resolved (Restore deserialization error ...). (todo)
| // tokens as possible. | ||
| // TODO: Restore deserialization error handling, in a way that provides info on the failure reason and allows | ||
| // the caller to choose whether to fail completely or recover some data. | ||
| return Set(allItems.flatMap({ try? PersistentToken.init(keychainDictionary:$0) })) |
There was a problem hiding this comment.
Explicit Init Violation: Explicitly calling .init() should be avoided. (explicit_init)
| let allItems = try allKeychainItems() | ||
| // This code intentionally ignores items which fail deserialization, instead opting to return as many readable | ||
| // tokens as possible. | ||
| // TODO: Restore deserialization error handling, in a way that provides info on the failure reason and allows |
There was a problem hiding this comment.
Todo Violation: TODOs should be resolved (Restore deserialization error ...). (todo)
Codecov Report
@@ Coverage Diff @@
## develop #179 +/- ##
===========================================
+ Coverage 97.19% 97.22% +0.03%
===========================================
Files 6 6
Lines 392 397 +5
===========================================
+ Hits 381 386 +5
Misses 11 11
Continue to review full report at Codecov.
|
A recent change to the behavior of
allPersistentTokens()caused any individual persistent token which failed deserialization to cause the entire fetch to fail. Previously, individual tokens failing deserialization would be ignored, and the method would return as many readable tokens as possible.That change has been the cause of a crash on launch for many users of Authenticator. This PR reverts the behavior change until the cause of the underlying deserialization failures can be understood, and a recovery path provided.