Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,18 @@ bgagent login \

Tokens are saved to `~/.bgagent/credentials.json` (mode 0600). The CLI automatically refreshes expired tokens using the cached refresh token.

**First login (invited users):** `admin invite-user` issues a *temporary* password, so your first login is a rotation. Cognito returns a `NEW_PASSWORD_REQUIRED` challenge and the CLI prompts you to set (and confirm) a permanent password, which replaces the admin-shared temp one. Run it interactively — **omit `--password`** so the CLI can prompt; passing `--password` (or piping on a non-TTY) skips the rotation prompt and fails with a clear "log in interactively" error rather than hanging. Temporary passwords also expire after a few days; if yours has lapsed the login reports that and asks the admin to re-run `invite-user`.

### `bgagent change-password`

Rotate the signed-in user's Cognito password (requires an active `bgagent login` session).

```
bgagent change-password
```

Prompts for your current password, then the new password twice (masked). Cognito enforces the pool's password policy on the new value — by default minimum 12 characters with an upper, lower, digit, and symbol. No flags: the current-password prompt doubles as verification, and the username is read from your cached session so you never retype your email.

### `bgagent submit`

Submit a new coding task.
Expand Down Expand Up @@ -341,7 +353,7 @@ Manage Cognito users with operator AWS credentials (`cognito-idp:Admin*` on the
```
bgagent admin invite-user <email> \
--stack-name backgroundagent-dev \
--password <pwd> # optional; auto-generated if omitted
--password <pwd> # optional temporary password; auto-generated if omitted

bgagent admin list-users \
--output <text|json>
Expand All @@ -352,7 +364,7 @@ bgagent admin reset-password <email> \
--password <pwd> # optional; auto-generated if omitted
```

`invite-user` creates the user, sets a permanent password, and writes credentials plus an optional configure bundle to `~/.bgagent/invites/<email>.txt` (mode 0600). Replaces Quick Start Step 5 raw `aws cognito-idp` commands.
`invite-user` creates the user with a *temporary* password (rotated on the teammate's first login via `bgagent login`) and writes credentials plus an optional configure bundle to `~/.bgagent/invites/<email>.txt` (mode 0600). The temp password stops being valid once they set their own, so the admin-shared string never becomes a standing credential. `reset-password` instead sets a *permanent* password for an existing user (no first-login rotation). Replaces Quick Start Step 5 raw `aws cognito-idp` commands.

## Output formats

Expand Down
246 changes: 234 additions & 12 deletions cli/src/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.

Copy link
Copy Markdown
Contributor

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. --password passed non-interactively) we surface a clear error rather than hanging" describes intent, not behavior: login.ts:38 always supplies the callback, so the guard at line 111 is dead code and the non-interactive path hangs (reproduced — see the login.ts:38 comment).

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_PASSWORD under Cognito's default 7-day TemporaryPasswordValidityDays — I checked, it is never set anywhere in cdk/. A teammate who logs in on day 8 gets a bare NotAuthorizedException with 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-run bgagent admin invite-user" would close the loop.

Copy link
Copy Markdown
Contributor Author

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 initial InitiateAuth and maps NotAuthorizedException to "Login failed: incorrect password, or your temporary password expired. Temporary passwords lapse after a few days — ask your admin to re-run bgagent admin invite-user for 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.

*/
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. '

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 NotAuthorizedException with nothing to distinguish "expired" from "wrong", and no secret is echoed here (the message is a constant — the test asserting the password's absence is a good pin). Mutation-tested: deleting the mapping fails maps NotAuthorizedException to an expired-temp-password hint.

The problem is the action. Tracing the journey this message sends the user on:

teammate: bgagent login              → "…ask your admin to re-run `bgagent admin invite-user`"
admin:    bgagent admin invite-user  → adminInviteUser (cognito-admin.ts:161)
                                     → adminCreateUser (:111) → AdminCreateUserCommand
                                     → user exists → UsernameExistsException
                                     → "User … already exists. Use a different email, or
                                        run `bgagent admin delete-user …` and try again."

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 sub, which is the platform user_id (cdk/src/handlers/shared/gateway.ts:38), orphaning every task the teammate ever submitted; or admin reset-password, which sets a permanent password and so silently opts them out of the first-login rotation this PR exists to enforce.

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 bgagent admin reset-password. The real fix: let invite-user re-issue a temp password for an existing FORCE_CHANGE_PASSWORD user (AdminSetUserPassword with Permanent: false), which preserves the sub and the rotation.

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,
Expand Down Expand Up @@ -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;
}
2 changes: 2 additions & 0 deletions cli/src/bin/bgagent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { makeAdminCommand } from '../commands/admin';
import { makeApiKeyCommand } from '../commands/api-key';
import { makeApproveCommand } from '../commands/approve';
import { makeCancelCommand } from '../commands/cancel';
import { makeChangePasswordCommand } from '../commands/change-password';
import { makeConfigureCommand } from '../commands/configure';
import { makeDenyCommand } from '../commands/deny';
import { makeEventsCommand } from '../commands/events';
Expand Down Expand Up @@ -66,6 +67,7 @@ program

program.addCommand(makeConfigureCommand());
program.addCommand(makeLoginCommand());
program.addCommand(makeChangePasswordCommand());
program.addCommand(makeSubmitCommand());
program.addCommand(makeListCommand());
program.addCommand(makeStatusCommand());
Expand Down
27 changes: 9 additions & 18 deletions cli/src/cognito-admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Comment thread
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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the adminSetPermanentPassword call is correct, and I want to be explicit that deleting the half-failure try/catch is not a lost safety net: that branch existed only because a second mutating call could fail after adminCreateUser succeeded. With one call there is no half-state to report, so the diagnostic is genuinely dead code. Good call, and the expect(input).not.toHaveProperty('Permanent') regression guard in the test is exactly the right pin.

Nit (N4): the function is now a one-line passthrough to adminCreateUser with an 11-line docstring above it — the docstring is longer than the body, and the error diagnostics were the only thing that distinguished the two. Consider collapsing it into the call site at commands/admin.ts:141 and keeping the why as a one-line comment, or keep the wrapper as the stable seam and trim the prose. Either is fine; the current shape just reads like a wrapper that outlived its reason.

Related: adminSetPermanentPassword (line 140) is now reached only through adminResetPassword, so the exported surface here could shrink — worth a look if knip flags it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 admin.test.ts now drives it directly) and trimmed the 11-line docstring to ~2 lines: "Invite a user with a temporary password (#238): the FORCE_CHANGE_PASSWORD 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." Thanks for confirming the half-failure try/catch deletion was correct. On the exported-surface note: adminSetPermanentPassword is still reached via adminResetPassword (and the Permanent-flag regression test pins it), and the dead-code ratchet is advisory/non-blocking here, so I have left the export as-is rather than reshaping the surface in this PR.

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(
Expand Down
Loading
Loading