fix(auth): read device metadata with the caller's username during sign-in - #3
Open
dmorrow wants to merge 3 commits into
Open
fix(auth): read device metadata with the caller's username during sign-in#3dmorrow wants to merge 3 commits into
dmorrow wants to merge 3 commits into
Conversation
…n-in `ConfirmDevice` stores device metadata under `signedInData.inputUsername` (the username the caller signed in with), but the sign-in flow read it back using `parameters["USERNAME"]` — the value Cognito echoes, which is the sub for pools configured with alias sign-in. On those pools the read never matched the write, so `DEVICE_KEY` was omitted from the password-verifier response. Cognito treated every sign-in as a new device: it returned fresh `NewDeviceMetadata`, `ConfirmDevice` stored it under the email again, and the next sign-in repeated the cycle — MFA on every sign-in and a new device record per attempt (one user had accumulated 50). Pools where the username *is* the email are unaffected, since the two values coincide there. Reads now use the same value as the write: - VerifyPasswordSRP: `inputUsername` - VerifySignInChallenge (both call sites): `challenge.inputUsername ?? username` Nothing sent to Cognito changes — `username` still supplies `USERNAME` on every request; only the keychain lookups moved. Known gap: the DeviceSRP actions (InitiateAuthDeviceSRP, VerifyDevicePasswordSRP) still look up by the echoed username. That path only executes once Cognito starts accepting the device key, so it is now reachable and tracked separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Commit 122689e changed the sign-in flow to read device metadata with the caller's `inputUsername` (matching how `ConfirmDevice` writes it), but left the `deviceNotFound` recovery paths deleting under `username` — the value Cognito echoes, which is the sub for pools with alias sign-in. When a stored device key is stale, Cognito rejects the password verifier with `ResourceNotFoundException`. The recovery path then deletes the wrong keychain entry, leaving the real one in place, so the retry re-reads the same stale key, Cognito rejects it again, and sign-in loops indefinitely. Read and delete now use the same value in both actions: - VerifyPasswordSRP: `inputUsername` - VerifySignInChallenge: `challenge.inputUsername ?? username` Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Once a device is remembered, Cognito answers the password verifier with DEVICE_SRP_AUTH instead of an MFA challenge, routing sign-in through InitiateAuthDeviceSRP / VerifyDevicePasswordSRP. Both looked the stored device metadata up by the Cognito-echoed username (the sub, on pools with alias sign-in) rather than the `inputUsername` it was written under, so the lookup missed and the request omitted DEVICE_KEY — Cognito then rejected sign-in with "Missing required parameter DEVICE_KEY". - UserPoolSignInHelper: carry `inputUsername ?? username` into `.initiateDeviceSRP` so InitiateAuthDeviceSRP reads (and re-sends) under the right key. - VerifyDevicePasswordSRP: read metadata with `inputUsername`; the request still sends `username` (the echoed value), which is what Cognito expects. Completes the alias-pool device-remembering fix (see prior two commits); the DEVICE_SRP_AUTH path is only reachable once DEVICE_KEY is accepted, so it could not be exercised until the earlier read/delete fixes landed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #2 (upstream sync) so the diff here is just the fix — retarget to
mainonce #2 merges.The bug
ConfirmDevicestores device metadata undersignedInData.inputUsername— the username the caller signed in with — but the sign-in flow read it back withparameters["USERNAME"], the value Cognito echoes. On a pool with alias sign-in that echoed value is the sub, so the read never matched the write:With no metadata loaded,
buildInputomitsDEVICE_KEY(InitiateAuthInput+Amplify.swift:135), so Cognito treats every sign-in as a new device: freshNewDeviceMetadata, a new device record, and an MFA challenge — forever. One affected user had accumulated 50 device records, one per sign-in, every one withlastAuthenticatedDate == createdDate.Pools where the username is the email are unaffected, because the two values coincide — which is why this wasn't caught by the tests added in b3b8320.
The fix
Read with the same value the write uses:
VerifyPasswordSRP.swiftfor: usernamefor: inputUsernameVerifySignInChallenge.swift(challenge response)for: usernamefor: challenge.inputUsername ?? usernameVerifySignInChallenge.swift(handleConfirmSignInWithPassword)for: usernamefor: challenge.inputUsername ?? usernameNothing sent to Cognito changes.
usernamestill suppliesUSERNAMEon every request; only the keychain lookups moved.Known gap
InitiateAuthDeviceSRPandVerifyDevicePasswordSRPstill look up by the echoed username. That path only runs once Cognito accepts a device key — i.e. it becomes reachable because of this fix. If it misses there,VerifyDevicePasswordSRPhits itsguard case .metadataand throwsSRPError.calculation, which fails sign-in rather than falling back to MFA. Deliberately left out of this PR; the two candidate fixes are threadinginputUsernamethrough the DeviceSRP events/state, or havingConfirmDevicewrite under both usernames.Testing
Compiled (
swift build --target AWSCognitoAuthPlugin) before the upstream merge; not re-built or exercised against a live pool since.InputUsernameDeviceKeyTests.swiftis the right home for a regression case whereparameters["USERNAME"]differs from the input username — not added here.🤖 Generated with Claude Code