mirror of
https://github.com/Sea-Haven-Industries/afterhours-shift-manager.git
synced 2026-09-30 06:43:12 +00:00
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
This commit is contained in:
parent
90c10d38d3
commit
43bfbf34c2
3 changed files with 156 additions and 19 deletions
|
|
@ -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
|
||||
try:
|
||||
self.table.update_item(
|
||||
Key={"PK": "ROSTER", "SK": extension},
|
||||
UpdateExpression="SET slack_user_id = :sid",
|
||||
ExpressionAttributeValues={":sid": slack_user_id},
|
||||
# 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(
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
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,26 @@ 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
|
||||
# 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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue