Skip to content

feat(auth): FAM/Cognito SPA sign-in via Amplify (Story 1.2) - #300

Merged
gpascucci merged 12 commits into
mainfrom
feat/fam-auth-1-2-signin
Aug 18, 2026
Merged

feat(auth): FAM/Cognito SPA sign-in via Amplify (Story 1.2)#300
gpascucci merged 12 commits into
mainfrom
feat/fam-auth-1-2-signin

Conversation

@gpascucci

@gpascucci gpascucci commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Story 1.2 — Sign in with FAM/Cognito (frontend)

Adds the real Amplify (Cognito OIDC) auth provider beside the mock, behind a single AuthProvider seam. Co-deploys with Story 1.1 (#290) to flip ilcr.security.enabled on.

What's here

  • Auth seam — Real (Amplify v6) vs Mock chosen by a runtime double-gate (window.amplifyConfig.mockUser === true && isLocalHost); deployed builds are never mock. Unified useAuth context keeps user/hasRole and adds isAuthenticated/isLoading/signIn/signOut.
  • Sign-in — Hosted UI (signInWithRedirect, auth-code + PKCE, openid); fetchAuthSessionID token; identity/roles from GET /api/v1/me (backend is source of truth).
  • Token attach — ID-token Bearer on every APIService request; a 401 forces one refresh and retries, bouncing to sign-in only if the refresh token is gone (no mid-form data loss). Mock mode keeps X-Mock-Groups.
  • Deep-link preserved via customState; sign-out runs the OAuth logout chain (ends the upstream session).
  • Mock role-selector renders only under the mock provider (absent from real builds); a test asserts it does not render under the real provider.
  • Runtime config via window.amplifyConfig (same image every env; ConfigMap overrides per env; no secrets committed).

Domain note

Removed the combined admin/submitter mock user — a user holds exactly one ILCR role (ILCR_ADMIN or ILCR_SUBMITTER).

⚠️ Deploy prerequisite (before the 1.1+1.2 flip)

The frontend OpenShift ConfigMap must supply the real Cognito pool/client/domain/redirects with mockUser:false — analogous to the backend COGNITO_CLIENT_ID. Until then real aud validation and sign-in aren't active in a deployed env.

Test plan

  • New: env, amplify-initializer, AuthProvider, RealAuthProvider, MockUserSelector, api-service.auth tests (19 tests) covering authed/unauthenticated-bounce/refresh/logout/deep-link/mock-selector-absent-under-real.
  • Full unit suite 782/782; changed-file coverage 86.5%; lint clean.
  • Adds aws-amplify@^6 (1 transitive npm audit high — follow-up).

🤖 Generated with Claude Code


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

Add a real Amplify (Cognito OIDC) auth provider beside the mock, behind a single
AuthProvider seam selected by a runtime double-gate (window.amplifyConfig.mockUser
&& localhost) — deployed builds are never mock.

- Unified useAuth context (user, isAuthenticated, isLoading, hasRole, signIn,
  signOut); the old mock-only context/hook are removed.
- RealAuthProvider: Hosted-UI sign-in (signInWithRedirect, auth-code/PKCE,
  openid); fetchAuthSession -> ID token; identity/role from GET /api/v1/me
  (backend is source of truth); signOut() runs the OAuth logout chain (ends the
  upstream session); Hub handles signedIn/signedOut/tokenRefresh_failure;
  deep-link preserved via customState; unauthenticated visitors auto-bounce
  (guarded against the OAuth-callback race).
- APIService: mock keeps X-Mock-Groups; real attaches the ID-token Bearer; a 401
  forces one refresh and retries, bouncing to sign-in only if the refresh token
  is gone (no mid-form data loss). Tokens are never logged.
- MockUserSelector renders only under the mock provider (absent from real
  builds); removed the combined admin/submitter mock user (one role per user).
- Runtime Cognito config via window.amplifyConfig (index.html script + local
  public/amplify-config.js template); no secrets committed. Real per-env values
  come from the OpenShift ConfigMap (deploy prerequisite for the 1.1+1.2 enable).

Adds aws-amplify ^6. Full unit suite 782/782; changed-file coverage 86.5%.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@Rylan-cgi Rylan-cgi left a comment

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.

The PR is excellent overall, but two infinite redirect loops must be fixed before
merging:


  1. Infinite Redirect Loop on /v1/me Gateway/Network Failures
  • Where: frontend/src/context/auth/RealAuthProvider.tsx (loadUser())
  • Problem: If Cognito has a valid session but /v1/me fails temporarily (500 error,
    gateway timeout, or network disconnect), the broad catch block sets user = null.
    This immediately triggers the mounting useEffect to call signIn(), redirecting to
    Cognito Hosted UI. Because the user's Cognito session cookie is valid, Cognito
    instantly redirects back to the app, which hits /v1/me again, fails, and
    redirects again—looping infinitely.
  • Fix: Check axios.isAxiosError(error). If it is a 5xx or network error, set a
    local connection error state and show a fallback screen instead of redirecting.

  1. Infinite Redirect Loop on 403 Forbidden / Unauthorized Users
  • Where: frontend/src/context/auth/RealAuthProvider.tsx (loadUser())
  • Problem: If a user logs into Cognito/FAM but is not mapped to any valid
    groups/roles in the directory, /v1/me rejects with 403 Forbidden (or returns
    empty roles). Setting user = null causes the app to treat them as unauthenticated
    and redirect back to Cognito. Cognito sees they are logged in and redirects back
    immediately, creating another infinite loop.
  • Fix: Catch the 403 (or check for roles.length === 0), set an unauthorized error
    state, and render an "Access Denied" page with a Sign Out button instead of
    redirecting.

Suggested Fix for RealAuthProvider.tsx
Replace the state initialization, loadUser function, redirect useEffect, and return
block in RealAuthProvider.tsx with this pattern:

  1   const [user, setUser] = useState<AuthUser | null>(null)
   2   const [isLoading, setIsLoading] = useState(true)
   3   const [error, setError] = useState<{ status?: number; message: string } |
     null>(null)
   4   const redirectingRef = useRef(false)
   5
   6   const loadUser = useCallback(async () => {
   7     setIsLoading(true)
   8     setError(null)
   9     try {
  10       const session = await fetchAuthSession()
  11       if (!session.tokens?.idToken) {
  12         setUser(null)
  13         return
  14       }
  15       const { data } = await
     apiService.getAxiosInstance().get<AuthUser>('/v1/me')
  16
  17       if (!data.roles || data.roles.length === 0) {
  18         setError({ status: 403, message: 'Access Denied: You do not have
     permissions mapped to this application.' })
  19         setUser(null)
  20         return
  21       }
  22       setUser(data)
  23     } catch (err: unknown) {
  24       if (axios.isAxiosError(err)) {
  25         const status = err.response?.status
  26         if (status === 401) {
  27           setUser(null) // Token expired: trigger login bounce
  28         } else if (status === 403) {
  29           setError({ status: 403, message: 'Access Denied: You do not have
     permission to access this application.' })
  30           setUser(null)
  31         } else {
  32           setError({ status, message: 'The server is temporarily unavailable.
     Please try again later.' })
  33           setUser(null)
  34         }
  35       } else {
  36         setUser(null)
  37       }
  38     } finally {
  39       setIsLoading(false)
  40     }
  41   }, [])
  42
  43   useEffect(() => {
  44     // Only bounce if unauthenticated AND no error is present
  45     if (!isLoading && !user && !error && !isOAuthCallback() &&
     !redirectingRef.current) {
  46       redirectingRef.current = true
  47       void signIn()
  48     }
  49   }, [isLoading, user, error, signIn])
  50
  51   // Render error screen to break the redirect loop
  52   if (error) {
  53     return (
  54       <div style={{ padding: '2rem', textAlign: 'center' }}>
  55         <h2>{error.status === 403 ? 'Access Denied' : 'Connection Error'}</h2>
  56         <p>{error.message}</p>
  57         <button onClick={error.status === 403 ? () => void doSignOut() : () =>
     void loadUser()}>
  58           {error.status === 403 ? 'Sign Out' : 'Retry'}
  59         </button>
  60       </div>
  61     )
  62   }

gpascucci and others added 6 commits August 17, 2026 10:23
…FAM config example

- amplify-initializer: sign-in still returns to the current origin in dev (the DEV
  client allow-lists http://localhost:3000/), but sign-out now uses the configured
  redirectSignOut verbatim. The bare origin is not a registered Cognito sign-out
  URL, so the dev override made local logout fail; the configured value carries the
  FAM/loginproxy logout chain and is registered.
- Add frontend/amplify-config.local.example.js: copy over public/amplify-config.js
  to run real Cognito login locally against the DEV client (public identifiers, not
  secrets; repo default stays mock).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For manual testing on a real IDIR login, let a dev override the role the SPA uses
(nav + route guards) without a re-login. It is FRONTEND-ONLY: the backend still
enforces the real token, so admin APIs still 403 for a non-admin — not a security
boundary.

Gated to import.meta.env.DEV: RealAuthProvider exposes devRoleSwitch (and applies
the override) only in local dev, so it is inert/tree-shaken in every deployed build
(including deployed DEV). The DevRoleSwitcher renders nothing when absent.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A persistent warning strip appears whenever the local-dev View-as override is on,
so it's obvious the SPA is not showing the real FAM role — it names the override
and the real role, and reminds that the backend still enforces the real one.
Renders only when devRoleSwitch.override is set (local dev); absent otherwise.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A header action (Logout icon) that calls signOut() — which runs the Cognito/
loginproxy logout chain, ending the upstream session, then redirects back. Shown
for a real authenticated session; hidden in mock mode (fixed dev user — use the
selector) and when unauthenticated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, sign-out)

Document the two auth modes and the local testing flow: mock-mode role switching,
real Hosted-UI login (copy amplify-config example + backend envs), and the
dev-only aids (View-as role override + banner, header sign-out button).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A failed /v1/me or session lookup previously nulled the user, which the
redirect effect read as "unauthenticated" and bounced to the Hosted UI —
but a valid Cognito session returns straight back, re-runs the failing
call, and loops. Distinguish the failure modes instead:

- 401: leave error null so the effect re-authenticates (token expired).
- 403: render an "Access denied" screen with Sign out (not a loop).
- 5xx / network / Amplify session failure: render a "Connection error"
  screen with Retry (not a loop).

The redirect effect now guards on `!error`, so only a genuinely
unauthenticated visitor is sent to sign-in. roles:[] stays a non-null
user (handled by the Story 1.3 no-access screen, not an error here).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gpascucci

Copy link
Copy Markdown
Contributor Author

Thanks @Rylan-cgi — both redirect loops are fixed in d900036.

loadUser() now classifies the failure instead of blindly nulling the user (which the redirect effect was reading as "unauthenticated" and bouncing on a still-valid Cognito session → loop):

  • 401 — leave error null so the effect re-authenticates (token genuinely expired/rejected).
  • 403 — render an "Access denied" screen with a Sign out button. No redirect.
  • 5xx / network / fetchAuthSession failure — render a "Connection error" screen with a Retry button. No redirect.

The redirect effect now guards on !error, so only a genuinely unauthenticated visitor is sent to the Hosted UI.

On the roles.length === 0 → error suggestion: I kept roles:[] as a non-null user rather than an error. A valid FAM token with no ILCR group isn't a failure — it's the O8 no-access case, and Story 1.3 renders a dedicated "no ILCR access — contact an administrator" screen for it (AC4). Treating it as an error here would duplicate that and fight the 1.3 RouteGuard/NoAccess design. Since the user stays non-null, there's no null→bounce loop either.

Tests added/updated in RealAuthProvider.test.tsx: session-lookup failure → connection error (asserts no signInWithRedirect); /me 500 → connection error + Retry re-attempts and authenticates; /me 403 → access-denied + Sign out. Full suite green (794 passing) and lint clean. Also merged forward into the 1.3 branch (#301).

@Rylan-cgi Rylan-cgi left a comment

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.

Issues fixed. Looks good!

…firing on no-op overrides

Two local-dev fixes for the "View as (dev)" role override:

- The banner rendered between the fixed brand header and <Content>, with
  no top offset, so the fixed header painted over it. Move it inside
  <Content> so it inherits the `.cds--header ~ .cds--content` offset.
- The banner fired on ANY stored override, including one equal to the
  role you already hold — e.g. a stale "view as ILCR_SUBMITTER" replayed
  from localStorage for a real submitter, which changes nothing. Show it
  only when the override actually differs from the real role set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@paulushcgcj paulushcgcj left a comment

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.

The auth flow now covers both local mock and Cognito paths, including token attachment, one-time refresh handling, deep-link restoration, and a recovery screen for /me failures. The focused tests and production build pass, and the retry guard is a particularly useful protection against recursive 401 handling. I found two runtime/deployment edge cases inline.

Comment thread frontend/src/config/auth/amplify-initializer.ts Outdated
// Hosted UI if the refresh token itself is gone — so a submitter mid-form is not evicted (O7).
original.retriedAfterRefresh = true
try {
const session = await fetchAuthSession({ forceRefresh: true })

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.

The one-retry boundary is a strong improvement. Marking the original request before retrying prevents a persistent 401 from recursively re-entering the refresh flow, while still giving a valid refreshed session one chance to complete the request.

Comment thread frontend/src/service/api-service.ts Outdated
…ken refresh

Addresses two runtime/deployment edge cases from PR review (paulushcgcj):

1. configureAmplify() now fails loudly when redirectSignIn or redirectSignOut
   is absent, alongside pool/client/domain. Previously a missing redirect URL
   reached Amplify as the literal string "undefined" and only surfaced mid-flow
   at the Hosted UI; the guard makes a deployment misconfig deterministic.

2. The 401 forced-refresh path no longer bounces to the Hosted UI on every
   fetchAuthSession({forceRefresh:true}) rejection. A transient network/provider
   outage would otherwise discard the user's route and start a fresh login even
   though the refresh token still exists. Now, on a refresh error we bounce only
   when Amplify confirms no usable session remains (hasUsableSession); a still-
   present session preserves the route and lets the 401 surface (O7).

Tests: missing redirectSignIn/redirectSignOut → throws; transient refresh
failure with a live session → no signInWithRedirect; refresh failure with no
session → bounces. Full suite 798 passing, lint clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gpascucci

Copy link
Copy Markdown
Contributor Author

Thanks @paulushcgcj — both edge cases are fixed in 6d0bfc4.

1. Redirect config guarded up front (amplify-initializer.ts). configureAmplify() now includes redirectSignIn and redirectSignOut in the fail-loud guard alongside pool/client/domain, and no longer coerces them through String(...). A ConfigMap that omits either now throws a named error at startup (… missing one of: … redirectSignIn / redirectSignOut) instead of letting the literal "undefined" reach Amplify and only failing mid-flow at the Hosted UI. Added a missing-value test for each.

2. Transient refresh no longer discards the route (api-service.ts). You're right that the old catch bounced on every fetchAuthSession({forceRefresh:true}) rejection. Now, on a forced-refresh error I check hasUsableSession() (a plain non-forced fetchAuthSession): if a session still remains — including a cached-but-expired id token — I preserve the route and reject the 401 to the caller rather than redirecting. I only bounce to the Hosted UI when Amplify confirms no usable session remains (the refresh path returns no token, or the non-forced lookup also comes back empty). This matches the intended only-bounce-when-the-refresh-token-is-gone behavior.

Tests added: transient forced-refresh failure with a live session → no signInWithRedirect; forced-refresh failure with no session → bounces (plus the existing forceRefresh-returns-no-token → bounce case). Full frontend suite 798 passing, lint clean. Also merged forward into the 1.3 branch (#301).

gpascucci and others added 2 commits August 18, 2026 09:43
… URL

The data-independent @smoke opens the app and expects the shell chrome. It
was pointed at the deployed PR URL, but Story 1.2 turned auth on and
isMockAuth() double-gates on isLocalHost() (src/env.ts) — so on a deployed
(non-localhost) host an anonymous visit correctly bounces to the FAM/Cognito
Hosted UI and the shell never renders, failing the smoke on every 1.2 commit.

Serve the app on localhost with the repo-default { mockUser: true } config so
mock auth engages and the shell renders client-side; the scenarios still abort
every /api call, keeping the smoke auth- AND backend-independent. Dropped the
deployed E2E_BASE_URL so Playwright's baseURL defaults to http://localhost:3000.

Verified locally: `--project=smoke` passes against a localhost dev server.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gpascucci
gpascucci merged commit 1f2f32e into main Aug 18, 2026
26 checks passed
@gpascucci
gpascucci deleted the feat/fam-auth-1-2-signin branch August 18, 2026 17:00
@DerekRoberts DerekRoberts moved this to Done in DevOps (NR) Aug 18, 2026
@github-project-automation github-project-automation Bot moved this from Done to Waiting in DevOps (NR) Aug 18, 2026
@DerekRoberts DerekRoberts moved this from Waiting to Done in DevOps (NR) Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants