diff --git a/src/shared/shared/schedule.py b/src/shared/shared/schedule.py index 4adad38..2d792a5 100644 --- a/src/shared/shared/schedule.py +++ b/src/shared/shared/schedule.py @@ -21,6 +21,15 @@ EASTERN = ZoneInfo("America/New_York") WEEKEND_DAYS = {"Saturday", "Sunday"} FALLBACK_EXTENSION = "100" + +class ExtensionAlreadyRegistered(Exception): + """Raised when a Slack user tries to claim an extension owned by another.""" + + def __init__(self, extension: str): + self.extension = extension + super().__init__(f"Extension {extension} is already registered to another user") + + # Defaults used when CONFIG omits the holiday-feature settings. DEFAULT_HOLIDAY_MULTIPLIER = Decimal("1.5") DEFAULT_HOLIDAY_QUEUE = "802" @@ -62,14 +71,32 @@ class ShiftSchedule: return None def register_user(self, slack_user_id: str, extension: str) -> dict | None: + """Link a Slack user to a roster extension. + + Returns the employee on success, ``None`` if the extension is not in the + roster, and raises :class:`ExtensionAlreadyRegistered` if the extension + is already bound to a *different* Slack user. Re-registering your own + extension is a no-op success, so this is safe to call idempotently. + """ employee = self.get_employee_by_extension(extension) if not employee: return None - self.table.update_item( - Key={"PK": "ROSTER", "SK": extension}, - UpdateExpression="SET slack_user_id = :sid", - ExpressionAttributeValues={":sid": slack_user_id}, - ) + try: + self.table.update_item( + Key={"PK": "ROSTER", "SK": extension}, + UpdateExpression="SET slack_user_id = :sid", + # Only allow the write when the extension is unclaimed + # (no/empty slack_user_id) or already claimed by this same user. + # Blocks one Slack user from hijacking another's extension. + ConditionExpression=( + "attribute_not_exists(slack_user_id) " + "OR slack_user_id = :empty " + "OR slack_user_id = :sid" + ), + ExpressionAttributeValues={":sid": slack_user_id, ":empty": ""}, + ) + except self.table.meta.client.exceptions.ConditionalCheckFailedException: + raise ExtensionAlreadyRegistered(extension) from None employee["slack_user_id"] = slack_user_id return employee @@ -123,6 +150,38 @@ class ShiftSchedule: except self.table.meta.client.exceptions.ConditionalCheckFailedException: return False + def reassign_if_held_by( + self, + date_str: str, + from_extension: str, + extension: str, + name: str, + shift_type: str = "night", + ) -> bool: + """Atomically reassign a shift only if it's still ``from_extension``'s. + + Used by the swap-accept path: the shift moves to the target only when + the override is still in the requester's name, or when no override + exists yet (weekly-schedule fallback, i.e. still the requester's by + default). Fails if the shift was independently claimed (e.g. a pickup) + by someone else after the swap was initiated. + """ + sk = f"{date_str}-DAY" if shift_type == "day" else date_str + try: + self.table.put_item( + Item={ + "PK": "OVERRIDE", + "SK": sk, + "extension": extension, + "name": name, + }, + ConditionExpression="attribute_not_exists(PK) OR extension = :from", + ExpressionAttributeValues={":from": from_extension}, + ) + return True + except self.table.meta.client.exceptions.ConditionalCheckFailedException: + return False + def mark_open(self, date_str: str, shift_type: str = "night") -> None: sk = f"{date_str}-DAY" if shift_type == "day" else date_str self.table.put_item( diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index cdbddba..bdd4cd6 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -36,6 +36,7 @@ from shared.ring_scheduler import update_queue_routing from shared.schedule import ( FALLBACK_EXTENSION, WEEKEND_DAYS, + ExtensionAlreadyRegistered, ShiftSchedule, determine_shift_type, ) @@ -383,7 +384,7 @@ def dispatch_oncall(command, respond, client, schedule, schedule_channel): elif text == "pay": _show_pay(respond, schedule) elif text.startswith("rate"): - _handle_rate(respond, schedule, text) + _handle_rate(respond, schedule, text, is_admin) elif text.startswith("register"): _handle_register(respond, schedule, user_id, text) elif text.startswith("pick"): @@ -554,7 +555,13 @@ def _show_pay(respond, schedule): ) -def _handle_rate(respond, schedule, text): +def _handle_rate(respond, schedule, text, is_admin): + # Setting pay rates is an admin-only operation; reading them is gated too + # since rates are sensitive payroll data. + if not is_admin: + respond(text="Rate commands are restricted. Contact an administrator.") + return + parts = text.split() # /oncall rate — show current rates if len(parts) == 1: @@ -625,7 +632,16 @@ def _handle_register(respond, schedule, user_id, text): return ext = parts[1].strip() - employee = schedule.register_user(user_id, ext) + try: + employee = schedule.register_user(user_id, ext) + except ExtensionAlreadyRegistered: + respond( + text=( + f"Extension {ext} is already registered to another person. " + "If this is your extension, ask an admin to clear it." + ) + ) + return if not employee: respond( text=f"Extension {ext} not found in the roster. Check `/oncall roster`." @@ -761,7 +777,24 @@ def _pick_regular( ) return - schedule.set_override(date_str, employee["extension"], employee["name"], shift_type) + # Already this employee's own shift — nothing to claim, just confirm. + already_mine = ( + bool(assignees) and assignees[0]["extension"] == employee["extension"] + ) + if not already_mine: + # Atomic conditional claim: only one of two concurrent pickers wins, so + # the loser is told it's taken instead of silently overwriting (TOCTOU). + claimed = schedule.claim_open_shift( + date_str, employee["extension"], employee["name"], shift_type + ) + if not claimed: + respond( + text=( + f"The *{date_label}*{shift_label} shift was just picked up by " + "someone else." + ) + ) + return if is_today(date_str) and _is_active_shift_type(shift_type): _update_3cx_routing(employee["extension"]) @@ -1164,9 +1197,47 @@ def handle_swap_accept(body, respond, client, schedule, schedule_channel): if _holiday_window_active(date_str): _set_holiday_queue_agents(schedule, date_str) else: - schedule.set_override( - date_str, swap["target_ext"], swap["target_name"], shift_type + # Re-resolve the current holder at accept time. reassign_if_held_by + # below trusts "no override row" as "still the requester's", but a + # weekly-held shift also has no override row — so an admin clear or a + # weekly-schedule edit between swap-init and accept could move the shift + # to a third party without ever creating an override, and the bare + # conditional write would not catch it. Confirm the requester is still + # the resolved holder before reassigning. + accept_day_name = datetime.strptime(date_str, "%Y-%m-%d").strftime("%A") + current_ext, _, _ = schedule.resolve_shift( + date_str, accept_day_name, shift_type ) + if current_ext != swap["requester_ext"]: + schedule.clear_swap(date_str, shift_type) + respond( + replace_original=True, + blocks=build_swap_resolved_blocks( + "This shift is no longer assigned to the person who " + "requested the swap, so it couldn't be applied." + ), + ) + return + # Only move the shift if it's still the requester's (or still on the + # weekly fallback). If it was independently claimed after the swap was + # initiated, abort instead of silently overwriting the new holder. + moved = schedule.reassign_if_held_by( + date_str, + swap["requester_ext"], + swap["target_ext"], + swap["target_name"], + shift_type, + ) + if not moved: + schedule.clear_swap(date_str, shift_type) + respond( + replace_original=True, + blocks=build_swap_resolved_blocks( + "This shift was already picked up by someone else, so the " + "swap couldn't be applied." + ), + ) + return if is_today(date_str) and _is_active_shift_type(shift_type): _update_3cx_routing(swap["target_ext"]) schedule.mark_swap_verified(date_str, shift_type) diff --git a/template.yaml b/template.yaml index 68f325a..e2532d4 100644 --- a/template.yaml +++ b/template.yaml @@ -156,16 +156,19 @@ Resources: - DynamoDBCrudPolicy: TableName: !Ref ShiftTable - Statement: + # Least privilege: only the Slack bot token, not the whole namespace. - Effect: Allow Action: - secretsmanager:GetSecretValue Resource: - - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/*" + - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/slack-bot-token-*" - Effect: Allow Action: - ses:SendEmail Resource: - - !Sub "arn:aws:ses:${AWS::Region}:${AWS::AccountId}:identity/*" + # Only the single sending identity (noreply@seahaven.com), not + # every identity in the account. + - !Sub "arn:aws:ses:${AWS::Region}:${AWS::AccountId}:identity/noreply@seahaven.com" # The sending identity has a default configuration set # (seahaven-email-events); SES authorizes SendEmail against the # config-set resource too, so it must be granted alongside the @@ -207,11 +210,12 @@ Resources: - DynamoDBCrudPolicy: TableName: !Ref ShiftTable - Statement: + # Least privilege: only the 3cx-* secrets this function reads. - Effect: Allow Action: - secretsmanager:GetSecretValue Resource: - - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/*" + - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/3cx-*" Events: # Daily at 6am ET (before the 7am schedule post and 8am 3CX scheduler) # EST: 6am ET = 11:00 UTC (Nov-Mar) @@ -246,14 +250,15 @@ Resources: QUEUE_NUMBER: !Ref QueueNumber TZ: !Ref Timezone Policies: - - DynamoDBCrudPolicy: + - DynamoDBReadPolicy: TableName: !Ref ShiftTable - Statement: + # Least privilege: only the 3cx-* secrets this function reads. - Effect: Allow Action: - secretsmanager:GetSecretValue Resource: - - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/*" + - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/3cx-*" Events: # Daily at 8am ET — update after-hours routing DailyScheduleEST: @@ -328,11 +333,12 @@ Resources: - DynamoDBCrudPolicy: TableName: !Ref ShiftTable - Statement: + # Least privilege: only the 3cx-* secrets this function reads. - Effect: Allow Action: - secretsmanager:GetSecretValue Resource: - - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/*" + - !Sub "arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:afterhours-shift-manager/3cx-*" # Lambda error alarm for the holiday router. Mirrors the account-wide # operational convention (Lambda-Errors-, threshold 1 over one 5-min diff --git a/tests/slack_bot/test_handle_register_rate.py b/tests/slack_bot/test_handle_register_rate.py index b262f7b..ef97a9a 100644 --- a/tests/slack_bot/test_handle_register_rate.py +++ b/tests/slack_bot/test_handle_register_rate.py @@ -18,47 +18,75 @@ class TestRegister: slackbot_app._handle_register(respond, schedule, "U_ALICE", "register") assert "Usage" in text_of(respond) + def test_register_reregister_self_is_idempotent( + self, slackbot_app, schedule, seed, respond, text_of + ): + seed.roster("114", "Alice") + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + assert "Linked" in text_of(respond) + assert schedule.get_employee_by_extension("114")["slack_user_id"] == "U_ALICE" + + def test_register_cannot_hijack_others_extension( + self, slackbot_app, schedule, seed, respond, text_of + ): + seed.roster("114", "Alice") + # Alice claims her extension first. + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + # Mallory tries to claim Alice's extension — must be rejected. + slackbot_app._handle_register(respond, schedule, "U_MALLORY", "register 114") + assert "already registered" in text_of(respond).lower() + # The binding must still point at Alice, not Mallory. + assert schedule.get_employee_by_extension("114")["slack_user_id"] == "U_ALICE" + class TestRate: + def test_non_admin_rejected(self, slackbot_app, schedule, seed, respond, text_of): + seed.roster("114", "Alice") + slackbot_app._handle_rate(respond, schedule, "rate 114 90", False) + assert "restricted" in text_of(respond).lower() + # The rate must not have been changed by a non-admin. + assert schedule.get_shift_rate("114") == 0.0 + def test_show_rates(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") seed.roster("114", "Alice", shift_rate="75") - slackbot_app._handle_rate(respond, schedule, "rate") + slackbot_app._handle_rate(respond, schedule, "rate", True) text = text_of(respond) assert "$50.00" in text and "Alice" in text and "$75.00" in text def test_show_rates_no_custom(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") - slackbot_app._handle_rate(respond, schedule, "rate") + slackbot_app._handle_rate(respond, schedule, "rate", True) assert "No per-person rates" in text_of(respond) def test_set_default(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") - slackbot_app._handle_rate(respond, schedule, "rate default 60") + slackbot_app._handle_rate(respond, schedule, "rate default 60", True) assert schedule.get_shift_rate() == 60.0 assert "$60.00" in text_of(respond) def test_set_default_invalid_amount(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate default abc") + slackbot_app._handle_rate(respond, schedule, "rate default abc", True) assert "Invalid amount" in text_of(respond) def test_set_default_usage(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate default") + slackbot_app._handle_rate(respond, schedule, "rate default", True) assert "Usage" in text_of(respond) def test_set_per_employee(self, slackbot_app, schedule, seed, respond, text_of): seed.roster("114", "Alice") - slackbot_app._handle_rate(respond, schedule, "rate 114 90") + slackbot_app._handle_rate(respond, schedule, "rate 114 90", True) assert schedule.get_shift_rate("114") == 90.0 assert "Alice" in text_of(respond) and "$90.00" in text_of(respond) def test_set_per_employee_unknown(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate 999 90") + slackbot_app._handle_rate(respond, schedule, "rate 999 90", True) assert "not found" in text_of(respond).lower() def test_set_per_employee_strips_dollar_sign( self, slackbot_app, schedule, seed, respond ): seed.roster("114", "Alice") - slackbot_app._handle_rate(respond, schedule, "rate 114 $90") + slackbot_app._handle_rate(respond, schedule, "rate 114 $90", True) assert schedule.get_shift_rate("114") == 90.0 diff --git a/tests/slack_bot/test_swap_accept_decline.py b/tests/slack_bot/test_swap_accept_decline.py index 6498ee6..0d779e3 100644 --- a/tests/slack_bot/test_swap_accept_decline.py +++ b/tests/slack_bot/test_swap_accept_decline.py @@ -42,8 +42,11 @@ def _channels(client): class TestAccept: @freeze_time(MON) def test_applies_override_marks_verified_notifies( - self, slackbot_app, schedule, respond, client, routing_spy + self, slackbot_app, schedule, seed, respond, client, routing_spy ): + # Requester holds the shift via the weekly schedule (production + # invariant: swap-create validates the requester is the holder). + seed.weekly("Monday", "114", "Alice") _pending(schedule, "2026-06-01") slackbot_app.handle_swap_accept( _accept_body("2026-06-01"), respond, client, schedule, "C_TEST" @@ -58,8 +61,9 @@ class TestAccept: @freeze_time(MON) def test_future_shift_no_3cx( - self, slackbot_app, schedule, respond, client, routing_spy + self, slackbot_app, schedule, seed, respond, client, routing_spy ): + seed.weekly("Wednesday", "114", "Alice") _pending(schedule, "2026-06-03") # Wednesday slackbot_app.handle_swap_accept( _accept_body("2026-06-03"), respond, client, schedule, "C_TEST" @@ -116,7 +120,27 @@ class TestAccept: routing_spy.assert_not_called() @freeze_time(MON) - def test_weekend_day_suffix(self, slackbot_app, schedule, respond, client): + def test_aborts_if_requester_no_longer_holder( + self, slackbot_app, schedule, seed, respond, client, routing_spy + ): + # RIHB-1 regression: requester held via weekly, swap pending to Bob, + # then the holder changes to Carol (e.g. weekly edit / admin reassign) + # before Bob accepts. The accept must NOT steal the shift from Carol. + seed.weekly("Monday", "114", "Alice") + _pending(schedule, "2026-06-01") + # Holder is now Carol (116), not the requester Alice (114). + seed.weekly("Monday", "116", "Carol") + slackbot_app.handle_swap_accept( + _accept_body("2026-06-01"), respond, client, schedule, "C_TEST" + ) + # No override written to Bob; the swap is cleared and 3CX untouched. + assert schedule.get_override("2026-06-01") is None + assert "no longer assigned" in _resolved_text(respond).lower() + routing_spy.assert_not_called() + + @freeze_time(MON) + def test_weekend_day_suffix(self, slackbot_app, schedule, seed, respond, client): + seed.weekly("Saturday", "114", "Alice", shift_type="day") _pending(schedule, "2026-06-06", shift_type="day") slackbot_app.handle_swap_accept( _accept_body("2026-06-06", suffix="_day"),