Skip to content

Commit ef742db

Browse files
Add per-user job ownership for cancel authorization
Record the Slack user ID in request.json when submitting transfer jobs and enforce ownership checks in cancel_job so only the submitting user can cancel their own jobs. Legacy jobs without a submitted_by field remain cancellable by anyone for backwards compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9da7243 commit ef742db

File tree

4 files changed

+136
-2
lines changed

4 files changed

+136
-2
lines changed

src/xfer/slackbot/app.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ def handle_mention(event: dict[str, Any], say: Any) -> None:
177177
channel_id=channel,
178178
thread_ts=thread_ts,
179179
conversation_history=history.copy() if history else None,
180+
user_id=user,
180181
)
181182

182183
# Store the exchange
@@ -228,6 +229,7 @@ def handle_message(event: dict[str, Any], say: Any) -> None:
228229
channel_id=channel,
229230
thread_ts=thread_ts,
230231
conversation_history=history.copy() if history else None,
232+
user_id=user,
231233
)
232234

233235
conversations.append(
@@ -291,6 +293,7 @@ def handle_message(event: dict[str, Any], say: Any) -> None:
291293
channel_id=channel,
292294
thread_ts=thread_ts,
293295
conversation_history=history.copy() if history else None,
296+
user_id=user,
294297
)
295298

