-
Notifications
You must be signed in to change notification settings - Fork 39
feat(cli): bgagent change-password + force-rotate on first login (#238) #680
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -19,8 +19,11 @@ | |
|
|
||
| import { | ||
| AuthFlowType, | ||
| ChallengeNameType, | ||
| ChangePasswordCommand, | ||
| CognitoIdentityProviderClient, | ||
| InitiateAuthCommand, | ||
| RespondToAuthChallengeCommand, | ||
| } from '@aws-sdk/client-cognito-identity-provider'; | ||
| import { loadConfig, loadCredentials, saveCredentials } from './config'; | ||
| import { debug } from './debug'; | ||
|
|
@@ -41,26 +44,132 @@ const TOKEN_REFRESH_BUFFER_MS = TOKEN_REFRESH_BUFFER_MINUTES * 60 * 1000; | |
| */ | ||
| let inFlightRefresh: Promise<void> | null = null; | ||
|
|
||
| /** Authenticate with username/password and cache tokens. */ | ||
| export async function login(username: string, password: string): Promise<void> { | ||
| /** | ||
| * Prompt callback invoked when Cognito returns the ``NEW_PASSWORD_REQUIRED`` | ||
| * challenge on first login. Returning the new password lets ``login`` respond | ||
| * to the challenge without pulling TTY/readline concerns into the auth layer. | ||
| * The command layer supplies the interactive implementation. | ||
| */ | ||
| export type NewPasswordPrompt = () => Promise<string>; | ||
|
|
||
| /** | ||
| * Authenticate with username/password and cache tokens. | ||
| * | ||
| * First-login rotation: admins invite users with a *temporary* password, so | ||
| * the initial ``InitiateAuth`` returns a ``NEW_PASSWORD_REQUIRED`` challenge | ||
| * instead of tokens. When ``promptNewPassword`` is supplied, we prompt for a | ||
| * replacement, answer the challenge via ``RespondToAuthChallenge``, and persist | ||
| * the resulting tokens. Without a prompt (e.g. ``--password`` passed | ||
| * non-interactively) we surface a clear error rather than hanging. | ||
| */ | ||
| export async function login( | ||
| username: string, | ||
| password: string, | ||
| promptNewPassword?: NewPasswordPrompt, | ||
| ): Promise<void> { | ||
| const config = loadConfig(); | ||
| debug(`Cognito region: ${config.region}, client_id: ${config.client_id}, user_pool_id: ${config.user_pool_id}`); | ||
| const client = new CognitoIdentityProviderClient({ region: config.region }); | ||
|
|
||
| const result = await client.send(new InitiateAuthCommand({ | ||
| AuthFlow: AuthFlowType.USER_PASSWORD_AUTH, | ||
| ClientId: config.client_id, | ||
| AuthParameters: { | ||
| USERNAME: username, | ||
| PASSWORD: password, | ||
| }, | ||
| })); | ||
| let result; | ||
| try { | ||
| result = await client.send(new InitiateAuthCommand({ | ||
| AuthFlow: AuthFlowType.USER_PASSWORD_AUTH, | ||
| ClientId: config.client_id, | ||
| AuthParameters: { | ||
| USERNAME: username, | ||
| PASSWORD: password, | ||
| }, | ||
| })); | ||
| } catch (err) { | ||
| // Invited users sit in FORCE_CHANGE_PASSWORD under Cognito's default 7-day | ||
| // TemporaryPasswordValidityDays. Once that window lapses the temp password | ||
| // is dead and Cognito answers with a bare NotAuthorizedException — the same | ||
| // error a genuinely wrong password produces, with nothing to say the temp | ||
| // one merely expired. Point the teammate at the fix (a fresh invite) rather | ||
| // than letting them retype a password that can never work again. We do not | ||
| // include the attempted password or any secret in the message. | ||
| if (err instanceof Error && err.name === 'NotAuthorizedException') { | ||
| throw new CliError( | ||
| 'Login failed: incorrect password, or your temporary password expired. ' | ||
|
Contributor
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. Mapping FIXED — but the remedy it prescribes dead-ends (N7, non-blocking). The mapping is right and I want it kept: a day-8 teammate previously got a bare The problem is the action. Tracing the journey this message sends the user on: So the fix this message names is refused by the very next command. The admin's actual options are both bad: delete-then-recreate destroys the Cognito The new test asserts the message wording but not that the prescribed remedy works — which is why this slipped through. Cheapest honest fix: point at Secondary: an established user who typos their long-standing permanent password now reads two sentences about temporary passwords that do not apply to them. Worth a follow-up issue, not a blocker — the mapping is still a net improvement on the bare SDK error. |
||
| + 'Temporary passwords lapse after a few days — ask your admin to re-run ' | ||
| + '`bgagent admin invite-user` for a fresh one.', | ||
| ); | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| const auth = result.AuthenticationResult; | ||
| if (result.ChallengeName === ChallengeNameType.NEW_PASSWORD_REQUIRED) { | ||
| const auth = await respondToNewPasswordChallenge( | ||
| client, | ||
| config.client_id, | ||
| username, | ||
| result.Session, | ||
| promptNewPassword, | ||
| ); | ||
| persistAuthResult(auth); | ||
| return; | ||
| } | ||
|
|
||
| persistAuthResult(result.AuthenticationResult); | ||
| } | ||
|
|
||
| /** | ||
| * Answer the first-login ``NEW_PASSWORD_REQUIRED`` challenge: prompt for a new | ||
| * password (via the caller-supplied prompt) and exchange it for tokens. | ||
| * ``ChallengeResponses`` carries the same ``USERNAME`` Cognito challenged plus | ||
| * the ``NEW_PASSWORD``; the ``Session`` echoes the value from ``InitiateAuth``. | ||
| */ | ||
| async function respondToNewPasswordChallenge( | ||
| client: CognitoIdentityProviderClient, | ||
| clientId: string, | ||
| username: string, | ||
| session: string | undefined, | ||
| promptNewPassword?: NewPasswordPrompt, | ||
| ): Promise<AuthResult> { | ||
| if (!promptNewPassword) { | ||
| throw new CliError( | ||
| 'This account requires a new password on first login. ' | ||
| + 'Run `bgagent login --username <email>` interactively (omit --password) ' | ||
| + 'so the CLI can prompt you to set one.', | ||
| ); | ||
| } | ||
|
|
||
| const newPassword = await promptNewPassword(); | ||
| try { | ||
| const challengeResult = await client.send(new RespondToAuthChallengeCommand({ | ||
| ClientId: clientId, | ||
| ChallengeName: ChallengeNameType.NEW_PASSWORD_REQUIRED, | ||
| Session: session, | ||
| ChallengeResponses: { | ||
| USERNAME: username, | ||
| NEW_PASSWORD: newPassword, | ||
| }, | ||
| })); | ||
| return challengeResult.AuthenticationResult; | ||
| } catch (err) { | ||
| // Cognito rejects a policy-violating new password with | ||
| // InvalidPasswordException; surface the server's guidance verbatim rather | ||
| // than leaking a raw SDK stack. | ||
| if (err instanceof Error && err.name === 'InvalidPasswordException') { | ||
| throw new CliError(`New password rejected: ${err.message}`); | ||
| } | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| /** Shared shape of the tokens both ``InitiateAuth`` and challenge responses return. */ | ||
| type AuthResult = { | ||
| IdToken?: string; | ||
| RefreshToken?: string; | ||
| ExpiresIn?: number; | ||
| } | undefined; | ||
|
|
||
| /** Validate and persist a Cognito auth result to the credentials cache. */ | ||
| function persistAuthResult(auth: AuthResult): void { | ||
| if (!auth?.IdToken || !auth.RefreshToken || !auth.ExpiresIn) { | ||
| throw new CliError('Unexpected authentication response from Cognito.'); | ||
| } | ||
|
|
||
| const expiry = new Date(Date.now() + auth.ExpiresIn * 1000).toISOString(); | ||
| saveCredentials({ | ||
| id_token: auth.IdToken, | ||
|
|
@@ -158,3 +267,116 @@ async function refreshToken(creds: Credentials): Promise<void> { | |
| throw new CliError(`Token refresh failed (${detail}). Retry, or run \`bgagent login\` if it persists.`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Rotate the current user's Cognito password. | ||
| * | ||
| * ``ChangePassword`` requires a Cognito **access token**, which the CLI does | ||
| * not persist (only the ID + refresh tokens the REST authorizer needs). Rather | ||
| * than widen the on-disk credential surface, we re-authenticate with the | ||
| * supplied *current* password to mint a short-lived access token in memory. | ||
| * That re-auth doubles as verification of the current password: a wrong one | ||
| * fails here with a clear "current password is incorrect" message before any | ||
| * change is attempted. Cognito enforces the password policy on the new value | ||
| * server-side; a policy violation surfaces as ``InvalidPasswordException``. | ||
| * | ||
| * Requires an existing ``bgagent login`` session — the username is read from | ||
| * the cached ID token so the user does not re-type their email. | ||
| */ | ||
| export async function changePassword( | ||
| currentPassword: string, | ||
| newPassword: string, | ||
| ): Promise<void> { | ||
| const config = loadConfig(); | ||
| const username = usernameFromSession(); | ||
| const client = new CognitoIdentityProviderClient({ region: config.region }); | ||
|
|
||
| const accessToken = await accessTokenFor(client, config.client_id, username, currentPassword); | ||
|
|
||
| try { | ||
| await client.send(new ChangePasswordCommand({ | ||
| AccessToken: accessToken, | ||
| PreviousPassword: currentPassword, | ||
| ProposedPassword: newPassword, | ||
| })); | ||
| } catch (err) { | ||
| if (err instanceof Error && err.name === 'InvalidPasswordException') { | ||
| // Cognito's message already states which policy rule failed. | ||
| throw new CliError(`New password rejected: ${err.message}`); | ||
| } | ||
| if (err instanceof Error && err.name === 'LimitExceededException') { | ||
| throw new CliError('Too many password-change attempts. Wait a few minutes and try again.'); | ||
| } | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Read the signed-in user's Cognito username from the cached ID token. | ||
| * | ||
| * Requires a ``bgagent login`` session. The username lives in the ``email`` | ||
| * claim (the pool's sign-in alias); older tokens may only carry | ||
| * ``cognito:username``. Never logs the token or its claims. | ||
| */ | ||
| function usernameFromSession(): string { | ||
| const creds = loadCredentials(); | ||
| if (!creds) { | ||
| throw new CliError('Not authenticated. Run `bgagent login` first.'); | ||
| } | ||
| const JWT_SEGMENTS = 3; // header.payload.signature | ||
| const parts = creds.id_token.split('.'); | ||
| if (parts.length !== JWT_SEGMENTS) { | ||
| throw new CliError('Credentials file is corrupt. Run `bgagent login` to re-authenticate.'); | ||
| } | ||
| let payload: { 'email'?: string; 'cognito:username'?: string }; | ||
| try { | ||
| payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString('utf-8')); | ||
| } catch { | ||
| throw new CliError('Credentials file is corrupt. Run `bgagent login` to re-authenticate.'); | ||
| } | ||
| const username = payload.email ?? payload['cognito:username']; | ||
| if (!username) { | ||
| throw new CliError('Could not read your account identity. Run `bgagent login` to re-authenticate.'); | ||
| } | ||
| return username; | ||
| } | ||
|
|
||
| /** | ||
| * Re-authenticate to obtain a fresh, in-memory access token for | ||
| * ``ChangePassword``. A wrong current password fails here with | ||
| * ``NotAuthorizedException`` — surfaced as an actionable message. | ||
| */ | ||
| async function accessTokenFor( | ||
| client: CognitoIdentityProviderClient, | ||
| clientId: string, | ||
| username: string, | ||
| currentPassword: string, | ||
| ): Promise<string> { | ||
| let result; | ||
| try { | ||
| result = await client.send(new InitiateAuthCommand({ | ||
| AuthFlow: AuthFlowType.USER_PASSWORD_AUTH, | ||
| ClientId: clientId, | ||
| AuthParameters: { | ||
| USERNAME: username, | ||
| PASSWORD: currentPassword, | ||
| }, | ||
| })); | ||
| } catch (err) { | ||
| if (err instanceof Error && err.name === 'NotAuthorizedException') { | ||
| throw new CliError('Current password is incorrect.'); | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| const accessToken = result.AuthenticationResult?.AccessToken; | ||
| if (!accessToken) { | ||
| // A challenge (e.g. NEW_PASSWORD_REQUIRED) or an unexpected shape — the | ||
| // account is not in a state where a self-service change applies. | ||
| throw new CliError( | ||
| 'Could not verify your current password. If this is your first login, ' | ||
| + 'run `bgagent login` to set a permanent password instead.', | ||
| ); | ||
| } | ||
| return accessToken; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ import { | |
| } from '@aws-sdk/client-cognito-identity-provider'; | ||
| import { tryLoadConfig } from './config'; | ||
| import { CliError } from './errors'; | ||
| import { DEFAULT_STACK_NAME, resolveOperatorContext } from './operator-context'; | ||
| import { resolveOperatorContext } from './operator-context'; | ||
| import { getStackOutput, resolveConfigureBundleFromStack } from './stack-outputs'; | ||
| import { CliConfig } from './types'; | ||
|
|
||
|
|
@@ -152,28 +152,19 @@ export async function adminSetPermanentPassword( | |
| })); | ||
| } | ||
|
|
||
| /** Create user + set permanent password; surfaces half-failure diagnostics. */ | ||
| /** | ||
| * Invite a user with a *temporary* password (#238): the FORCE_CHANGE_PASSWORD | ||
|
isadeks marked this conversation as resolved.
|
||
| * state it leaves them in forces a first-login rotation, so the admin-generated | ||
| * string stops being a valid credential once the teammate sets their own. Kept | ||
| * as a named seam over ``adminCreateUser`` for the command layer and tests. | ||
| */ | ||
| export async function adminInviteUser( | ||
|
Contributor
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. Removing the Nit (N4): the function is now a one-line passthrough to Related:
Contributor
Author
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. Done — kept the wrapper as a stable seam (smaller diff than inlining, and |
||
| ctx: CognitoAdminContext, | ||
| email: string, | ||
| password: string, | ||
| temporaryPassword: string, | ||
| ): Promise<void> { | ||
| const client = cognitoClient(ctx.region); | ||
| await adminCreateUser(client, ctx.userPoolId, email, password); | ||
| try { | ||
| await adminSetPermanentPassword(client, ctx.userPoolId, email, password); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| const errorName = err instanceof Error ? err.name : 'Error'; | ||
| throw new CliError( | ||
| `User ${email} was created but the password could not be set ` | ||
| + `(${errorName}: ${message}). The user is now stuck in FORCE_CHANGE_PASSWORD ` | ||
| + 'state and cannot log in. Either:\n' | ||
| + ` 1. Delete and re-run: bgagent admin delete-user ${email} --stack-name ${DEFAULT_STACK_NAME}\n` | ||
| + ' 2. Or reset the password: bgagent admin reset-password ' | ||
| + `${email} --stack-name ${DEFAULT_STACK_NAME}`, | ||
| ); | ||
| } | ||
| await adminCreateUser(client, ctx.userPoolId, email, temporaryPassword); | ||
| } | ||
|
|
||
| export async function adminDeleteUser( | ||
|
|
||
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.
Blocking (B1, same defect) — this sentence is factually false given the only caller.
"Without a prompt (e.g.
--passwordpassed non-interactively) we surface a clear error rather than hanging" describes intent, not behavior:login.ts:38always supplies the callback, so the guard at line 111 is dead code and the non-interactive path hangs (reproduced — see thelogin.ts:38comment).This is the most valuable comment in the diff to get right, because a future maintainer reading it will reasonably assume the non-interactive case is handled and not re-test it. Once the callback is gated on
process.stdin.isTTY && !opts.password, the sentence becomes true as written and no edit is needed.Worth adding while you are here: invited users now sit in
FORCE_CHANGE_PASSWORDunder Cognito's default 7-dayTemporaryPasswordValidityDays— I checked, it is never set anywhere incdk/. A teammate who logs in on day 8 gets a bareNotAuthorizedExceptionwith no hint that the temp password merely expired and the admin must re-issue. Permanent passwords never expired, so this failure mode is new to this PR; mapping it to "your temporary password expired — ask your admin to re-runbgagent admin invite-user" would close the loop.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.
Addressed. With the callback now gated on
process.stdin.isTTY && !opts.password(see the login.ts thread), the docstring sentence "Without a prompt … we surface a clear error rather than hanging" is true as written, so no edit was needed there. I also added the expired-temp-password mapping you suggested:login()now wraps the initialInitiateAuthand mapsNotAuthorizedExceptionto "Login failed: incorrect password, or your temporary password expired. Temporary passwords lapse after a few days — ask your admin to re-runbgagent admin invite-userfor a fresh one." No secret is included in the message (the attempted password is never echoed), and non-auth errors are rethrown unchanged. Two new auth.test.ts cases cover the mapping (asserting the message and that the password is not leaked) and the rethrow path.