Allow public issue participation
- SHA
458c49445477151d241cdac7333ada2ed6186b9c- Parents
-
19cf11c - Tree
c569dc1
458c494
458c49445477151d241cdac7333ada2ed6186b9c19cf11c
c569dc1| Status | File | + | - |
|---|---|---|---|
| M |
docs/internal/issues.md
|
7 | 3 |
| M |
docs/internal/permissions.md
|
9 | 6 |
| M |
internal/auth/policy/actions.go
|
7 | 0 |
| M |
internal/auth/policy/policy.go
|
46 | 20 |
| M |
internal/auth/policy/policy_test.go
|
47 | 2 |
docs/internal/issues.mdmodified@@ -86,6 +86,9 @@ SQL `INSERT … ON CONFLICT DO NOTHING`. | ||
| 86 | 86 | Public reads still pass through `policy.Can(ActionIssueRead)` so |
| 87 | 87 | private repos return the existence-leak 404. RequireUser on writes |
| 88 | 88 | gives anonymous browsers a `/login` redirect instead of the same 404. |
| 89 | +Issue creation and commenting follow GitHub's public-participation | |
| 90 | +model: any logged-in user may open or comment on issues in a public | |
| 91 | +repo, while private repos require `read` access. | |
| 89 | 92 | |
| 90 | 93 | ## Cross-reference indexing |
| 91 | 94 | |
@@ -123,9 +126,10 @@ the cached HTML doesn't contain `<script` while still rendering | ||
| 123 | 126 | |
| 124 | 127 | `SetLock` flips `locked` + emits a `locked`/`unlocked` event. Comments |
| 125 | 128 | hit the locked gate inside `AddComment`: non-collaborators get |
| 126 | -`ErrIssueLocked`. The `IsCollab` flag is the caller's responsibility — | |
| 127 | -for v1 the web handler treats the repo owner as the only collaborator; | |
| 128 | -S15's `repo_collaborators` table is wired but the lookup is deferred. | |
| 129 | +`ErrIssueLocked`. The `IsCollab` flag is the caller's responsibility; | |
| 130 | +the web handler resolves it via `policy.HasRoleAtLeast(..., RoleTriage)`, | |
| 131 | +so owners, direct collaborators, and team-derived triage+ roles can | |
| 132 | +comment through a lock. | |
| 129 | 133 | |
| 130 | 134 | ## Rate limit |
| 131 | 135 | |
docs/internal/permissions.mdmodified@@ -56,8 +56,8 @@ The complete map (also enforced by the matrix test): | ||
| 56 | 56 | | `repo:transfer` | `admin` | |
| 57 | 57 | | `repo:visibility` | `admin` | |
| 58 | 58 | | `issue:read` | `read` (private) | |
| 59 | -| `issue:create` | `write` | | |
| 60 | -| `issue:comment` | `write` | | |
| 59 | +| `issue:create` | logged in on public; `read` on private | | |
| 60 | +| `issue:comment` | logged in on public; `read` on private | | |
| 61 | 61 | | `issue:close` | `triage` | |
| 62 | 62 | | `issue:label` | `triage` | |
| 63 | 63 | | `issue:assign` | `triage` | |
@@ -88,14 +88,17 @@ the verdict. Ordered from most-decisive to least: | ||
| 88 | 88 | 4. **Anonymous + private repo** → deny (`DenyVisibility`). Caller maps |
| 89 | 89 | to 404, not 403, to avoid existence leak. |
| 90 | 90 | 5. **Public repo + read** → allow. |
| 91 | -6. Compute effective role (owner ⇒ admin; collaborator ⇒ row.role; | |
| 91 | +6. **Public issue participation** → logged-in users can create and | |
| 92 | + comment on issues in public repos, subject to the suspended actor, | |
| 93 | + archived repo, and suspended org write gates. | |
| 94 | +7. Compute effective role (owner ⇒ admin; collaborator ⇒ row.role; | |
| 92 | 95 | else `RoleNone`). |
| 93 | -7. **Archived repo + write** → deny (`DenyArchived`). Even owners | |
| 96 | +8. **Archived repo + write** → deny (`DenyArchived`). Even owners | |
| 94 | 97 | can't push to archived repos. |
| 95 | -8. **Min role for action** vs effective role. Below threshold + private | |
| 98 | +9. **Min role for action** vs effective role. Below threshold + private | |
| 96 | 99 | repo + no role → deny as visibility (404). Below threshold with any |
| 97 | 100 | role → deny as `DenyRoleTooLow` (403). |
| 98 | -9. **Login-required actions** (star/fork) on anonymous → deny | |
| 101 | +10. **Login-required actions** (star/fork) on anonymous → deny | |
| 99 | 102 | (`DenyAnonymous`). |
| 100 | 103 | |
| 101 | 104 | ## Existence-leak guard |
internal/auth/policy/actions.gomodified@@ -98,3 +98,10 @@ func isWriteAction(a Action) bool { | ||
| 98 | 98 | // isReadAction is the inverse, broken out for readability at call sites |
| 99 | 99 | // that branch on intent rather than on the absence of writes. |
| 100 | 100 | func isReadAction(a Action) bool { return !isWriteAction(a) } |
| 101 | + | |
| 102 | +// isIssueParticipationAction is the GitHub-shaped issue conversation | |
| 103 | +// surface: any logged-in user can open or comment on issues in a public | |
| 104 | +// repo, while private repos require normal read access. | |
| 105 | +func isIssueParticipationAction(a Action) bool { | |
| 106 | + return a == ActionIssueCreate || a == ActionIssueComment | |
| 107 | +} | |
internal/auth/policy/policy.gomodified@@ -102,7 +102,26 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef) | ||
| 102 | 102 | return allow("public repo read") |
| 103 | 103 | } |
| 104 | 104 | |
| 105 | - // 6. From here we need the actor's effective role on the repo. | |
| 105 | + // 6. Public issue participation: any logged-in user can open or | |
| 106 | + // comment on issues in a public repo. Private repos still fall | |
| 107 | + // through to the role check below, where read access is required. | |
| 108 | + // Archive/org-suspension write gates stay below role resolution | |
| 109 | + // for the general case, so enforce them explicitly here before | |
| 110 | + // allowing a non-collaborator public-repo issue action. | |
| 111 | + if isIssueParticipationAction(action) && repo.IsPublic() { | |
| 112 | + if actor.IsAnonymous { | |
| 113 | + return deny(DenyAnonymous, "anonymous cannot create/comment on issues") | |
| 114 | + } | |
| 115 | + if repo.IsArchived { | |
| 116 | + return deny(DenyArchived, "repo archived") | |
| 117 | + } | |
| 118 | + if repo.OwnerOrgID != 0 && isOrgSuspended(ctx, d, repo.OwnerOrgID) { | |
| 119 | + return deny(DenyOrgSuspended, "owning org suspended") | |
| 120 | + } | |
| 121 | + return allow("public issue participation") | |
| 122 | + } | |
| 123 | + | |
| 124 | + // 7. From here we need the actor's effective role on the repo. | |
| 106 | 125 | // Owner short-circuits to admin; collaborator role from DB |
| 107 | 126 | // otherwise; org membership stub for S31. |
| 108 | 127 | role, err := effectiveRole(ctx, d, actor, repo) |
@@ -112,7 +131,7 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef) | ||
| 112 | 131 | return deny(DenyDBError, "role lookup failed: "+err.Error()) |
| 113 | 132 | } |
| 114 | 133 | |
| 115 | - // 6a. Author-self-close on issues and PRs. The author of an issue or | |
| 134 | + // 7a. Author-self-close on issues and PRs. The author of an issue or | |
| 116 | 135 | // PR is allowed to close (and reopen — same Action) their own |
| 117 | 136 | // thread regardless of their collaborator role. Handlers populate |
| 118 | 137 | // `repo.AuthorUserID` on the close path; everywhere else the |
@@ -125,33 +144,23 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef) | ||
| 125 | 144 | return allow("author of thread") |
| 126 | 145 | } |
| 127 | 146 | |
| 128 | - // 7. Archived repos: writes denied even for owners. Reads still go | |
| 147 | + // 8. Archived repos: writes denied even for owners. Reads still go | |
| 129 | 148 | // through the role check above. (We could short-circuit reads |
| 130 | 149 | // earlier but keeping the flow uniform makes the matrix readable.) |
| 131 | 150 | if repo.IsArchived && isWriteAction(action) { |
| 132 | 151 | return deny(DenyArchived, "repo archived") |
| 133 | 152 | } |
| 134 | 153 | |
| 135 | - // 7b. Org suspension (S30): writes against any repo owned by a | |
| 154 | + // 8b. Org suspension (S30): writes against any repo owned by a | |
| 136 | 155 | // suspended org are denied uniformly. Reads stay allowed (the |
| 137 | 156 | // org's contributions to the broader graph aren't erased). |
| 138 | 157 | // The check is gated on a write action AND a non-zero |
| 139 | 158 | // OwnerOrgID so user-owned repos pay nothing for it. |
| 140 | - if repo.OwnerOrgID != 0 && isWriteAction(action) { | |
| 141 | - if d.Pool != nil { | |
| 142 | - var suspended bool | |
| 143 | - err := d.Pool.QueryRow( | |
| 144 | - ctx, | |
| 145 | - `SELECT suspended_at IS NOT NULL FROM orgs WHERE id = $1`, | |
| 146 | - repo.OwnerOrgID, | |
| 147 | - ).Scan(&suspended) | |
| 148 | - if err == nil && suspended { | |
| 149 | - return deny(DenyOrgSuspended, "owning org suspended") | |
| 150 | - } | |
| 151 | - } | |
| 159 | + if repo.OwnerOrgID != 0 && isWriteAction(action) && isOrgSuspended(ctx, d, repo.OwnerOrgID) { | |
| 160 | + return deny(DenyOrgSuspended, "owning org suspended") | |
| 152 | 161 | } |
| 153 | 162 | |
| 154 | - // 8. Map action → minimum required role; check. | |
| 163 | + // 9. Map action → minimum required role; check. | |
| 155 | 164 | want := minRoleFor(action) |
| 156 | 165 | if want != RoleNone && !RoleAtLeast(role, want) { |
| 157 | 166 | // No role at all + private repo → look like a visibility deny |
@@ -163,7 +172,7 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef) | ||
| 163 | 172 | return deny(DenyRoleTooLow, "role too low") |
| 164 | 173 | } |
| 165 | 174 | |
| 166 | - // 9. Login-required actions: star/fork/watch-set need any | |
| 175 | + // 10. Login-required actions: star/fork/watch-set need any | |
| 167 | 176 | // logged-in user. Anonymous reaches here only on a public repo |
| 168 | 177 | // (see step 4); we deny with the anonymous code so the handler |
| 169 | 178 | // can render a friendly "log in to star" prompt. |
@@ -175,6 +184,19 @@ func Can(ctx context.Context, d Deps, actor Actor, action Action, repo RepoRef) | ||
| 175 | 184 | return allow("granted") |
| 176 | 185 | } |
| 177 | 186 | |
| 187 | +func isOrgSuspended(ctx context.Context, d Deps, orgID int64) bool { | |
| 188 | + if d.Pool == nil { | |
| 189 | + return false | |
| 190 | + } | |
| 191 | + var suspended bool | |
| 192 | + err := d.Pool.QueryRow( | |
| 193 | + ctx, | |
| 194 | + `SELECT suspended_at IS NOT NULL FROM orgs WHERE id = $1`, | |
| 195 | + orgID, | |
| 196 | + ).Scan(&suspended) | |
| 197 | + return err == nil && suspended | |
| 198 | +} | |
| 199 | + | |
| 178 | 200 | // IsVisibleTo is a convenience wrapper around Can(actor, repo:read, …). |
| 179 | 201 | // Used by listing endpoints that need to filter results by visibility |
| 180 | 202 | // without caring about the deny reason. |
@@ -390,10 +412,14 @@ func minRoleFor(action Action) Role { | ||
| 390 | 412 | return RoleTriage |
| 391 | 413 | |
| 392 | 414 | // Write tier — code push, branch create, PR open/comment. |
| 393 | - case ActionRepoWrite, ActionPullCreate, ActionPullReview, ActionPullClose, | |
| 394 | - ActionIssueCreate, ActionIssueComment: | |
| 415 | + case ActionRepoWrite, ActionPullCreate, ActionPullReview, ActionPullClose: | |
| 395 | 416 | return RoleWrite |
| 396 | 417 | |
| 418 | + // Issue participation on private repos requires read access. Public | |
| 419 | + // repos are handled by Can's public issue participation branch above. | |
| 420 | + case ActionIssueCreate, ActionIssueComment: | |
| 421 | + return RoleRead | |
| 422 | + | |
| 397 | 423 | // Maintain tier — most settings except dangerous ones. |
| 398 | 424 | case ActionRepoSettingsGeneral, ActionRepoSettingsBranches: |
| 399 | 425 | return RoleMaintain |
internal/auth/policy/policy_test.gomodified@@ -160,6 +160,15 @@ func expect(actor actorKind, repo repoKind, action policy.Action) bool { | ||
| 160 | 160 | return true |
| 161 | 161 | } |
| 162 | 162 | |
| 163 | + // Public issue participation: any logged-in user can open/comment | |
| 164 | + // on issues in a non-archived public repo. | |
| 165 | + if (action == policy.ActionIssueCreate || action == policy.ActionIssueComment) && !isPrivate { | |
| 166 | + if actor == actorAnonymous || isArchived { | |
| 167 | + return false | |
| 168 | + } | |
| 169 | + return true | |
| 170 | + } | |
| 171 | + | |
| 163 | 172 | // Compute role. |
| 164 | 173 | var have policy.Role |
| 165 | 174 | switch actor { |
@@ -205,8 +214,9 @@ func mirrorMinRoleFor(a policy.Action) policy.Role { | ||
| 205 | 214 | return policy.RoleRead |
| 206 | 215 | case policy.ActionIssueClose, policy.ActionIssueLabel, policy.ActionIssueAssign: |
| 207 | 216 | return policy.RoleTriage |
| 208 | - case policy.ActionRepoWrite, policy.ActionPullCreate, policy.ActionPullReview, policy.ActionPullClose, | |
| 209 | - policy.ActionIssueCreate, policy.ActionIssueComment: | |
| 217 | + case policy.ActionIssueCreate, policy.ActionIssueComment: | |
| 218 | + return policy.RoleRead | |
| 219 | + case policy.ActionRepoWrite, policy.ActionPullCreate, policy.ActionPullReview, policy.ActionPullClose: | |
| 210 | 220 | return policy.RoleWrite |
| 211 | 221 | case policy.ActionRepoSettingsGeneral, policy.ActionRepoSettingsBranches: |
| 212 | 222 | return policy.RoleMaintain |
@@ -252,6 +262,41 @@ func TestCan_Matrix(t *testing.T) { | ||
| 252 | 262 | } |
| 253 | 263 | } |
| 254 | 264 | |
| 265 | +func TestCan_PublicIssueParticipation(t *testing.T) { | |
| 266 | + t.Parallel() | |
| 267 | + | |
| 268 | + d := policy.Deps{} | |
| 269 | + publicRepo := makeRepo(repoPublic) | |
| 270 | + privateRepo := makeRepo(repoPrivate) | |
| 271 | + archivedPublicRepo := makeRepo(repoArchivedPublic) | |
| 272 | + stranger := makeActor(actorUnrelated) | |
| 273 | + readCollab := makeActor(actorCollabRead) | |
| 274 | + | |
| 275 | + for _, action := range []policy.Action{policy.ActionIssueCreate, policy.ActionIssueComment} { | |
| 276 | + action := action | |
| 277 | + t.Run(string(action), func(t *testing.T) { | |
| 278 | + t.Parallel() | |
| 279 | + | |
| 280 | + if got := policy.Can(context.Background(), d, stranger, action, publicRepo); !got.Allow { | |
| 281 | + t.Fatalf("logged-in non-collab on public repo should be allowed to %s: %#v", action, got) | |
| 282 | + } | |
| 283 | + if got := policy.Can(context.Background(), d, policy.AnonymousActor(), action, publicRepo); got.Allow || got.Code != policy.DenyAnonymous { | |
| 284 | + t.Fatalf("anonymous public repo %s = %#v, want DenyAnonymous", action, got) | |
| 285 | + } | |
| 286 | + ctx := ctxWithCollabRole(t, actorCollabRead) | |
| 287 | + if got := policy.Can(ctx, d, readCollab, action, privateRepo); !got.Allow { | |
| 288 | + t.Fatalf("read collab on private repo should be allowed to %s: %#v", action, got) | |
| 289 | + } | |
| 290 | + if got := policy.Can(context.Background(), d, stranger, action, privateRepo); got.Allow || got.Code != policy.DenyVisibility { | |
| 291 | + t.Fatalf("stranger private repo %s = %#v, want DenyVisibility", action, got) | |
| 292 | + } | |
| 293 | + if got := policy.Can(context.Background(), d, stranger, action, archivedPublicRepo); got.Allow || got.Code != policy.DenyArchived { | |
| 294 | + t.Fatalf("archived public repo %s = %#v, want DenyArchived", action, got) | |
| 295 | + } | |
| 296 | + }) | |
| 297 | + } | |
| 298 | +} | |
| 299 | + | |
| 255 | 300 | func TestRoleAtLeast(t *testing.T) { |
| 256 | 301 | t.Parallel() |
| 257 | 302 | cases := []struct { |