From d88c40539e7e2476ed303687acf17c8c9fce69c8 Mon Sep 17 00:00:00 2001 From: Oliver Lorenz Date: Wed, 24 Jan 2024 09:39:37 +0100 Subject: [PATCH] KeychainAttribute: Return correct names of keychain attributes This commit fixes issue google/GTMAppAuth#236: KeychainAttribute.keyName should not return the quoted constants but rather the Foundation constants itself -- e.g. kSecUseDataProtectionKeychain instead of "kSecUseDataProtectionKeychain". KeychainWrapper uses the return value as-is to assemble the query dictionary for SecItem(Add|Delete|CopyMatching). Therefore, using the wrong property names means the attributes never had any effect. --- .../KeychainStore/KeychainAttribute.swift | 11 +++++++--- .../KeychainStore/KeychainHelper.swift | 9 -------- .../KeychainStore/KeychainStoreTests.swift | 21 +++++++++++++++++++ 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift index bfc1d243..c8c775d8 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift @@ -24,6 +24,7 @@ public final class KeychainAttribute: NSObject { /// Indicates whether to treat macOS keychain items like iOS keychain items. /// /// This attribute will set `kSecUseDataProtectionKeychain` as true in the Keychain query. + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) case useDataProtectionKeychain /// The `String` name for the access group to use in the Keychain query. case accessGroup(String) @@ -32,9 +33,13 @@ public final class KeychainAttribute: NSObject { public var keyName: String { switch self { case .useDataProtectionKeychain: - return "kSecUseDataProtectionKeychain" + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + return kSecUseDataProtectionKeychain as String + } + + fatalError(".useDataProtectionKeychain is not available") case .accessGroup: - return "kSecKeychainAccessGroup" + return kSecAttrAccessGroup as String } } } @@ -53,7 +58,7 @@ public final class KeychainAttribute: NSObject { /// Creates an instance of `KeychainAttribute` whose attribute is set to /// `.useDataProtectionKeychain`. /// - Returns: An instance of `KeychainAttribute`. - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) @objc public static let useDataProtectionKeychain = KeychainAttribute( attribute: .useDataProtectionKeychain ) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift index 1017745e..fc9accdd 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift @@ -34,11 +34,6 @@ public protocol KeychainHelper { final class KeychainWrapper: KeychainHelper { let accountName = "OAuth" let keychainAttributes: Set - @available(macOS 10.15, *) - private var isMaxMacOSVersionGreaterThanTenOneFive: Bool { - let tenOneFive = OperatingSystemVersion(majorVersion: 10, minorVersion: 15, patchVersion: 0) - return ProcessInfo().isOperatingSystemAtLeast(tenOneFive) - } init(keychainAttributes: Set = []) { self.keychainAttributes = keychainAttributes @@ -54,11 +49,7 @@ final class KeychainWrapper: KeychainHelper { keychainAttributes.forEach { configuration in switch configuration.attribute { case .useDataProtectionKeychain: -#if os(macOS) && isMaxMacOSVersionGreaterThanTenOneFive - if #available(macOS 10.15, *) { query[configuration.attribute.keyName] = kCFBooleanTrue - } -#endif case .accessGroup(let name): query[configuration.attribute.keyName] = name } diff --git a/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift b/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift index 4b621884..7478b87c 100644 --- a/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift +++ b/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift @@ -66,6 +66,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + let comparisonQuery = comparisonKeychainQuery( withAttributes: useDataProtectionAttributeSet, accountName: fakeWithDataProtection.accountName, @@ -93,6 +95,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithAccessGroup.accountName, @@ -130,6 +134,9 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithDataProtectionAndAccessGroup.accountName, @@ -172,6 +179,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + let comparisonQuery = comparisonKeychainQuery( withAttributes: useDataProtectionAttributeSet, accountName: fakeWithDataProtection.accountName, @@ -200,6 +209,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithAccessGroup.accountName, @@ -238,6 +249,9 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithDataProtectionAndAccessGroup.accountName, @@ -280,6 +294,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + let comparisonQuery = comparisonKeychainQuery( withAttributes: useDataProtectionAttributeSet, accountName: fakeWithDataProtection.accountName, @@ -308,6 +324,8 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithAccessGroup.accountName, @@ -346,6 +364,9 @@ class KeychainStoreTests: XCTestCase { return } + XCTAssertEqual(testQuery[kSecUseDataProtectionKeychain as String], kCFBooleanTrue) + XCTAssertEqual(testQuery[kSecAttrAccessGroup as String], expectedGroupName) + let comparisonQuery = comparisonKeychainQuery( withAttributes: accessGroupAttributeSet, accountName: fakeWithDataProtectionAndAccessGroup.accountName,