Skip to content

Standardize error responses in Middleware. Add case for unprovisioned users and session expiration#360

Closed
tarratsco wants to merge 1 commit into
CMS-Enterprise:mainfrom
tarratsco:tarratsco/bugfix/403-fix-misleading-err-msg-for-unprov-users
Closed

Standardize error responses in Middleware. Add case for unprovisioned users and session expiration#360
tarratsco wants to merge 1 commit into
CMS-Enterprise:mainfrom
tarratsco:tarratsco/bugfix/403-fix-misleading-err-msg-for-unprov-users

Conversation

@tarratsco

@tarratsco tarratsco commented Jun 22, 2026

Copy link
Copy Markdown

Summary

Closes the BE half of CMS-Enterprise/ztmf-ui#403.

Pre-existing bug: the auth middleware collapsed three semantically distinct failures into one response — http.Error(w, "unauthorized", 401) for a missing/invalid session, for an IdP-authed identity with no ZTMF row, and for a soft-deleted account. The FE had no signal to tell them apart, so a valid IdP session with no app account rendered as "Your session has expired" with the infinite Sign in loop documented in the issue.

This PR splits those cases into distinguishable responses: 401 + UNAUTHORIZED for no/invalid session (semantics unchanged), 403 + ACCOUNT_NOT_PROVISIONED for an authenticated identity with no usable app account (never-provisioned and soft-deleted collapse to the same code, distinguished in server logs only), and 500 for upstream lookup failures that aren't a credential problem. CSRF rejections also gain a typed code (FORBIDDEN_ORIGIN).

Frontend dependency

Pairs with the FE handling change: CMS-Enterprise/ztmf-ui#418. The new code field only has user-visible effect once the FE starts reading it.

Deploy order: FE first, BE second. Mirrors the deploy order stated in #418. With #418 deployed first against the current (unchanged) BE, every new FE code path stays dormant, since the BE never emits code, so the FE falls back to existing behavior and matches today. Once this BE lands, ACCOUNT_NOT_PROVISIONED drives the NO_ACCOUNT terminal state and the loop is broken. The reverse order (this BE first, #418 second) is worse than the current bug: the new 403 + code is unrecognized by the old FE interceptor, which surfaces the BE message via the OOS toast on subsequent /api/* calls, causing a confusing toast on a stuck page instead of the familiar (if misleading) loop. Coordinated single-deploy is best.

Changes

  • backend/cmd/api/internal/auth/middleware.go
    • Added typed response codes: CodeUnauthorized, CodeForbiddenOrigin, CodeAccountNotProvisioned. Mirrored on the FE in utils/authCodes.ts.
    • Replaced http.Error(...) plain-text rejections with writeJSONError(...). Single shape {error, code} across 401 / 403 / 500. Sets X-Content-Type-Options: nosniff on the response.
    • Split the post-lookup error handling: errors.Is(err, model.ErrNoData) → 403 ACCOUNT_NOT_PROVISIONED; any other non-nil err → 500 (opaque, no code, so the FE falls through to ServerErrorPage rather than misclassifying as auth-expired). Local-dev auto-create path unchanged.
    • Soft-deleted user now 403 + ACCOUNT_NOT_PROVISIONED (was 401). Same response as never-provisioned; log line distinguishes them for ops/support without exposing the distinction to clients.
    • CSRF origin rejection now 403 + FORBIDDEN_ORIGIN (was plain "forbidden").
    • Lookup calls go through package-level findUserByID / findUserByEmail vars so tests can stub without a database. Compile-time signature assertion at the bottom of the file guards against future signature drift in the model package.
  • backend/cmd/api/internal/auth/middleware_test.go
    • New TestMiddleware covering the three contract cases from #403 plus three regression bonuses:
      • no auth → 401 UNAUTHORIZED
      • bearer present but invalid token → 401 UNAUTHORIZED
      • authed + unprovisioned → 403 ACCOUNT_NOT_PROVISIONED
      • authed + provisioned → next handler called
      • authed + soft-deleted → 403 ACCOUNT_NOT_PROVISIONED
      • authed + lookup errors (non-ErrNoData) → 500 (no code)

Test plan

Backend unit tests:

  • middleware_test.TestMiddleware covers all six cases above.
  • Existing TestClaimsFromRequest, TestIsSafeMethod, TestSameOrigin, plus auth/{ratelimit,session,token}_test.go and controller/auth_test.go continue to pass.

go test ./cmd/api/... and go vet ./cmd/api/... green.

Container smoke (compose-dev rebuilt against this branch). First, mint a bearer for the local admin:

