-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): Keychain Sharing (No App Reload Required) #3811
base: main
Are you sure you want to change the base?
Changes from 5 commits
09f0583
8dd74ed
3dfa43b
f5c01b1
a999a54
26fb82c
01e2268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// | ||
// Copyright Amazon.com Inc. or its affiliates. | ||
// All Rights Reserved. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
|
||
import Foundation | ||
|
||
public struct AccessGroup { | ||
public let name: String? | ||
public let migrateKeychainItems: Bool | ||
|
||
public init(name: String, migrateKeychainItemsOfUserSession: Bool = false) { | ||
self.init(name: name, migrateKeychainItems: migrateKeychainItemsOfUserSession) | ||
thisisabhash marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public static func none(migrateKeychainItemsOfUserSession: Bool) -> AccessGroup { | ||
return .init(migrateKeychainItems: migrateKeychainItemsOfUserSession) | ||
} | ||
|
||
public static var none: AccessGroup { | ||
return .none(migrateKeychainItemsOfUserSession: false) | ||
} | ||
|
||
private init(name: String? = nil, migrateKeychainItems: Bool) { | ||
self.name = name | ||
self.migrateKeychainItems = migrateKeychainItems | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ struct AWSCognitoAuthCredentialStore { | |||||
|
||||||
// Credential store constants | ||||||
private let service = "com.amplify.awsCognitoAuthPlugin" | ||||||
private let sharedService = "com.amplify.awsCognitoAuthPluginShared" | ||||||
private let sessionKey = "session" | ||||||
private let deviceMetadataKey = "deviceMetadata" | ||||||
private let deviceASFKey = "deviceASF" | ||||||
|
@@ -25,14 +26,33 @@ struct AWSCognitoAuthCredentialStore { | |||||
private var isKeychainConfiguredKey: String { | ||||||
"\(userDefaultsNameSpace).isKeychainConfigured" | ||||||
} | ||||||
private var accessGroupKey: String { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a small comment explaining the usage of the |
||||||
"\(userDefaultsNameSpace).accessGroup" | ||||||
} | ||||||
|
||||||
private let authConfiguration: AuthConfiguration | ||||||
private let keychain: KeychainStoreBehavior | ||||||
private let userDefaults = UserDefaults.standard | ||||||
private let accessGroup: String? | ||||||
|
||||||
init(authConfiguration: AuthConfiguration, accessGroup: String? = nil) { | ||||||
init( | ||||||
authConfiguration: AuthConfiguration, | ||||||
accessGroup: String? = nil, | ||||||
migrateKeychainItemsOfUserSession: Bool = false | ||||||
) { | ||||||
self.authConfiguration = authConfiguration | ||||||
self.keychain = KeychainStore(service: service, accessGroup: accessGroup) | ||||||
self.accessGroup = accessGroup | ||||||
if let accessGroup { | ||||||
self.keychain = KeychainStore(service: sharedService, accessGroup: accessGroup) | ||||||
} else { | ||||||
self.keychain = KeychainStore(service: service) | ||||||
} | ||||||
|
||||||
if migrateKeychainItemsOfUserSession { | ||||||
try? migrateKeychainItemsToAccessGroup() | ||||||
} | ||||||
|
||||||
try? saveStoredAccessGroup() | ||||||
|
||||||
if !userDefaults.bool(forKey: isKeychainConfiguredKey) { | ||||||
try? clearAllCredentials() | ||||||
|
@@ -182,6 +202,62 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior { | |||||
private func clearAllCredentials() throws { | ||||||
try keychain._removeAll() | ||||||
} | ||||||
|
||||||
private func retrieveStoredAccessGroup() throws -> String? { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method is not throwing, so can be renamed without throws.
Suggested change
|
||||||
return userDefaults.string(forKey: accessGroupKey) | ||||||
} | ||||||
|
||||||
private func saveStoredAccessGroup() throws { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method is not throwing, so can be renamed without throws.
Suggested change
|
||||||
if let accessGroup { | ||||||
userDefaults.set(accessGroup, forKey: accessGroupKey) | ||||||
} else { | ||||||
userDefaults.removeObject(forKey: accessGroupKey) | ||||||
} | ||||||
} | ||||||
|
||||||
private func migrateKeychainItemsToAccessGroup() throws { | ||||||
let oldAccessGroup = try? retrieveStoredAccessGroup() | ||||||
let oldKeychain: KeychainStoreBehavior | ||||||
|
||||||
if oldAccessGroup == accessGroup { | ||||||
log.verbose("[AWSCognitoAuthCredentialStore] Stored access group is the same as current access group, aborting migration") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than verbose, I think this log statement should be of type |
||||||
return | ||||||
} | ||||||
|
||||||
if let oldAccessGroup { | ||||||
oldKeychain = KeychainStore(service: sharedService, accessGroup: oldAccessGroup) | ||||||
} else { | ||||||
oldKeychain = KeychainStore(service: service) | ||||||
} | ||||||
|
||||||
let authCredentialStoreKey = generateSessionKey(for: authConfiguration) | ||||||
let authCredentialData: Data | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the data need to be declared outside the |
||||||
let awsCredential: AmplifyCredentials | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
do { | ||||||
authCredentialData = try oldKeychain._getData(authCredentialStoreKey) | ||||||
awsCredential = try decode(data: authCredentialData) | ||||||
} catch { | ||||||
log.verbose("[AWSCognitoAuthCredentialStore] Could not retrieve previous credentials in keychain under old access group, nothing to migrate") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log should be of type |
||||||
return | ||||||
} | ||||||
|
||||||
guard awsCredential.areValid() else { | ||||||
log.verbose("[AWSCognitoAuthCredentialStore] Credentials found are not valid (expired) in old access group keychain, aborting migration") | ||||||
return | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this check might no be accurate, as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. I'm okay with removing this part. |
||||||
|
||||||
let oldService = oldAccessGroup != nil ? sharedService : service | ||||||
let newService = accessGroup != nil ? sharedService : service | ||||||
|
||||||
do { | ||||||
thisisabhash marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
try KeychainStoreMigrator(oldService: oldService, newService: newService, oldAccessGroup: oldAccessGroup, newAccessGroup: accessGroup).migrate() | ||||||
} catch { | ||||||
log.error("[AWSCognitoAuthCredentialStore] Migration has failed") | ||||||
return | ||||||
} | ||||||
|
||||||
log.verbose("[AWSCognitoAuthCredentialStore] Migration of keychain items from old access group to new access group successful") | ||||||
} | ||||||
|
||||||
} | ||||||
|
||||||
|
@@ -205,3 +281,11 @@ private extension AWSCognitoAuthCredentialStore { | |||||
} | ||||||
|
||||||
} | ||||||
|
||||||
extension AWSCognitoAuthCredentialStore: DefaultLogger { | ||||||
public static var log: Logger { | ||||||
Amplify.Logging.logger(forNamespace: String(describing: self)) | ||||||
} | ||||||
|
||||||
public nonisolated var log: Logger { Self.log } | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// | ||
// Copyright Amazon.com Inc. or its affiliates. | ||
// All Rights Reserved. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
|
||
import Foundation | ||
import Amplify | ||
|
||
public struct AWSCognitoSecureStoragePreferences { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: doc comments. |
||
|
||
/// The access group that the keychain will use for auth items | ||
public let accessGroup: AccessGroup? | ||
|
||
public init(accessGroup: AccessGroup? = nil) { | ||
self.accessGroup = accessGroup | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add doc comments explaining what is the intent of the variable, this comment applies to anything that is being made public here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
(Its just an example, please make modifications as you feel are correct)