tenseleyflow/shithub / 1f1cde9

Browse files

C1: thread IsSuspended through CurrentUser to policy.UserActor

Authored by mfwolffe <wolffemf@dukes.jmu.edu>
SHA
1f1cde96a8f90b3cd493e53f880aebbaa4f78bb8
Parents
c21250d
Tree
604e221

6 changed files

StatusFile+-
M internal/web/auth_wiring.go 13 6
M internal/web/handlers/auth/auth_test.go 15 7
M internal/web/handlers/auth/tokens_test.go 15 7
M internal/web/handlers/repo/lifecycle.go 13 6
M internal/web/handlers/repo/repo.go 10 4
M internal/web/middleware/auth.go 26 9
internal/web/auth_wiring.gomodified
@@ -25,6 +25,7 @@ import (
2525
 	usersdb "github.com/tenseleyFlow/shithub/internal/users/sqlc"
2626
 	apih "github.com/tenseleyFlow/shithub/internal/web/handlers/api"
2727
 	authh "github.com/tenseleyFlow/shithub/internal/web/handlers/auth"
28
+	"github.com/tenseleyFlow/shithub/internal/web/middleware"
2829
 	"github.com/tenseleyFlow/shithub/internal/web/render"
2930
 )
3031
@@ -42,16 +43,22 @@ func buildAPIHandlers(pool *pgxpool.Pool) (*apih.Handlers, error) {
4243
 }
4344
4445
 // usernameLookup returns the lookup function consumed by middleware.OptionalUser.
45
-// It resolves both the username and the user's current session_epoch so the
46
-// auth middleware can refuse stale cookies (bumped by "log out everywhere").
47
-func usernameLookup(pool *pgxpool.Pool) func(context.Context, int64) (string, int32, error) {
46
+// It resolves the username, the user's current session_epoch (so the auth
47
+// middleware can refuse stale cookies bumped by "log out everywhere"), and
48
+// the suspended state (so policy.UserActor can deny writes by suspended
49
+// accounts at the web layer — the audit found this gap, S00-S25 audit C1).
50
+func usernameLookup(pool *pgxpool.Pool) middleware.UserLookup {
4851
 	q := usersdb.New()
49
-	return func(ctx context.Context, id int64) (string, int32, error) {
52
+	return func(ctx context.Context, id int64) (middleware.UserLookupResult, error) {
5053
 		u, err := q.GetUserByID(ctx, pool, id)
5154
 		if err != nil {
52
-			return "", 0, err
55
+			return middleware.UserLookupResult{}, err
5356
 		}
54
-		return u.Username, u.SessionEpoch, nil
57
+		return middleware.UserLookupResult{
58
+			Username:     u.Username,
59
+			SessionEpoch: u.SessionEpoch,
60
+			IsSuspended:  u.SuspendedAt.Valid,
61
+		}, nil
5562
 	}
5663
 }
5764
internal/web/handlers/auth/auth_test.gomodified
@@ -4,6 +4,7 @@ package auth_test
44
55
 import (
66
 	"context"
7
+	"database/sql"
78
 	"html"
89
 	"io"
910
 	"io/fs"
@@ -134,21 +135,28 @@ func newTestServerWithPool(t *testing.T, requireVerify bool) (*httptest.Server,
134135
 	r.Use(middleware.RequestID)
135136
 	r.Use(middleware.RealIP(middleware.RealIPConfig{}))
136137
 	r.Use(middleware.SessionLoader(store, logger))
137
-	r.Use(middleware.OptionalUser(func(ctx context.Context, id int64) (string, int32, error) {
138
+	r.Use(middleware.OptionalUser(func(ctx context.Context, id int64) (middleware.UserLookupResult, error) {
138139
 		// Cheap lookup against the test pool — settings handlers use the
139140
 		// username, and the epoch comparison enforces log-out-everywhere
140141
 		// across the same suite.
141142
 		u, err := pool.Acquire(ctx)
142143
 		if err != nil {
143
-			return "", 0, err
144
+			return middleware.UserLookupResult{}, err
144145
 		}
145146
 		defer u.Release()
146
-		var name string
147
-		var epoch int32
147
+		var (
148
+			name        string
149
+			epoch       int32
150
+			suspendedAt sql.NullTime
151
+		)
148152
 		err = u.QueryRow(ctx,
149
-			"SELECT username, session_epoch FROM users WHERE id = $1", id,
150
-		).Scan(&name, &epoch)
151
-		return name, epoch, err
153
+			"SELECT username, session_epoch, suspended_at FROM users WHERE id = $1", id,
154
+		).Scan(&name, &epoch, &suspendedAt)
155
+		return middleware.UserLookupResult{
156
+			Username:     name,
157
+			SessionEpoch: epoch,
158
+			IsSuspended:  suspendedAt.Valid,
159
+		}, err
152160
 	}))
153161
 	csrf := middleware.CSRF(middleware.CSRFConfig{
154162
 		FailureHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
internal/web/handlers/auth/tokens_test.gomodified
@@ -4,6 +4,7 @@ package auth_test
44
55
 import (
66
 	"context"
7
+	"database/sql"
78
 	"encoding/json"
89
 	"io"
910
 	"log/slog"
@@ -72,18 +73,25 @@ func newTokenServer(t *testing.T) (srv *httptest.Server, cli *client, captor *ca
7273
 	r.Use(middleware.RequestID)
7374
 	r.Use(middleware.RealIP(middleware.RealIPConfig{}))
7475
 	r.Use(middleware.SessionLoader(store, logger))
75
-	r.Use(middleware.OptionalUser(func(ctx context.Context, id int64) (string, int32, error) {
76
+	r.Use(middleware.OptionalUser(func(ctx context.Context, id int64) (middleware.UserLookupResult, error) {
7677
 		c, err := pool.Acquire(ctx)
7778
 		if err != nil {
78
-			return "", 0, err
79
+			return middleware.UserLookupResult{}, err
7980
 		}
8081
 		defer c.Release()
81
-		var name string
82
-		var epoch int32
82
+		var (
83
+			name        string
84
+			epoch       int32
85
+			suspendedAt sql.NullTime
86
+		)
8387
 		err = c.QueryRow(ctx,
84
-			"SELECT username, session_epoch FROM users WHERE id = $1", id,
85
-		).Scan(&name, &epoch)
86
-		return name, epoch, err
88
+			"SELECT username, session_epoch, suspended_at FROM users WHERE id = $1", id,
89
+		).Scan(&name, &epoch, &suspendedAt)
90
+		return middleware.UserLookupResult{
91
+			Username:     name,
92
+			SessionEpoch: epoch,
93
+			IsSuspended:  suspendedAt.Valid,
94
+		}, err
8795
 	}))
8896
 	csrf := middleware.CSRF(middleware.CSRFConfig{
8997
 		FailureHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
internal/web/handlers/repo/lifecycle.gomodified
@@ -71,10 +71,16 @@ func (h *Handlers) loadRepoAndAuthorize(w http.ResponseWriter, r *http.Request,
7171
 		return reposdb.Repo{}, usersdb.User{}, false
7272
 	}
7373
 	viewer := middleware.CurrentUserFromContext(r.Context())
74
-	actor := policy.UserActor(viewer.ID, viewer.Username, false, false)
74
+	actor := policy.UserActor(viewer.ID, viewer.Username, viewer.IsSuspended, false)
7575
 	repoRef := policy.NewRepoRefFromRepo(row)
76
-	if !policy.Can(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, action, repoRef).Allow {
77
-		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
76
+	dec := policy.Can(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, action, repoRef)
77
+	if !dec.Allow {
78
+		// Maybe404 picks 404 for "shouldn't know it exists" denials
79
+		// (private + non-collab) and 403 for honest "you can't do that
80
+		// to a repo you can see" denials (owner pushing to archived).
81
+		// Without this we leaked existence by always 404'ing — fixed
82
+		// per S00-S25 audit, finding H7.
83
+		h.d.Render.HTTPError(w, r, policy.Maybe404(dec, repoRef, actor), "")
7884
 		return reposdb.Repo{}, usersdb.User{}, false
7985
 	}
8086
 	return row, owner, true
@@ -252,9 +258,10 @@ func (h *Handlers) transferCancel(w http.ResponseWriter, r *http.Request) {
252258
 		return
253259
 	}
254260
 	viewer := middleware.CurrentUserFromContext(r.Context())
255
-	actor := policy.UserActor(viewer.ID, viewer.Username, false, false)
256
-	if !policy.Can(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, policy.ActionRepoAdmin, policy.NewRepoRefFromRepo(repo)).Allow {
257
-		h.d.Render.HTTPError(w, r, http.StatusNotFound, "")
261
+	actor := policy.UserActor(viewer.ID, viewer.Username, viewer.IsSuspended, false)
262
+	repoRef := policy.NewRepoRefFromRepo(repo)
263
+	if dec := policy.Can(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, policy.ActionRepoAdmin, repoRef); !dec.Allow {
264
+		h.d.Render.HTTPError(w, r, policy.Maybe404(dec, repoRef, actor), "")
258265
 		return
259266
 	}
260267
 	if err := lifecycle.CancelTransfer(r.Context(), h.lifecycleDeps(), viewer.ID, id); err != nil {
internal/web/handlers/repo/repo.gomodified
@@ -185,7 +185,7 @@ func (h *Handlers) repoHome(w http.ResponseWriter, r *http.Request) {
185185
 	owner := chi.URLParam(r, "owner")
186186
 	name := chi.URLParam(r, "repo")
187187
188
-	row, err := h.lookupRepoForViewer(r.Context(), owner, name, middleware.CurrentUserFromContext(r.Context()).ID)
188
+	row, err := h.lookupRepoForViewer(r.Context(), owner, name, middleware.CurrentUserFromContext(r.Context()))
189189
 	if err != nil {
190190
 		// Maybe the (owner, name) is a stale name; look up the redirect
191191
 		// table and 301 to the canonical URL so old bookmarks keep
@@ -239,7 +239,7 @@ func (h *Handlers) repoHome(w http.ResponseWriter, r *http.Request) {
239239
 //   - AND the viewer is allowed to see it (public OR viewer is owner).
240240
 //
241241
 // Anything else returns ErrNoRows so the caller can 404 uniformly.
242
-func (h *Handlers) lookupRepoForViewer(ctx context.Context, ownerName, repoName string, viewerID int64) (reposdb.Repo, error) {
242
+func (h *Handlers) lookupRepoForViewer(ctx context.Context, ownerName, repoName string, viewer middleware.CurrentUser) (reposdb.Repo, error) {
243243
 	owner, err := h.uq.GetUserByUsername(ctx, h.d.Pool, ownerName)
244244
 	if err != nil {
245245
 		return reposdb.Repo{}, err
@@ -255,11 +255,17 @@ func (h *Handlers) lookupRepoForViewer(ctx context.Context, ownerName, repoName
255255
 	// when the decision denies, return ErrNoRows so the caller can 404.
256256
 	repoRef := policy.NewRepoRefFromRepo(row)
257257
 	var actor policy.Actor
258
-	if viewerID == 0 {
258
+	if viewer.IsAnonymous() {
259259
 		actor = policy.AnonymousActor()
260260
 	} else {
261
-		actor = policy.UserActor(viewerID, "", false, false)
261
+		actor = policy.UserActor(viewer.ID, viewer.Username, viewer.IsSuspended, false)
262262
 	}
263
+	// ActionRepoRead deny on a private repo with a non-collab viewer is
264
+	// indistinguishable from "doesn't exist" — Maybe404 returns 404 in
265
+	// that shape, so the caller's pgx.ErrNoRows fallthrough is the
266
+	// right shape regardless of whether the row was missing or just
267
+	// invisible. Honest 403s (e.g. ActionRepoWrite on archived) are
268
+	// gated through `loadRepoAndAuthorize`, not this read-only helper.
263269
 	if !policy.Can(ctx, policy.Deps{Pool: h.d.Pool}, actor, policy.ActionRepoRead, repoRef).Allow {
264270
 		return reposdb.Repo{}, pgx.ErrNoRows
265271
 	}
internal/web/middleware/auth.gomodified
@@ -12,19 +12,35 @@ var currentUserKey = ctxKey{name: "current_user"}
1212
1313
 // CurrentUser carries the loaded user identity in request context. Handlers
1414
 // pull this out via CurrentUserFromContext. Anonymous requests have ID == 0.
15
+//
16
+// IsSuspended is sourced from users.suspended_at and is the canonical input
17
+// to policy.UserActor for web requests. Without it, every handler that
18
+// constructs an actor with a hard-coded `false` lets a suspended account
19
+// keep writing — the audit found this gap (S00-S25 audit, finding C1).
1520
 type CurrentUser struct {
16
-	ID       int64
17
-	Username string
21
+	ID          int64
22
+	Username    string
23
+	IsSuspended bool
1824
 }
1925
2026
 // IsAnonymous reports whether this is an unauthenticated request.
2127
 func (u CurrentUser) IsAnonymous() bool { return u.ID == 0 }
2228
29
+// UserLookupResult is what UserLookup returns. It's a struct so future
30
+// fields (e.g. is_admin once S34 lands) don't keep widening the signature
31
+// and forcing every callsite to update.
32
+type UserLookupResult struct {
33
+	Username     string
34
+	SessionEpoch int32
35
+	IsSuspended  bool
36
+}
37
+
2338
 // UserLookup resolves a user_id into the data the auth middleware needs.
24
-// epoch is the users.session_epoch column; the middleware compares it to
25
-// the session's recorded epoch on every request so "log out everywhere"
26
-// (which bumps the column) invalidates stale cookies on the next hit.
27
-type UserLookup func(ctx context.Context, userID int64) (username string, epoch int32, err error)
39
+// SessionEpoch is the users.session_epoch column; the middleware compares
40
+// it to the session's recorded epoch on every request so "log out
41
+// everywhere" (which bumps the column) invalidates stale cookies on the
42
+// next hit.
43
+type UserLookup func(ctx context.Context, userID int64) (UserLookupResult, error)
2844
2945
 // OptionalUser populates CurrentUser into context from the loaded session
3046
 // when present. Does not redirect or reject — pages that don't need a
@@ -47,11 +63,12 @@ func OptionalUser(lookup UserLookup) func(http.Handler) http.Handler {
4763
 				u := CurrentUser{ID: s.UserID}
4864
 				bind := true
4965
 				if lookup != nil {
50
-					if name, epoch, err := lookup(ctx, s.UserID); err == nil {
51
-						if epoch != s.Epoch {
66
+					if res, err := lookup(ctx, s.UserID); err == nil {
67
+						if res.SessionEpoch != s.Epoch {
5268
 							bind = false
5369
 						} else {
54
-							u.Username = name
70
+							u.Username = res.Username
71
+							u.IsSuspended = res.IsSuspended
5572
 						}
5673
 					}
5774
 				}