tenseleyflow/shithub / 2fad8e4

Browse files

C3: locked-issue gate uses real collab role via policy.HasRoleAtLeast

Authored by mfwolffe <wolffemf@dukes.jmu.edu>
SHA
2fad8e457d3bdc3b66a7975d6cb5dff5584c2a31
Parents
0996131
Tree
955d1ff

3 changed files

StatusFile+-
M internal/auth/policy/policy.go 41 0
M internal/issues/issues_test.go 38 0
M internal/web/handlers/repo/issues.go 24 6
internal/auth/policy/policy.gomodified
@@ -105,6 +105,19 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef)
105
 		return deny(DenyDBError, "role lookup failed: "+err.Error())
105
 		return deny(DenyDBError, "role lookup failed: "+err.Error())
106
 	}
106
 	}
107
 
107
 
108
+	// 6a. Author-self-close on issues and PRs. The author of an issue or
109
+	//     PR is allowed to close (and reopen — same Action) their own
110
+	//     thread regardless of their collaborator role. Handlers populate
111
+	//     `repo.AuthorUserID` on the close path; everywhere else the
112
+	//     field is zero and this branch is dead. Suspension and archived
113
+	//     gates above still apply — they ran before this. Note: we do NOT
114
+	//     extend this to `ActionIssueLabel`/`ActionIssueAssign`; only the
115
+	//     close action is author-self by design (matches GitHub).
116
+	if (action == ActionIssueClose || action == ActionPullClose) &&
117
+		repo.AuthorUserID != 0 && repo.AuthorUserID == actor.UserID {
118
+		return allow("author of thread")
119
+	}
120
+
108
 	// 7. Archived repos: writes denied even for owners. Reads still go
121
 	// 7. Archived repos: writes denied even for owners. Reads still go
109
 	//    through the role check above. (We could short-circuit reads
122
 	//    through the role check above. (We could short-circuit reads
110
 	//    earlier but keeping the flow uniform makes the matrix readable.)
123
 	//    earlier but keeping the flow uniform makes the matrix readable.)
@@ -251,3 +264,31 @@ func minRoleFor(action Action) Role {
251
 // errBadAction is returned by Can when a caller passes a value that
264
 // errBadAction is returned by Can when a caller passes a value that
252
 // doesn't match any registered Action. Reserved for tests.
265
 // doesn't match any registered Action. Reserved for tests.
253
 var errBadAction = errors.New("policy: unregistered action")
266
 var errBadAction = errors.New("policy: unregistered action")
267
+
268
+// EffectiveRole returns the actor's resolved role on repo, taking
269
+// owner-equals-admin into account and consulting the per-request cache.
270
+//
271
+// Use this when a handler needs to ask "is this actor at least X" for a
272
+// rule that doesn't map cleanly to an Action — for example, the
273
+// locked-issue gate which lets triage+ comment despite the lock without
274
+// granting them any other write permission.
275
+//
276
+// Returns RoleNone for anonymous actors and on any DB error; callers
277
+// should treat RoleNone as "no permissions."
278
+func EffectiveRole(ctx context.Context, d Deps, actor Actor, repo RepoRef) Role {
279
+	if actor.IsAnonymous {
280
+		return RoleNone
281
+	}
282
+	r, err := effectiveRole(ctx, d, actor, repo)
283
+	if err != nil {
284
+		return RoleNone
285
+	}
286
+	return r
287
+}
288
+
289
+// HasRoleAtLeast is the convenience shorthand for the common case:
290
+// "does this actor hold at least `want` on this repo?" Wraps
291
+// EffectiveRole + RoleAtLeast.
292
+func HasRoleAtLeast(ctx context.Context, d Deps, actor Actor, repo RepoRef, want Role) bool {
293
+	return RoleAtLeast(EffectiveRole(ctx, d, actor, repo), want)
294
+}
internal/issues/issues_test.gomodified
@@ -182,6 +182,44 @@ func TestAddComment_LockedRejectsNonCollab(t *testing.T) {
182
 	}
182
 	}
183
 }
183
 }
184
 
184
 
185
+// TestAddComment_LockedAllowsCollab is the positive companion to the
186
+// previous test: when the orchestrator is told `IsCollab=true` the
187
+// locked gate must yield. This guards the policy contract — a triage+
188
+// collaborator is allowed to post past a lock so they can wrap up
189
+// drive-by spam threads. (S00-S25 audit, finding C3.)
190
+func TestAddComment_LockedAllowsCollab(t *testing.T) {
191
+	pool, deps, uid, rid := setup(t)
192
+	ctx := context.Background()
193
+	row, err := issues.Create(ctx, deps, issues.CreateParams{
194
+		RepoID: rid, AuthorUserID: uid, Title: "lock-me", Body: "",
195
+	})
196
+	if err != nil {
197
+		t.Fatalf("Create: %v", err)
198
+	}
199
+	if err := issues.SetLock(ctx, deps, uid, row.ID, true, "spam"); err != nil {
200
+		t.Fatalf("SetLock: %v", err)
201
+	}
202
+	uq := usersdb.New()
203
+	collab, err := uq.CreateUser(ctx, pool, usersdb.CreateUserParams{
204
+		Username: "tessie", DisplayName: "Tessie", PasswordHash: fixtureHash,
205
+	})
206
+	if err != nil {
207
+		t.Fatalf("CreateUser: %v", err)
208
+	}
209
+	c, err := issues.AddComment(ctx, deps, issues.CommentCreateParams{
210
+		IssueID:      row.ID,
211
+		AuthorUserID: collab.ID,
212
+		Body:         "wrapping up the thread",
213
+		IsCollab:     true,
214
+	})
215
+	if err != nil {
216
+		t.Fatalf("expected lock bypass for collab, got %v", err)
217
+	}
218
+	if c.ID == 0 {
219
+		t.Errorf("returned comment had zero ID")
220
+	}
221
+}
222
+
185
 func TestSetState_EmitsEvent(t *testing.T) {
223
 func TestSetState_EmitsEvent(t *testing.T) {
186
 	pool, deps, uid, rid := setup(t)
224
 	pool, deps, uid, rid := setup(t)
187
 	ctx := context.Background()
225
 	ctx := context.Background()
internal/web/handlers/repo/issues.gomodified
@@ -66,6 +66,7 @@ func (h *Handlers) issuesDeps() issues.Deps {
66
 		Pool:    h.d.Pool,
66
 		Pool:    h.d.Pool,
67
 		Limiter: h.d.Limiter,
67
 		Limiter: h.d.Limiter,
68
 		Logger:  h.d.Logger,
68
 		Logger:  h.d.Logger,
69
+		Audit:   h.d.Audit,
69
 	}
70
 	}
70
 }