cd ztmf-ui && make frontend-env EMAIL=Grand.Moff@DeathStar.Empire
# Uncomment VITE_AUTH_TOKEN3 in .env.development.local, then:
export TOKEN='<paste the JWT>'
  • No auth → 401 UNAUTHORIZED
    curl -s -i 'http://localhost:8080/api/v1/users/current' | head -20
    # HTTP/1.1 401 Unauthorized
    # Content-Type: application/json
    # X-Content-Type-Options: nosniff
    # {"error":"Your session has expired. Please sign in again.","code":"UNAUTHORIZED"}
  • Invalid bearer → 401 UNAUTHORIZED (same shape)
    curl -s -i -H "Authorization: Bearer not.a.real.token" \
      'http://localhost:8080/api/v1/users/current' | head -20
  • Provisioned bearer → 200 (regression check)
    curl -s -H "Authorization: Bearer $TOKEN" \
      'http://localhost:8080/api/v1/users/current' | jq
    # {"data": {"userid": "...", "email": "Grand.Moff@DeathStar.Empire", ...}}
  • Soft-deleted user → 403 ACCOUNT_NOT_PROVISIONED
    # Soft-delete a seeded user
    docker exec backend-postgre-1 psql -U admin -d ztmf -p 54321 -c \
      "UPDATE users SET deleted = true WHERE email = 'Grand.Admiral.Thrawn@chiss.empire';"
    
    # Mint their token, paste below
    cd ztmf-ui && make frontend-env EMAIL=Grand.Admiral.Thrawn@chiss.empire
    export GHOST_TOKEN='<paste>'
    
    curl -s -i -H "Authorization: Bearer $GHOST_TOKEN" \
      'http://localhost:8080/api/v1/users/current' | head -20
    # HTTP/1.1 403 Forbidden
    # {"error":"Your ZTMF account is no longer active. Contact your administrator.","code":"ACCOUNT_NOT_PROVISIONED"}
    
    # Restore
    docker exec backend-postgre-1 psql -U admin -d ztmf -p 54321 -c \
      "UPDATE users SET deleted = false WHERE email = 'Grand.Admiral.Thrawn@chiss.empire';"
  • Never-provisioned bearer → 403 ACCOUNT_NOT_PROVISIONED
    The local-dev auto-create branch masks this path under ENVIRONMENT=local, so flip the env temporarily.
    # Edit backend/dev.compose.env: ENVIRONMENT=local → ENVIRONMENT=test
    docker compose -f backend/compose-dev.yml restart api
    
    cd ztmf-ui && make frontend-env EMAIL=nobody@nowhere.test
    export GHOST_TOKEN='<paste>'
    
    curl -s -i -H "Authorization: Bearer $GHOST_TOKEN" \
      'http://localhost:8080/api/v1/users/current' | head -20
    # HTTP/1.1 403 Forbidden
    # {"error":"Your ZTMF account is not set up. Contact your administrator to request access.","code":"ACCOUNT_NOT_PROVISIONED"}
    
    # Revert backend/dev.compose.env: ENVIRONMENT=test → ENVIRONMENT=local
    docker compose -f backend/compose-dev.yml restart api
  • Controller-level 403 stays unchanged (regression check, as this confirms the FE's "code present = middleware, code absent = controller" discriminator)
    Mint a non-admin bearer (e.g. Admiral.Piett@executor.empire, role ISSO), hit an admin-only route:
    cd ztmf-ui && make frontend-env EMAIL=Admiral.Piett@executor.empire
    export ISSO_TOKEN='<paste>'
    
    curl -s -i -H "Authorization: Bearer $ISSO_TOKEN" \
      'http://localhost:8080/api/v1/users' | head -20
    # HTTP/1.1 403 Forbidden
    # {"data":null,"error":"forbidden"}        <- no `code` field, unchanged shape

End-to-end with #418 applied locally:

  • Sign in as a normal admin, soft-delete the row in another shell, reload → NoAccountTerminal renders the BE message verbatim, no Sign in button, no infinite loop. Restore the row → normal flow resumes.

Notes for reviewers

  • The response-shape split between middleware and controllers is intentional. Middleware-rejected responses carry {error, code}. Controller-rejected responses (everything through controller.respond) stay at {data, error} with no code field. Presence/absence of code is the FE's discriminator. Widening code to controller responses is a separate future change; not needed for this ticket.
  • error is user-facing copy here. Because the paired FE renders the BE message verbatim on the NoAccountTerminal screen, the strings use sentence case with periods rather than the lowercase-no-period style of errors.New(...) sentinels elsewhere in the codebase. ST1005 does not apply, because these are response-body strings inside writeJSONError(...) calls, not error sentinels. The FE keeps a NO_ACCOUNT_FALLBACK_MESSAGE for resilience if the body is ever empty.
  • Never-provisioned and soft-deleted collapse to the same code. Same practical UX outcome ("contact your administrator"); the distinction is preserved in server logs so support can tell offboarded from never-onboarded without grepping the users table. Easy to split into a second code (ACCOUNT_REVOKED) later if a client-visible distinction is wanted.
  • Non-ErrNoData lookup errors now 500. Previously a DB connection blip during the user lookup was reported to the FE as 401, misclassifying server failures as auth problems and contributing to the misleading "session expired" UX. The FE now correctly falls through to ServerErrorPage for these.
  • CSRF check is semantically unchanged, but now returns a structured JSON body with FORBIDDEN_ORIGIN so the FE can surface a defensive message rather than the generic 403 toast. Paired FE PR wires this to a console.error + permission toast.

…dd case for unprovisioned users and session expiration
tarratsco pushed a commit to tarratsco/ztmf-ui that referenced this pull request Jun 22, 2026
…authenticated users now hit a terminal "contact your administrator" state at /signin instead of looping through "Your session has expired".
@tarratsco tarratsco changed the title enhancement(middleware): Standardize error responses in Middleware. Add case for unprovisioned users and session expiration Standardize error responses in Middleware. Add case for unprovisioned users and session expiration Jun 23, 2026
tarratsco pushed a commit to tarratsco/ztmf-ui that referenced this pull request Jun 23, 2026
…authenticated users now hit a terminal "contact your administrator" state at /signin instead of looping through "Your session has expired".
@tarratsco tarratsco marked this pull request as ready for review June 23, 2026 17:33
@danielbowne

Copy link
Copy Markdown
Collaborator

Thanks @tarratsco. Rehomed onto an in-repo branch as #362 so Snyk/CI (org secrets unavailable on forks) can run; your commit is preserved as-authored. One small addition there: the no-auth 401 emberfall assertion needed updating to match the new JSON error body. Continuing review on #362.

danielbowne added a commit that referenced this pull request Jun 23, 2026
…d expired sessions (#362)

Rehome of #360 onto an in-repo branch so Snyk/CI run. Original work and
credit: @tarratsco (Onix Tarrats Calderon); his commit is preserved
as-authored.

## What this does

Middleware now returns a single standardized JSON error shape (`{
"error": ..., "code": ... }`) on every rejection, replacing the prior
plaintext `http.Error` responses. This lets the frontend interceptor
branch on a stable `code` rather than parsing status alone (see
ztmf-ui#403).

Rejection cases are now distinguished:
- Missing/invalid session: 401 `UNAUTHORIZED` ("your session has
expired")
- Authenticated identity with no ZTMF account, or a soft-deleted
account: 403 `ACCOUNT_NOT_PROVISIONED` (terminal "contact your
administrator", no retry loop)
- CSRF origin mismatch: 403 `FORBIDDEN_ORIGIN`
- Upstream lookup failure (DB blip, decode error): 500 with an opaque
body and no code, so a transient failure never triggers the terminal
provisioning copy

Previously a not-found user and a DB error both collapsed into 401,
which the frontend could not tell apart from an expired session.

## Changes layered on top of the original PR

- Added `test(emberfall)`: the no-auth 401 assertion on `/users/current`
was matching the old plaintext `unauthorized` body; updated it to match
the new JSON `code` field. (The original fork PR could not run the E2E
suite because Snyk/CI org secrets are unavailable on forks, so this
drift was not caught there.)

## Testing

- `make test-unit` (auth package): pass, 8/8 including the new
`TestMiddleware` cases
- `make test-e2e` (isolated ephemeral DB): pass, 118/118

Closes #360

---------

Co-authored-by: Onix Tarrats Calderon <onix.tarratscalderon@aquia.us>
danielbowne added a commit to CMS-Enterprise/ztmf-ui that referenced this pull request Jun 23, 2026
…d header (#428)

* enshancement(auth): Paired CMS-Enterprise/ztmf#360 Unprovisioned IdP-authenticated users now hit a terminal "contact your administrator" state at /signin instead of looping through "Your session has expired".

* fix(auth): surface backend error message for expired session in cold-load 401 path

* fix(auth): Remove misleading session expired message for unprovisioned users

---------

Co-authored-by: Onix Tarrats Calderon <onix.tarratscalderon@aquia.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants