Skip to content

Commit 7cf0cb1

Browse files
review:review-all is the explicit tag-scoping bypass, not read:read-all
Review feedback on #712 (peterrrock2): the *-all read/update/delete scopes govern access across CMS authorship boundaries; letting read:read-all also widen moderation reach conflated two axes. New TokenScope.review_all_content is the only scope that lifts the review_tags restriction, and read:read-all no longer does (covered by a regression test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 03adac1 commit 7cf0cb1

4 files changed

Lines changed: 30 additions & 9 deletions

File tree

backend/app/comments/main.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -569,15 +569,17 @@ def allowed_review_tags(auth_result: dict) -> list[str] | None:
569569
tags via ReviewTagAssignment rows, minted into the JWT as a `review_tags`
570570
claim (sorted list of tag slugs). Semantics:
571571
572-
- `read:read-all` in the token scopes → None (unrestricted): that scope
573-
means unrestricted read everywhere, and the CMS strips it from
574-
tag-scoped reviewers so it can act as the admin/superuser escape hatch.
572+
- `review:review-all` in the token scopes → None (unrestricted): the
573+
explicit tag-scoping bypass. The CMS never grants it to tag-scoped
574+
reviewers, so it acts as the admin/superuser escape hatch. (read:read-all
575+
deliberately does NOT bypass: the *-all read/update/delete scopes govern
576+
CMS authorship boundaries, a different axis from moderation reach.)
575577
- `review_tags` claim absent → None (unrestricted): users with no
576578
assignments are unrestricted (back-compat for internal reviewers).
577579
- otherwise → the claim's list; an empty list allows nothing.
578580
"""
579581
token_scopes = (auth_result.get("scope") or "").split()
580-
if TokenScope.read_all_content in token_scopes:
582+
if TokenScope.review_all_content in token_scopes:
581583
return None
582584
review_tags = auth_result.get("review_tags")
583585
if review_tags is None:

backend/app/core/security.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ class TokenScope:
2929
delete_all_content = "delete:delete-all"
3030

3131
review_content = "create:content_review"
32+
# Explicit bypass of per-reviewer tag scoping (the `review_tags` claim).
33+
# Deliberately separate from read:read-all: the *-all read/update/delete
34+
# scopes govern access across CMS authorship boundaries, while this one
35+
# widens moderation reach.
36+
review_all_content = "review:review-all"
3237

3338

3439
class UnauthorizedException(HTTPException):

backend/tests/test_auth_contract.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,8 @@ def test_multiple_required_scopes(verifier, keypair, jwks):
157157

158158

159159
def test_review_tags_claim_round_trips(verifier, keypair, jwks):
160-
"""Tag-scoped reviewer token: the CMS strips read:read-all and mints a
161-
sorted review_tags claim; the verifier must hand the claim through
160+
"""Tag-scoped reviewer token: the CMS withholds review:review-all and
161+
mints a sorted review_tags claim; the verifier must hand the claim through
162162
unchanged for allowed_review_tags (app/comments/main.py) to enforce."""
163163
private_pem, _ = keypair
164164
kid = jwks["keys"][0]["kid"]
@@ -172,7 +172,7 @@ def test_review_tags_claim_round_trips(verifier, keypair, jwks):
172172
payload = run_verify(verifier, token, ["create:content_review"])
173173

174174
assert payload["review_tags"] == ["environment", "schools"]
175-
assert "read:read-all" not in payload["scope"].split()
175+
assert "review:review-all" not in payload["scope"].split()
176176

177177

178178
def test_review_tags_claim_absent_by_default(verifier, keypair, jwks):

backend/tests/test_comments.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1491,7 +1491,7 @@ class TestReviewTagScoping:
14911491
"""Per-user review scoping on the admin moderation endpoints.
14921492
14931493
The CMS mints a `review_tags` claim for reviewers with
1494-
ReviewTagAssignments (and strips `read:read-all` from their scopes);
1494+
ReviewTagAssignments (and never grants them `review:review-all`);
14951495
allowed_review_tags + apply_allowed_tags_filter in app/comments/main.py
14961496
enforce it. The conftest client fixture overrides auth.verify with a
14971497
payload dict, so each test injects the claim/scope combination it needs.
@@ -1560,7 +1560,9 @@ def test_unrestricted_without_claim_sees_all(self, client, tagged_comments):
15601560
assert response.status_code == 200
15611561
assert len(response.json()) == 3
15621562

1563-
def test_read_all_scope_overrides_claim(self, client, tagged_comments):
1563+
def test_read_all_scope_does_not_bypass_tag_scoping(self, client, tagged_comments):
1564+
# read:read-all governs authorship-boundary reads, not moderation
1565+
# reach — it must NOT widen a tag-scoped reviewer.
15641566
self._set_auth(
15651567
{
15661568
"sub": "42",
@@ -1570,6 +1572,18 @@ def test_read_all_scope_overrides_claim(self, client, tagged_comments):
15701572
)
15711573
response = client.get("/api/comments/admin/list")
15721574
assert response.status_code == 200
1575+
assert [c["title"] for c in response.json()] == ["Environment Comment"]
1576+
1577+
def test_review_all_scope_lists_everything(self, client, tagged_comments):
1578+
self._set_auth(
1579+
{
1580+
"sub": "42",
1581+
"scope": "create:content_review review:review-all",
1582+
"review_tags": ["environment"],
1583+
}
1584+
)
1585+
response = client.get("/api/comments/admin/list")
1586+
assert response.status_code == 200
15731587
assert len(response.json()) == 3
15741588

15751589
def test_review_comment_in_scope_succeeds(self, client, tagged_comments):

0 commit comments

Comments
 (0)