71
 }
71
 
72
 
@@ -332,10 +333,13 @@ func (h *Handlers) issueComment(w http.ResponseWriter, r *http.Request) {
332
 	viewer := middleware.CurrentUserFromContext(r.Context())
333
 	viewer := middleware.CurrentUserFromContext(r.Context())
333
 	body := r.PostFormValue("body")
334
 	body := r.PostFormValue("body")
334
 
335
 
335
-	// IsCollab — for v1 only the repo owner counts as collaborator
336
+	// IsCollab is the locked-issue bypass: triage+ on the repo can comment
336
-	// (S15 attaches collaborators table; lookup deferred for the
337
+	// past a `locked=true` flag (the gate exists to silence drive-by
337
-	// locked-issue gate). Owners always pass.
338
+	// posters). We resolve the *real* role via the policy package — owner
338
-	isCollab := row.OwnerUserID.Valid && row.OwnerUserID.Int64 == viewer.ID
339
+	// is implicit admin, and any explicit collaborator with role >= triage
340
+	// passes. Read fails (DB miss, unknown role) fail closed via RoleNone.
341
+	actor := policy.UserActor(viewer.ID, viewer.Username, viewer.IsSuspended, false)
342
+	isCollab := policy.HasRoleAtLeast(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, policy.NewRepoRefFromRepo(row), policy.RoleTriage)
339
 
343
 
340
 	_, err := issues.AddComment(r.Context(), h.issuesDeps(), issues.CommentCreateParams{
344
 	_, err := issues.AddComment(r.Context(), h.issuesDeps(), issues.CommentCreateParams{
341
 		IssueID:      issue.ID,
345
 		IssueID:      issue.ID,
@@ -351,7 +355,12 @@ func (h *Handlers) issueComment(w http.ResponseWriter, r *http.Request) {
351
 }
355
 }
352
 
356
 
353
 func (h *Handlers) issueSetState(w http.ResponseWriter, r *http.Request) {
357
 func (h *Handlers) issueSetState(w http.ResponseWriter, r *http.Request) {
354
-	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionIssueClose)
358
+	// Two-pass authorization: read access first, then ActionIssueClose
359
+	// with `repo.AuthorUserID = issue.AuthorUserID` set so the policy
360
+	// engine grants author-self-close. Without the second pass, an
361
+	// issue's reporter who isn't a triage collaborator couldn't close
362
+	// their own thread — which the audit flagged (S00-S25, H1).
363
+	row, owner, ok := h.loadRepoAndAuthorize(w, r, policy.ActionIssueRead)
355
 	if !ok {
364
 	if !ok {
356
 		return
365
 		return
357
 	}
366
 	}
@@ -359,9 +368,18 @@ func (h *Handlers) issueSetState(w http.ResponseWriter, r *http.Request) {
359
 	if !ok {
368
 	if !ok {
360
 		return
369
 		return
361
 	}
370
 	}
371
+	viewer := middleware.CurrentUserFromContext(r.Context())
372
+	actor := policy.UserActor(viewer.ID, viewer.Username, viewer.IsSuspended, false)
373
+	repoRef := policy.NewRepoRefFromRepo(row)
374
+	if issue.AuthorUserID.Valid {
375
+		repoRef.AuthorUserID = issue.AuthorUserID.Int64
376
+	}
377
+	if dec := policy.Can(r.Context(), policy.Deps{Pool: h.d.Pool}, actor, policy.ActionIssueClose, repoRef); !dec.Allow {
378
+		h.d.Render.HTTPError(w, r, policy.Maybe404(dec, repoRef, actor), "")
379
+		return
380
+	}
362
 	state := strings.TrimSpace(r.PostFormValue("state"))
381
 	state := strings.TrimSpace(r.PostFormValue("state"))
363
 	reason := strings.TrimSpace(r.PostFormValue("reason"))
382
 	reason := strings.TrimSpace(r.PostFormValue("reason"))
364
-	viewer := middleware.CurrentUserFromContext(r.Context())
365
 	if err := issues.SetState(r.Context(), h.issuesDeps(), viewer.ID, issue.ID, state, reason); err != nil {
383
 	if err := issues.SetState(r.Context(), h.issuesDeps(), viewer.ID, issue.ID, state, reason); err != nil {
366
 		h.handleIssueWriteError(w, r, owner.Username, row, issue, err)
384
 		h.handleIssueWriteError(w, r, owner.Username, row, issue, err)
367
 		return
385
 		return