fix(jira): keep BT Start Date across reopens, resolve Jira accounts on the fly everywhere
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
- 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.
This commit is contained in:
@@ -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,15 +968,7 @@ def push_assignee_update(db: Session, test: Test, assignee: User) -> None:
|
||||
if not link:
|
||||
return
|
||||
|
||||
jira_account_id = getattr(assignee, "jira_account_id", None)
|
||||
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)
|
||||
jira_account_id = _resolve_jira_account_id(db, assignee)
|
||||
if not jira_account_id:
|
||||
return
|
||||
|
||||
@@ -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()),
|
||||
})
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user