Skip to content

Commit

Permalink
Ensure token item and metadata keychain accessibility settings are co…
Browse files Browse the repository at this point in the history
…nsistent. (#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.
  • Loading branch information
mikenachbaur-okta authored Jul 18, 2024
1 parent 3a2c4de commit 15a3286
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 []
}
Expand Down Expand Up @@ -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)

Expand All @@ -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))
Expand Down Expand Up @@ -140,7 +152,6 @@ final class KeychainTokenStorage: TokenStorage {
synchronizable: accessibility.isSynchronizable,
label: nil,
description: nil,
generic: nil,
value: data)

var context: KeychainAuthenticationContext?
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion Sources/AuthFoundation/Token Management/Token.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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`).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
125 changes: 123 additions & 2 deletions Tests/AuthFoundationTests/KeychainTokenStorageTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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)

Expand All @@ -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()
Expand All @@ -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)
}
Expand All @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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

0 comments on commit 15a3286

Please sign in to comment.