Fix keychain key bug (#237)

Examples/Example-macOS/Example-macOS.entitlements

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>keychain-access-groups</key>
+	<array>
+		<string>$(AppIdentifierPrefix)com.example.GTMAppAuth.Example-macOS.test-group</string>
+	</array>
+</dict>
+</plist>

Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj

@@ -3,7 +3,7 @@
 	archiveVersion = 1;
 	classes = {
 	};
-	objectVersion = 52;
+	objectVersion = 54;
 	objects = {
 
 /* Begin PBXBuildFile section */
@@ -21,6 +21,7 @@
 
 /* Begin PBXFileReference section */
 		7355833228AFFC0B00AC24DC /* GTMAppAuth */ = {isa = PBXFileReference; lastKnownFileType = wrapper; name = GTMAppAuth; path = ../..; sourceTree = "<group>"; };
+		73B03D4C2B5EF66300EEA5DC /* Example-macOS.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = "Example-macOS.entitlements"; sourceTree = "<group>"; };
 		C1AF3AE62818785B003BAEFF /* README.md */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = net.daringfireball.markdown; path = README.md; sourceTree = "<group>"; };
 		F6016E701D2AC11F003497D7 /* Example-macOS.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "Example-macOS.app"; sourceTree = BUILT_PRODUCTS_DIR; };
 		F6016E8B1D2BD988003497D7 /* AppDelegate.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = AppDelegate.m; path = Source/AppDelegate.m; sourceTree = "<group>"; };
@@ -66,6 +67,7 @@
 		F6016E671D2AC11E003497D7 = {
 			isa = PBXGroup;
 			children = (
+				73B03D4C2B5EF66300EEA5DC /* Example-macOS.entitlements */,
 				7355833128AFFC0B00AC24DC /* Packages */,
 				C1AF3AE62818785B003BAEFF /* README.md */,
 				F6016E8A1D2BD973003497D7 /* Source */,
@@ -292,29 +294,39 @@
 			isa = XCBuildConfiguration;
 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				CODE_SIGN_ENTITLEMENTS = "Example-macOS.entitlements";
+				CODE_SIGN_IDENTITY = "Apple Development";
+				CODE_SIGN_STYLE = Automatic;
 				COMBINE_HIDPI_IMAGES = YES;
+				DEVELOPMENT_TEAM = "";
 				INFOPLIST_FILE = "$(SRCROOT)/Source/Info.plist";
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/../Frameworks",
 				);
 				PRODUCT_BUNDLE_IDENTIFIER = "com.example.GTMAppAuth.Example-macOS";
 				PRODUCT_NAME = "Example-macOS";
+				PROVISIONING_PROFILE_SPECIFIER = "";
 			};
 			name = Debug;
 		};
 		F6016E831D2AC11F003497D7 /* Release */ = {
 			isa = XCBuildConfiguration;
 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				CODE_SIGN_ENTITLEMENTS = "Example-macOS.entitlements";
+				CODE_SIGN_IDENTITY = "Apple Development";
+				CODE_SIGN_STYLE = Automatic;
 				COMBINE_HIDPI_IMAGES = YES;
+				DEVELOPMENT_TEAM = "";
 				INFOPLIST_FILE = "$(SRCROOT)/Source/Info.plist";
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
 					"@executable_path/../Frameworks",
 				);
 				PRODUCT_BUNDLE_IDENTIFIER = "com.example.GTMAppAuth.Example-macOS";
 				PRODUCT_NAME = "Example-macOS";
+				PROVISIONING_PROFILE_SPECIFIER = "";
 			};
 			name = Release;
 		};

Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m

@@ -30,6 +30,18 @@
 
 #import "AppDelegate.h"
 
+/*! @brief The bundle ID will use in constructing the app group string for keychain queries.
+    @discussion The string here is a combination of this example app's bundle ID and the keychain
+        access group name added in the app's entitlements file.
+ */
+static NSString *kBundleIDAccessGroup = @"com.example.GTMAppAuth.Example-macOS.test-group";
+
+/*! @brief The team ID you will use in constructing the app group string for keychain queries.
+    @discussion The team ID you will use can be found in your developer team profile page on
+        developer.apple.com.
+ */
+static NSString *const kTeamIDPrefix = @"YOUR_TEAM_ID";
+
 /*! @brief The OIDC issuer from which the configuration will be discovered.
  */
 static NSString *const kIssuer = @"https://accounts.google.com";