296299
conversations.append(

src/xfer/slackbot/claude_agent.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@ def execute_tool(
405405
tool_input: dict[str, Any],
406406
channel_id: str,
407407
thread_ts: str,
408+
user_id: str = "",
408409
) -> str:
409410
"""Execute a tool and return the result as a string."""
410411
if tool_name == "submit_transfer":
@@ -426,6 +427,7 @@ def execute_tool(
426427
time_limit=tool_input.get("time_limit"),
427428
job_name=tool_input.get("job_name"),
428429
rclone_flags=tool_input.get("rclone_flags"),
430+
user_id=user_id,
429431
)
430432
return json.dumps(result.__dict__, default=str)
431433

@@ -472,6 +474,7 @@ def execute_tool(
472474
tool_input["job_id"],
473475
channel_id,
474476
thread_ts,
477+
user_id=user_id,
475478
)
476479
return json.dumps({"success": success, "message": message})
477480

@@ -716,6 +719,7 @@ def process_message(
716719
channel_id: str,
717720
thread_ts: str,
718721
conversation_history: list[dict] | None = None,
722+
user_id: str = "",
719723
) -> str:
720724
"""
721725
Process a user message and return Claude's response.
@@ -748,6 +752,7 @@ def process_message(
748752
block.input,
749753
channel_id,
750754
thread_ts,
755+
user_id=user_id,
751756
)
752757
tool_results.append(
753758
{

src/xfer/slackbot/slurm_tools.py

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ def submit_transfer(
310310
time_limit: Optional[str] = None,
311311
job_name: Optional[str] = None,
312312
rclone_flags: Optional[str] = None,
313+
user_id: str = "",
313314
) -> TransferResult:
314315
"""
315316
Submit a data transfer job via xfer.
@@ -393,6 +394,7 @@ def submit_transfer(
393394
"prepare_job_id": job_id,
394395
"num_shards": num_shards,
395396
"submitted_at": datetime.utcnow().isoformat() + "Z",
397+
"submitted_by": user_id,
396398
}
397399
(run_dir / "request.json").write_text(json.dumps(request_meta, indent=2))
398400

@@ -630,11 +632,14 @@ def get_transfer_progress_by_job(job_id: str) -> Optional[dict]:
630632
return progress
631633

632634

633-
def cancel_job(job_id: str, channel_id: str, thread_ts: str) -> tuple[bool, str]:
635+
def cancel_job(
636+
job_id: str, channel_id: str, thread_ts: str, *, user_id: str = ""
637+
) -> tuple[bool, str]:
634638
"""
635639
Cancel a Slurm job.
636640
637-
Validates that the job belongs to the requesting thread via comment.
641+
Validates that the job belongs to the requesting thread via comment,
642+
and that the requesting user is the one who submitted the job.
638643
"""
639644
# First, verify the job belongs to this thread
640645
job_info = get_job_status(job_id)
@@ -645,6 +650,18 @@ def cancel_job(job_id: str, channel_id: str, thread_ts: str) -> tuple[bool, str]
645650
if expected_comment not in job_info.comment:
646651
return False, f"Job {job_id} does not belong to this thread. Cannot cancel."
647652

653+
# Check user ownership via request.json in the job's work_dir
654+
if user_id and job_info.work_dir:
655+
request_file = Path(job_info.work_dir) / "request.json"
656+
if request_file.exists():
657+
try:
658+
meta = json.loads(request_file.read_text())
659+
submitted_by = meta.get("submitted_by", "")
660+
if submitted_by and submitted_by != user_id:
661+
return False, "Only the user who submitted this job can cancel it."
662+
except (json.JSONDecodeError, IOError):
663+
pass # Allow cancel if request.json is unreadable
664+
648665
# Cancel the job
649666
try:
650667
run_cmd(["scancel", job_id], check=True)

tests/test_slackbot_dryrun.py

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@
2525
slack_comment,
2626
)
2727
from xfer.slackbot.slurm_tools import (
28+
JobInfo,
2829
TransferResult,
2930
_parse_job_from_sacct_json,
3031
_write_prepare_script,
32+
cancel_job,
3133
get_allowed_backends,
3234
get_transfer_progress,
3335
validate_backend,
@@ -506,6 +508,110 @@ def test_full_flow_simulation():
506508
print("\n✓ Full flow simulation passed")
507509

508510

511+
def test_cancel_job_by_submitter():
512+
"""Test that the user who submitted a job can cancel it."""
513+
print("\n=== Testing cancel job by submitter ===")
514+
515+
channel = "C07ABC123"
516+
thread_ts = "1234567890.123456"
517+
comment = slack_comment(channel, thread_ts)
518+
519+
with tempfile.TemporaryDirectory() as tmpdir:
520+
run_dir = Path(tmpdir)
521+
request_meta = {"submitted_by": "U_ALICE"}
522+
(run_dir / "request.json").write_text(json.dumps(request_meta))
523+
524+
job_info = JobInfo(
525+
job_id="123",
526+
array_job_id=None,
527+
state="RUNNING",
528+
name="xfer-slack",
529+
comment=comment,
530+
work_dir=str(run_dir),
531+
submit_time=None,
532+
start_time=None,
533+
end_time=None,
534+
partition="transfer",
535+
)
536+
537+
with patch("xfer.slackbot.slurm_tools.get_job_status", return_value=job_info), \
538+
patch("xfer.slackbot.slurm_tools.run_cmd"):
539+
success, message = cancel_job("123", channel, thread_ts, user_id="U_ALICE")
540+
assert success, f"Expected success, got: {message}"
541+
assert "cancelled" in message
542+
543+
print("✓ Cancel job by submitter test passed")
544+
545+
546+
def test_cancel_job_by_other_user():
547+
"""Test that a different user cannot cancel someone else's job."""
548+
print("\n=== Testing cancel job by other user ===")
549+
550+
channel = "C07ABC123"
551+
thread_ts = "1234567890.123456"
552+
comment = slack_comment(channel, thread_ts)
553+
554+
with tempfile.TemporaryDirectory() as tmpdir:
555+
run_dir = Path(tmpdir)
556+
request_meta = {"submitted_by": "U_ALICE"}
557+
(run_dir / "request.json").write_text(json.dumps(request_meta))
558+
559+
job_info = JobInfo(
560+
job_id="123",
561+
array_job_id=None,
562+
state="RUNNING",
563+
name="xfer-slack",
564+
comment=comment,
565+
work_dir=str(run_dir),
566+
submit_time=None,
567+
start_time=None,
568+
end_time=None,
569+
partition="transfer",
570+
)
571+
572+
with patch("xfer.slackbot.slurm_tools.get_job_status", return_value=job_info):
573+
success, message = cancel_job("123", channel, thread_ts, user_id="U_BOB")
574+
assert not success, f"Expected failure, got success: {message}"
575+
assert "Only the user who submitted" in message
576+
577+
print("✓ Cancel job by other user test passed")
578+
579+
580+
def test_cancel_job_legacy_no_submitter():
581+
"""Test that jobs without submitted_by field can be cancelled by anyone (backwards compat)."""
582+
print("\n=== Testing cancel job legacy (no submitter) ===")
583+
584+
channel = "C07ABC123"
585+
thread_ts = "1234567890.123456"
586+
comment = slack_comment(channel, thread_ts)
587+
588+
with tempfile.TemporaryDirectory() as tmpdir:
589+
run_dir = Path(tmpdir)
590+
request_meta = {"source": "s3src:bucket/data", "dest": "s3dst:archive/data"}
591+
(run_dir / "request.json").write_text(json.dumps(request_meta))
592+
593+
job_info = JobInfo(
594+
job_id="123",
595+
array_job_id=None,
596+
state="RUNNING",
597+
name="xfer-slack",
598+
comment=comment,
599+
work_dir=str(run_dir),
600+
submit_time=None,
601+
start_time=None,
602+
end_time=None,
603+
partition="transfer",
604+
)
605+
606+
with patch("xfer.slackbot.slurm_tools.get_job_status", return_value=job_info), \
607+
patch("xfer.slackbot.slurm_tools.run_cmd"):
608+
success, message = cancel_job("123", channel, thread_ts, user_id="U_ANYONE")
609+
assert success, f"Expected success for legacy job, got: {message}"
610+
assert "cancelled" in message
611+
612+
print("✓ Cancel job legacy (no submitter) test passed")
613+
614+
509615
def main():
510616
"""Run all dry-run tests."""
511617
print("=" * 60)
@@ -524,6 +630,9 @@ def main():
524630
test_triage_skip()
525631
test_triage_error_defaults_to_respond()
526632
test_full_flow_simulation()
633+
test_cancel_job_by_submitter()
634+
test_cancel_job_by_other_user()
635+
test_cancel_job_legacy_no_submitter()
527636

528637
print("\n" + "=" * 60)
529638
print("All dry-run tests passed! ✓")

0 commit comments

Comments
 (0)