From 15a3286988a9ff9264bc9f1e614d314929b32fac Mon Sep 17 00:00:00 2001 From: Alex Nachbaur <74688448+mikenachbaur-okta@users.noreply.github.com> Date: Thu, 18 Jul 2024 12:32:31 -0700 Subject: [PATCH] Ensure token item and metadata keychain accessibility settings are consistent. (#200) This addresses #198 by: 1. Ensuring the token metadata is stored with the appropriate accessibility settings, respecting the parent item's iCloud sync setting. 2. Returning token IDs only if both their item and metadata keychain entries can be found. 3. Establishing the `Credential.Security.isDefaultSynchronizable` property, allowing the default identifier to be shared. This also adds unit test coverage to validate these changes. --- .../Internal/KeychainTokenStorage.swift | 30 ++++- .../Token Management/Token.swift | 2 +- .../User Management/CredentialSecurity.swift | 3 + .../KeychainTokenStorageTests.swift | 125 +++++++++++++++++- 4 files changed, 151 insertions(+), 9 deletions(-) diff --git a/Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift b/Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift index 66e91274c..176694e3e 100644 --- a/Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift +++ b/Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift @@ -46,10 +46,16 @@ final class KeychainTokenStorage: TokenStorage { var allIDs: [String] { do { - return try Keychain + let itemIDs = try Keychain + .Search(service: KeychainTokenStorage.serviceName) + .list() + .sorted(by: { $0.creationDate < $1.creationDate }) + .map(\.account) + let metadataIDs = try Keychain .Search(service: KeychainTokenStorage.metadataName) .list() .map(\.account) + return itemIDs.filter { metadataIDs.contains($0) } } catch { return [] } @@ -78,7 +84,7 @@ final class KeychainTokenStorage: TokenStorage { .isEmpty let data = try encoder.encode(token) - let accessibility = security.accessibility ?? .afterFirstUnlock + let accessibility = security.accessibility ?? .afterFirstUnlockThisDeviceOnly let accessGroup = security.accessGroup let accessControl = try security.createAccessControl(accessibility: accessibility) @@ -90,12 +96,18 @@ final class KeychainTokenStorage: TokenStorage { synchronizable: accessibility.isSynchronizable, label: nil, description: nil, - generic: nil, value: data) + let metadataAccessibility: Keychain.Accessibility + if accessibility.isSynchronizable { + metadataAccessibility = .afterFirstUnlock + } else { + metadataAccessibility = .afterFirstUnlockThisDeviceOnly + } + let metadataItem = Keychain.Item(account: id, service: KeychainTokenStorage.metadataName, - accessibility: .afterFirstUnlock, + accessibility: metadataAccessibility, accessGroup: accessGroup, synchronizable: accessibility.isSynchronizable, value: try encoder.encode(metadata)) @@ -140,7 +152,6 @@ final class KeychainTokenStorage: TokenStorage { synchronizable: accessibility.isSynchronizable, label: nil, description: nil, - generic: nil, value: data) var context: KeychainAuthenticationContext? @@ -217,9 +228,16 @@ final class KeychainTokenStorage: TokenStorage { private func saveDefault() throws { if let tokenIdData = defaultTokenID?.data(using: .utf8) { + let accessibility: Keychain.Accessibility + if Credential.Security.isDefaultSynchronizable { + accessibility = .afterFirstUnlock + } else { + accessibility = .afterFirstUnlockThisDeviceOnly + } + try Keychain .Item(account: KeychainTokenStorage.defaultTokenName, - accessibility: .afterFirstUnlock, + accessibility: accessibility, value: tokenIdData) .save() } else { diff --git a/Sources/AuthFoundation/Token Management/Token.swift b/Sources/AuthFoundation/Token Management/Token.swift index 9d2a75900..423618401 100644 --- a/Sources/AuthFoundation/Token Management/Token.swift +++ b/Sources/AuthFoundation/Token Management/Token.swift @@ -26,7 +26,7 @@ public final class Token: Codable, Equatable, Hashable, Expires { /// The unique identifier for this token. public internal(set) var id: String - // The date this token was issued at. + /// The date this token was issued at. public let issuedAt: Date? /// The string type of the token (e.g. `Bearer`). diff --git a/Sources/AuthFoundation/User Management/CredentialSecurity.swift b/Sources/AuthFoundation/User Management/CredentialSecurity.swift index 6a55d21b5..75aef98bf 100644 --- a/Sources/AuthFoundation/User Management/CredentialSecurity.swift +++ b/Sources/AuthFoundation/User Management/CredentialSecurity.swift @@ -46,6 +46,9 @@ extension Credential { /// /// If you wish to change the default security threshold for Keychain items, you can assign a new value here. Additionally, if a ``context(_:)`` value is assigned to the ``standard`` property, that context will be used when fetching credentials unless otherwise specified. public static var standard: [Security] = [.accessibility(.afterFirstUnlockThisDeviceOnly)] + + /// Determines whether or not the ``Credential/default`` setting is synchronized across a user's devices using iCloud Keychain. + public static var isDefaultSynchronizable: Bool = false #else public static var standard: [Security] = [] #endif diff --git a/Tests/AuthFoundationTests/KeychainTokenStorageTests.swift b/Tests/AuthFoundationTests/KeychainTokenStorageTests.swift index fda608c48..2f378be66 100644 --- a/Tests/AuthFoundationTests/KeychainTokenStorageTests.swift +++ b/Tests/AuthFoundationTests/KeychainTokenStorageTests.swift @@ -64,6 +64,57 @@ final class KeychainTokenStorageTests: XCTestCase { storage = nil } + func testEmptyAllIDs() throws { + mock.expect(errSecSuccess, result: [] as CFArray) + mock.expect(errSecSuccess, result: [] as CFArray) + + XCTAssertEqual(storage.allIDs, []) + XCTAssertEqual(mock.operations.count, 2) + + // - Listing the token items + XCTAssertEqual(mock.operations[0].action, .copy) + XCTAssertEqual(mock.operations[0].query["svce"] as? String, KeychainTokenStorage.serviceName) + XCTAssertEqual(mock.operations[0].query["class"] as? String, "genp") + XCTAssertEqual(mock.operations[0].query["m_Limit"] as? String, "m_LimitAll") + + // - Listing the token metadata + XCTAssertEqual(mock.operations[1].action, .copy) + XCTAssertEqual(mock.operations[1].query["svce"] as? String, KeychainTokenStorage.metadataName) + XCTAssertEqual(mock.operations[1].query["class"] as? String, "genp") + XCTAssertEqual(mock.operations[1].query["m_Limit"] as? String, "m_LimitAll") + } + + func testAllIDs() throws { + func listItem(id: String, service: String) -> CFDictionary { + [ + "tomb": 0, + "svce": service, + "musr": nil, + "class": "genp", + "sync": 0, + "cdat": Date(), + "mdat": Date(), + "pdmn": "ak", + "agrp": "com.okta.sample.app", + "acct": "SomeAccount\(id)", + "sha": "someshadata".data(using: .utf8), + "UUID": UUID().uuidString + ] as CFDictionary + } + mock.expect(errSecSuccess, result: [ + listItem(id: "1", service: KeychainTokenStorage.serviceName), + listItem(id: "2", service: KeychainTokenStorage.metadataName), + ] as CFArray) + mock.expect(errSecSuccess, result: [ + listItem(id: "1", service: KeychainTokenStorage.serviceName) + ] as CFArray) + + let allIds = storage.allIDs + XCTAssertEqual(mock.operations.count, 2) + XCTAssertEqual(allIds.count, 1) + XCTAssertEqual(allIds.first, "SomeAccount1") + } + func testDefaultToken() throws { mock.expect(errSecSuccess, result: [] as CFArray) mock.expect(errSecSuccess, result: [] as CFArray) @@ -79,7 +130,8 @@ final class KeychainTokenStorageTests: XCTestCase { mock.expect(noErr) mock.expect(noErr, result: dummyGetResult) - try storage.add(token: token, metadata: nil, security: []) + Credential.Security.isDefaultSynchronizable = true + try storage.add(token: token, metadata: nil, security: [.accessibility(.unlocked)]) XCTAssertEqual(mock.operations.count, 9) // Adding the new token @@ -104,6 +156,7 @@ final class KeychainTokenStorageTests: XCTestCase { XCTAssertEqual(mock.operations[3].action, .add) XCTAssertEqual(mock.operations[3].query["acct"] as? String, token.id) XCTAssertEqual(mock.operations[3].query["svce"] as? String, KeychainTokenStorage.serviceName) + XCTAssertEqual(mock.operations[3].query["pdmn"] as? String, Keychain.Accessibility.unlocked.rawValue) let tokenQuery = mock.operations[3].query // - Preemptively deleting the newly-added metadata @@ -115,7 +168,8 @@ final class KeychainTokenStorageTests: XCTestCase { XCTAssertEqual(mock.operations[5].action, .add) XCTAssertEqual(mock.operations[5].query["acct"] as? String, token.id) XCTAssertEqual(mock.operations[5].query["svce"] as? String, KeychainTokenStorage.metadataName) - + XCTAssertEqual(mock.operations[5].query["pdmn"] as? String, Keychain.Accessibility.afterFirstUnlock.rawValue) + // - Loading the current defaultTokenID XCTAssertEqual(mock.operations[6].action, .copy) XCTAssertNil(mock.operations[6].query["svce"] as? String) @@ -130,6 +184,7 @@ final class KeychainTokenStorageTests: XCTestCase { XCTAssertEqual(mock.operations[8].action, .add) XCTAssertEqual(mock.operations[8].query["acct"] as? String, KeychainTokenStorage.defaultTokenName) XCTAssertEqual(mock.operations[8].query["v_Data"] as? Data, token.id.data(using: .utf8)) + XCTAssertEqual(mock.operations[8].query["pdmn"] as? String, Keychain.Accessibility.afterFirstUnlock.rawValue) XCTAssertEqual(storage.defaultTokenID, token.id) @@ -138,6 +193,7 @@ final class KeychainTokenStorageTests: XCTestCase { tokenResult["cdat"] = Date() mock.reset() mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertEqual(storage.allIDs.count, 1) mock.reset() @@ -148,11 +204,14 @@ final class KeychainTokenStorageTests: XCTestCase { mock.reset() mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertEqual(storage.allIDs.count, 1) + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertThrowsError(try storage.add(token: token, metadata: nil, security: [])) + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertEqual(storage.allIDs.count, 1) } @@ -178,6 +237,7 @@ final class KeychainTokenStorageTests: XCTestCase { tokenResult["mdat"] = Date() tokenResult["cdat"] = Date() + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertEqual(storage.allIDs.count, 1) @@ -209,6 +269,7 @@ final class KeychainTokenStorageTests: XCTestCase { XCTAssertEqual(storage.defaultTokenID, token.id) + mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) mock.expect(noErr, result: NSArray(arrayLiteral: tokenResult as CFDictionary) as CFArray) XCTAssertEqual(storage.allIDs.count, 1) @@ -259,6 +320,66 @@ final class KeychainTokenStorageTests: XCTestCase { XCTAssertEqual(updateOperation.attributes?["pdmn"] as? String, "akpu") XCTAssertEqual(updateOperation.attributes?["agrp"] as? String, "otherGroup") } + + func testAddTokenWithSecurity() throws { + // - Find duplicate items + mock.expect(errSecSuccess, result: [] as CFArray) + + // - Determine if we're implicitly changing the default + mock.expect(errSecSuccess, result: [] as CFArray) + + // - Save the item + mock.expect(noErr) + mock.expect(noErr, result: dummyGetResult) + mock.expect(noErr) + mock.expect(noErr, result: dummyGetResult) + + // Compare existing defaultTokenID + mock.expect(errSecSuccess, result: [] as CFArray) + + // Save new defaultTokenID + mock.expect(noErr) + mock.expect(noErr, result: dummyGetResult) + + Credential.Security.isDefaultSynchronizable = false + try storage.add(token: token, + metadata: Token.Metadata(token: token, + tags: ["tag": "value"]), + security: [.accessibility(.unlockedThisDeviceOnly), + .accessGroup("com.example.myapp")]) + + XCTAssertEqual(mock.operations.count, 9) + + // - Preemptively deleting the newly-added token + XCTAssertEqual(mock.operations[2].action, .delete) + XCTAssertEqual(mock.operations[2].query["acct"] as? String, token.id) + XCTAssertEqual(mock.operations[2].query["svce"] as? String, KeychainTokenStorage.serviceName) + + // - Adding the new token + XCTAssertEqual(mock.operations[3].action, .add) + XCTAssertEqual(mock.operations[3].query["acct"] as? String, token.id) + XCTAssertEqual(mock.operations[3].query["svce"] as? String, KeychainTokenStorage.serviceName) + XCTAssertEqual(mock.operations[3].query["pdmn"] as? String, Keychain.Accessibility.unlockedThisDeviceOnly.rawValue) + + // - Preemptively deleting the newly-added metadata + XCTAssertEqual(mock.operations[4].action, .delete) + XCTAssertEqual(mock.operations[4].query["acct"] as? String, token.id) + XCTAssertEqual(mock.operations[4].query["svce"] as? String, KeychainTokenStorage.metadataName) + + // - Adding the new metadata + XCTAssertEqual(mock.operations[5].action, .add) + XCTAssertEqual(mock.operations[5].query["acct"] as? String, token.id) + XCTAssertEqual(mock.operations[5].query["svce"] as? String, KeychainTokenStorage.metadataName) + XCTAssertEqual(mock.operations[5].query["pdmn"] as? String, Keychain.Accessibility.afterFirstUnlockThisDeviceOnly.rawValue) + + // Adding the new default token ID + XCTAssertEqual(mock.operations[8].action, .add) + XCTAssertEqual(mock.operations[8].query["acct"] as? String, KeychainTokenStorage.defaultTokenName) + XCTAssertEqual(mock.operations[8].query["v_Data"] as? Data, token.id.data(using: .utf8)) + XCTAssertEqual(mock.operations[5].query["pdmn"] as? String, Keychain.Accessibility.afterFirstUnlockThisDeviceOnly.rawValue) + + XCTAssertEqual(storage.defaultTokenID, token.id) + } } #endif