diff --git a/backend/app/dependencies/auth.py b/backend/app/dependencies/auth.py index d06ffd7..33daf0a 100644 --- a/backend/app/dependencies/auth.py +++ b/backend/app/dependencies/auth.py @@ -253,6 +253,30 @@ def require_any_role(*roles: str) -> Callable[..., object]: return role_checker +def require_any_role_strict(*roles: str) -> Callable[..., object]: + """Return a FastAPI dependency that enforces **any** of the given *roles*. + + Unlike ``require_any_role``, admins do **not** automatically pass — use + this for actions that belong exclusively to Red/Blue operators, leads, + or managers (e.g. executing, reviewing, validating, or resolving a + disputed test), where "admin" must mean site administration only, not + a backdoor into the test workflow itself. + """ + + async def role_checker( + current_user: User = Depends(get_current_user), + ) -> User: + if current_user.role not in roles: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Not enough permissions", + ) + _check_api_key_scope(current_user, "write") + return current_user + + return role_checker + + def require_scope(scope: str): """Return a dependency that enforces the API key carries *scope*. diff --git a/backend/app/routers/tests.py b/backend/app/routers/tests.py index 189d776..d25cacc 100644 --- a/backend/app/routers/tests.py +++ b/backend/app/routers/tests.py @@ -42,7 +42,7 @@ from sqlalchemy.orm import Session from app.database import get_db # Import get_current_user, require_any_role from app.dependencies.auth -from app.dependencies.auth import get_current_user, require_any_role +from app.dependencies.auth import get_current_user, require_any_role, require_any_role_strict # Import UnitOfWork from app.domain.unit_of_work from app.domain.unit_of_work import UnitOfWork @@ -595,8 +595,8 @@ def update_test_classification( # Entry: db db: Session = Depends(get_db), # Entry: current_user - current_user: User = Depends(require_any_role( - "admin", "manager", "red_tech", "red_lead", "blue_tech", "blue_lead", + current_user: User = Depends(require_any_role_strict( + "manager", "red_tech", "red_lead", "blue_tech", "blue_lead", )), ) -> TestOut: """Update the data classification label for a test. @@ -964,12 +964,12 @@ def review_red( test_id: uuid.UUID, payload: TestRedReview, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("red_lead", "admin")), + current_user: User = Depends(require_any_role_strict("red_lead")), ) -> TestOut: """Assigned Red Lead approves or reopens a test sitting in red_review.""" test = crud_get_test_or_raise(db, test_id) - if current_user.role != "admin" and test.red_reviewer_assignee != current_user.id: + if test.red_reviewer_assignee != current_user.id: raise HTTPException(status_code=403, detail="You are not the assigned reviewer for this test") with UnitOfWork(db) as uow: @@ -994,12 +994,12 @@ def review_blue( test_id: uuid.UUID, payload: TestBlueReview, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("blue_lead", "admin")), + current_user: User = Depends(require_any_role_strict("blue_lead")), ) -> TestOut: """Assigned Blue Lead approves, reopens, or flags a capability gap on a test in blue_review.""" test = crud_get_test_or_raise(db, test_id) - if current_user.role != "admin" and test.blue_reviewer_assignee != current_user.id: + if test.blue_reviewer_assignee != current_user.id: raise HTTPException(status_code=403, detail="You are not the assigned reviewer for this test") with UnitOfWork(db) as uow: @@ -1109,7 +1109,7 @@ def validate_red( # Entry: db db: Session = Depends(get_db), # Entry: current_user - current_user: User = Depends(require_any_role("red_lead")), + current_user: User = Depends(require_any_role_strict("red_lead")), ) -> TestOut: """Red Lead approves or rejects the red side of a test. @@ -1166,7 +1166,7 @@ def validate_blue( # Entry: db db: Session = Depends(get_db), # Entry: current_user - current_user: User = Depends(require_any_role("blue_lead")), + current_user: User = Depends(require_any_role_strict("blue_lead")), ) -> TestOut: """Blue Lead approves or rejects the blue side of a test. @@ -1218,7 +1218,7 @@ def resolve_dispute( test_id: uuid.UUID, payload: TestResolveDispute, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("red_lead", "blue_lead", "admin")), + current_user: User = Depends(require_any_role_strict("red_lead", "blue_lead")), ) -> TestOut: """The lead who approved flips their vote to reject, choosing which team must redo the work.""" test = crud_get_test_with_technique(db, test_id) @@ -1242,7 +1242,7 @@ def reopen( # Entry: db db: Session = Depends(get_db), # Entry: current_user - current_user: User = Depends(require_any_role("red_lead", "blue_lead")), + current_user: User = Depends(require_any_role_strict("red_lead", "blue_lead")), ) -> TestOut: """Reopen a rejected test, moving it back to ``draft``. @@ -1269,7 +1269,7 @@ def reopen( # --------------------------------------------------------------------------- -# POST /tests/{id}/assign — assign red_tech / blue_tech operators (leads + admin) +# POST /tests/{id}/assign — assign red_tech / blue_tech operators (leads + managers) # --------------------------------------------------------------------------- @@ -1278,9 +1278,9 @@ def assign_test_operators( test_id: uuid.UUID, payload: TestAssign, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("admin", "red_lead", "blue_lead")), + current_user: User = Depends(require_any_role_strict("manager", "red_lead", "blue_lead")), ): - """Assign red_tech and/or blue_tech operators to a test. Admin/leads only.""" + """Assign red_tech and/or blue_tech operators to a test. Leads/managers only — not admin, who administers the site rather than coordinating operators.""" test = crud_get_test_or_raise(db, test_id) newly_assigned: User | None = None @@ -1332,7 +1332,7 @@ def hold_test( test_id: uuid.UUID, payload: TestHold, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("red_tech", "blue_tech", "admin")), + current_user: User = Depends(require_any_role_strict("red_tech", "blue_tech")), ): """Place a test on hold with a mandatory reason. Posts comment + transitions Jira.""" from datetime import datetime as _dt @@ -1372,7 +1372,7 @@ def hold_test( def resume_test( test_id: uuid.UUID, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("red_tech", "blue_tech", "admin")), + current_user: User = Depends(require_any_role_strict("red_tech", "blue_tech")), ): """Resume a test that was placed on hold.""" from app.services.jira_service import push_hold_event @@ -1644,7 +1644,7 @@ def sync_tempo( def request_discussion( test_id: uuid.UUID, db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("red_lead", "blue_lead", "admin")), + current_user: User = Depends(require_any_role_strict("red_lead", "blue_lead")), ): """Called when the approving lead confirms their vote in a disputed test. @@ -1663,12 +1663,12 @@ def request_discussion( role = current_user.role # Identify who the "other lead" is (the one who rejected) - if (role in ("red_lead", "admin")) and test.red_validation_status == "approved": + if role == "red_lead" and test.red_validation_status == "approved": # Red approved, Blue rejected → notify Blue Lead who rejected rejector_id = test.blue_validated_by rejector_label = "Blue Lead" requester_label = "Red Lead" - elif (role in ("blue_lead", "admin")) and test.blue_validation_status == "approved": + elif role == "blue_lead" and test.blue_validation_status == "approved": # Blue approved, Red rejected → notify Red Lead who rejected rejector_id = test.red_validated_by rejector_label = "Red Lead" diff --git a/backend/app/routers/users.py b/backend/app/routers/users.py index 4bfe0db..a4bd4f6 100644 --- a/backend/app/routers/users.py +++ b/backend/app/routers/users.py @@ -13,7 +13,7 @@ from sqlalchemy.orm import Session from app.database import get_db # Import require_role from app.dependencies.auth -from app.dependencies.auth import require_role, require_any_role +from app.dependencies.auth import require_role, require_any_role_strict # Import UnitOfWork from app.domain.unit_of_work from app.domain.unit_of_work import UnitOfWork @@ -156,13 +156,13 @@ def create_user_route( @router.get("/operators", response_model=list[UserOperatorOut]) def list_operators_route( db: Session = Depends(get_db), - current_user: User = Depends(require_any_role("admin", "red_lead", "blue_lead")), + current_user: User = Depends(require_any_role_strict("manager", "red_lead", "blue_lead")), ) -> list[UserOperatorOut]: - """Return active red/blue operators and leads, for lead/admin assignment pickers. + """Return active red/blue operators and leads, for lead/manager assignment pickers. - Deliberately excludes viewers and managers, and returns only - id/username/role — no emails or tokens — since this is reachable by - non-admin leads. + Not reachable by admin — admin administers the site, leads/managers + coordinate operators. Returns only id/username/role — no emails or + tokens — since this is reachable by non-admin leads. """ return ( db.query(User) diff --git a/backend/app/services/test_workflow_service.py b/backend/app/services/test_workflow_service.py index 4751a55..8095389 100644 --- a/backend/app/services/test_workflow_service.py +++ b/backend/app/services/test_workflow_service.py @@ -1228,13 +1228,12 @@ def resolve_dispute(db: Session, test: Test, user: User, target_team: str, notes if target_team not in ("red", "blue"): raise InvalidOperationError("target_team must be 'red' or 'blue'") - if user.role != "admin": - is_red_approver = user.role == "red_lead" and test.red_validation_status == "approved" - is_blue_approver = user.role == "blue_lead" and test.blue_validation_status == "approved" - if not (is_red_approver or is_blue_approver): - raise InvalidOperationError( - "Only the lead who approved can flip their vote to resolve this dispute" - ) + is_red_approver = user.role == "red_lead" and test.red_validation_status == "approved" + is_blue_approver = user.role == "blue_lead" and test.blue_validation_status == "approved" + if not (is_red_approver or is_blue_approver): + raise InvalidOperationError( + "Only the lead who approved can flip their vote to resolve this dispute" + ) entity = TestEntity.from_orm(test) entity.resolve_dispute_reject(target_team) diff --git a/backend/tests/test_admin_excluded_from_test_workflow.py b/backend/tests/test_admin_excluded_from_test_workflow.py new file mode 100644 index 0000000..f32a635 --- /dev/null +++ b/backend/tests/test_admin_excluded_from_test_workflow.py @@ -0,0 +1,33 @@ +"""Admin must be locked out of every Test-workflow *action* endpoint. + +Admin administers the site; leads/managers/operators run the test workflow. +The permission dependency runs before any DB lookup, so a random UUID is +enough to prove the 403 — the request never reaches the handler body. +""" + +import uuid + +import pytest + +DUMMY_ID = str(uuid.uuid4()) + + +@pytest.mark.parametrize( + "method,path,payload", + [ + ("post", f"/api/v1/tests/{DUMMY_ID}/review-red", {"decision": "approve"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/review-blue", {"decision": "approve"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/validate-red", {"red_validation_status": "approved"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/validate-blue", {"blue_validation_status": "approved"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/resolve-dispute", {"target_team": "red"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/request-discussion", None), + ("post", f"/api/v1/tests/{DUMMY_ID}/reopen", None), + ("post", f"/api/v1/tests/{DUMMY_ID}/hold", {"reason": "x"}), + ("post", f"/api/v1/tests/{DUMMY_ID}/resume", None), + ("post", f"/api/v1/tests/{DUMMY_ID}/assign", {"red_tech_assignee": DUMMY_ID}), + ("patch", f"/api/v1/tests/{DUMMY_ID}/classification", {"data_classification": "pii"}), + ], +) +def test_admin_forbidden(client, auth_headers, method, path, payload): + resp = getattr(client, method)(path, json=payload, headers=auth_headers) + assert resp.status_code == 403, f"{method.upper()} {path} -> {resp.status_code}: {resp.text}" diff --git a/backend/tests/test_assign_operators.py b/backend/tests/test_assign_operators.py index d8d3266..6a50407 100644 --- a/backend/tests/test_assign_operators.py +++ b/backend/tests/test_assign_operators.py @@ -1,4 +1,4 @@ -"""Tests for POST /tests/{id}/assign — lead/admin manual operator assignment.""" +"""Tests for POST /tests/{id}/assign — lead/manager manual operator assignment.""" from unittest.mock import MagicMock, patch @@ -67,6 +67,32 @@ def test_red_tech_cannot_assign(client, db, red_tech_headers, red_tech_user): assert resp.status_code == 403 +def test_admin_cannot_assign(client, db, auth_headers, admin_user, red_tech_user): + """Admin administers the site — coordinating operators is a lead/manager call.""" + technique = _seed_technique(db) + test = _seed_test(db, technique, admin_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"red_tech_assignee": str(red_tech_user.id)}, + headers=auth_headers, + ) + assert resp.status_code == 403 + + +def test_manager_can_assign(client, db, manager_headers, manager_user, red_tech_user): + technique = _seed_technique(db) + test = _seed_test(db, technique, manager_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/assign", + json={"red_tech_assignee": str(red_tech_user.id)}, + headers=manager_headers, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["red_tech_assignee"] == str(red_tech_user.id) + + def test_clearing_assignee_does_not_touch_jira(client, db, red_lead_headers, red_lead_user, red_tech_user): """Sending null explicitly clears the assignee without a Jira sync call.""" technique = _seed_technique(db) diff --git a/backend/tests/test_data_classification.py b/backend/tests/test_data_classification.py index 03ecf86..db51fb7 100644 --- a/backend/tests/test_data_classification.py +++ b/backend/tests/test_data_classification.py @@ -61,7 +61,9 @@ def test_create_test_endpoint_uses_tactic_heuristic(client, db, api, auth_header assert resp.json()["data_classification"] == "internal_use_only" -def test_admin_can_update_classification(client, db, admin_user, admin_token, red_lead_user): +def test_admin_cannot_update_classification(client, db, admin_user, admin_token, red_lead_user): + """Admin administers the site, not test content — classification is a + lead/manager/operator call.""" technique = _seed_technique(db) test = Test( technique_id=technique.id, @@ -77,6 +79,25 @@ def test_admin_can_update_classification(client, db, admin_user, admin_token, re json={"data_classification": "pii"}, headers={"Authorization": f"Bearer {admin_token}"}, ) + assert response.status_code == 403 + + +def test_manager_can_update_classification(client, db, manager_user, manager_headers, red_lead_user): + technique = _seed_technique(db) + test = Test( + technique_id=technique.id, + name="Classify me", + created_by=red_lead_user.id, + state=TestState.draft, + ) + db.add(test) + db.commit() + + response = client.patch( + f"/api/v1/tests/{test.id}/classification", + json={"data_classification": "pii"}, + headers=manager_headers, + ) assert response.status_code == 200 assert response.json()["data_classification"] == "pii" diff --git a/backend/tests/test_list_operators.py b/backend/tests/test_list_operators.py index 1ecc446..7a8cbb9 100644 --- a/backend/tests/test_list_operators.py +++ b/backend/tests/test_list_operators.py @@ -1,4 +1,4 @@ -"""Tests for GET /users/operators — lead/admin-reachable operator picker list.""" +"""Tests for GET /users/operators — lead/manager-reachable operator picker list.""" def test_red_lead_can_list_operators(client, red_lead_headers, red_tech_user, blue_tech_user, red_lead_user): @@ -31,3 +31,16 @@ def test_operators_list_excludes_viewers(client, red_lead_headers, db): def test_red_tech_cannot_list_operators(client, red_tech_headers): resp = client.get("/api/v1/users/operators", headers=red_tech_headers) assert resp.status_code == 403 + + +def test_admin_cannot_list_operators(client, auth_headers): + """Admin administers the site — coordinating operators is a lead/manager call.""" + resp = client.get("/api/v1/users/operators", headers=auth_headers) + assert resp.status_code == 403 + + +def test_manager_can_list_operators(client, manager_headers, red_tech_user): + resp = client.get("/api/v1/users/operators", headers=manager_headers) + assert resp.status_code == 200, resp.text + usernames = {u["username"] for u in resp.json()} + assert "redtech" in usernames diff --git a/backend/tests/test_review_gates_router.py b/backend/tests/test_review_gates_router.py index 89db740..96ef9d1 100644 --- a/backend/tests/test_review_gates_router.py +++ b/backend/tests/test_review_gates_router.py @@ -143,14 +143,15 @@ def test_review_red_reopen_moves_to_red_executing( assert body["red_review_notes"] == "add more detail" -def test_review_red_admin_can_always_act( +def test_review_red_forbidden_for_admin( client, db, api, auth_headers, red_tech_headers, red_lead_user, technique ): + """Admin administers the site, not the test workflow — reviewing is a + red_lead-only action now, with no admin override.""" test_id = _reach_red_review(client, db, api, auth_headers, red_tech_headers, technique) resp = api("post", f"/api/v1/tests/{test_id}/review-red", auth_headers, json={"decision": "approve"}) - assert resp.status_code == 200, resp.text - assert resp.json()["state"] == "blue_evaluating" + assert resp.status_code == 403 # --------------------------------------------------------------------------- diff --git a/backend/tests/test_workflow.py b/backend/tests/test_workflow.py index 5d573d2..1bd985b 100644 --- a/backend/tests/test_workflow.py +++ b/backend/tests/test_workflow.py @@ -829,7 +829,9 @@ class TestResolveDispute: resolve_dispute(db, test, red_lead, "purple") @patch("app.services.test_workflow_service.log_action") - def test_admin_can_resolve_dispute_regardless_of_vote(self, mock_log): + def test_admin_cannot_resolve_dispute(self, mock_log): + """Admin administers the site — only the lead who approved may flip + their vote, with no admin override.""" test = _make_test( TestState.disputed, red_validation_status="approved", @@ -839,8 +841,8 @@ class TestResolveDispute: db = _make_db() from app.services.test_workflow_service import resolve_dispute - result = resolve_dispute(db, test, admin, "red") - assert result.state == TestState.red_executing + with pytest.raises(InvalidOperationError): + resolve_dispute(db, test, admin, "red") # =========================================================================== diff --git a/frontend/src/api/tests.ts b/frontend/src/api/tests.ts index 25f6a95..92f4f40 100644 --- a/frontend/src/api/tests.ts +++ b/frontend/src/api/tests.ts @@ -179,7 +179,7 @@ export async function updateTestClassification( return data; } -/** Assign red_tech/blue_tech operators to a test (leads + admin only). */ +/** Assign red_tech/blue_tech operators to a test (leads + managers only). */ export async function assignTestOperators( testId: string, payload: { red_tech_assignee?: string | null; blue_tech_assignee?: string | null }, diff --git a/frontend/src/api/users.ts b/frontend/src/api/users.ts index 238cc14..4af17b2 100644 --- a/frontend/src/api/users.ts +++ b/frontend/src/api/users.ts @@ -36,7 +36,7 @@ export interface OperatorOut { role: string; } -/** Fetch red/blue operators + leads for assignment pickers (leads + admin). */ +/** Fetch red/blue operators + leads for assignment pickers (leads + managers). */ export async function getOperators(): Promise { const { data } = await client.get("/users/operators"); return data; diff --git a/frontend/src/components/test-detail/AssigneeControl.tsx b/frontend/src/components/test-detail/AssigneeControl.tsx index c88e52a..7b18ee6 100644 --- a/frontend/src/components/test-detail/AssigneeControl.tsx +++ b/frontend/src/components/test-detail/AssigneeControl.tsx @@ -9,11 +9,13 @@ interface Props { canEdit: boolean; isSaving: boolean; onAssign: (userId: string | null) => void; + /** "sm" = compact pill (default), "lg" = full-size button matching the action bar. */ + size?: "sm" | "lg"; } const SIDE_STYLE = { - red: "border-orange-500/30 bg-orange-900/20 text-orange-400", - blue: "border-indigo-500/30 bg-indigo-900/20 text-indigo-400", + red: "border-orange-500/40 bg-orange-900/20 text-orange-400 hover:bg-orange-900/40", + blue: "border-indigo-500/40 bg-indigo-900/20 text-indigo-400 hover:bg-indigo-900/40", }; const SIDE_ROLES: Record<"red" | "blue", string[]> = { @@ -21,20 +23,28 @@ const SIDE_ROLES: Record<"red" | "blue", string[]> = { blue: ["blue_tech", "blue_lead", "admin"], }; -/** Lead/admin picker for the red_tech_assignee / blue_tech_assignee fields. */ -export default function AssigneeControl({ side, assigneeId, operators, canEdit, isSaving, onAssign }: Props) { +/** Lead/manager picker for the red_tech_assignee / blue_tech_assignee fields. */ +export default function AssigneeControl({ + side, assigneeId, operators, canEdit, isSaving, onAssign, size = "sm", +}: Props) { const [expanded, setExpanded] = useState(false); const current = operators.find((o) => o.id === assigneeId); const label = current ? current.username : "Unassigned"; const eligible = operators.filter((o) => SIDE_ROLES[side].includes(o.role)); + const sideLabel = side === "red" ? "RT" : "BT"; - const badgeClass = `inline-flex items-center gap-1 rounded-full border px-2 py-0.5 text-[10px] font-medium ${SIDE_STYLE[side]}`; + const badgeClass = + size === "lg" + ? `flex items-center gap-1.5 rounded-lg border px-3 py-2 text-sm font-medium ${SIDE_STYLE[side]}` + : `inline-flex items-center gap-1 rounded-full border px-2 py-0.5 text-[10px] font-medium ${SIDE_STYLE[side]}`; + const iconSize = size === "lg" ? "h-4 w-4" : "h-2.5 w-2.5"; + const chevronSize = size === "lg" ? "h-3.5 w-3.5" : "h-2.5 w-2.5"; if (!canEdit) { return ( - - {side === "red" ? "RT" : "BT"}: {label} + + {sideLabel}: {label} ); } @@ -43,15 +53,15 @@ export default function AssigneeControl({ side, assigneeId, operators, canEdit,
{expanded && ( -
+
{isSaving ? (
diff --git a/frontend/src/components/test-detail/TestDetailHeader.tsx b/frontend/src/components/test-detail/TestDetailHeader.tsx index b604828..2d47054 100644 --- a/frontend/src/components/test-detail/TestDetailHeader.tsx +++ b/frontend/src/components/test-detail/TestDetailHeader.tsx @@ -158,7 +158,7 @@ export default function TestDetailHeader({ const HOLDABLE_STATES: TestState[] = ["draft", "red_executing", "blue_evaluating"]; const canHold = HOLDABLE_STATES.includes(test.state) && - (role === "red_tech" || role === "blue_tech" || role === "admin"); + (role === "red_tech" || role === "blue_tech"); const renderActions = () => { const buttons: React.ReactNode[] = []; @@ -224,7 +224,7 @@ export default function TestDetailHeader({ // Red Lead assigned to review this submission -> Review Red Submission if ( test.state === "red_review" && - (role === "admin" || (role === "red_lead" && test.red_reviewer_assignee === user?.id)) + role === "red_lead" && test.red_reviewer_assignee === user?.id ) { buttons.push(
+
+ onAssignOperator("red", userId)} + size="lg" + /> + onAssignOperator("blue", userId)} + size="lg" + /> +
{renderLiveTimer()} {renderActions()}
diff --git a/frontend/src/pages/TestDetailPage.tsx b/frontend/src/pages/TestDetailPage.tsx index 2883f13..b1bf45a 100644 --- a/frontend/src/pages/TestDetailPage.tsx +++ b/frontend/src/pages/TestDetailPage.tsx @@ -135,7 +135,7 @@ export default function TestDetailPage() { enabled: !!testId && !!test && (test.retest_of !== null || test.retest_count > 0), }); - const canAssignOperators = ["red_lead", "blue_lead", "admin"].includes(user?.role ?? ""); + const canAssignOperators = ["red_lead", "blue_lead", "manager"].includes(user?.role ?? ""); const { data: operators = [] } = useQuery({ queryKey: ["operators"], queryFn: getOperators, @@ -629,7 +629,7 @@ export default function TestDetailPage() { {test.technique_name && ( - \u2014 {test.technique_name} + \— {test.technique_name} )}