feat(auth): Keychain Sharing (No App Reload Required) #3811

yaroluchko · 2024-08-12T23:27:42Z

Issue #

Description

This allows Amplify Swift developers to set the access group they would like the auth session to be shared on. This PR, as opposed to this PR, does not require the other app with which the auth session is being shared with to be reloaded. This helps immensely when writing a product with app extensions.

Changes made

AWSCognitoCredentialAuthCredentialStore now uses access group to create keychain instance with said access group.
If an access group is specified:

a different keychain service is used, specifically for shared items.
fetchAuthSession reconfigures the plugin when called, so that app reload is not required

Migration:

Amplify developers can specify if they want migration to happen by setting the migrateKeychainItemsOfUserSession to true within the accessGroup struct
migration involves moving any auth keychain items from the old access group (old access group name is stored in UserDefaults) to the new access group
migration is a batched operation, and the items will no longer be accessible from the old access group

Usage

Migrating to a Shared Keychain

To use a shared keychain:

In Xcode, go to Project Settings → Signing & Capabilities
Click +Capability
Add Keychain Sharing capability
Add a keychain group
Repeat for all apps for which you want to share auth state, adding the same keychain group for all of them

To move to the shared keychain using this new keychain access group, specify the accessGroup parameter when instantiating the AWSCognitoAuthPlugin. If a user is currently signed-in, they will be logged out when first using the access group:

let accessGroup = AccessGroup(name: "\(teamID).com.example.sharedItems")
let secureStoragePreferences = AWSCognitoSecureStoragePreferences(
  accessGroup: accessGroup)
try Amplify.add(
  plugin: AWSCognitoAuthPlugin(
    secureStoragePreferences: secureStoragePreferences))
try Amplify.configure()

If you would prefer the user session to be migrated (which will allow the user to continue to be signed-in), then specify the migrateKeychainItemsOfUserSession boolean in the AccessGroup struct to be true like so:

let accessGroup = AccessGroup.none(migrateKeychainItemsOfUserSession: true)
let secureStoragePreferences = AWSCognitoSecureStoragePreferences(
  accessGroup: accessGroup)
try Amplify.add(
  plugin: AWSCognitoAuthPlugin(
    secureStoragePreferences: secureStoragePreferences))
try Amplify.configure()

Sign in a user with any sign-in method within one app that uses this access group. After reloading another app that uses this access group, the user will be signed in. Likewise, signing out of one app will sign out the other app after reloading it.

Migrating to another Shared Keychain

To move to a different access group, update the name parameter of the AccessGroup to be the new access group. Set migrateKeychainItemsOfUserSession to true to migrate an existing user session under the previously used access group.

Migrating from a Shared Keychain

If you’d like to stop sharing state between this app and other apps, you can set the access group to be AccessGroup.none or AccessGroup.none(migrateKeychainItemsOfUserSession: true) if you’d like the session to be migrated.

General Checklist

Added new tests to cover change, if needed
Build succeeds with all target using Swift Package Manager
All unit tests pass
All integration tests pass
Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
Documentation update for the change if required
PR title conforms to conventional commit style
New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-08-12T23:34:16Z

API Breakage Report

💔 Public API Breaking Change detected:

Module: Amplify
Constructor AuthFetchSessionRequest.init(options:) has been removed

Module: AWSPluginsCore
Func KeychainStoreBehavior._getAll() has been added as a protocol requirement

* Remove migrateKeychainItemsOfUserSession bool from SecureStoragePreferences

github-actions · 2024-08-13T00:02:43Z

API Breakage Report

💔 Public API Breaking Change detected:

Module: AWSPluginsCore
Func KeychainStoreBehavior._getAll() has been added as a protocol requirement

github-actions · 2024-08-13T00:08:36Z

API Breakage Report

💔 Public API Breaking Change detected:

Module: AWSPluginsCore
Func KeychainStoreBehavior._getAll() has been added as a protocol requirement

