diff --git a/cmd/server/main.go b/cmd/server/main.go index 89f6b9d..229a69a 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -57,17 +57,18 @@ func run(cfg config.Config, logger *slog.Logger) error { return err } authFlow, err := auth.New(ctx, auth.Config{ - Issuer: cfg.OIDCIssuer.String(), - ClientID: cfg.OIDCClientID, - ClientSecret: cfg.OIDCClientSecret.Reveal(), - PublicURL: cfg.PublicURL, - SecretKey: []byte(cfg.SecretKey.Reveal()), + Issuer: cfg.OIDCIssuer.String(), + ClientID: cfg.OIDCClientID, + ClientSecret: cfg.OIDCClientSecret.Reveal(), + PublicURL: cfg.PublicURL, + SecretKey: []byte(cfg.SecretKey.Reveal()), + AllowedGroups: cfg.OIDCAllowedGroups, }, sessions, logger) if err != nil { return err } - router, err := server.NewRouter(logger, version, authFlow, frontend.Assets()) + router, err := server.NewRouter(logger, version, authFlow, sessions, frontend.Assets()) if err != nil { return err } diff --git a/internal/auth/actor.go b/internal/auth/actor.go new file mode 100644 index 0000000..ac65b84 --- /dev/null +++ b/internal/auth/actor.go @@ -0,0 +1,35 @@ +package auth + +import ( + "context" + "slices" +) + +// Actor is the signed-in household member a request acts for. Handlers +// record Actor.Subject as updated_by on edits. +type Actor struct { + Subject string + Email string + Name string + Groups []string +} + +// Actor returns the household member s belongs to. +func (s Session) Actor() Actor { + return Actor{Subject: s.Subject, Email: s.Email, Name: s.Name, Groups: slices.Clone(s.Groups)} +} + +type actorKey struct{} + +// WithActor returns a copy of ctx carrying actor. The session middleware +// calls it for every authenticated request. +func WithActor(ctx context.Context, actor Actor) context.Context { + return context.WithValue(ctx, actorKey{}, actor) +} + +// ActorFrom returns the actor WithActor stored in ctx. ok is false when ctx +// has none, which behind the session middleware means a wiring bug. +func ActorFrom(ctx context.Context) (actor Actor, ok bool) { + actor, ok = ctx.Value(actorKey{}).(Actor) + return actor, ok +} diff --git a/internal/auth/actor_test.go b/internal/auth/actor_test.go new file mode 100644 index 0000000..c2f239e --- /dev/null +++ b/internal/auth/actor_test.go @@ -0,0 +1,32 @@ +package auth + +import ( + "context" + "slices" + "testing" +) + +func TestActorContext(t *testing.T) { + t.Parallel() + if _, ok := ActorFrom(context.Background()); ok { + t.Error("ActorFrom found an actor in an empty context") + } + want := Actor{Subject: "user-123", Email: "cook@example.com", Name: "Home Cook", Groups: []string{"mealquest-admin"}} + got, ok := ActorFrom(WithActor(context.Background(), want)) + if !ok || got.Subject != want.Subject || got.Email != want.Email || got.Name != want.Name || !slices.Equal(got.Groups, want.Groups) { + t.Errorf("ActorFrom = %+v (ok=%v), want %+v", got, ok, want) + } +} + +func TestSessionActor(t *testing.T) { + t.Parallel() + s := Session{Subject: "user-123", Email: "cook@example.com", Name: "Home Cook", Groups: []string{"mealquest-admin"}, CSRFToken: "t"} + a := s.Actor() + if a.Subject != s.Subject || a.Email != s.Email || a.Name != s.Name || !slices.Equal(a.Groups, s.Groups) { + t.Errorf("Actor = %+v, want the session's identity", a) + } + a.Groups[0] = "changed" + if s.Groups[0] != "mealquest-admin" { + t.Error("Actor shares the session's groups slice") + } +} diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 629ed19..b1cf2c9 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -12,6 +12,7 @@ import ( "log/slog" "net/http" "net/url" + "slices" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -65,15 +66,20 @@ type Config struct { PublicURL *url.URL // SecretKey is the server key the login cookie's signing key derives from. SecretKey []byte + // AllowedGroups are the household's groups; a sign-in is refused unless + // the account is in at least one of them. + AllowedGroups []string } // Auth serves the login and callback routes. type Auth struct { oauth oauth2.Config verifier *oidc.IDTokenVerifier + provider *oidc.Provider client *http.Client cookie loginCookie sessions Sessions + allowed []string // endSessionURL is the provider's end_session_endpoint; logout ends // there so the provider's own session ends too. endSessionURL string @@ -92,6 +98,8 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger return nil, errors.New("auth: secret key is required") case sessions == nil: return nil, errors.New("auth: session layer is required") + case len(cfg.AllowedGroups) == 0: + return nil, errors.New("auth: at least one allowed group is required") } if err := checkEndpoint("issuer", cfg.Issuer); err != nil { @@ -118,6 +126,9 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger {"token endpoint", endpoint.TokenURL}, {"JWKS endpoint", discovered.JWKSURI}, {"end session endpoint", discovered.EndSessionURI}, + // Groups come from userinfo when the ID token carries none, and the + // access token is sent there. + {"userinfo endpoint", provider.UserInfoEndpoint()}, } { if err := checkEndpoint(e.name, e.raw); err != nil { return nil, err @@ -138,7 +149,9 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger ClientID: cfg.ClientID, SupportedSigningAlgs: []string{oidc.RS256}, }), + provider: provider, client: client, + allowed: slices.Clone(cfg.AllowedGroups), cookie: newLoginCookie(cfg.SecretKey), sessions: sessions, endSessionURL: discovered.EndSessionURI, @@ -218,7 +231,9 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) { } // callback completes the flow: it checks state against the login cookie, -// exchanges the code, verifies the ID token and nonce, then starts a session. +// exchanges the code, verifies the ID token and nonce, then starts a session +// if the account is in an allowed group. Membership is checked on every +// login, so an account removed from the group is refused at its next one. func (a *Auth) callback(w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-store") ctx := r.Context() @@ -256,6 +271,15 @@ func (a *Auth) callback(w http.ResponseWriter, r *http.Request) { http.Error(w, "sign-in failed", http.StatusUnauthorized) return } + if !a.member(claims.Groups) { + if claims.Groups == nil { + a.logger.WarnContext(ctx, "sign-in refused: no groups claim in the ID token or userinfo", "subject", claims.Subject) + } else { + a.logger.WarnContext(ctx, "sign-in refused: not in an allowed group", "subject", claims.Subject) + } + a.refuse(w, r) + return + } if err := a.sessions.StartSession(w, r, claims); err != nil { a.logger.ErrorContext(ctx, "start session", "error", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -278,7 +302,8 @@ func (a *Auth) logout(w http.ResponseWriter, r *http.Request) { // verify exchanges code for tokens and returns the verified ID token claims. // The verifier enforces RS256, issuer, audience and expiry; nonce is checked -// here. +// here. When the ID token has no groups claim, Groups comes from userinfo; +// it stays nil when neither has one. func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claims, error) { token, err := a.oauth.Exchange(context.WithValue(ctx, oauth2.HTTPClient, a.client), code, oauth2.VerifierOption(state.Verifier)) @@ -303,9 +328,50 @@ func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claim if claims.Subject == "" { return Claims{}, errors.New("ID token has no subject") } + if claims.Groups == nil { + groups, err := a.userinfoGroups(ctx, token, claims.Subject) + if err != nil { + return Claims{}, err + } + claims.Groups = groups + } return claims, nil } +// userinfoGroups returns the groups claim from the userinfo endpoint, nil if +// it has none. The userinfo subject must match the ID token's subject. +func (a *Auth) userinfoGroups(ctx context.Context, token *oauth2.Token, subject string) ([]string, error) { + info, err := a.provider.UserInfo(oidc.ClientContext(ctx, a.client), oauth2.StaticTokenSource(token)) + if err != nil { + return nil, userinfoError(err) + } + if info.Subject != subject { + return nil, errors.New("userinfo subject does not match the ID token") + } + var claims struct { + Groups []string `json:"groups"` + } + if err := info.Claims(&claims); err != nil { + return nil, fmt.Errorf("decode userinfo claims: %w", err) + } + return claims.Groups, nil +} + +// userinfoError reduces a failed userinfo request to a fixed category, like +// exchangeError: go-oidc's error text can carry the response body. +func userinfoError(err error) error { + var transport *url.Error + if errors.As(err, &transport) { + return fmt.Errorf("userinfo: request failed: %s: %w", transport.Op, transport.Err) + } + return errors.New("userinfo: endpoint returned an unusable response") +} + +// member reports whether groups contains an allowed group. +func (a *Auth) member(groups []string) bool { + return slices.ContainsFunc(groups, func(g string) bool { return slices.Contains(a.allowed, g) }) +} + // exchangeError reduces a failed code exchange to a fixed category and safe // metadata. The oauth2 error text can carry the token endpoint's response // body or error_description, which may echo credentials, so it is dropped. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index a9d83e6..4ecf409 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -36,6 +36,9 @@ const ( var testPublicURL = &url.URL{Scheme: "https", Host: "meals.example.com"} +// testAllowedGroups are the household's groups in tests. +var testAllowedGroups = []string{"mealquest-admin", "mealquest-family"} + // fakeProvider is an OIDC provider serving discovery, JWKS and a token // endpoint. Each issued code maps to the ID token the token endpoint returns. type fakeProvider struct { @@ -50,6 +53,12 @@ type fakeProvider struct { // tokenReply, when set, is returned by the token endpoint instead of a // token. tokenReply *tokenReply + // userinfo is the userinfo endpoint's response; userinfoHits counts + // requests to it. + userinfo map[string]any + userinfoHits int + // userinfoReply, when set, is returned by the userinfo endpoint instead. + userinfoReply *tokenReply } type tokenReply struct { @@ -73,6 +82,7 @@ func newFakeProvider(t *testing.T) *fakeProvider { mux.HandleFunc("GET /.well-known/openid-configuration", p.discovery) mux.HandleFunc("GET /jwks", p.jwks) mux.HandleFunc("POST /token", p.token) + mux.HandleFunc("GET /userinfo", p.userinfoEndpoint) p.srv = httptest.NewServer(mux) t.Cleanup(p.srv.Close) return p @@ -86,6 +96,7 @@ func (p *fakeProvider) discovery(w http.ResponseWriter, _ *http.Request) { "authorization_endpoint": p.issuer() + "/authorize", "token_endpoint": p.issuer() + "/token", "jwks_uri": p.issuer() + "/jwks", + "userinfo_endpoint": p.issuer() + "/userinfo", "end_session_endpoint": p.issuer() + "/end-session/", "response_types_supported": []string{"code"}, "subject_types_supported": []string{"public"}, @@ -148,6 +159,41 @@ func (p *fakeProvider) token(w http.ResponseWriter, r *http.Request) { }) } +// userinfoEndpoint checks the access token and returns p.userinfo, which +// defaults to the test user without a groups claim. +func (p *fakeProvider) userinfoEndpoint(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer access-token" { + http.Error(w, `{"error":"invalid_token"}`, http.StatusUnauthorized) + return + } + p.mu.Lock() + p.userinfoHits++ + info, reply := p.userinfo, p.userinfoReply + p.mu.Unlock() + if reply != nil { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(reply.status) + io.WriteString(w, reply.body) + return + } + if info == nil { + info = map[string]any{"sub": "user-123", "email": "cook@example.com"} + } + writeJSON(w, info) +} + +func (p *fakeProvider) setUserinfo(info map[string]any) { + p.mu.Lock() + defer p.mu.Unlock() + p.userinfo = info +} + +func (p *fakeProvider) userinfoRequests() int { + p.mu.Lock() + defer p.mu.Unlock() + return p.userinfoHits +} + // authorize stands in for the user signing in at the provider: it records a // code bound to the PKCE challenge and returns it. func (p *fakeProvider) authorize(challenge, idToken string) string { @@ -291,11 +337,12 @@ func newHarness(t *testing.T) *harness { sessions := &memorySessions{} logs := &syncBuffer{} a, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("server-secret-key"), + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("server-secret-key"), + AllowedGroups: testAllowedGroups, }, sessions, slog.New(slog.NewTextHandler(logs, nil))) if err != nil { t.Fatal(err) @@ -730,11 +777,12 @@ func TestNewFailsWhenDiscoveryFails(t *testing.T) { srv := httptest.NewServer(http.NotFoundHandler()) t.Cleanup(srv.Close) _, err := New(context.Background(), Config{ - Issuer: srv.URL, - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("k"), + Issuer: srv.URL, + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, }, &memorySessions{}, slog.New(slog.DiscardHandler)) if err == nil { t.Fatal("New succeeded without a discovery document") @@ -747,11 +795,12 @@ func TestNewFailsOnIssuerMismatch(t *testing.T) { // Discovery trims the trailing slash, so this fetches the provider's // document, which reports the issuer without it. _, err := New(context.Background(), Config{ - Issuer: p.issuer() + "/", - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("k"), + Issuer: p.issuer() + "/", + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, }, &memorySessions{}, slog.New(slog.DiscardHandler)) if err == nil { t.Fatal("New accepted a provider reporting a different issuer") @@ -772,11 +821,12 @@ func TestNewRejectsRemoteHTTPIssuer(t *testing.T) { t.Parallel() for _, issuer := range []string{"http://auth.example.com/application/o/mealquest/", "http://192.0.2.10"} { _, err := New(context.Background(), Config{ - Issuer: issuer, - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("k"), + Issuer: issuer, + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, }, &memorySessions{}, slog.New(slog.DiscardHandler)) if err == nil || !strings.Contains(err.Error(), "issuer") { t.Errorf("New(issuer %q) error = %v, want issuer rejected", issuer, err) @@ -793,6 +843,7 @@ func TestNewRejectsRemoteHTTPEndpoint(t *testing.T) { {"token_endpoint", "token endpoint"}, {"jwks_uri", "JWKS endpoint"}, {"end_session_endpoint", "end session endpoint"}, + {"userinfo_endpoint", "userinfo endpoint"}, } for _, tc := range tests { t.Run(tc.field, func(t *testing.T) { @@ -800,11 +851,12 @@ func TestNewRejectsRemoteHTTPEndpoint(t *testing.T) { p := newFakeProvider(t) p.discoveryOverrides = map[string]any{tc.field: "http://auth.example.com/" + tc.field} _, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("k"), + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, }, &memorySessions{}, slog.New(slog.DiscardHandler)) if err == nil || !strings.Contains(err.Error(), tc.name) { t.Errorf("New error = %v, want %s rejected", err, tc.name) @@ -840,11 +892,12 @@ func TestCallbackRefusesTokenRedirectToHTTP(t *testing.T) { sessions := &memorySessions{} logs := &syncBuffer{} a, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("server-secret-key"), + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("server-secret-key"), + AllowedGroups: testAllowedGroups, }, sessions, slog.New(slog.NewTextHandler(logs, nil))) if err != nil { t.Fatal(err) diff --git a/internal/auth/gate_test.go b/internal/auth/gate_test.go new file mode 100644 index 0000000..fffa832 --- /dev/null +++ b/internal/auth/gate_test.go @@ -0,0 +1,218 @@ +package auth + +import ( + "context" + "io" + "log/slog" + "net/http" + "slices" + "strings" + "testing" +) + +// signInWith completes a login whose ID token has claims edited by edit. +func (h *harness) signInWith(t *testing.T, edit func(claims map[string]any)) *http.Response { + t.Helper() + l := h.login(t, LoginPath) + c := h.provider.claims(l.query.Get("nonce")) + edit(c) + 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. +func assertRefused(t *testing.T, resp *http.Response) { + t.Helper() + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("callback status = %d, want 403", resp.StatusCode) + } + if ct := resp.Header.Get("Content-Type"); ct != "text/html; charset=utf-8" { + t.Errorf("Content-Type = %q, want text/html; charset=utf-8", ct) + } + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{ + "This kitchen is for the household only", + `
`, + "Sign out", + } { + if !strings.Contains(string(body), want) { + t.Errorf("refused page lacks %q:\n%s", want, body) + } + } + for _, c := range resp.Cookies() { + if c.Name == SessionCookieName || c.Name == "test_session" { + t.Errorf("refused sign-in set session cookie %s", c.Name) + } + } + assertLoginCookieCleared(t, resp) +} + +func TestGroupGateMember(t *testing.T) { + t.Parallel() + // Any one allowed group is enough, alongside groups that are not allowed. + h := newHarness(t) + resp := h.signInWith(t, func(c map[string]any) { c["groups"] = []string{"guests", "mealquest-family"} }) + if resp.StatusCode != http.StatusSeeOther { + t.Fatalf("callback status = %d, want 303", resp.StatusCode) + } + if started := h.sessions.started(); len(started) != 1 || !slices.Equal(started[0].Groups, []string{"guests", "mealquest-family"}) { + t.Errorf("sessions started = %+v, want one with the ID token groups", started) + } + if n := h.provider.userinfoRequests(); n != 0 { + t.Errorf("userinfo requested %d times, want 0 when the ID token has groups", n) + } +} + +func TestGroupGateRefused(t *testing.T) { + t.Parallel() + tests := []struct { + name string + groups any // ID token groups claim; nil removes it + userinfo map[string]any + }{ + {name: "non-member", groups: []string{"guests", "MEALQUEST-ADMIN"}}, + {name: "empty groups claim", groups: []string{}}, + {name: "groups claim missing from ID token and userinfo", groups: nil}, + { + name: "non-member in userinfo", + groups: nil, + userinfo: map[string]any{"sub": "user-123", "groups": []string{"guests"}}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h := newHarness(t) + if tc.userinfo != nil { + h.provider.setUserinfo(tc.userinfo) + } + resp := h.signInWith(t, func(c map[string]any) { + if tc.groups == nil { + delete(c, "groups") + } else { + c["groups"] = tc.groups + } + }) + assertRefused(t, resp) + if started := h.sessions.started(); len(started) != 0 { + t.Errorf("sessions started = %+v, want none", started) + } + if !strings.Contains(h.logs.String(), "sign-in refused") { + t.Errorf("refusal not logged:\n%s", h.logs.String()) + } + }) + } +} + +func TestGroupGateUsesUserinfoWhenIDTokenHasNoGroups(t *testing.T) { + t.Parallel() + h := newHarness(t) + h.provider.setUserinfo(map[string]any{"sub": "user-123", "groups": []string{"mealquest-admin"}}) + resp := h.signInWith(t, func(c map[string]any) { delete(c, "groups") }) + if resp.StatusCode != http.StatusSeeOther { + t.Fatalf("callback status = %d, want 303", resp.StatusCode) + } + if started := h.sessions.started(); len(started) != 1 || !slices.Equal(started[0].Groups, []string{"mealquest-admin"}) { + t.Errorf("sessions started = %+v, want one with the userinfo groups", started) + } + if n := h.provider.userinfoRequests(); n != 1 { + t.Errorf("userinfo requested %d times, want 1", n) + } +} + +func TestGroupGateUserinfoFailures(t *testing.T) { + t.Parallel() + tests := []struct { + name string + userinfo map[string]any + }{ + {"subject mismatch", map[string]any{"sub": "someone-else", "groups": []string{"mealquest-admin"}}}, + {"malformed groups", map[string]any{"sub": "user-123", "groups": "mealquest-admin"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h := newHarness(t) + h.provider.setUserinfo(tc.userinfo) + resp := h.signInWith(t, func(c map[string]any) { delete(c, "groups") }) + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("callback status = %d, want 401", resp.StatusCode) + } + if started := h.sessions.started(); len(started) != 0 { + t.Errorf("sessions started = %+v, want none", started) + } + }) + } +} + +// A failed userinfo request fails the sign-in without logging the +// endpoint's response, which may echo credentials. +func TestGroupGateUserinfoErrorNotLogged(t *testing.T) { + t.Parallel() + h := newHarness(t) + h.provider.mu.Lock() + h.provider.userinfoReply = &tokenReply{status: http.StatusInternalServerError, body: `{"error":"leaked-s3cret"}`} + h.provider.mu.Unlock() + resp := h.signInWith(t, func(c map[string]any) { delete(c, "groups") }) + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("callback status = %d, want 401", resp.StatusCode) + } + if started := h.sessions.started(); len(started) != 0 { + t.Errorf("sessions started = %+v, want none", started) + } + logs := h.logs.String() + if !strings.Contains(logs, "userinfo") || strings.Contains(logs, "leaked-s3cret") { + t.Errorf("logs = %s, want the userinfo failure without the response body", logs) + } +} + +// Membership is checked at every login: after removal from the group, the +// next login is refused and starts no session. +func TestGroupGateRemovedMemberRefusedAtNextLogin(t *testing.T) { + t.Parallel() + h := newSessionHarness(t) + h.signIn(t) + if n := len(h.rows(t)); n != 1 { + t.Fatalf("sessions after first login = %d, want 1", n) + } + + resp := h.signInWith(t, func(c map[string]any) { c["groups"] = []string{"former-household"} }) + assertRefused(t, resp) + if n := len(h.rows(t)); n != 1 { + t.Errorf("sessions after refused login = %d, want still 1", n) + } +} + +func TestNewRequiresAllowedGroups(t *testing.T) { + t.Parallel() + p := newFakeProvider(t) + _, err := New(context.Background(), Config{ + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + }, &memorySessions{}, slog.New(slog.DiscardHandler)) + if err == nil || !strings.Contains(err.Error(), "allowed group") { + t.Errorf("New error = %v, want missing allowed groups named", err) + } +} + +func TestNewRequiresUserinfoEndpoint(t *testing.T) { + t.Parallel() + p := newFakeProvider(t) + p.discoveryOverrides = map[string]any{"userinfo_endpoint": nil} + _, err := New(context.Background(), Config{ + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, + }, &memorySessions{}, slog.New(slog.DiscardHandler)) + if err == nil || !strings.Contains(err.Error(), "userinfo endpoint") { + t.Errorf("New error = %v, want the missing userinfo endpoint named", err) + } +} diff --git a/internal/auth/refused.go b/internal/auth/refused.go new file mode 100644 index 0000000..399e705 --- /dev/null +++ b/internal/auth/refused.go @@ -0,0 +1,37 @@ +package auth + +import ( + "io" + "net/http" +) + +// refusedPage is shown when a sign-in succeeds at the provider but the +// account is not in the household's group. Signing out ends the provider +// session too, so another account can sign in. +const refusedPage = ` + + + + +MealQuest + + +
+

