Repository navigation
fix(auth): AmplifyCognitoClient follow-ups from the beta review - #4401
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4401 +/- ##
===========================================
+ Coverage 75.71% 86.87% +11.16%
===========================================
Files 1360 565 -795
Lines 57392 28128 -29264
===========================================
- Hits 43452 24437 -19015
+ Misses 13940 3691 -10249
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| launch after an update, holding a guest in memory, can write that guest back over the login the app's | ||
| access-group migration just moved when it next refreshes: relaunch the extension after an update. | ||
| reads it at every `fetchAuthSession`. An extension on the Auth plugin that holds a guest in memory while the app's | ||
| access-group migration moves its login can write that guest back over it only from a guest refresh already running |
There was a problem hiding this comment.
The extension write-back note now says "only" a guest refresh already running, but the plugin's user-pool calls start one too (low)
README.md:1887-1889 says an extension on the Auth plugin that holds a guest in memory while the app's access-group migration moves its login "can write that guest back over it only from a guest refresh already running at that moment". The plugin re-reads the shared keychain only in AWSAuthFetchSessionTask (AWSAuthFetchSessionTask.swift:47-48). Its 18 tasks that need an access token go through AWSAuthTaskHelper.getAccessToken() (AWSAuthTaskHelper.swift:74-77), which calls FetchAuthSessionOperationHelper.fetch directly. For a guest whose AWS credentials need refreshing, refreshIfRequired sends .refreshSession (FetchAuthSessionOperationHelper.swift:80-89), and the refreshed guest is saved under the session key with no check, over whatever the migration moved there.
Scenario: a widget on the plugin, in the app's access group, fetched a guest before the app's first launch after the update, and the guest's AWS credentials have since expired. The app's migration moves the user's login into the shared service. The widget then calls Amplify.Auth.fetchUserAttributes() (AWSAuthFetchUserAttributeTask.swift:38). Nothing re-reads the keychain first, so the guest is refreshed and saved over the user's login, and the call throws signedOut. The app then reads the guest in the user's place.
This is the second case this #4349 thread named; 1387c86 kept only the first and added "only". I'd name both: a guest refresh already running while the app migrates, and one that a user-pool call (fetchUserAttributes, the attribute, device, MFA, TOTP and WebAuthn calls, updatePassword, deleteUser) starts once the guest's AWS credentials need refreshing, since those calls don't re-read the shared keychain. 0a0e015 dropped the write-back caveat from RELEASE-NOTES-PENDING.md:36, the row plugin-only apps read, so the narrowed sentence could go back there too.
There was a problem hiding this comment.
Thanks. The README now names both cases: a guest refresh already running while the app migrates, and a refresh that one of the extension's user-pool calls starts once the guest's AWS credentials need refreshing (it lists those calls), since those calls don't re-read the shared keychain first (b787ff7). The access-group row describes 2.63.0, so it left the pending file; the 2.63.0 release notes carry the write-back caveat again, naming both cases.
There was a problem hiding this comment.
Thanks. Both cases are named now: README.md:1903-1907, whose call list matches the 18 AWSAuthTaskHelper.getAccessToken() callers, and the 2.63.0 release notes' "no longer strands a login behind a guest" entry. Putting the caveat there rather than in the pending file is right, since that fix shipped in 2.63.0 and this fetch path hasn't changed since.
Three things are left in that sentence:
- b787ff7 brings back "Relaunch the extension after an update." (README.md:1907, and the same sentence in the release notes). The feat(auth): AmplifyCognitoClient beta, a standalone multi-session Cognito client #4349 thread asked to drop it, and ab2b040 (1387c86 before the force-push) did, because it can't be followed: an app can't relaunch its extensions, and the extension holding the guest already started after the update, before the app's first launch. I'd drop it in both places, or tell extensions what they can do instead: call
fetchAuthSession()right before those calls. It re-reads the shared keychain first (AWSAuthFetchSessionTask.swift:47-48) and never defers for a guest (.signedOut,:155-158), so it adopts a login moved before it. If none has moved, it refreshes the guest itself when needed, so the call after it starts no refresh. That closes the second case; the first stays. No test inAWSAuthFetchSessionTaskKeychainSharingTestsstarts from.signedOutyet, so if the README gives that advice, one there would pin it. updatePassword(README.md:1906) was my wording, but no method has that name. The plugin's isupdate(oldPassword:to:)(AWSCognitoAuthPlugin+UserBehavior.swift:98), the same name README.md:1181 uses for the client's.- Those calls also write over the moved login from an extension that holds no session yet: one that configured before the app migrated, found the shared service empty and hasn't fetched since. From authorization
.configured,fetchsends.fetchUnAuthSession(FetchAuthSessionOperationHelper.swift:34-40). The new guest is then saved under the session key with no check (AuthorizationState+Resolver.swift:183-197,PersistCredentials.swift:25), on the extension's first such call, with no expiry needed. "holds a guest, or no session yet," with "a guest fetch or refresh" in the second case, would cover it. CallingfetchAuthSession()first closes this one too: the re-read counts.configuredas no credentials (AWSAuthFetchSessionTask.swift:127-128), so the moved login differs and is adopted.
| /// in memory, set when a refresh in this launch found the refresh token dead. After a relaunch, until a refresh | ||
| /// finds it again, a closed page keeps the session signed in. | ||
| private nonisolated func afterClosedLogout(_ payload: Data, global: Bool, _ error: AuthClientError) async throws -> EngineSignOutOutcome { | ||
| guard await isExpired else { |
There was a problem hiding this comment.
A closed logout page can revoke and sign out a new login another process saved after this core found the old one expired (low)
afterClosedLogout decides on this core's isExpired alone (:237), but the payload it then revokes and signs out is whatever SessionSignOut read from the store (SessionSignOut.swift:161, :195). signOut takes the gate without rereadIfExpired() (SessionCore+Records.swift:137-161), and only apply clears the flag (SessionCore.swift:199-201). So a login another writer saved after this core marked the old one expired inherits that expiry when the page is closed.
Scenario: apps A and B share work through an access group. In A, a user signed in through the hosted UI with shared cookies, and their refresh token was later revoked; A's fetchAuthSession() reports sessionExpired, so A's isExpired is set. B signs work out and back in through the hosted UI with shared cookies, saving a new login Q. Before any fetch or provider call in A, the user taps Sign out in A (signOut(presentationAnchor:)) and closes the page. A revokes Q's refresh token, which also ends B's session, signs work out on the device, and returns .partial with a hostedUIError saying the session's sign-in had expired. Per README.md:1385-1388 and this function's own reason ("its tokens are dead anyway", SessionCore+WebUI.swift:231), it should return .failed(.userCancelled) and keep Q.
Main has the same check, so this isn't a regression, but this PR reworked the decision and its docs. I'd remember the payload markExpired() proved dead and also require, here, that the payload being signed out has its refresh token (sameRefreshToken, SessionCore+Refresh.swift:413). Comparing whole credentials would read the plugin's re-save of the same dead tokens, with its expired flag set, as a new login. A test: copy testClosingTheLogoutPageOfAnExpiredSessionStillSignsOut (WebUISignOutTests.swift:953) and write HostedUIFixtures.hostedUIPayload(version: 2) over the record after the fetch that marks the session expired, as testOnlyTheFirstAttemptPresents writes one.
There was a problem hiding this comment.
Thanks, confirmed. The core now remembers the credentials it proved dead (expiredCredentials), and afterClosedLogout signs out past a closed page only when the payload being signed out still has that refresh token, compared with sameRefreshToken, so the plugin's re-save of the dead tokens with its expired flag still signs out (a4cfd52). testClosingTheLogoutPageKeepsALoginSavedAfterTheExpiry writes hostedUIPayload(version: 2) over the record after the fetch that marks the session expired, and expects .failed(.userCancelled), nothing signed out without the page, and the new login kept.
There was a problem hiding this comment.
Thanks, verified for a login saved before the sign-out starts: afterClosedLogout now signs out past a closed page only when the payload still has the refresh token markExpired(installing:) proved dead (SessionCore+WebUI.swift:244), and testClosingTheLogoutPageKeepsALoginSavedAfterTheExpiry covers it.
One timing is still open: a new login saved while the page is showing.
- The closed page's
.skiprevokes the dead login (SessionCore+WebUI.swift:247). ThenSessionSignOutre-reads (SessionSignOut.swift:221) and finds the new login. - A shared-cookie hosted-UI sign-in in the other app usually returns the same user. So the loop treats the new login as that user's moved credentials (
:237-241) and revokes it on the retry (:195). - The retry goes straight to
engine.revoke(SessionCore+Records.swift:153-154), and the session is then signed out:.partialwithhostedUIError.userCancelled.
README.md:1393-1394, RELEASE-NOTES-PENDING.md:30 and the description say instead that a login saved since the expiry stays signed in, with .failed(.userCancelled).
A test: copy testClosingTheLogoutPageKeepsALoginSavedAfterTheExpiry, but write hostedUIPayload(version: 2) inside the .present script before it throws, as testOnlyTheFirstAttemptPresents does. Today I'd expect plans ["present", "skip", "skip"], .partial, and the record signed out.
To close it, either keep the closed page's .userCancelled when the retry's login doesn't have the dead refresh token, or say in the docs that only a login saved before the sign-out began is kept.
Also, no test pins the refresh-token half of the check. The three closed-page tests of an expired session (WebUISignOutTests.swift:953, :1000, :1148) sign out either the exact bytes proved dead or a login with another refresh token, so engine.sameCredentials(payload, dead) in place of sameRefreshToken at SessionCore+WebUI.swift:244 passes them all. A case that saves the same login again with the same refresh token but other decoded credentials, as the plugin's flagged save does, before the page is closed, and expects .partial with hostedUIError .userCancelled, would pin it.
…Client logins back
…ault sign-out clears
…t an identity pool A login saved under a configuration with an identity pool can keep its AWS credentials after the identity pool is removed: the plugin's rule copies .default's record as it is. The refresh test counted those credentials, so once they expired every access-token request, signed-in operation and fetchAuthSession refreshed and rewrote the record until the user pool tokens next needed a refresh. Without an identity pool only the user pool tokens now decide.
… signed in A refresh of a carried session waiting for its identity, whose commit lost to another process's sign-out, recorded an identity failure for a user no longer there and failed every fetchAuthSession field with sessionExpired, and the providers with the identity step's error. The signed-out record is now the answer, as another writer's complete record already was.
The first-factor check accepted WEB_AUTHN from a fixed list on every OS, where the plugin's factor parser refuses it below iOS 17.4 and macOS 13.5 and on tvOS and watchOS. Sent there, Cognito's WebAuthn challenge ended the sign-in. The answer is now refused with validation, and the selection kept, unless the step offers WebAuthn.
…-out The epoch check on a challenge ran before its save took the record's gate. A sign-out holds that gate across its revoke and moves the epoch only at its end, so the save could wait behind it and signIn returned a challenge the sign-out had already ended. The epoch is checked again after the save, and the sign-in throws the cancelled invalidState.
…uld not finish After a failed global sign-out the revoke error is the plugin's empty placeholder for a RevokeToken never sent, and it was preferred as the underlying error of a contended or unreadable sign-out. The global sign-out failure now comes first.
A second refusal of a reused refresh token, at least 30 seconds after the first with the record unchanged, expires the session, forced or not. It was reported with the text of a refused refresh token, "has expired or been revoked", with refreshTokenInvalid underneath. It is still sessionExpired, now saying that the token was already used and no refreshed tokens were saved, so the session cannot be renewed, with refreshTokenReused underneath, and the session answers with it until its credentials change.
…and the access-group signed-out record (#4405) Plugin-visible fixes and the remaining AmplifyCognitoClient follow-ups: - confirmSignIn: no longer hangs when the identity pool step fails; it signs out and revokes the sign-in's own tokens, as signIn does. - Logs: the challenge session, the confirmSignIn answer, the TOTP-setup session and AuthSignUpStep's auto sign-in session are redacted; the MFA-type and auth-factor parsers no longer repeat the parsed value. - Access groups: a plugin sign-out saves the signed-out record over a login, so an extension's first migration can't sign the user back in. - Client: a session's dead-token expiry is kept across launches; review follow-ups from #4401. - CI and sandbox: a weekly Lambda extends the code sink's API key; expired keys are extended in place before being replaced; `replace <pool>`; a phase-1 `--apply` refuses once phase 2 exists; the first-run guard requires the profile's own credentials; selector fixes and a bundled-file check. The parser change updates one locked golden log line.
Summary
This PR has the client-side follow-ups from the review of #4349, the
AmplifyCognitoClientbeta that shipped in2.63.0. It fixes behaviour and texts in
AmplifyCognitoClient. It also changes the shared Cognito engine(
InternalAWSCognitoAuth) where the client needed more from it, and an app that uses only the Auth plugin can't seethose engine changes.
after a client sign-out, and a few texts, with tests.
deleteUser, a sign-out's revoke and global sign-out, the passkey credential calls and the passkey sign-in (therein a failure that prints as the engine error). A signed-out record also stops the AWSMobileClient migration now.
The plugin reads only the engine error, and only the client writes a signed-out record, so plugin errors, logs and
keychain queries are unchanged (see "What plugin-only apps see").
hold only the changes since 2.63.0; 2.63.0's own rows go in its GitHub release.
The client is still beta, behind
@_spi(AmplifyExperimental). No public declaration is added, removed or changed."Before" below means 2.63.0.
Behaviour changes
Sessions and refresh
sessionExpiredwith the text of a refused token ("has expired or been revoked") andrefreshTokenInvalidunderneathsessionExpired, forced or not. The text now says the refresh token was already used and no refreshed tokens were saved, so the session can't be renewed, withrefreshTokenReusedunderneath. While the session is in memory (until every handle and provider for it is released, or a relaunch), it keeps giving this error until it signs in again or out, or a login with another refresh token is saved.default, an app extension over a named session), or the plugin's save of them with its expired flag set.sessionExpired.sessionExpired. A login with another refresh token ends itsessionExpiredsaying the refresh token had expired, and suggesting a sign-in, which the client refuses on a federated sessionsessionExpired, with the same underlying error. It now says the provider token is no longer accepted, and suggests callingfederateToIdentityPoolwith a new onefetchAuthSession'suserSubResultanduserPoolTokensResult,getCurrentUser, and the token provider'snotConfiguredclearFederationToIdentityPool()or signing the session out first. The operations and the two fields say the session has no user pool usernotSignedIn, including a guest's user pool tokens and sub"WEB_AUTHN"answer to a first-factor selection that doesn't offer it.validation(field: "challengeResponse")before anything is sent. The selection stays pendingconfirmSignInafter a TOTP setup whose last request failed with a step (.resetPassword,.confirmSignUp).invalidState("Cannot use confirmSignIn in the current state. Call signIn to start the sign-in again.") and ends the attempt, as the Auth plugin refuses it.invalidStatefetchAuthSessionfield withsessionExpired, and failed the providers with the identity step's errornotSignedIn(.discard).defaultlogin copied from a configuration with an identity pool, after that pool is removedfetchAuthSessionrefreshed and rewrote the recordErrors
Cognito's own failures are
.servicein every call. That means anInternalErrorExceptionfrom eitherpool, and an error status the SDK doesn't model (an empty-body 5xx included). Both are now
AuthClientError.servicewith no code, where they were.unknown. A 5xx or an internal error suggests a retry,and any other status says the service refused the request with an error the client doesn't recognise.
deleteUser, a sign-out's revoke or global sign-out, the passkey credential calls, and theUSER_AUTH, auto,device SRP and passkey sign-ins are included. These now get the SDK exception through the engine, so an internal
error there is
.servicetoo. It used to be.unknown, with a "file an issue" suggestion.In a
.partialsign-out, the revoke and global sign-out keep the sign-out's suggestion. When Cognito failsthe request itself, the suggestion says the request will not be retried, as it already did for a transport
failure: the sign-out has already finished on this device. The global sign-out's says the user's other sessions
stay signed in, without the "unless the request reached the service" that only a transport failure's has.
Transport failures that the engine reports as a code-less
.service(a refresh, a revoke, a passkey call)now read the same as everywhere else:
read." and a retry suggestion;
Before, a passkey call that failed while offline said "This should not happen", and a revoke over a body that
isn't JSON had an empty description.
A sign-out that couldn't finish now has the first server failure as its underlying error, the global
sign-out's first, mapped as any other call maps it, with the network or retry suggestion. That happens when the
record keeps changing (
.storageUnavailable(.interrupted)) or can't be read (.unknown). Before, after a failedglobal sign-out, the underlying error was the empty placeholder for a revoke that was never sent, and a revoke
that failed on the network said the session was signed out on this device and nothing would be retried, though it
was still signed in. A revoke that throws an error the engine didn't map gets the same plain mapping.
Hosted UI and passkeys
.defaultsession that the Auth plugin had saved as expired, a closed logoutpage used to sign the session out, with no
hostedUIError. Now the result is.failed(.userCancelled)unless arefresh found the session expired while the session was in memory. The plugin's saved flag no longer decides it.
Even then, the page signs out only the login whose refresh token was found dead: a login another app in the access
group saved since keeps the session signed in (
.failed(.userCancelled)), where it was revoked and signed out.another session's sheet, a second sign-in now gets
browserBusy(holder:)naming the session that holds thesheet, with "already waiting for the system sheet". It used to name the requesting session. If the first neither
holds nor waits for the sheet (before it asks, or committing after), the second gets "Session "work" already has a
hosted-UI sign-in in progress.", naming the session holding the sheet if another does, else the session itself as
holder.begins now makes the sign-in throw the sign-in-cancelled
.invalidState, as it does at any other point. It usedto throw
.userCancelled.sign-in's nonce, no single user) now suggests signing in again. If that keeps failing, it suggests checking that
the configuration's user pool, app client and hosted UI domain belong together. A user who isn't the one
identityExpectationasked for still gets theprompt: [.login]suggestion.cancelWebUISignIn()now answers once its sheet is released, as a hosted-UIsign-in already did. If the sheet isn't released within a second, it throws anyway and the sheet stays held until
the ceremony finishes. Before, it answered while the sheet was still closing, so an immediate retry got
browserBusy.AWSMobileClient migration after a client sign-out
.defaultsession now clears the AWSMobileClient (Amplify v1) keychainlogins before it writes the signed-out record or deletes the login. If that write or delete fails, or the
app is stopped right after it, there's no login left for a plugin build to migrate back.
{"noCredentials":{}}) like a login:nothing migrates over it, the old logins are cleared, and it logs
A signed-out session already exists, discarding legacy credentialsat info level. Before (2.63.0 included), the migration read that record as nosession. So an AWSMobileClient login left behind after a client sign-out was migrated over it, and the user was
signed back in.
migrate again.
fetchAuthSession()on.defaultthat saves a guest over a signed-out record now clears theAWSMobileClient logins again first, best effort. Not over an absent record, a label set before anyone signed in
included (it writes only the client's sidecar): a client-only app that never signed in keeps them for a plugin
build to migrate. Never for a named session.
Smaller texts
.configuration) now has a suggestion that names the user pool'ssettings, the identity pool's settings and the client's configuration. It used to name only the user pool's.
resendSignUpCodewith an empty username now says "Username is required to resendSignUpCode". It used to nameconfirmSignUp.resendSignUpCodeanswer with no code delivery details is.unknownwith the plugin's texts.or federated session's next restore deletes the record. A signed-out or guest session's next restore may offer
the challenge again until it is 15 minutes old.
What plugin-only apps see
AuthErrorsWebAuthnError.service, whose new failure type gives its engine error.AuthError(converting:)reads the passkey calls' new failure type through its engine error, so it gives the sameAuthErroras before.defaultsaves itA signed-out session already exists, discarding legacy credentials, behind that same signed-out record. In the release notes, the plugin's\(#fileID)log sites go from 254 to 255. No other engine change adds, removes or changes a log line. A failed passkey sign-in step's state, in the verbose "Auth state change" line, prints as before: the exception it now keeps is in a failure that describes, debug-describes and reflects as the engine error (WebAuthnServiceFailureLogTestspins the line)cause, and no keychain query changesmain, so they leave the pending file, which now holds only changes since 2.63.0. Their corrected text goes in 2.63.0's GitHub release, separately from this PRThe separate plugin PR makes the plugin's sign-out under an access group save the signed-out record too. Once both
PRs are merged, the migration rule above also reaches plugin-only apps with an access group. Whichever PR merges
second updates the engine comment and the release-note rows that say plugin-only apps aren't affected.
Docs
AmplifyClients/AmplifyCognitoClient/README.md:.partialresult;confirmSignInrefusal after a failed TOTP setup;configureUserPoolClientlock;AuthClientError,Options.configureUserPoolClient,cancelWebUISignIn(),WebUIOptions(an emptynonce),AuthClientTOTPSetupDetails,signOutStoredSession,StoredSession.labelandunauthenticatedIdentitiesEnabled.docs/design/RELEASE-NOTES-PENDING.md: only the changes since 2.63.0, with a pointer to 2.63.0's release notes.docs/design/AGREED-DESIGN-AmplifyCognitoClient.mdanddocs/design/issues/rollback-behaviours.md: updated tothe behaviour above, with the plugin releases named. Dated notes are dropped.
docs/design/auth-client-DECISIONS.md: the decisions from this review, and where each one is written.Testing
AmplifyCognitoClientTests)AWSCognitoAuthPluginUnitTests)InternalAmplifyKeychainTests)AmplifyTests)swift test --filter '^AmplifyTests\.' --skip 'testPreconditionFailure|FatalError|shouldThrowFatalError': the precondition tests can't run underswift test, which stops at the first onerefreshTokenReuse.reset()in therefreshTokenInvalidbranch survived the suite, untested: thereuse proof outlives the session core, so a new client of a session an earlier one proved dead refreshes it
again, and without the reset a
refreshTokenInvalidthere answered "already used" for good.testAnInvalidRefreshTokenAfterAReuseProofOfAReleasedClientAnswersWithItsOwnTextnow pins it.(
AWSAuthFetchSessionTask.shouldDeferReconcile's.signingInarm madereturn false), bytestInflightSigningInWithRemoteSignedOut_doesNotReconfigureandtestInflightSigningInWithNoRemoteRecord_doesNotReconfigure, now seeded from a guest; and the AWSMobileClientclear before an unreadable
.defaultrecord's replacement, bytestASignOutWhoseReplacementOfAnUnreadableLoginFailsHasClearedThem.again on the final tree: every one fails its tests.
saved again after it (whole-credential comparison instead of the refresh token fails the plugin's flagged re-save;
never ending the expiry fails a new login), the guest clear over a label alone, and the passkey sign-in's state
line (the earlier form, which kept the exception unwrapped, fails the pinned line; the line was captured with the
steps' code from before the exception was kept).
sheet lock's release-wait test and every
assertCancelled/assertBrowserBusycall, and the sign-out andsign-in waits in the core and engine race tests. Other test waits keep no bound of their own.
Not in this PR
handles. Until then the expiry lasts while the session is in memory, as the docs say.
process saved since). They guard code that shipped in 2.63.0.
earlier configuration. The next restore then carries the session from that configuration again.
signOutStoredSessionrevokes without the app'sconfigureUserPoolClient, so a custom endpoint isn't applied.Breaking changes
AmplifyCognitoClientis beta and may change. This PR changes no public API signatures, but code that matcheson error cases can see different cases:
AuthClientError.unknowntoAuthClientError.servicewith no code, in every call, every sign-in flow included. Code that matched.unknownfor these must match
.service(nil, …)instead. The credentials providers report both cases the same way, so aprovider's
CredentialsErrorand itsdispositiondon't change..defaultsession the Auth plugin saved as expired is now.failed(.userCancelled), and the session stays signed in. It used to sign the session out (.complete, or.partialwith nohostedUIError), unless a refresh had found the session expired while it was in memory. Aclosed page on a session the client found expired, over a login another app saved since, is also
.failed(.userCancelled), where it was.partial.fetchAuthSessionfields used to fail withsessionExpired, and both providers with the identity step's error(
CredentialsError.notConfiguredfor a known refusal, else.unknown). Both providers now throw.notSignedIn,whose disposition is
.discard, so this one does change a provider's disposition.(
.userCancelledto.invalidState), a sign-in whose challenge save waited behind a sign-out (a challenge to.invalidState), a"WEB_AUTHN"answer the step doesn't offer (now.validation), and aconfirmSignInafter afailed TOTP setup (now
.invalidState, where it never returned). Also,browserBusyfor a second hosted-UIsign-in now names the session that holds the sheet, or the session itself when none does.