INFRA-106: nightly-sweep security remediation (auth/race/IAM) (#125)

* Fix auth and race-condition flaws in shift commands

Four confirmed findings from the 2026-06-17 security sweep:

- register_user let any Slack user overwrite an extension already
  bound to a different user (account takeover). Add a DynamoDB
  ConditionExpression so a write only succeeds when the extension is
  unclaimed or already this user's; raise ExtensionAlreadyRegistered
  otherwise and surface a clear Slack message.
- The `rate` subcommand was routed without the is_admin flag, so any
  user could set $0 pay rates. Gate _handle_rate on is_admin, matching
  the admin-command guard.
- `/oncall pick` used a plain put_item (TOCTOU): two concurrent picks
  both won. Use the atomic claim_open_shift conditional claim so the
  loser gets an "already picked up" message.
- swap-accept overwrote a shift independently claimed after the swap
  was initiated. Add reassign_if_held_by, a conditional write that only
  applies the swap while the override is still the requester's (or on
  the weekly fallback), and notify the accepter otherwise.

Add tests for the register-ownership guard and the rate admin guard.

Refs: INFRA

* Scope shift-manager Lambda IAM to least privilege

The nightly sweep flagged four over-broad permissions. Scope each to
only what the function actually reads (verified against source):

- WeeklyPost: secrets to slack-bot-token-* only (was the whole
  afterhours-shift-manager/* namespace); SES SendEmail to the single
  noreply@seahaven.com identity (was identity/*).
- RosterSync and RingScheduler: secrets to 3cx-* only (was the whole
  namespace); both read only the 3cx domain/client-id/client-secret.

SlackBotFunction and HolidayRouter wildcards are left unchanged — out
of scope for this sweep.

Refs: INFRA

* fix: re-validate shift holder on swap-accept (sh-security-review RIHB-1)

reassign_if_held_by trusted 'no override row' as 'still the requester's',
but a weekly-held shift also has no override row. An admin clear or weekly
edit between swap-init and accept could move the shift to a third party
with no override, letting the accept steal it (CWE-367, confirmed HIGH).
Re-resolve the current holder at accept and abort if it is no longer the
requester. Adds regression test + seeds the holder in existing accept tests.

* fix: complete IAM least-privilege sweep (sh-security-review)

HolidayRouter secrets scope afterhours-shift-manager/* -> /3cx-* (reads
only 3cx secrets); RingScheduler DynamoDBCrudPolicy -> DynamoDBReadPolicy
(read-only at runtime). SlackBot wildcard left as-is (reads across all
sub-prefixes; verified defensible).

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(

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)

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-<fn>, threshold 1 over one 5-min

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

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"),