This kitchen is for the household only

+

The account you signed in with is not part of our kitchen.

+ + + +
+ + +` + +// refuse writes the refused-sign-in page with 403. +func (a *Auth) refuse(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusForbidden) + if _, err := io.WriteString(w, refusedPage); err != nil { + a.logger.WarnContext(r.Context(), "write refused sign-in page", "error", err) + } +} diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go index 20e03fe..3f86f02 100644 --- a/internal/auth/session_test.go +++ b/internal/auth/session_test.go @@ -62,11 +62,12 @@ func newSessionHarness(t *testing.T) *sessionHarness { p := newFakeProvider(t) logs := &syncBuffer{} a, err := New(ctx, Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("server-secret-key"), + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("server-secret-key"), + AllowedGroups: testAllowedGroups, }, sessions, slog.New(slog.NewTextHandler(logs, nil))) if err != nil { t.Fatal(err) @@ -490,11 +491,12 @@ func TestNewRequiresEndSessionEndpoint(t *testing.T) { p := newFakeProvider(t) p.discoveryOverrides = map[string]any{"end_session_endpoint": nil} _, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("k"), + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, }, &memorySessions{}, slog.New(slog.DiscardHandler)) if err == nil || !strings.Contains(err.Error(), "end session endpoint") { t.Errorf("New error = %v, want the missing end session endpoint named", err) diff --git a/internal/server/server.go b/internal/server/server.go index 45a8471..4ae887b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -22,9 +22,10 @@ type RouteMounter interface { } // NewRouter returns the HTTP handler for the service. version is reported by -// GET /health; auth mounts the /auth routes; assets is the web build served -// for every other GET. -func NewRouter(logger *slog.Logger, version string, auth RouteMounter, assets fs.FS) (http.Handler, error) { +// GET /health; auth mounts the /auth routes; sessions guards /api/v1 and +// /media; assets is the web build served for every other GET. /health, /auth +// and the web build are public. +func NewRouter(logger *slog.Logger, version string, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) { notFound := apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found") methodNotAllowed := apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed") @@ -39,13 +40,23 @@ func NewRouter(logger *slog.Logger, version string, auth RouteMounter, assets fs r.Get("/health", health(logger, version)) auth.Mount(r) + // The session middleware wraps the whole /api/v1 and /media subrouters + // rather than being added with Use: chi skips a subrouter's middleware + // when it has no routes, which would leave it unguarded. + signedIn := requireSession(logger, sessions) + apiV1, media := chi.NewRouter(), chi.NewRouter() + for _, sub := range []*chi.Mux{apiV1, media} { + sub.MethodNotAllowed(methodNotAllowed) + sub.NotFound(notFound) + } r.Route("/api", func(r chi.Router) { r.MethodNotAllowed(methodNotAllowed) r.NotFound(notFound) + r.Mount("/v1", signedIn(apiV1)) }) - // Catch-all for the PWA. Unknown /auth paths and /media (no routes yet) - // reach the frontend handler, which passes them to notFound rather than - // the SPA fallback. + r.Mount("/media", signedIn(media)) + // Catch-all for the PWA. Unknown /auth paths reach the frontend handler, + // which passes them to notFound rather than the SPA fallback. r.Method(http.MethodGet, "/*", web) r.Method(http.MethodHead, "/*", web) return r, nil diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 21d63e8..2b2cee1 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -27,7 +27,7 @@ func newTestRouter(t *testing.T, version string) http.Handler { "index.html": {Data: []byte(fixtureIndex)}, "assets/index-AbC123.js": {Data: []byte(fixtureAsset)}, } - r, err := NewRouter(logger, version, stubAuth{}, assets) + r, err := NewRouter(logger, version, stubAuth{}, stubSessions{}, assets) if err != nil { t.Fatal(err) } @@ -126,6 +126,8 @@ func TestReservedPathsNotFound(t *testing.T) { }{ {http.MethodGet, "/api"}, {http.MethodGet, "/api/unknown"}, + {http.MethodGet, "/api/v1/unknown"}, + {http.MethodPost, "/api/v1/unknown"}, {http.MethodPost, "/api/unknown"}, {http.MethodGet, "/auth/unknown"}, {http.MethodGet, "/media/photo.jpg"}, @@ -137,8 +139,11 @@ func TestReservedPathsNotFound(t *testing.T) { t.Run(tt.method+" "+tt.path, func(t *testing.T) { t.Parallel() + // Signed in, so /api/v1 and /media reach their routers. + req := httptest.NewRequest(tt.method, tt.path, nil) + req.AddCookie(validSessionCookie) rec := httptest.NewRecorder() - router.ServeHTTP(rec, httptest.NewRequest(tt.method, tt.path, nil)) + router.ServeHTTP(rec, req) if rec.Code != http.StatusNotFound { t.Errorf("status = %d, want 404", rec.Code) diff --git a/internal/server/session.go b/internal/server/session.go new file mode 100644 index 0000000..ade1f51 --- /dev/null +++ b/internal/server/session.go @@ -0,0 +1,37 @@ +package server + +import ( + "errors" + "log/slog" + "net/http" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" +) + +// SessionLookup finds the live session a request carries; it returns +// auth.ErrNoSession when there is none. *auth.SessionManager implements it. +type SessionLookup interface { + SessionFor(r *http.Request) (auth.Session, error) +} + +// requireSession is the RequireSession middleware. It passes requests with a +// live session to next with the session's actor in the context (read it with +// auth.ActorFrom); others get 401 unauthenticated. +func requireSession(logger *slog.Logger, sessions SessionLookup) func(http.Handler) http.Handler { + unauthenticated := apiErrorHandler(logger, http.StatusUnauthorized, "unauthenticated", "sign in required") + internal := apiErrorHandler(logger, http.StatusInternalServerError, "internal", "internal error") + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + s, err := sessions.SessionFor(r) + switch { + case errors.Is(err, auth.ErrNoSession): + unauthenticated(w, r) + case err != nil: + logger.ErrorContext(r.Context(), "look up session", "error", err) + internal(w, r) + default: + next.ServeHTTP(w, r.WithContext(auth.WithActor(r.Context(), s.Actor()))) + } + }) + } +} diff --git a/internal/server/session_test.go b/internal/server/session_test.go new file mode 100644 index 0000000..a8b4ac0 --- /dev/null +++ b/internal/server/session_test.go @@ -0,0 +1,151 @@ +package server + +import ( + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "slices" + "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" +) + +const ( + unauthenticatedJSON = `{"error":{"code":"unauthenticated","message":"sign in required","retryable":false}}` + internalJSON = `{"error":{"code":"internal","message":"internal error","retryable":false}}` +) + +var ( + validSessionCookie = &http.Cookie{Name: auth.SessionCookieName, Value: "valid"} + brokenSessionCookie = &http.Cookie{Name: auth.SessionCookieName, Value: "broken"} + testSession = auth.Session{ + Subject: "user-123", + Email: "cook@example.com", + Name: "Home Cook", + Groups: []string{"mealquest-admin"}, + } +) + +// stubSessions knows one valid session cookie; the broken cookie fails the +// lookup as a database error would. +type stubSessions struct{} + +func (stubSessions) SessionFor(r *http.Request) (auth.Session, error) { + c, err := r.Cookie(auth.SessionCookieName) + switch { + case err != nil: + return auth.Session{}, auth.ErrNoSession + case c.Value == validSessionCookie.Value: + return testSession, nil + case c.Value == brokenSessionCookie.Value: + return auth.Session{}, errors.New("database down") + default: + return auth.Session{}, auth.ErrNoSession + } +} + +func TestProtectedPathsRequireSession(t *testing.T) { + t.Parallel() + + router := newTestRouter(t, "dev") + for _, tc := range []struct { + method, path string + cookie *http.Cookie + }{ + {http.MethodGet, "/api/v1/settings/household", nil}, + {http.MethodPatch, "/api/v1/settings/household", nil}, + {http.MethodGet, "/api/v1", nil}, + {http.MethodGet, "/media/photo.jpg", nil}, + {http.MethodGet, "/api/v1/settings/household", &http.Cookie{Name: auth.SessionCookieName, Value: "unknown"}}, + } { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + req := httptest.NewRequest(tc.method, tc.path, nil) + if tc.cookie != nil { + req.AddCookie(tc.cookie) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want 401", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if got := rec.Body.String(); got != unauthenticatedJSON { + t.Errorf("body = %s, want %s", got, unauthenticatedJSON) + } + }) + } +} + +func TestPublicPathsNeedNoSession(t *testing.T) { + t.Parallel() + + router := newTestRouter(t, "dev") + for _, path := range []string{"/health", "/auth/login", "/", "/assets/index-AbC123.js", "/quests/42"} { + t.Run(path, func(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if rec.Code != http.StatusOK { + t.Errorf("GET %s without a cookie = %d, want 200", path, rec.Code) + } + }) + } +} + +func TestSessionLookupFailure(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodGet, "/api/v1/settings/household", nil) + req.AddCookie(brokenSessionCookie) + rec := httptest.NewRecorder() + newTestRouter(t, "dev").ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError || rec.Body.String() != internalJSON { + t.Errorf("response = %d %s, want 500 %s", rec.Code, rec.Body.String(), internalJSON) + } +} + +func TestRequireSessionAttachesActor(t *testing.T) { + t.Parallel() + + var got auth.Actor + var ok bool + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got, ok = auth.ActorFrom(r.Context()) + w.WriteHeader(http.StatusNoContent) + }) + h := requireSession(slog.New(slog.NewTextHandler(io.Discard, nil)), stubSessions{})(next) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/settings/household", nil) + req.AddCookie(validSessionCookie) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want the handler's 204", rec.Code) + } + if !ok || got.Subject != "user-123" || got.Email != "cook@example.com" || got.Name != "Home Cook" || + !slices.Equal(got.Groups, []string{"mealquest-admin"}) { + t.Errorf("actor = %+v (ok=%v), want the session's household member", got, ok) + } +} + +func TestRequireSessionStopsUnauthenticated(t *testing.T) { + t.Parallel() + + called := false + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { called = true }) + h := requireSession(slog.New(slog.NewTextHandler(io.Discard, nil)), stubSessions{})(next) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/v1/settings/household", nil)) + + if called || rec.Code != http.StatusUnauthorized { + t.Errorf("handler called = %v, status = %d; want not called and 401", called, rec.Code) + } +}