From 1b7fd6fb3647b69374b72e584d382a71664f813e Mon Sep 17 00:00:00 2001 From: kitos Date: Tue, 14 Jul 2026 17:24:05 +0200 Subject: [PATCH] fix(tests): restrict On Hold/Resume to whichever team currently owns the test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canHold checked role OR role instead of matching the current phase, so a red_tech still saw (and could call) Hold/Resume on a test already sitting in blue_evaluating. Fixed on both frontend and backend — the API had the same gap, not just the button visibility. --- backend/app/routers/tests.py | 19 ++++++ backend/tests/test_hold_resume.py | 60 +++++++++++++++++++ .../test-detail/TestDetailHeader.tsx | 9 ++- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/backend/app/routers/tests.py b/backend/app/routers/tests.py index 11b4a28..2bd37d0 100644 --- a/backend/app/routers/tests.py +++ b/backend/app/routers/tests.py @@ -1310,6 +1310,21 @@ def assign_test_operators( # --------------------------------------------------------------------------- +def _check_hold_permission(current_user: User, test) -> None: + """Only whichever team currently owns the test may hold/resume it. + + ``require_any_role_strict("red_tech", "blue_tech")`` alone lets either + role through regardless of phase — a red_tech could otherwise hold (or + resume) a test that has already moved into blue_evaluating. + """ + expected_role = "blue_tech" if test.state == TestState.blue_evaluating else "red_tech" + if current_user.role != expected_role: + raise HTTPException( + status_code=403, + detail=f"Only {expected_role.replace('_', ' ')} can do this while the test is in '{test.state}'.", + ) + + @router.post("/{test_id}/hold", response_model=TestOut) def hold_test( test_id: uuid.UUID, @@ -1330,6 +1345,8 @@ def hold_test( detail=f"Cannot hold a test in state '{test.state}'. Only pre-validation states can be held.", ) + _check_hold_permission(current_user, test) + if test.is_on_hold: raise HTTPException(status_code=400, detail="Test is already on hold") @@ -1370,6 +1387,8 @@ def resume_test( if not test.is_on_hold: raise HTTPException(status_code=400, detail="Test is not on hold") + _check_hold_permission(current_user, test) + test.is_on_hold = False test.hold_reason = None test.held_at = None diff --git a/backend/tests/test_hold_resume.py b/backend/tests/test_hold_resume.py index 8364f58..967637b 100644 --- a/backend/tests/test_hold_resume.py +++ b/backend/tests/test_hold_resume.py @@ -31,6 +31,20 @@ def _seed_executing_test(db, technique, created_by) -> Test: return test +def _seed_blue_evaluating_test(db, technique, created_by) -> Test: + test = Test( + technique_id=technique.id, + name="Holdable test (blue)", + created_by=created_by, + state=TestState.blue_evaluating, + blue_started_at=datetime.utcnow() - timedelta(minutes=10), + ) + db.add(test) + db.commit() + db.refresh(test) + return test + + def test_hold_pauses_the_running_timer(client, db, red_tech_headers, red_tech_user): technique = _seed_technique(db) test = _seed_executing_test(db, technique, red_tech_user.id) @@ -88,3 +102,49 @@ def test_hold_does_not_double_pause_an_already_paused_timer(client, db, red_tech db.refresh(test) assert test.paused_at == manual_pause_time + + +def test_red_tech_cannot_hold_a_test_in_blue_evaluating( + client, db, red_tech_headers, red_tech_user, +): + """Once a test has moved into Blue's queue, only blue_tech may hold it — + a red_tech is no longer involved at this stage.""" + technique = _seed_technique(db) + test = _seed_blue_evaluating_test(db, technique, red_tech_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/hold", + json={"reason": "waiting on lab access"}, + headers=red_tech_headers, + ) + assert resp.status_code == 403 + + +def test_blue_tech_can_hold_a_test_in_blue_evaluating( + client, db, blue_tech_headers, red_tech_user, +): + technique = _seed_technique(db) + test = _seed_blue_evaluating_test(db, technique, red_tech_user.id) + + resp = client.post( + f"/api/v1/tests/{test.id}/hold", + json={"reason": "waiting on EDR access"}, + headers=blue_tech_headers, + ) + assert resp.status_code == 200, resp.text + + +def test_blue_tech_cannot_resume_a_test_that_was_held_during_red_executing( + client, db, api, red_tech_headers, blue_tech_headers, red_tech_user, +): + technique = _seed_technique(db) + test = _seed_executing_test(db, technique, red_tech_user.id) + + resp = api( + "post", f"/api/v1/tests/{test.id}/hold", red_tech_headers, + json={"reason": "waiting on lab access"}, + ) + assert resp.status_code == 200, resp.text + + resp = api("post", f"/api/v1/tests/{test.id}/resume", blue_tech_headers) + assert resp.status_code == 403 diff --git a/frontend/src/components/test-detail/TestDetailHeader.tsx b/frontend/src/components/test-detail/TestDetailHeader.tsx index 763d357..11a54cc 100644 --- a/frontend/src/components/test-detail/TestDetailHeader.tsx +++ b/frontend/src/components/test-detail/TestDetailHeader.tsx @@ -158,10 +158,13 @@ export default function TestDetailHeader({ // ── Contextual action buttons ──────────────────────────────────── - const HOLDABLE_STATES: TestState[] = ["draft", "red_executing", "blue_evaluating"]; + // Whichever team currently "owns" the test is the only one who should see + // Hold — a red_tech shouldn't be able to hold a test that's already moved + // into blue_evaluating (and vice versa), even though both roles can hold + // during their own phase. const canHold = - HOLDABLE_STATES.includes(test.state) && - (role === "red_tech" || role === "blue_tech"); + (["draft", "red_executing"].includes(test.state) && role === "red_tech") || + (test.state === "blue_evaluating" && role === "blue_tech"); const renderActions = () => { const buttons: React.ReactNode[] = [];