@@ -65,9 +77,17 @@ @implementation GTMAppAuthExampleViewController
 - (void)viewDidLoad {
   [super viewDidLoad];
 
-  self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey];
+  GTMKeychainAttribute *dataProtection = [GTMKeychainAttribute useDataProtectionKeychain];
+  NSString *testGroup = [NSString stringWithFormat:@"%@.%@", kTeamIDPrefix, kBundleIDAccessGroup];
+  GTMKeychainAttribute *accessGroup = [GTMKeychainAttribute keychainAccessGroupWithName:testGroup];
+  NSSet *attributes = [NSSet setWithArray:@[dataProtection, accessGroup]];
+  self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey
+                                               keychainAttributes:attributes];
 #if !defined(NS_BLOCK_ASSERTIONS)
 
+  NSAssert(![kTeamIDPrefix isEqualToString:@"YOUR_TEAM_ID"],
+           @"Update kTeamIDPrefix with your own team ID.");
+
   // NOTE:
   //
   // To run this sample, you need to register your own Google API client at

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
+        } else {
+          fatalError("`KeychainAttribute.Attribute.useDataProtectionKeychain is only available on macOS 10.15 and greater")
+        }
       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
   )

GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift

@@ -26,6 +26,7 @@ public protocol KeychainHelper {
   func password(forService service: String) throws -> String
   func passwordData(forService service: String) throws -> Data
   func removePassword(forService service: String) throws
+  func setPassword(_ password: String, forService service: String) throws
   func setPassword(_ password: String, forService service: String, accessibility: CFTypeRef) throws
   func setPassword(data: Data, forService service: String, accessibility: CFTypeRef?) throws
 }
