From c6bdded3f61afcf35a4b283a1e8bf37dd3e6c5b6 Mon Sep 17 00:00:00 2001 From: kitos Date: Tue, 14 Jul 2026 10:23:30 +0200 Subject: [PATCH] fix(jira): keep BT Start Date across reopens, resolve Jira accounts on the fly everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BT Start Date had the same bug RT Start Date did: pushed on every round, clobbering round 1's real pickup date. Now only pushed on round 1, mirroring the "first round survives every reopen" rule. - push_test_event's assignment branches (red_executing, red/blue reviewer, blue_evaluating reassignment) only read the cached jira_account_id and silently no-op'd if it was never populated — which is exactly what happened to a blue_lead who hadn't logged in since Jira was configured, making it look like Blue's reviewer reassignment was broken when Red's wasn't purely because that particular red_lead happened to have logged in already. Extracted the live-lookup fallback that only push_assignee_update had into a shared _resolve_jira_account_id(), used by every assignment call site now. --- backend/app/services/jira_service.py | 49 +++++++++++++------ backend/tests/test_jira_service.py | 71 ++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 14 deletions(-) diff --git a/backend/app/services/jira_service.py b/backend/app/services/jira_service.py index f8c78f2..e68fe75 100644 --- a/backend/app/services/jira_service.py +++ b/backend/app/services/jira_service.py @@ -342,6 +342,28 @@ def lookup_user_jira_account_id(db: Session, user: User) -> bool: return False +def _resolve_jira_account_id(db: Session, user: Optional[User]) -> Optional[str]: + """Return *user*'s Atlassian account ID, trying a fresh lookup if it + isn't cached yet. + + ``jira_account_id`` is normally populated on login, but a user assigned + or reviewing before their first login (or before Jira was configured) + would otherwise leave every Jira assignment silently no-op'd forever — + exactly the gap that caused a blue_lead's reviews to never reassign in + Jira while red_lead's did, since only push_assignee_update had this + fallback. Every Jira-assignment call site should use this, not read + ``jira_account_id`` directly. + """ + if user is None: + return None + jira_account_id = getattr(user, "jira_account_id", None) + if jira_account_id: + return jira_account_id + lookup_user_jira_account_id(db, user) + db.flush() + return getattr(user, "jira_account_id", None) + + # --------------------------------------------------------------------------- # Ticket content builders (inspired by the pentest-to-Jira script) # --------------------------------------------------------------------------- @@ -849,7 +871,7 @@ def push_test_event( # it, not the operator who should keep working it — the caller # passes the original red_tech_assignee as *assignee* in that case. if new_state == "red_executing": - jira_account_id = getattr(assignee or actor, "jira_account_id", None) + jira_account_id = _resolve_jira_account_id(db, assignee or actor) if jira_account_id: try: jira.assign_issue(link.jira_issue_key, account_id=jira_account_id) @@ -864,7 +886,7 @@ def push_test_event( ) if new_state in ("red_review", "blue_review") and assignee: - jira_account_id = getattr(assignee, "jira_account_id", None) + jira_account_id = _resolve_jira_account_id(db, assignee) if jira_account_id: try: jira.assign_issue(link.jira_issue_key, account_id=jira_account_id) @@ -887,7 +909,7 @@ def push_test_event( if new_state == "blue_evaluating": if test.blue_tech_assignee: reassignee = db.query(User).filter(User.id == test.blue_tech_assignee).first() - jira_account_id = getattr(reassignee, "jira_account_id", None) + jira_account_id = _resolve_jira_account_id(db, reassignee) else: jira_account_id = None try: @@ -946,17 +968,9 @@ def push_assignee_update(db: Session, test: Test, assignee: User) -> None: if not link: return - jira_account_id = getattr(assignee, "jira_account_id", None) + jira_account_id = _resolve_jira_account_id(db, assignee) if not jira_account_id: - # Normally populated on login (see lookup_user_jira_account_id) — - # but a lead may assign someone who has never logged in yet. Try - # a fresh lookup now instead of silently giving up, so assignment - # still syncs to Jira immediately. - lookup_user_jira_account_id(db, assignee) - db.flush() - jira_account_id = getattr(assignee, "jira_account_id", None) - if not jira_account_id: - return + return try: jira = get_admin_jira_client(db) @@ -1315,7 +1329,14 @@ def push_rt_submitted(db: Session, test: Test) -> None: def push_bt_started(db: Session, test: Test) -> None: - """Set the BT Start Date field — called when Blue picks up the test to evaluate.""" + """Set the BT Start Date field — called when Blue picks up the test to evaluate. + + Only pushed on round 1. On later rounds (after a reopen), the field + already holds round 1's real pickup date and must not be overwritten — + same "first round survives every reopen" rule as RT Start Date. + """ + if (test.blue_round_number or 1) > 1: + return _update_test_fields(db, test, { JIRA_FIELD_BT_START_DATE: _jira_datetime(datetime.utcnow()), }) diff --git a/backend/tests/test_jira_service.py b/backend/tests/test_jira_service.py index a3362c9..b44aba1 100644 --- a/backend/tests/test_jira_service.py +++ b/backend/tests/test_jira_service.py @@ -391,6 +391,42 @@ def test_push_test_event_red_executing_reassigns_original_operator_on_reopen(moc mock_jira.assign_issue.assert_called_once_with(link.jira_issue_key, account_id="red-tech-account") +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_test_event_reviewer_assignment_resolves_account_id_on_the_fly( + mock_get_client, mock_configured, db, +): + """Regression: a blue_lead assigned as reviewer had never logged in + (jira_account_id was still NULL), so the Jira reassignment silently + no-op'd and the ticket stayed on the operator forever — while red_lead + worked purely by coincidence of already having logged in once. Every + assignment call site must attempt a live lookup, not just + push_assignee_update.""" + from app.models.user import User + + mock_jira = MagicMock() + mock_jira.user_find_by_user_string.return_value = [ + {"accountId": "blue-lead-account", "emailAddress": "bluelead@test.com"}, + ] + mock_get_client.return_value = mock_jira + test = _make_test() + link = _make_link(test.id) + db.add(link) + db.commit() + + blue_lead = User( + username="bluelead-never-logged-in", email="bluelead@test.com", + hashed_password="x", role="blue_lead", jira_account_id=None, + ) + db.add(blue_lead) + db.commit() + + jira_service.push_test_event(db, test, MagicMock(username="bluetech"), "blue_review", assignee=blue_lead) + + assert blue_lead.jira_account_id == "blue-lead-account" + mock_jira.assign_issue.assert_called_once_with(link.jira_issue_key, account_id="blue-lead-account") + + @patch("app.services.jira_service.has_admin_jira_configured", return_value=True) @patch("app.services.jira_service.get_admin_jira_client") def test_push_bt_work_started_sets_in_progress(mock_get_client, mock_configured, db): @@ -412,6 +448,41 @@ def test_push_bt_work_started_sets_in_progress(mock_get_client, mock_configured, mock_jira.assign_issue.assert_called_once_with(link.jira_issue_key, account_id="bob-account-id") +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_bt_started_sets_date_field_on_round_one(mock_get_client, mock_configured, db): + mock_jira = MagicMock() + mock_get_client.return_value = mock_jira + test = _make_test(blue_round_number=1) + link = _make_link(test.id) + db.add(link) + db.commit() + + jira_service.push_bt_started(db, test) + + mock_jira.update_issue_field.assert_called_once() + fields = mock_jira.update_issue_field.call_args[1]["fields"] + assert jira_service.JIRA_FIELD_BT_START_DATE in fields + + +@patch("app.services.jira_service.has_admin_jira_configured", return_value=True) +@patch("app.services.jira_service.get_admin_jira_client") +def test_push_bt_started_preserves_round_one_date_on_reopen(mock_get_client, mock_configured, db): + """Regression: mirrors the RT Start Date bug — BT Start Date must keep + showing round 1's real pickup date even after the operator re-claims + the test via 'Start Evaluation' on a later round.""" + mock_jira = MagicMock() + mock_get_client.return_value = mock_jira + test = _make_test(blue_round_number=2) + link = _make_link(test.id) + db.add(link) + db.commit() + + jira_service.push_bt_started(db, test) + + mock_jira.update_issue_field.assert_not_called() + + @patch("app.services.jira_service.has_admin_jira_configured", return_value=True) @patch("app.services.jira_service.get_admin_jira_client") def test_push_rt_submitted_includes_attack_success(mock_get_client, mock_configured, db):