Skip to content

Commit 200c304

Browse files
authored
fix(auth): defer keychain-sharing reconfigure during in-flight sign-in (#4231)
* fix(auth): defer keychain-sharing reconfigure during in-flight sign-in When AWSCognitoAuthPlugin is configured with a shared keychain access group, every fetchAuthSession unconditionally sent a .reconfigure event to the auth state machine. If a fetchAuthSession landed between a sign-in step that returned .confirmSignInWithCustomChallenge and the caller's confirmSignIn, the reconfigure tore down the .signingIn substate and the next confirmSignIn threw AuthError.invalidState("User is not attempting signIn operation"). Replace the unconditional reconfigure with a reconcile that: - Reads the shared keychain credentials, compares against the local state machine's authZ credentials, and skips reconfigure when they match — eliminating wasted reconfigures on every fetch. - Defers reconfigure during locally-originated in-flight flows (.signingIn, .signingOut, .deletingUser, .federatingToIdentityPool, .clearingFederation) when the keychain has no signed-in credentials. - Adopts a sibling app's sign-in by reconfiguring even during .signingIn when the remote keychain has user-pool tokens. Relax the AWSAuthConfirmSignInTask top-level guard to accept .signedIn and return .done so the pending confirmSignIn resolves cleanly. Fixes #4224. * refactor(auth): consolidate confirmSignIn state dispatch into a switch Fold the .signedIn early-return into the same switch that handles .signingIn dispatch. The listener loop already returns .done on .signedIn(.sessionEstablished), so the early return was redundant — the only thing the .signedIn branch needs to do is skip the event dispatch that's specific to the .signingIn path. * refactor(auth): inline .signedIn handling into analyzeCurrentStateAndCreateEvent and clarify keychain/state-machine fetch helper names Move the .signedIn early-return for shared-keychain adoption into analyzeCurrentStateAndCreateEvent so execute() has a single guard + single dispatch. Rename fetchRemoteCredentials and the local-state snapshot helper to fetchCredentialsFromKeychain and fetchCredentialsFromStateMachine to make the source-of-truth pairing explicit at the call site.
1 parent 633a4fd commit 200c304

6 files changed

Lines changed: 383 additions & 14 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,13 +136,13 @@ extension AWSCognitoAuthPlugin: AuthCategoryBehavior {
136136
public func fetchAuthSession(options: AuthFetchSessionRequest.Options?) async throws -> AuthSession {
137137
let options = options ?? AuthFetchSessionRequest.Options()
138138
let request = AuthFetchSessionRequest(options: options)
139-
let forceReconfigure = secureStoragePreferences?.accessGroup?.name != nil
139+
let isKeychainSharingEnabled = secureStoragePreferences?.accessGroup?.name != nil
140140
let task = AWSAuthFetchSessionTask(
141141
request,
142142
authStateMachine: authStateMachine,
143143
configuration: authConfiguration,
144144
environment: authEnvironment,
145-
forceReconfigure: forceReconfigure
145+
isKeychainSharingEnabled: isKeychainSharingEnabled
146146
)
147147
return try await taskQueue.sync {
148148
return try await task.value

AmplifyPlugins/Auth/Sources/AWSCognitoAuthPlugin/CredentialStorage/AmplifyCredentials.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,14 @@ extension AmplifyCredentials: CustomDebugStringConvertible {
5757
}
5858

5959
}
60+
61+
extension AmplifyCredentials {
62+
var hasUserPoolTokens: Bool {
63+
switch self {
64+
case .userPoolOnly, .userPoolAndIdentityPool:
65+
return true
66+
case .identityPoolOnly, .identityPoolWithFederation, .noCredentials:
67+
return false
68+
}
69+
}
70+
}

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,11 @@ class AWSAuthConfirmSignInTask: AuthConfirmSignInTask, DefaultLogger {
5252
AuthPluginErrorConstants.invalidStateError, nil
5353
)
5454

55-
guard case .configured(let authNState, _, _) = await authStateMachine.currentState,
56-
case .signingIn(let signInState) = authNState else {
55+
guard case .configured(let authNState, _, _) = await authStateMachine.currentState else {
5756
throw invalidStateError
5857
}
5958

60-
try await analyzeCurrentStateAndCreateEvent(signInState, invalidStateError)
59+
try await analyzeCurrentStateAndCreateEvent(authNState, invalidStateError)
6160

6261
let stateSequences = await authStateMachine.listen()
6362
log.verbose("Waiting for response")
@@ -91,7 +90,19 @@ class AWSAuthConfirmSignInTask: AuthConfirmSignInTask, DefaultLogger {
9190
throw invalidStateError
9291
}
9392

94-
fileprivate func analyzeCurrentStateAndCreateEvent(_ signInState: SignInState, _ invalidStateError: AuthError) async throws {
93+
fileprivate func analyzeCurrentStateAndCreateEvent(_ authNState: AuthenticationState, _ invalidStateError: AuthError) async throws {
94+
// The shared keychain reconcile may have adopted a sibling app's
95+
// sign-in while this flow was waiting for an answer. No event needs
96+
// to be dispatched; the listener loop in execute() observes
97+
// .signedIn and returns .done.
98+
if case .signedIn = authNState {
99+
return
100+
}
101+
102+
guard case .signingIn(let signInState) = authNState else {
103+
throw invalidStateError
104+
}
105+
95106
switch signInState {
96107
case .resolvingChallenge(let challengeState, let challengeType, _):
97108
// Validate if request valid MFA selection

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

Lines changed: 114 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,16 @@
77

88
import Amplify
99
import Foundation
10+
@_spi(KeychainStore) import AWSPluginsCore
1011

1112
class AWSAuthFetchSessionTask: AuthFetchSessionTask, DefaultLogger {
1213
private let request: AuthFetchSessionRequest
1314
private let authStateMachine: AuthStateMachine
1415
private let fetchAuthSessionHelper: FetchAuthSessionOperationHelper
1516
private let taskHelper: AWSAuthTaskHelper
1617
private let configuration: AuthConfiguration
17-
private let forceReconfigure: Bool
18+
private let credentialsClient: CredentialStoreStateBehavior?
19+
private let isKeychainSharingEnabled: Bool
1820

1921
var eventName: HubPayloadEventName {
2022
HubPayload.EventName.Auth.fetchSessionAPI
@@ -25,30 +27,134 @@ class AWSAuthFetchSessionTask: AuthFetchSessionTask, DefaultLogger {
2527
authStateMachine: AuthStateMachine,
2628
configuration: AuthConfiguration,
2729
environment: Environment,
28-
forceReconfigure: Bool = false
30+
isKeychainSharingEnabled: Bool = false
2931
) {
3032
self.request = request
3133
self.authStateMachine = authStateMachine
3234
self.fetchAuthSessionHelper = FetchAuthSessionOperationHelper()
3335
fetchAuthSessionHelper.environment = environment
3436
self.taskHelper = AWSAuthTaskHelper(authStateMachine: authStateMachine)
3537
self.configuration = configuration
36-
self.forceReconfigure = forceReconfigure
38+
self.credentialsClient = (environment as? AuthEnvironment)?.credentialsClient
39+
self.isKeychainSharingEnabled = isKeychainSharingEnabled
3740
}
3841

3942
func execute() async throws -> AuthSession {
4043
log.verbose("Starting execution")
41-
if forceReconfigure {
42-
log.verbose("Reconfiguring auth state machine for keychain sharing")
43-
let event = AuthEvent(eventType: .reconfigure(configuration))
44-
await authStateMachine.send(event)
45-
}
4644
await taskHelper.didStateMachineConfigured()
45+
if isKeychainSharingEnabled {
46+
await reconcileWithSharedKeychainIfNeeded()
47+
}
4748
let doesNeedForceRefresh = request.options.forceRefresh
4849
return try await fetchAuthSessionHelper.fetch(
4950
authStateMachine,
5051
forceRefresh: doesNeedForceRefresh
5152
)
5253
}
5354

55+
/// When the plugin is configured with a shared keychain access group, the
56+
/// keychain is the source of truth across processes. Reconcile the local
57+
/// state machine with whatever is currently in the keychain — but only
58+
/// when doing so won't destroy locally-originated in-flight work.
59+
///
60+
/// Decision matrix (auth state vs. remote-vs-local credentials):
61+
/// - quiescent (.signedIn / .signedOut / .error / .configured / .notConfigured)
62+
/// - differ: reconfigure (sibling wrote — pick it up)
63+
/// - match: no-op
64+
/// - .signingIn
65+
/// - remote has user-pool tokens: reconfigure (adopt sibling's sign-in;
66+
/// confirmSignIn will resolve to .done)
67+
/// - remote has no user-pool tokens: defer (sign-in flow finishes;
68+
/// last-writer-wins on the keychain)
69+
/// - .signingOut / .deletingUser / .federatingToIdentityPool /
70+
/// .clearingFederation: defer (in-flight side effects must run; same end
71+
/// state is reached by the local flow)
72+
func reconcileWithSharedKeychainIfNeeded() async {
73+
guard let keychainCredentials = await fetchCredentialsFromKeychain() else {
74+
return
75+
}
76+
guard case .configured(let authNState, let authZState, _) = await authStateMachine.currentState else {
77+
return
78+
}
79+
let stateMachineCredentials = fetchCredentialsFromStateMachine(authZState)
80+
if let stateMachineCredentials, stateMachineCredentials == keychainCredentials {
81+
return
82+
}
83+
84+
if shouldDeferReconcile(authNState: authNState, remote: keychainCredentials) {
85+
log.verbose("Deferring keychain reconcile while auth flow is in progress")
86+
return
87+
}
88+
89+
log.verbose("Reconfiguring auth state machine for keychain sharing")
90+
let event = AuthEvent(eventType: .reconfigure(configuration))
91+
await authStateMachine.send(event)
92+
await taskHelper.didStateMachineConfigured()
93+
}
94+
95+
private func fetchCredentialsFromKeychain() async -> AmplifyCredentials? {
96+
do {
97+
let data = try await credentialsClient?.fetchData(type: .amplifyCredentials)
98+
if case .amplifyCredentials(let credentials) = data {
99+
return credentials
100+
}
101+
return nil
102+
} catch KeychainStoreError.itemNotFound {
103+
return .noCredentials
104+
} catch {
105+
log.verbose("Could not read shared keychain credentials: \(error)")
106+
return nil
107+
}
108+
}
109+
110+
/// Best-effort snapshot of the credentials the local state machine last
111+
/// observed. Returns nil for transient authZ states where we can't make a
112+
/// reliable comparison; in those cases callers fall back to deferring (the
113+
/// transient state will resolve shortly and a later fetch will reconcile).
114+
private func fetchCredentialsFromStateMachine(_ authZState: AuthorizationState) -> AmplifyCredentials? {
115+
switch authZState {
116+
case .sessionEstablished(let credentials),
117+
.storingCredentials(let credentials):
118+
return credentials
119+
case .refreshingSession(existingCredentials: let credentials, _):
120+
return credentials
121+
case .federatingToIdentityPool(_, _, existingCredentials: let credentials):
122+
return credentials
123+
case .signingOut(let credentials):
124+
return credentials ?? .noCredentials
125+
case .configured:
126+
return .noCredentials
127+
case .notConfigured,
128+
.clearingFederation,
129+
.fetchingUnAuthSession,
130+
.fetchingAuthSessionWithUserPool,
131+
.deletingUser,
132+
.error:
133+
return nil
134+
}
135+
}
136+
137+
private func shouldDeferReconcile(
138+
authNState: AuthenticationState,
139+
remote: AmplifyCredentials
140+
) -> Bool {
141+
switch authNState {
142+
case .signingIn:
143+
// Adopt sibling sign-in; otherwise defer until local flow completes
144+
return !remote.hasUserPoolTokens
145+
case .signingOut,
146+
.deletingUser,
147+
.federatingToIdentityPool,
148+
.clearingFederation:
149+
return true
150+
case .notConfigured,
151+
.configured,
152+
.signedIn,
153+
.signedOut,
154+
.federatedToIdentityPool,
155+
.error:
156+
return false
157+
}
158+
}
159+
54160
}

0 commit comments

Comments
 (0)