@@ -34,11 +35,6 @@ public protocol KeychainHelper {
 final class KeychainWrapper: KeychainHelper {
   let accountName = "OAuth"
   let keychainAttributes: Set<KeychainAttribute>
-  @available(macOS 10.15, *)
-  private var isMaxMacOSVersionGreaterThanTenOneFive: Bool {
-    let tenOneFive = OperatingSystemVersion(majorVersion: 10, minorVersion: 15, patchVersion: 0)
-    return ProcessInfo().isOperatingSystemAtLeast(tenOneFive)
-  }
 
   init(keychainAttributes: Set<KeychainAttribute> = []) {
     self.keychainAttributes = keychainAttributes
@@ -54,11 +50,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
+        query[configuration.attribute.keyName] = kCFBooleanTrue
       case .accessGroup(let name):
         query[configuration.attribute.keyName] = name
       }
@@ -109,6 +101,11 @@ final class KeychainWrapper: KeychainHelper {
       throw KeychainStore.Error.failedToDeletePassword(forItemName: service)
     }
   }
+
+  func setPassword(_ password: String, forService service: String) throws {
+    let passwordData = Data(password.utf8)
+    try setPassword(data: passwordData, forService: service, accessibility: nil)
+  }
 
   func setPassword(
     _ password: String,

GTMAppAuth/Sources/KeychainStore/KeychainStore.swift

@@ -62,10 +62,11 @@ public final class KeychainStore: NSObject, AuthSessionStore {
   /// - Parameters:
   ///   - itemName: The `String` name for the credential to store in the keychain.
   ///   - keychainHelper: An instance conforming to `KeychainHelper`.
+  /// - Note: The `KeychainHelper`'s `keychainAttributes` are used if present.
   @objc public convenience init(itemName: String, keychainHelper: KeychainHelper) {
     self.init(
       itemName: itemName,
-      keychainAttributes: [],
+      keychainAttributes: keychainHelper.keychainAttributes,
       keychainHelper: keychainHelper
     )
   }
@@ -100,12 +101,7 @@ public final class KeychainStore: NSObject, AuthSessionStore {
 
   @objc(saveAuthSession:error:)
   public func save(authSession: AuthSession) throws {
-    let authSessionData: Data = try authSessionData(fromAuthSession: authSession)
-    try keychainHelper.setPassword(
-      data: authSessionData,
-      forService: itemName,
-      accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly
-    )
+    try save(authSession: authSession, withItemName: itemName)
   }
 
   /// Saves the provided `AuthSession` using the provided item name.
@@ -118,10 +114,22 @@ public final class KeychainStore: NSObject, AuthSessionStore {
   @objc(saveAuthSession:withItemName:error:)
   public func save(authSession: AuthSession, withItemName itemName: String) throws {
     let authSessionData = try authSessionData(fromAuthSession: authSession)
+
+    var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly
+    if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
+    // On macOS, we must use `kSecUseDataProtectionKeychain` if using `kSecAttrAccessible`
+    // (https://developer.apple.com/documentation/security/ksecattraccessible?language=objc)
+#if os(macOS)
+      if !keychainAttributes.contains(.useDataProtectionKeychain) {
+        maybeAccessibility = nil
+      }
+#endif
+    }
+
     try keychainHelper.setPassword(
       data: authSessionData,
       forService: itemName,
-      accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly
+      accessibility: maybeAccessibility
     )
   }
 
@@ -268,6 +276,18 @@ public final class KeychainStore: NSObject, AuthSessionStore {
         .persistenceResponseString(forAuthSession: authSession) else {
       throw KeychainStore.Error.failedToCreateResponseStringFromAuthSession(authSession)
     }
+
+    if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
+    // On macOS, we must use `kSecUseDataProtectionKeychain` if using `kSecAttrAccessible`
+    // (https://developer.apple.com/documentation/security/ksecattraccessible?language=objc)
+#if os(macOS)
+      if !keychainAttributes.contains(.useDataProtectionKeychain) {
+        try keychainHelper.setPassword(persistence, forService: itemName)
+        return
+      }
+#endif
+    }
+
     try keychainHelper.setPassword(
       persistence,
       forService: itemName,

GTMAppAuth/Tests/Helpers/GTMAppAuthTestingHelpers.swift

@@ -26,6 +26,7 @@ public protocol Testing {
 
 @objc(GTMTestingConstants)
 public class TestingConstants: NSObject {
+  @objc public static let testAccessGroup = "testAccessGroup"
   @objc public static let testAccessToken = "access_token"
   @objc public static let accessTokenExpiresIn = 3600
   @objc public static let testRefreshToken = "refresh_token"

GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift

@@ -88,6 +88,21 @@ public class KeychainHelperFake: NSObject, KeychainHelper {
     }
   }
 
+  @objc public func setPassword(_ password: String, forService service: String) throws {
+    do {
+      try removePassword(forService: service)
+    } catch KeychainStore.Error.failedToDeletePasswordBecauseItemNotFound {
+      // No need to throw this error since we are setting a new password
+    } catch {
+      throw error
+    }
+
+    guard let passwordData = password.data(using: .utf8) else {
+      throw KeychainStore.Error.unexpectedPasswordData(forItemName: service)
+    }
+    try setPassword(data: passwordData, forService: service, accessibility: nil)
+  }
+
   @objc public func setPassword(
     _ password: String,
     forService service: String,
@@ -104,7 +119,7 @@ public class KeychainHelperFake: NSObject, KeychainHelper {
     guard let passwordData = password.data(using: .utf8) else {
       throw KeychainStore.Error.unexpectedPasswordData(forItemName: service)
     }
-    try setPassword(data: passwordData, forService: service, accessibility: nil)
+    try setPassword(data: passwordData, forService: service, accessibility: accessibility)
   }
 
   @objc public func setPassword(

GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift

@@ -29,6 +29,20 @@ class GTMOAuth2CompatibilityTests: XCTestCase {
   private lazy var testPersistenceString: String = {
     return "access_token=\(TestingConstants.testAccessToken)&refresh_token=\(TestingConstants.testRefreshToken)&scope=\(TestingConstants.testScope2)&serviceProvider=\(TestingConstants.testServiceProvider)&userEmail=foo%40foo.com&userEmailIsVerified=y&userID=\(TestingConstants.testUserID)"
   }()
+  private lazy var keychainStoreWithAttributes: KeychainStore = {
+    let attributes: Set<KeychainAttribute>
+    if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) {
+      attributes = [.useDataProtectionKeychain,
+                    .keychainAccessGroup(name: TestingConstants.testAccessGroup)]
+    } else {
+      attributes = [.keychainAccessGroup(name: TestingConstants.testAccessGroup)]
+    }
+    let keychainHelperWithAttributes = KeychainHelperFake(keychainAttributes: attributes)
+    return KeychainStore(
+      itemName: TestingConstants.testKeychainItemName,
+      keychainHelper: keychainHelperWithAttributes
+    )
+  }()
   private let keychainHelper = KeychainHelperFake(keychainAttributes: [])
   private lazy var keychainStore: KeychainStore = {
     return KeychainStore(
@@ -63,6 +77,9 @@ class GTMOAuth2CompatibilityTests: XCTestCase {
 
   func testSaveOAuth2AuthSession() throws {
     try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)
+    // Save with keychain attributes to simulate macOS environment with
+    // `kSecUseDataProtectionKeychain`
+    try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)
   }
 
   func testSaveGTMOAuth2AuthSessionThrowsError() {
@@ -80,7 +97,34 @@ class GTMOAuth2CompatibilityTests: XCTestCase {
 
   func testRemoveOAuth2AuthSession() throws {
     try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)
+    let _ = try keychainStore.retrieveAuthSessionInGTMOAuth2Format(
+      tokenURL: TestingConstants.testTokenURL,
+      redirectURI: TestingConstants.testRedirectURI,
+      clientID: TestingConstants.testClientID,
+      clientSecret: TestingConstants.testClientSecret
+    )
     try keychainStore.removeAuthSession()
+    XCTAssertThrowsError(try keychainStore.retrieveAuthSession()) { thrownError in
+      XCTAssertEqual(
+        thrownError as? KeychainStore.Error,
+        KeychainStore.Error.passwordNotFound(forItemName: TestingConstants.testKeychainItemName)
+      )
+    }
+
+    try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)
+    let _ = try keychainStoreWithAttributes.retrieveAuthSessionInGTMOAuth2Format(
+      tokenURL: TestingConstants.testTokenURL,
+      redirectURI: TestingConstants.testRedirectURI,
+      clientID: TestingConstants.testClientID,
+      clientSecret: TestingConstants.testClientSecret
+    )
+    try keychainStoreWithAttributes.removeAuthSession()
+    XCTAssertThrowsError(try keychainStoreWithAttributes.retrieveAuthSession()) { thrownError in
+      XCTAssertEqual(
+        thrownError as? KeychainStore.Error,
+        KeychainStore.Error.passwordNotFound(forItemName: TestingConstants.testKeychainItemName)
+      )
+    }
   }
 
   func testRemoveOAuth2AuthSessionhrowsError() {
@@ -116,6 +160,29 @@ class GTMOAuth2CompatibilityTests: XCTestCase {
     XCTAssertEqual(authSession.userEmailIsVerified, expectedAuthSession.userEmailIsVerified)
     XCTAssertEqual(authSession.canAuthorize, expectedAuthSession.canAuthorize)
   }
+
+  func testAuthSessionFromKeychainWithAttributesForName() throws {
+    try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)
+    let authSession = try keychainStoreWithAttributes.retrieveAuthSessionInGTMOAuth2Format(
+      tokenURL: TestingConstants.testTokenURL,
+      redirectURI: TestingConstants.testRedirectURI,
+      clientID: TestingConstants.testClientID,
+      clientSecret: TestingConstants.testClientID
+    )
+
+    XCTAssertEqual(authSession.authState.scope, expectedAuthSession.authState.scope)
+    XCTAssertEqual(
+      authSession.authState.lastTokenResponse?.accessToken,
+      expectedAuthSession.authState.lastTokenResponse?.accessToken
+    )
+    XCTAssertEqual(authSession.authState.refreshToken, expectedAuthSession.authState.refreshToken)
+    XCTAssertEqual(authSession.authState.isAuthorized, expectedAuthSession.authState.isAuthorized)
+    XCTAssertEqual(authSession.serviceProvider, expectedAuthSession.serviceProvider)
+    XCTAssertEqual(authSession.userID, expectedAuthSession.userID)
+    XCTAssertEqual(authSession.userEmail, expectedAuthSession.userEmail)
+    XCTAssertEqual(authSession.userEmailIsVerified, expectedAuthSession.userEmailIsVerified)
+    XCTAssertEqual(authSession.canAuthorize, expectedAuthSession.canAuthorize)
+  }
 
   func testAuthSessionFromKeychainForNameThrowsError() throws {
     try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession)