mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-10 12:08:54 +00:00
fix(auth/ux): cause-aware OIDC + session error surfacing (HIGH-7 + HIGH-8 closure)
Server (HIGH-7): the OIDC callback failure path now 302-redirects to /login?error=oidc_failed&reason=<category> instead of emitting a blank 400. `category` is the existing audit `failure_category` value; classifyOIDCFailure was extended with three new sentinel paths (email_domain_not_allowed, email_missing_but_required, pkce_invalid) so CRIT-5 + PKCE failures get distinguishable GUI rendering. Audit-log observability is unchanged — the same failure_category is written to the auth.oidc_login_failed audit row; the 302 is purely a UX leg layered on top. Server (HIGH-8): SessionMiddleware now stashes a cause classification on the request context when Validate returns an error, mapping the sentinels via classifySessionError (errors.Is-based, so wrapped sentinels still classify) to the stable wire-strings idle_timeout / absolute_timeout / back_channel_revoked / invalid_token. The 401 emit point in bearerSkipIfAuthenticated reads the stashed cause and emits WWW-Authenticate: Bearer realm="certctl", error="invalid_token", error_description=<cause> per RFC 6750 §3. GUI (HIGH-7): LoginPage reads ?error= + ?reason= from the URL via react-router useSearchParams and renders an operator-friendly amber-bordered banner above the form; OIDC_FAILURE_REASON_TEXT maps all 16 known categories with a defensive 'unspecified' fallback for forward-compat with future server-side categories. GUI (HIGH-8): api/client fetchJSON parses the WWW-Authenticate cause via parseWWWAuthenticateCause and attaches it to the 'certctl:auth-required' CustomEvent detail; AuthProvider redirects to /login?session_expired=<cause> on cause-aware 401s; LoginPage renders a blue-bordered session-cause banner. invalid_token stays on the current page (no hard redirect for opaque failures). Misc cleanup: ErrorState now accepts the title/message/data-testid form added by CRIT-4 BreakglassPage (was erroring tsc on master). Regression matrix: - internal/api/handler/oidc_redirect_categories_test.go pins all 16 failure categories to the 302 + reason= location + audit-row leg - internal/auth/session/www_authenticate_test.go pins the 4 stable cause categories on classifySessionError (incl. errors.Is wrapped sentinels) + the WWW-Authenticate emission across all 4 categories + the no-session-context fallback case - internal/api/handler/auth_session_oidc_test.go: 4 pre-existing TestLoginCallback_*Returns400 tests updated to assert 302 + reason= location (the wire shape changed from 400 to 302, but the audit observability and behaviour-equivalent failure-classification are preserved) - web/src/pages/LoginPage.test.tsx: 6 new cases pinning the failure banner, session-cause banner, unknown-reason fallback, and forward-compat 'unspecified' category Spec: cowork/auth-bundles-fixes-2026-05-10/08-high-7-8-error-surfacing.md Closes: HIGH-7, HIGH-8 of cowork/auth-bundles-audit-2026-05-10.md
This commit is contained in:
shankar0123
@@ -32,6 +32,7 @@ package session
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net/http"
|
||||
|
||||
"github.com/certctl-io/certctl/internal/auth"
|
||||
@@ -93,7 +94,13 @@ func NewSessionMiddleware(svc SessionValidator) func(http.Handler) http.Handler
|
||||
// the next middleware so a valid Bearer can still
|
||||
// authenticate. The auth combinator 401s if neither
|
||||
// works.
|
||||
next.ServeHTTP(w, r)
|
||||
//
|
||||
// Audit 2026-05-10 HIGH-8 — stash the cause classification
|
||||
// in context so the 401 emitter can emit a
|
||||
// WWW-Authenticate: Bearer error_description="<cause>"
|
||||
// header. OIDC users get cause-aware re-login UX.
|
||||
ctx := context.WithValue(r.Context(), sessionCauseKey{}, classifySessionError(verr))
|
||||
next.ServeHTTP(w, r.WithContext(ctx))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -225,6 +232,15 @@ func bearerSkipIfAuthenticated(bearerMW func(http.Handler) http.Handler) func(ht
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
// Audit 2026-05-10 HIGH-8 — emit WWW-Authenticate with the
|
||||
// classified cause so the GUI can render OIDC-aware
|
||||
// re-login UX. RFC 6750 §3 challenge format.
|
||||
cause, _ := r.Context().Value(sessionCauseKey{}).(string)
|
||||
if cause == "" {
|
||||
cause = "invalid_token"
|
||||
}
|
||||
w.Header().Set("WWW-Authenticate",
|
||||
`Bearer realm="certctl", error="invalid_token", error_description="`+cause+`"`)
|
||||
w.Header().Set("Content-Type", "application/json; charset=utf-8")
|
||||
http.Error(w, `{"error":"Authentication required"}`, http.StatusUnauthorized)
|
||||
})
|
||||
@@ -238,12 +254,42 @@ func bearerSkipIfAuthenticated(bearerMW func(http.Handler) http.Handler) func(ht
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
// Defer to Bearer.
|
||||
// Defer to Bearer. If the Bearer middleware 401s and there's
|
||||
// a stashed session cause, downstream callers see it via the
|
||||
// context key; the Bearer middleware's own 401 doesn't read
|
||||
// it (Bearer-only deployments have no session context to
|
||||
// stash from). Cause-aware UX needs session-mode auth.
|
||||
bearerInner.ServeHTTP(w, r)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// sessionCauseKey is the context key used by Audit 2026-05-10 HIGH-8.
|
||||
// SessionMiddleware stashes the failure-cause classification on the
|
||||
// context when Validate returns an error; the 401 emitter reads it
|
||||
// and renders WWW-Authenticate's error_description.
|
||||
type sessionCauseKey struct{}
|
||||
|
||||
// classifySessionError maps a session Validate error to a stable
|
||||
// wire-string the GUI consumes to render OIDC-aware re-login UX.
|
||||
// Stable categories: idle_timeout, absolute_timeout,
|
||||
// back_channel_revoked, invalid_token.
|
||||
func classifySessionError(err error) string {
|
||||
if err == nil {
|
||||
return ""
|
||||
}
|
||||
switch {
|
||||
case errors.Is(err, ErrSessionExpiredIdle):
|
||||
return "idle_timeout"
|
||||
case errors.Is(err, ErrSessionExpiredAbsolute):
|
||||
return "absolute_timeout"
|
||||
case errors.Is(err, ErrSessionRevoked):
|
||||
return "back_channel_revoked"
|
||||
default:
|
||||
return "invalid_token"
|
||||
}
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
// Helpers.
|
||||
// =============================================================================
|
||||
|
||||
@@ -0,0 +1,135 @@
|
||||
package session
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
sessiondomain "github.com/certctl-io/certctl/internal/auth/session/domain"
|
||||
)
|
||||
|
||||
// Audit 2026-05-10 HIGH-8 regression tests pinning the cause-aware
|
||||
// WWW-Authenticate header. Pre-fix, every session-cookie failure
|
||||
// emitted a generic 401 with no machine-readable cause; OIDC users
|
||||
// who hit idle-timeout / absolute-timeout / back-channel-revoked
|
||||
// got an indistinguishable "Authentication required" with no hint
|
||||
// about how to recover. Post-fix, the 401 emitter sets:
|
||||
//
|
||||
// WWW-Authenticate: Bearer realm="certctl", error="invalid_token",
|
||||
// error_description="<cause>"
|
||||
//
|
||||
// where <cause> ∈ {idle_timeout, absolute_timeout,
|
||||
// back_channel_revoked, invalid_token}. The GUI reads this on its
|
||||
// fetch wrapper and routes the user into OIDC re-login (vs a generic
|
||||
// "logged out" notice) when the cause is BCL revocation.
|
||||
|
||||
// classifySessionError direct-test matrix — pin the four stable
|
||||
// wire-strings the GUI consumes.
|
||||
func TestClassifySessionError_StableCategories(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
err error
|
||||
want string
|
||||
}{
|
||||
{"nil", nil, ""},
|
||||
{"idle", ErrSessionExpiredIdle, "idle_timeout"},
|
||||
{"absolute", ErrSessionExpiredAbsolute, "absolute_timeout"},
|
||||
{"revoked", ErrSessionRevoked, "back_channel_revoked"},
|
||||
{"opaque", errors.New("totally-other-cause"), "invalid_token"},
|
||||
// Wrapped sentinels still classify (errors.Is).
|
||||
{"wrapped_idle", wrap(ErrSessionExpiredIdle, "outer"), "idle_timeout"},
|
||||
{"wrapped_revoked", wrap(ErrSessionRevoked, "outer"), "back_channel_revoked"},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
got := classifySessionError(tc.err)
|
||||
if got != tc.want {
|
||||
t.Errorf("classifySessionError(%v) = %q; want %q",
|
||||
tc.err, got, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// HIGH-8: a 401 emitted from bearerSkipIfAuthenticated when no
|
||||
// Bearer middleware is wired must carry WWW-Authenticate with
|
||||
// error_description=<cause> when the upstream SessionMiddleware
|
||||
// stashed a cause classification.
|
||||
func TestBearerSkipIfAuthenticated_Emits_WWWAuthenticate_WithCause(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
sessErr error
|
||||
wantCause string
|
||||
}{
|
||||
{"idle_timeout", ErrSessionExpiredIdle, "idle_timeout"},
|
||||
{"absolute_timeout", ErrSessionExpiredAbsolute, "absolute_timeout"},
|
||||
{"back_channel_revoked", ErrSessionRevoked, "back_channel_revoked"},
|
||||
{"opaque_falls_back_to_invalid_token", errors.New("opaque"), "invalid_token"},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
stub := &stubSessionValidator{validateErr: tc.sessErr}
|
||||
// Bearer middleware nil so the chain emits its own 401.
|
||||
chain := ChainAuthSessionThenBearer(NewSessionMiddleware(stub), nil)(markAuthenticated())
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.AddCookie(&http.Cookie{
|
||||
Name: sessiondomain.PostLoginCookieName,
|
||||
Value: "v1.ses.sk.bad",
|
||||
})
|
||||
w := httptest.NewRecorder()
|
||||
chain.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("status = %d; want 401", w.Code)
|
||||
}
|
||||
ww := w.Header().Get("WWW-Authenticate")
|
||||
if !strings.Contains(ww, `Bearer realm="certctl"`) {
|
||||
t.Errorf("WWW-Authenticate = %q; want Bearer realm=\"certctl\"", ww)
|
||||
}
|
||||
if !strings.Contains(ww, `error="invalid_token"`) {
|
||||
t.Errorf("WWW-Authenticate = %q; want error=\"invalid_token\"", ww)
|
||||
}
|
||||
wantDesc := `error_description="` + tc.wantCause + `"`
|
||||
if !strings.Contains(ww, wantDesc) {
|
||||
t.Errorf("WWW-Authenticate = %q; want %s", ww, wantDesc)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// HIGH-8: a 401 emitted with NO upstream session context (no cookie
|
||||
// at all) still carries WWW-Authenticate, but with the
|
||||
// invalid_token fallback (no stashed cause).
|
||||
func TestBearerSkipIfAuthenticated_NoSessionContext_FallsBackToInvalidToken(t *testing.T) {
|
||||
stub := &stubSessionValidator{validateErr: ErrSessionInvalidCookie}
|
||||
chain := ChainAuthSessionThenBearer(NewSessionMiddleware(stub), nil)(markAuthenticated())
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
// No cookie at all → SessionMiddleware skips entirely and falls
|
||||
// through; bearerSkipIfAuthenticated emits 401 without a stashed
|
||||
// cause; should fall back to error_description="invalid_token".
|
||||
w := httptest.NewRecorder()
|
||||
chain.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("status = %d; want 401", w.Code)
|
||||
}
|
||||
ww := w.Header().Get("WWW-Authenticate")
|
||||
if !strings.Contains(ww, `error_description="invalid_token"`) {
|
||||
t.Errorf("WWW-Authenticate = %q; want fallback error_description=\"invalid_token\"", ww)
|
||||
}
|
||||
}
|
||||
|
||||
// wrap is a tiny errors.Wrap-style helper used by the wrapped-sentinel
|
||||
// classifier matrix above. We can't pull in fmt.Errorf with %w as a
|
||||
// const here, so this is the local convenience.
|
||||
func wrap(inner error, outer string) error {
|
||||
return &wrappedErr{inner: inner, outer: outer}
|
||||
}
|
||||
|
||||
type wrappedErr struct {
|
||||
inner error
|
||||
outer string
|
||||
}
|
||||
|
||||
func (w *wrappedErr) Error() string { return w.outer + ": " + w.inner.Error() }
|
||||
func (w *wrappedErr) Unwrap() error { return w.inner }
|
||||
Reference in New Issue
Block a user