[lavere]

Summary

Adds Cognito as a login option next to Auth0. The user pool is built in this app's own CDK stack, so there is no separate Cognito starter. This combines the earlier version of this PR with Brian's #7 and replaces both.

• Prompt: one choice of None / Auth0 / Cognito. Cognito is offered only with an AWS env. It then asks for the optional user admin UI, the login domain prefix (validated against Cognito's reserved words and length limit) and MFA (required, optional or off).
• Stack: a CognitoAuth construct creates the user pool, app client, managed login domain and branding, an admin group, and branded invitation and verification emails. Optional ses sender (with verifiedDomain).
• Runtime config: the stack writes the pool IDs to /config.json at deploy time and the app loads it before rendering, so one build fits every environment. Local dev reads VITE_APP_COGNITO_* from .env.
• Login: react-oidc-context, authorization code + PKCE against managed login. Tokens under 60 seconds from expiry are renewed first. A failed renewal is retried while the current token still works; invalid_grant sends the user to sign in again. The renewal changes made after the review have not been run against a real pool.
• Logout: revokes the refresh token, then calls Cognito's /logout. Handles the back button after signing out.
• Admin UI (optional): /admin routes to list, invite, edit, disable and delete users, reset passwords and MFA, and manage role membership. There is no backend: an identity pool gives members of the admin group temporary AWS credentials scoped to this pool.
• Outputs for the API: CognitoIssuer, CognitoUserPoolArn and CognitoClientId (used by create-taxum-api#5).

Includes the TanStack Router / auth0-react build fix from #5. Merge that first; this branch will need a rebase after.

Testing

• Generator: pnpm test, tsc and biome ci . pass.
• All six renders (none, auth0, cognito, cognito-admin, and cognito-admin with MFA optional and off) pass their ci-check.sh and pnpm build.
• Both Cognito renders passed cdk synth with ses set. That run predates the review fixes.
• End-to-end against real pools, once with only the Cognito part of the stack and the app on localhost, and once with the full stack behind CloudFront on a real domain:
  • /config.json is served uncached with the real pool IDs, and the deployed build logs in with it. Deep links load.
  • Login through managed login, with authenticator setup forced when MFA is required.
  • Access token carries the admin group.
  • Admin UI: list, invite and disable a user, add and remove a role, checked against Cognito's own records.
  • Token renewal with refresh-token rotation, without a trip to the login page.
  • Sign-out ends the session and lands on the login form.

Notes

• MFA cannot be reset when the pool requires it. Cognito does not list an authenticator that a user enrolls at sign-in, and it accepts AdminSetUserMFAPreference with Enabled=false without changing anything: the next sign-in still asks for a code. Tested from the admin UI and from the CLI. The user page now shows "Required at sign-in", Reset MFA stays disabled for those users, and the README says to delete and re-invite someone who lost their device (they get a new user ID). Reset MFA still applies to methods Cognito does list.
• Admin role permissions include AdminSetUserPassword and AdminUpdateUserAttributes, which the set-password and edit screens need. Any member of admin can use them from the browser, so an admin can take over any account. The README says so.
• App client flows are authFlows: { userSrp: false }. Managed login does not need SRP, and refresh-token auth conflicts with rotation.
• Login logo: Cognito rejects SVG attributes such as version and xml:space, so they were removed from the default favicon.svg.
• Route tree: a fresh project with the admin UI failed tsc because the shipped routeTree.gen.ts lacks the admin routes. The generator now runs a build after install to regenerate it.
• SES: the from address or its domain must already be verified. Cognito drops email from an unverified identity without an error.
• An earlier version had an apiIdentifier option that added an API scope. It was removed after review because the API never checked it; the API limits tokens to this app by client ID.
• Any admin can add any user to any group, so a group with an IAM role is open to every admin. The README says so.
• Refresh tokens last 7 days. The admin group is always created; it only gets AWS permissions when the admin UI is included.

🤖 Generated with Claude Code

[lavere]

Verdict: comment; 🔴 1 🟠 3 🟢 22 ❓ 2

Posted as a comment because this is the reviewer's own pull request.

#	Repo	Issue	Severity
1	create-react-app	CDK type check fails when the admin UI is picked with MFA optional or off	🔴 Blocker
2	create-react-app	A failed renewal redirects to login while the current token is still valid	🟠 Should-fix
3	create-react-app	The retry stops for good after a manual renewal times out	🟠 Should-fix
4	create-react-app	A Cognito project without the admin UI is never told how to create its first user	🟠 Should-fix
5	create-react-app	Every admin can join any group, so any future group with an IAM role is open to all admins	❓ Open question
6	create-react-app	[Both] The access scope from apiIdentifier is issued but the API never checks it	❓ Open question
7	create-react-app	returnTo is used without checking its shape	🟢 Nit
8	create-react-app	This catch cannot run	🟢 Nit
9	create-react-app	"Concurrent calls share one renewal" only holds per hook instance	🟢 Nit
10	create-react-app	The docblock's example is the one case the PR says does not work	🟢 Nit
11	create-react-app	FORCE_CHANGE_PASSWORD is not the same as "has not signed in yet"	🟢 Nit
12	create-react-app	The app bar reads only the email, not the name	🟢 Nit
13	create-react-app	The ref stops a second refresh, not a redirect	🟢 Nit
14	create-react-app	The last two outputs under this comment are not API values	🟢 Nit
15	create-react-app	The empty list only holds because rotation is on	🟢 Nit
16	create-react-app	The account takeover warning sits under "Lost authenticator"	🟢 Nit
17	create-react-app	These HTML comments are sent in every email	🟢 Nit
18	create-react-app	The suggested domain prefix can fail the prompt's own validation	🟢 Nit
19	create-react-app	Generator behavior with no test	🟢 Nit
20	create-react-app	Duplicate module augmentation	🟢 Nit
21	create-react-app	The props rename is half applied	🟢 Nit
22	create-react-app	No loader or pending component, unlike the sibling routes	🟢 Nit
23	create-react-app	The role chip stays clickable while a removal is pending	🟢 Nit
24	create-react-app	A failed second call reports failure after the first change was saved	🟢 Nit
25	create-react-app	The new test/ folder is never type checked	🟢 Nit
26	create-react-app	useCurrentUserGroups is exported but only used in this file	🟢 Nit
27	create-react-app	A leftover test-synth/ folder is no longer ignored	🟢 Nit
28	create-react-app	The commented-out ses line breaks on a title with a quote	🟢 Nit

Adds Cognito login with the user pool in the app's own CDK stack, runtime /config.json, and an optional browser-only user admin UI. The design holds up and matches the API side on issuer, client id, token type and group name. One change made today breaks the CDK type check for two of the three MFA choices when the admin UI is included, and the token renewal path sends users to the login page on a failed refresh that the guard was written to ride out.

src/tasks/features.ts

🟢 Nit: Generator behavior with no test

Nothing asserts that Cognito is only offered with an AWS env, that cognitoSettings is null unless Cognito is chosen (the templates render with strict variables), the MFA prompt, the ignore list in src/tasks/synth.ts:52-74, or the build step after install. A typo in an ignore path stays green in CI.

Pre-existing, not introduced by this pull request, so not a condition for merging:

.github/workflows/release.yml:37

🟢 Nit: The new test step only runs after merge

The workflow triggers on push to main only, so pnpm test does not run on a pull request, and no workflow runs the four test-synth presets. Fix: add a pull_request trigger.

src/test-cli.ts:54

🟢 Nit: The test account id has 9 digits

accountId: "123456789". create-taxum-api#5 fixed the same value to 12 digits. It matters once a preset needs a valid ARN.

Cross-repo checks that came back clean (against create-taxum-api#5)

• CognitoIssuer, CognitoClientId and CognitoUserPoolArn exist as outputs for both the cognito and cognito-admin feature sets, with the names the API README asks for.
• The app sends the access token, not the ID token. The ID token only goes to the identity pool.
• The group name admin is the same in the CDK construct, useIsAdmin.ts and the API README.
• Every VITE_APP_* read is declared in vite-env.d.ts.liquid and present in .env.liquid for all four presets. The config schema matches CognitoRuntimeConfig.
• The ignore list globs match the real files, and no rendered file imports an ignored one in any preset.
• All 18 SDK commands the admin UI uses are in the admin role's action list.

Security sweep, nothing found: no secrets in source, no dangerouslySetInnerHTML, email templates escape the project title, the ListUsers filter is escaped, the identity pool has allowClassicFlow: false, resolveAmbiguousRoles: false, and a trust policy limited to this pool.

Already stated in the description, not repeated as findings: MFA cannot be reset in a required pool, the admin role can set passwords and attributes, authFlows: { userSrp: false }, the favicon attributes, the build after install for the route tree, SES verification, 7 day refresh tokens, localStorage token storage, and the commits shared with #5.

Dimensions

• D1 AC consistency: no ticket referenced.
• D2 design fidelity: skipped, no design mock is configured for this workspace.
• D3 and D3a ran on sibling comparison and first principles, because no rules shelf is configured.
• Reviewer Would Flag: patterns unavailable, skipped.
• D5: the skeleton has no test runner on main, so no gap is raised for React components.

No CI checks are reported on this branch.

[lavere]

🔴 Blocker: CDK type check fails when the admin UI is picked with MFA optional or off

Line 88 renders const mfa: Mfa = Mfa.OFF; (or Mfa.OPTIONAL). TypeScript narrows the const to that one member, so mfa === Mfa.REQUIRED here fails with TS2367, "the types 'Mfa.OFF' and 'Mfa.REQUIRED' have no overlap". Checked with tsc against the real aws-cdk-lib types: Mfa.OFF errors, Mfa.REQUIRED passes.

That breaks pnpm --dir=./cdk tsc --noEmit in ci-check.sh and the CDK build before deploy, for every new project with the admin UI and MFA optional or off.

It went unseen because src/test-cli.ts:72 renders every preset with mfa: "required".

Fix: render the literal, for example mfaRequired: {% if cognitoSettings.mfa == "required" %}true{% else %}false{% endif %},, and add an optional or off preset to test-cli.ts so the other two branches are rendered and type checked.

[lavere]

🟠 Should-fix: A failed renewal redirects to login while the current token is still valid

signinSilent from useAuth() does not throw. react-oidc-context catches the failure, dispatches an error and returns null, so the catch on line 74 never runs. Any failed renewal in the last 60 seconds of a token, including a network blip, falls through to signinRedirect here, and open form state is lost.

That is the opposite of what AuthGuard.tsx.liquid:65-68 promises (a transient failure keeps the page and retries) and of "A failed renewal is retried" in the description.

Fix: when the renewal returns null and current is not expired, send the request with current.access_token. Redirect only when the token has really expired or the error is invalid_grant.

[lavere]

🟠 Should-fix: The retry stops for good after a manual renewal times out

oidc-client-ts retries a timed out automatic renewal itself and raises no error for it (_tokenExpiring, retry in 5 seconds). So an ErrorTimeout only reaches this effect from a manual signinSilent(): this 30 second retry, or the one in useAuthenticatedFetch. Nothing retries those.

Sequence: the automatic renewal fails with a network error, the retry 30 seconds later hangs and times out, this effect returns without a timer, and renewal is dead until the tokens expire.

Fix: drop the ErrorTimeout condition, and correct the comment above, which says each failed attempt re-arms the retry.

[lavere]

🟠 Should-fix: A Cognito project without the admin UI is never told how to create its first user

The README says users are invited by an administrator and self sign-up is disabled. The only instructions for creating a user, "First administrator" at line 102, sit inside the cognito-admin block. A project with Cognito alone deploys an app nobody can sign in to until someone works out aws cognito-idp admin-create-user.

Fix: move the admin-create-user command outside the admin block, and keep only the admin-add-user-to-group step tied to the admin UI.

[lavere]

❓ Open question: Every admin can join any group, so any future group with an IAM role is open to all admins

The admin role has AdminAddUserToGroup on the whole pool, so an admin can add anyone, themselves included, to any group. Today only admin carries an IAM role, so nothing escalates beyond what the description discloses.

README.md.liquid:74 invites projects to "Add further groups next to it". If one of those groups gets a stronger role, every admin can take it. Should the README or a comment at the group say so?

[lavere]

🟢 Nit: A failed second call reports failure after the first change was saved

If the delete-attributes call fails after the update succeeded, the user sees "Failed to update user" although the email was already changed. With nothing changed, no call is made but the page still says "User has been updated". Fix: skip the mutation when the form is not dirty.

[lavere]

🟢 Nit: The new test/ folder is never type checked

tsconfig.json includes only src/**/* and tsx does not type check. Fix: a tsconfig for tests, or run tsc --noEmit over test.

[lavere]

🟢 Nit: useCurrentUserGroups is exported but only used in this file

Fix: drop the export.

[lavere]

🟢 Nit: A leftover test-synth/ folder is no longer ignored

/test-synth became /test-synth-*, so a folder from before this PR shows as untracked. Fix: keep /test-synth ignored too.

[lavere]

🟢 Nit: The commented-out ses line breaks on a title with a quote

fromName: "{{project.title}}" is unescaped inside a string, so uncommenting it in a project whose title has a double quote gives invalid code. The same line is in env-production.ts.liquid:33. Fix: render the title through a JSON filter.

[DASPRiD]

This is a solid, carefully built Cognito integration. The security-sensitive parts look right to me: the identity pool only maps the admin group to a role, ambiguous roles are denied, classic flow is off, and the role is scoped to the one user pool. I found nothing that should block the merge. The main gap is the "Optional" MFA choice: no generated project gives users a way to enroll an authenticator. CI also runs only one small unit test, and none of the synth presets that carry almost all of this change. The rest is small cleanup.

Nits:

• src/tasks/synth.ts:28 Full production build only to regenerate the route tree

  pnpm vite build runs a complete production build of the new project during synth. That costs time, and it leaves dist/ and the visualizer's stats.html behind in the fresh project. With Sentry enabled and SENTRY_AUTH_TOKEN set in the developer's shell, it also turns on the Sentry plugin, before the project is a git repo and with no BITBUCKET_COMMIT.

  Running only the TanStack Router generator (tsr generate), or deleting dist afterward, would keep synth side-effect free.

• skeleton/src/routes/admin/users_/$username/-components/RolesCard.tsx:54 This catch can't run: signinSilent resolves to null

  react-oidc-context's signinSilent catches errors itself. It dispatches them to the auth state and resolves to null, as the comment in useAuthenticatedFetch.ts.liquid already notes. So this try/catch never catches anything, and the comment describes a path that never runs. A failed refresh shows up as error.source === "signinSilent" in AuthGuard instead. Checking the resolved value (or just awaiting it) would match what actually happens. The same pattern is in edit.tsx at line 81.

• skeleton/src/routes/admin/users_/$username/edit.tsx:76 Comment says the app bar shows the name; it shows only the email

  UserMenu renders auth.user?.profile.email and nothing else from the token, so "reads name and email" is wrong about the name. The try/catch below it also can't run, because signinSilent resolves to null instead of throwing.

• skeleton/src/mutations/admin-user.ts:28 Inline option type in invalidateUser

  { refetchDetail?: boolean } is an inline object shape in the signature. A named alias such as InvalidateUserOptions above the function matches the named-type pattern the rest of the file follows (UsernameValues, CreateAdminUserValues, …).

• src/tasks/features.ts:13 Name the cognitoSettings shape

  FeaturesContext.cognitoSettings inlines its object shape. A named CognitoSettings type would also let featuresTask (FeaturesContext["cognitoSettings"]) and test-cli.ts refer to it directly.

• src/test-cli.ts:40 Preset shape is written out twice

  { features: Feature[]; mfa?: MfaMode } appears here and in the satisfies clause on line 28. One named Preset type would keep both in sync.

• skeleton/src/routes/admin/-components/UserStatusChips.tsx:6 Inline value type in statuses record

  The record's value type { label: string; color: ChipProps["color"] } is inline, and the fallback on line 20 repeats that shape. A named StatusChip type would cover both.

• src/tasks/synth.ts:61 signOut.ts filename isn't dash-case like its sibling

  src/utils/signOut.ts exports a plain function, so by filename convention it should be sign-out.ts. It sits next to the new dash-case src/utils/cognito-error.ts in the same directory. The ignore list entry here and the imports in UserMenu and AuthGuard would need to follow the rename.

[DASPRiD]

Non-blocking: "Optional" MFA leaves users no way to enroll

With mfa: "optional", nothing in the generated project lets a user set up an authenticator:

• cognito-auth.ts.liquid says managed login only walks users through setup when MFA is required.
• The app requests only openid email profile, so the access token lacks aws.cognito.signin.user.admin. Without that scope the user can't call AssociateSoftwareToken/VerifySoftwareToken themselves.
• There is no MFA setup UI.

So "Optional" behaves exactly like "Off", except the pool reports MFA as optional. It also means UserMFASettingList stays empty and Reset MFA in the admin UI is always disabled. Either drop the choice, or add a self-service enrollment path (with the extra scope) and mention it in the README.

[DASPRiD]

Non-blocking: CI covers only validateDomainPrefix

The new Test step runs test/features.test.ts, and that file only exercises validateDomainPrefix. Most of the change is template output: the Liquid conditionals across main.tsx, AuthGuard, useAuthenticatedFetch, app-stack.ts and cognito-auth.ts, plus the per-feature ignoreList entries such as the admin-*.ts globs. None of that is checked in CI.

The new presets in test-cli.ts would catch a broken template, but CI never runs them. Consider a CI job that synthesizes each preset and runs tsc/biome on it, at least for none, auth0, cognito and cognito-admin. suggestDomainPrefix has no test either.