diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b1cf2c9..4d3c1fb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -277,6 +277,13 @@ func (a *Auth) callback(w http.ResponseWriter, r *http.Request) { } else { a.logger.WarnContext(ctx, "sign-in refused: not in an allowed group", "subject", claims.Subject) } + // A session this browser already holds ends too, so a refused + // sign-in never leaves it signed in. + if err := a.sessions.EndSession(w, r); err != nil { + a.logger.ErrorContext(ctx, "end session", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } a.refuse(w, r) return } diff --git a/internal/auth/gate_test.go b/internal/auth/gate_test.go index fffa832..0f18362 100644 --- a/internal/auth/gate_test.go +++ b/internal/auth/gate_test.go @@ -2,9 +2,11 @@ package auth import ( "context" + "errors" "io" "log/slog" "net/http" + "net/url" "slices" "strings" "testing" @@ -19,7 +21,8 @@ func (h *harness) signInWith(t *testing.T, edit func(claims map[string]any)) *ht return h.callback(l, l.query.Get("state"), h.provider.signRS256(c)) } -// assertRefused checks resp is the refused-sign-in page and sets no session. +// assertRefused checks resp is the refused-sign-in page and starts no +// session. func assertRefused(t *testing.T, resp *http.Response) { t.Helper() if resp.StatusCode != http.StatusForbidden { @@ -42,7 +45,8 @@ func assertRefused(t *testing.T, resp *http.Response) { } } for _, c := range resp.Cookies() { - if c.Name == SessionCookieName || c.Name == "test_session" { + // Clearing a session cookie (negative Max-Age) is expected. + if (c.Name == SessionCookieName || c.Name == "test_session") && c.MaxAge >= 0 { t.Errorf("refused sign-in set session cookie %s", c.Name) } } @@ -185,6 +189,48 @@ func TestGroupGateRemovedMemberRefusedAtNextLogin(t *testing.T) { } } +// A refused sign-in from a browser that still holds a session ends that +// session and clears its cookie. +func TestGroupGateRefusalEndsExistingSession(t *testing.T) { + t.Parallel() + h := newSessionHarness(t) + _, cookie := h.signIn(t) + + l := h.login(t, LoginPath) + c := h.provider.claims(l.query.Get("nonce")) + c["groups"] = []string{"former-household"} + code := h.provider.authorize(l.query.Get("code_challenge"), h.provider.signRS256(c)) + q := url.Values{"code": {code}, "state": {l.query.Get("state")}} + resp := h.get(CallbackPath+"?"+q.Encode(), l.cookie, cookie) + + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("callback status = %d, want 403", resp.StatusCode) + } + if n := len(h.rows(t)); n != 0 { + t.Errorf("sessions after refused login = %d, want 0", n) + } + if _, err := h.use(cookie); !errors.Is(err, ErrNoSession) { + t.Errorf("old session lookup error = %v, want ErrNoSession", err) + } + cleared := false + for _, c := range resp.Cookies() { + cleared = cleared || (c.Name == SessionCookieName && c.MaxAge < 0) + } + if !cleared { + t.Error("refused sign-in did not clear the session cookie") + } +} + +func TestGroupGateRefusalEndSessionError(t *testing.T) { + t.Parallel() + h := newHarness(t) + h.sessions.err = errors.New("database down") + resp := h.signInWith(t, func(c map[string]any) { c["groups"] = []string{"guests"} }) + if resp.StatusCode != http.StatusInternalServerError { + t.Errorf("callback status = %d, want 500", resp.StatusCode) + } +} + func TestNewRequiresAllowedGroups(t *testing.T) { t.Parallel() p := newFakeProvider(t)