github-actions · 2024-08-13T00:13:21Z

API Breakage Report

💔 Public API Breaking Change detected:

Module: AWSPluginsCore
Func KeychainStoreBehavior._getAll() has been added as a protocol requirement

Amplify/Categories/Auth/Models/AccessGroup.swift

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/AWSCognitoAuthPlugin+Configure.swift

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/AWSCognitoAuthPlugin.swift

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Task/AWSAuthFetchSessionTask.swift

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

...sts/AuthHostApp/AuthIntegrationTests/CredentialStore/CredentialStoreConfigurationTests.swift

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift

Amplify/Categories/Auth/Models/AccessGroup.swift

github-actions · 2024-08-16T06:05:20Z

API Breakage Report

💔 Public API Breaking Change detected:

Module: AWSPluginsCore
Func KeychainStoreBehavior._getAll() has been added as a protocol requirement

thisisabhash

LGTM

harsh62 · 2024-08-20T19:10:18Z

Amplify/Categories/Auth/Models/AccessGroup.swift

+import Foundation
+
+public struct AccessGroup {
+    public let name: String?


Can we add doc comments explaining what is the intent of the variable, this comment applies to anything that is being made public here.

Example:
(Its just an example, please make modifications as you feel are correct)

/// A structure representing an access group for managing keychain items. public struct AccessGroup { /// The name of the access group. public let name: String? /// A flag indicating whether to migrate keychain items. public let migrateKeychainItems: Bool /** Initializes an `AccessGroup` with the specified name and migration option. - Parameter name: The name of the access group. - Parameter migrateKeychainItemsOfUserSession: A flag indicating whether to migrate keychain items. Defaults to `false`. */ public init(name: String, migrateKeychainItemsOfUserSession: Bool = false) { self.init(name: name, migrateKeychainItems: migrateKeychainItemsOfUserSession) } /** Creates an `AccessGroup` instance with no specified name. - Parameter migrateKeychainItemsOfUserSession: A flag indicating whether to migrate keychain items. - Returns: An `AccessGroup` instance with the migration option set. */ public static func none(migrateKeychainItemsOfUserSession: Bool) -> AccessGroup { return .init(migrateKeychainItems: migrateKeychainItemsOfUserSession) } /** A static property representing an `AccessGroup` with no name and no migration. - Returns: An `AccessGroup` instance with no name and the migration option set to `false`. */ public static var none: AccessGroup { return .none(migrateKeychainItemsOfUserSession: false) } }

harsh62 · 2024-08-20T19:33:13Z

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

@@ -25,14 +26,33 @@ struct AWSCognitoAuthCredentialStore {
    private var isKeychainConfiguredKey: String {
        "\(userDefaultsNameSpace).isKeychainConfigured"
    }
+    private var accessGroupKey: String {


Can you add a small comment explaining the usage of the accessGroupKey like for isKeychainConfiguredKey?

harsh62 · 2024-08-20T19:41:40Z

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

+        return userDefaults.string(forKey: accessGroupKey)
+    }
+
+    private func saveStoredAccessGroup() throws {


The method is not throwing, so can be renamed without throws.

Suggested change

private func saveStoredAccessGroup() throws {

private func saveStoredAccessGroup() {

harsh62 · 2024-08-20T19:43:06Z

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

@@ -182,6 +202,62 @@ extension AWSCognitoAuthCredentialStore: AmplifyAuthCredentialStoreBehavior {
    private func clearAllCredentials() throws {
        try keychain._removeAll()
    }
+
+    private func retrieveStoredAccessGroup() throws -> String? {


The method is not throwing, so can be renamed without throws.

Suggested change

private func retrieveStoredAccessGroup() throws -> String? {

private func retrieveStoredAccessGroup() -> String? {

harsh62 · 2024-08-20T19:48:43Z

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

+        let oldKeychain: KeychainStoreBehavior
+
+        if oldAccessGroup == accessGroup {
+            log.verbose("[AWSCognitoAuthCredentialStore] Stored access group is the same as current access group, aborting migration")


Rather than verbose, I think this log statement should be of type info.

harsh62 · 2024-08-20T20:02:16Z

...ifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/Models/AWSCognitoSecureStoragePreferences.swift

+import Foundation
+import Amplify
+
+public struct AWSCognitoSecureStoragePreferences {


nit: doc comments.

harsh62 · 2024-08-20T20:09:00Z

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift

+    @_spi(KeychainStore)
+    /// Retrieves all key-value pairs in the keychain
+    /// This System Programming Interface (SPI) may have breaking changes in future updates.
+    func _getAll() throws -> [(key: String, value: Data)]


Why does this need to return a tuple rather than a dictionary? Shouldn't it be something like:

Suggested change

func _getAll() throws -> [(key: String, value: Data)]

func _getAll() throws -> [[String: Data]]

With KeychainStoreMigrator in place, the _getAll() function is no longer used anywhere. I forgot to remove it. If we go back to using _getAll() by the end of the review I will update this to be a dictionary instead.

harsh62 · 2024-08-20T20:09:47Z

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStore.swift

+        query[Constants.MatchLimit] = Constants.MatchLimitAll
+        query[Constants.ReturnData] = kCFBooleanTrue
+        query[Constants.ReturnAttributes] = kCFBooleanTrue
+        query[Constants.ReturnRef] = kCFBooleanTrue


What does ReturnRef exactly do and why do we need it?

harsh62 · 2024-08-20T20:14:01Z

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift

+        // Remove any current items to avoid duplicate item error
+        try? KeychainStore(service: newAttributes.service, accessGroup: newAttributes.accessGroup)._removeAll()


The usage of _removeAll is really necessary here? It seems very dangerous, if for some reason this logic runs in a scenario where we don't want it to.

I placed it here as any current items in the keychain of the access group we are migrating to will cause the batch update to fail. I could, instead, only call _removeAll when necessary (i.e only when a duplicate item error occurs). It just makes the code a little more nested. Let me know what you think.

harsh62 · 2024-08-20T20:16:14Z

AmplifyPlugins/Core/AWSPluginsCore/Keychain/KeychainStoreMigrator.swift

+extension KeychainStoreMigrator: DefaultLogger {
+    public static var log: Logger {
+        Amplify.Logging.logger(forNamespace: String(describing: self))
+    }
+
+    public nonisolated var log: Logger { Self.log }
+}


Just double check, I don't think you need to define these methods again, just the following should be enough.

Suggested change

extension KeychainStoreMigrator: DefaultLogger {

public static var log: Logger {

Amplify.Logging.logger(forNamespace: String(describing: self))

}

public nonisolated var log: Logger { Self.log }

}

extension KeychainStoreMigrator: DefaultLogger { }

harsh62 · 2024-08-22T01:28:57Z

AmplifyPlugins/Auth/Tests/AuthHostApp/AuthIntegrationTests/AWSAuthBaseTest.swift

+    let keychainAccessGroupWatch = "W3DRXD72QU.com.amazon.aws.amplify.swift.AuthWatchAppShared"
+    let keychainAccessGroupWatch2 = "W3DRXD72QU.com.amazon.aws.amplify.swift.AuthWatchAppShared2"


Does the team id need to be part of the group?

harsh62 · 2024-08-22T01:36:08Z

...gins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AWSCognitoAuthCredentialStore.swift

+        guard awsCredential.areValid() else {
+            log.verbose("[AWSCognitoAuthCredentialStore] Credentials found are not valid (expired) in old access group keychain, aborting migration")
+            return
+        }


I think this check might no be accurate, as the areValid method doesn't validate the refresh token.. The refresh token might still be valid.. So I think we should skip this check, and if its not valid, the fetchAuthSession will fail, which is acceptable I think.

That makes sense. I'm okay with removing this part.

AmplifyPlugins/Auth/Tests/AuthHostApp/AuthWatchApp.entitlements

…check, only delete items if absolutely necessary

codecov · 2024-08-22T03:09:42Z

Codecov Report

Attention: Patch coverage is 39.60396% with 61 lines in your changes missing coverage. Please review.

Project coverage is 68.47%. Comparing base (acb7dee) to head (01e2268).
Report is 14 commits behind head on main.

Files	Patch %	Lines
...WSPluginsCore/Keychain/KeychainStoreMigrator.swift	0.00%	28 Missing ⚠️
...dentialStorage/AWSCognitoAuthCredentialStore.swift	60.00%	14 Missing ⚠️
Amplify/Categories/Auth/Models/AccessGroup.swift	0.00%	9 Missing ⚠️
...s/Core/AWSPluginsCore/Keychain/KeychainStore.swift	0.00%	7 Missing ⚠️
...gnitoAuthPlugin/Task/AWSAuthFetchSessionTask.swift	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3811      +/-   ##
==========================================
- Coverage   68.54%   68.47%   -0.08%     
==========================================
  Files        1080     1083       +3     
  Lines       37608    37695      +87     
==========================================
+ Hits        25778    25810      +32     
- Misses      11830    11885      +55

Flag	Coverage Δ
API_plugin_unit_test	`68.96% <ø> (ø)`
AWSPluginsCore	`67.61% <2.77%> (-0.95%)`	⬇️
Amplify	`47.51% <0.00%> (-0.05%)`	⬇️
Analytics_plugin_unit_test	`84.52% <ø> (ø)`
Auth_plugin_unit_test	`79.69% <69.64%> (-0.07%)`	⬇️
DataStore_plugin_unit_test	`81.24% <ø> (-0.03%)`	⬇️
Geo_plugin_unit_test	`72.00% <ø> (ø)`
Logging_plugin_unit_test	`62.95% <ø> (ø)`
Predictions_plugin_unit_test	`37.32% <ø> (ø)`
PushNotifications_plugin_unit_test	`86.21% <ø> (ø)`
Storage_plugin_unit_test	`76.81% <ø> (ø)`
unit_tests	`68.47% <39.60%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yaroluchko force-pushed the yaluchko/auth-keychain-sharing-no-reload branch from 9473eb0 to c5cb449 Compare August 12, 2024 23:57

yaroluchko temporarily deployed to Fortify August 12, 2024 23:57 — with GitHub Actions Inactive

feat(Auth) Keychain Sharing (App Reload Required)

09f0583

* Remove migrateKeychainItemsOfUserSession bool from SecureStoragePreferences

Reconfigure when fetching auth session if sharing keychain

8dd74ed

yaroluchko force-pushed the yaluchko/auth-keychain-sharing-no-reload branch from e98f334 to 8dd74ed Compare August 13, 2024 00:03

yaroluchko temporarily deployed to Fortify August 13, 2024 00:03 — with GitHub Actions Inactive

yaroluchko closed this Aug 13, 2024

yaroluchko reopened this Aug 13, 2024

yaroluchko temporarily deployed to IntegrationTest August 13, 2024 00:07 — with GitHub Actions Inactive

Update API dumps for new version

3dfa43b

thisisabhash reviewed Aug 15, 2024

View reviewed changes

Indentation, clean up, and batch migration to avoid inconsistent state

f5c01b1

yaroluchko temporarily deployed to Fortify August 16, 2024 06:00 — with GitHub Actions Inactive

Update API dumps for new version

a999a54

thisisabhash previously approved these changes Aug 16, 2024

View reviewed changes

yaroluchko temporarily deployed to IntegrationTest August 16, 2024 21:54 — with GitHub Actions Inactive

yaroluchko temporarily deployed to IntegrationTest August 16, 2024 21:55 — with GitHub Actions Inactive

yaroluchko marked this pull request as ready for review August 20, 2024 18:35

yaroluchko requested review from a team as code owners August 20, 2024 18:35

harsh62 changed the title ~~feat(auth) - Keychain Sharing (No App Reload Required)~~ feat(auth): Keychain Sharing (No App Reload Required) Aug 20, 2024

harsh62 changed the title ~~feat(auth): Keychain Sharing (No App Reload Required)~~ feat(Auth): Keychain Sharing (No App Reload Required) Aug 20, 2024

yaroluchko mentioned this pull request Aug 20, 2024

Enable Keychain Sharing for App Extensions #2508

Open

harsh62 reviewed Aug 20, 2024

View reviewed changes

harsh62 changed the title ~~feat(Auth): Keychain Sharing (No App Reload Required)~~ feat(auth): Keychain Sharing (No App Reload Required) Aug 22, 2024

harsh62 requested changes Aug 22, 2024

View reviewed changes

Addressing review comments: documentation, no more credentials valid …

26fb82c

…check, only delete items if absolutely necessary

yaroluchko dismissed thisisabhash’s stale review via 26fb82c August 22, 2024 02:46

yaroluchko had a problem deploying to Fortify August 22, 2024 02:47 — with GitHub Actions Error

Style fixes

01e2268

yaroluchko temporarily deployed to Fortify August 22, 2024 02:52 — with GitHub Actions Inactive

yaroluchko requested review from harsh62 and thisisabhash August 22, 2024 02:53

yaroluchko temporarily deployed to IntegrationTest August 22, 2024 02:54 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Keychain Sharing (No App Reload Required) #3811

feat(auth): Keychain Sharing (No App Reload Required) #3811

yaroluchko commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 16, 2024

thisisabhash left a comment

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

yaroluchko Aug 21, 2024

harsh62 Aug 20, 2024

harsh62 Aug 20, 2024

yaroluchko Aug 21, 2024 •

edited

Loading

harsh62 Aug 20, 2024

harsh62 Aug 22, 2024

yaroluchko Aug 22, 2024

harsh62 Aug 22, 2024

yaroluchko Aug 22, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

	private func saveStoredAccessGroup() throws {
	private func saveStoredAccessGroup() {

	private func retrieveStoredAccessGroup() throws -> String? {
	private func retrieveStoredAccessGroup() -> String? {

	func _getAll() throws -> [(key: String, value: Data)]
	func _getAll() throws -> [[String: Data]]

		// Remove any current items to avoid duplicate item error
		try? KeychainStore(service: newAttributes.service, accessGroup: newAttributes.accessGroup)._removeAll()

		let keychainAccessGroupWatch = "W3DRXD72QU.com.amazon.aws.amplify.swift.AuthWatchAppShared"
		let keychainAccessGroupWatch2 = "W3DRXD72QU.com.amazon.aws.amplify.swift.AuthWatchAppShared2"

feat(auth): Keychain Sharing (No App Reload Required) #3811

Are you sure you want to change the base?

feat(auth): Keychain Sharing (No App Reload Required) #3811

Conversation

yaroluchko commented Aug 12, 2024 • edited Loading

Issue #

Description

Changes made

Usage

Migrating to a Shared Keychain

Migrating to another Shared Keychain

Migrating from a Shared Keychain

General Checklist

github-actions bot commented Aug 12, 2024

API Breakage Report

💔 Public API Breaking Change detected:

github-actions bot commented Aug 13, 2024

API Breakage Report

💔 Public API Breaking Change detected:

github-actions bot commented Aug 13, 2024

API Breakage Report

💔 Public API Breaking Change detected:

github-actions bot commented Aug 13, 2024

API Breakage Report

💔 Public API Breaking Change detected:

github-actions bot commented Aug 16, 2024

API Breakage Report

💔 Public API Breaking Change detected:

thisisabhash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroluchko Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

yaroluchko commented Aug 12, 2024 •

edited

Loading

yaroluchko Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading