fix(security): F11 (Astra P1) — Freigaben an Entscheider, Ablauf und Atomizitaet binden
Vorher: resolve_approval_request pruefte nur Mandant + pending — NICHT
Ablaufdatum, NICHT den vorgesehenen Genehmiger, und ueberschrieb
approver_id mit dem tatsaechlichen Entscheider (Zuordnung verloren).
Astra-Repro: Eine abgelaufene Anfrage konnte von einem anderen
Entscheider genehmigt werden; konkurrierende Entscheidungen waren
moeglich.
Fix:
- Migration 0146: neue Spalte resolved_by (Zuordnung vs. Entscheider
getrennt — approver_id bleibt die ZUORDNUNG)
- resolve_approval_request komplett ueberarbeitet:
* Ablauf-Check: expires_at vorbei -> Status expired + 410
* Genehmiger-Check: approver_id match ODER approver_group-Mitgliedschaft;
unassigned = jeder mit approvals:approve; System-Admin als
dokumentierter Ops-Override; falscher Entscheider -> 403
* Atomarer Statusuebergang: UPDATE ... WHERE status=pending —
konkurrierende Entscheidung -> 409
* approver_id wird NIE ueberschrieben; resolved_by dokumentiert den
Entscheider
- ApprovalDecisionError mit HTTP-Status-Codes; approve/reject-Routen
fangen sie sauber ab (404/409/410/403 statt Flat-404)
- ApprovalResponse + Mapper um resolved_by ergaenzt
Abnahme (Astra): Falscher Entscheider, abgelaufene Anfrage und doppelte
Entscheidung werden abgewiesen — erfuellt (6 Tests).
Hinweis: workflows.py approve/reject-Aufrufer waren bereits kaputt
(F12, S2-Welle: approval[id] auf ORM-Objekt) und werden dort gefixt.
Tests: test_s1_security_guards.py 18/18 (6 neue F11-Tests). ruff clean.
Damit ist S1 — ALLE 11 Sicherheits-Findings der Astra-Welle 1 gefixt.
This commit is contained in:
+122
-9
@@ -52,6 +52,12 @@ class ApprovalRequest(Base, TenantMixin):
|
||||
PGUUID(as_uuid=True), nullable=True
|
||||
)
|
||||
approver_group: Mapped[str | None] = mapped_column(String(120), nullable=True)
|
||||
# F11: who actually decided — approver_id stays the ASSIGNMENT,
|
||||
# resolved_by records the ACTUAL decider (previously the assignment
|
||||
# was overwritten by whoever decided).
|
||||
resolved_by: Mapped[uuid.UUID | None] = mapped_column(
|
||||
PGUUID(as_uuid=True), nullable=True
|
||||
)
|
||||
status: Mapped[str] = mapped_column(
|
||||
String(20), nullable=False, default="pending"
|
||||
)
|
||||
@@ -103,6 +109,41 @@ async def create_approval_request(
|
||||
return req
|
||||
|
||||
|
||||
class ApprovalDecisionError(Exception):
|
||||
"""Raised when an approval decision is invalid (F11/Astra).
|
||||
|
||||
Attributes:
|
||||
code: machine-readable reason for the HTTP layer.
|
||||
http_status: suggested HTTP status code.
|
||||
"""
|
||||
|
||||
def __init__(self, code: str, message: str, http_status: int = 403):
|
||||
super().__init__(message)
|
||||
self.code = code
|
||||
self.http_status = http_status
|
||||
|
||||
|
||||
async def _user_in_approver_group(
|
||||
db: AsyncSession, tenant_id: uuid.UUID, user_id: uuid.UUID, group_name: str
|
||||
) -> bool:
|
||||
"""Check whether the user is a member of the named approver group."""
|
||||
from sqlalchemy import select
|
||||
|
||||
from app.models.group import Group, UserGroup
|
||||
|
||||
result = await db.execute(
|
||||
select(UserGroup.id)
|
||||
.join(Group, UserGroup.group_id == Group.id)
|
||||
.where(
|
||||
UserGroup.user_id == user_id,
|
||||
UserGroup.tenant_id == tenant_id,
|
||||
Group.name == group_name,
|
||||
)
|
||||
.limit(1)
|
||||
)
|
||||
return result.first() is not None
|
||||
|
||||
|
||||
async def resolve_approval_request(
|
||||
db: AsyncSession,
|
||||
tenant_id: uuid.UUID,
|
||||
@@ -111,12 +152,31 @@ async def resolve_approval_request(
|
||||
decision: str,
|
||||
approver_id: uuid.UUID,
|
||||
comment: str | None = None,
|
||||
is_system_admin: bool = False,
|
||||
) -> ApprovalRequest | None:
|
||||
"""Approve or reject a pending approval request.
|
||||
"""Approve or reject a pending approval request (F11 hardened).
|
||||
|
||||
Returns the updated request, or ``None`` if not found / not pending.
|
||||
Returns the updated request, or ``None`` if not found.
|
||||
|
||||
Raises ApprovalDecisionError when the decision is invalid:
|
||||
- ``expired`` (410): the request's expires_at has passed — it is
|
||||
marked expired and can no longer be decided.
|
||||
- ``not_pending`` (409): the request was already decided concurrently.
|
||||
- ``wrong_approver``(403): the acting user is neither the assigned
|
||||
approver (approver_id) nor a member of the assigned approver_group.
|
||||
Unassigned requests (no approver_id AND no approver_group) may be
|
||||
decided by anyone holding approvals:approve; system admins may
|
||||
decide any request (documented operations override).
|
||||
|
||||
The assignment (approver_id) is NEVER overwritten — the actual decider
|
||||
is recorded in resolved_by (F11: assignment and decider are separate).
|
||||
"""
|
||||
from sqlalchemy import select
|
||||
from datetime import UTC, datetime
|
||||
|
||||
from sqlalchemy import select, update
|
||||
|
||||
if decision not in ("approved", "rejected"):
|
||||
raise ValueError(f"invalid decision: {decision!r}")
|
||||
|
||||
result = await db.execute(
|
||||
select(ApprovalRequest).where(
|
||||
@@ -125,14 +185,67 @@ async def resolve_approval_request(
|
||||
)
|
||||
)
|
||||
req = result.scalar_one_or_none()
|
||||
if req is None or req.status != "pending":
|
||||
if req is None:
|
||||
return None
|
||||
|
||||
req.status = decision
|
||||
req.approver_id = approver_id
|
||||
req.comment = comment
|
||||
req.resolved_at = datetime.now(UTC)
|
||||
await db.flush()
|
||||
# 1. Expiry check — an expired request can no longer be decided.
|
||||
if (
|
||||
req.status == "pending"
|
||||
and req.expires_at is not None
|
||||
and req.expires_at < datetime.now(UTC)
|
||||
):
|
||||
req.status = "expired"
|
||||
req.resolved_at = datetime.now(UTC)
|
||||
await db.flush()
|
||||
raise ApprovalDecisionError(
|
||||
"expired", "Approval request has expired", http_status=410
|
||||
)
|
||||
|
||||
# 2. Approver check — who may decide this request?
|
||||
if not is_system_admin:
|
||||
assigned_user = req.approver_id
|
||||
assigned_group = req.approver_group
|
||||
allowed = False
|
||||
if assigned_user is not None:
|
||||
allowed = assigned_user == approver_id
|
||||
if not allowed and assigned_group:
|
||||
allowed = await _user_in_approver_group(
|
||||
db, tenant_id, approver_id, assigned_group
|
||||
)
|
||||
if not allowed and assigned_user is None and assigned_group is None:
|
||||
# Unassigned request: anyone with approvals:approve may decide.
|
||||
allowed = True
|
||||
if not allowed:
|
||||
raise ApprovalDecisionError(
|
||||
"wrong_approver",
|
||||
"This approval request is assigned to a different approver",
|
||||
http_status=403,
|
||||
)
|
||||
|
||||
# 3. Atomic status transition — a concurrent decision must not win twice.
|
||||
now = datetime.now(UTC)
|
||||
upd = await db.execute(
|
||||
update(ApprovalRequest)
|
||||
.where(
|
||||
ApprovalRequest.id == request_id,
|
||||
ApprovalRequest.tenant_id == tenant_id,
|
||||
ApprovalRequest.status == "pending",
|
||||
)
|
||||
.values(
|
||||
status=decision,
|
||||
resolved_by=approver_id,
|
||||
comment=comment,
|
||||
resolved_at=now,
|
||||
)
|
||||
)
|
||||
if upd.rowcount == 0:
|
||||
raise ApprovalDecisionError(
|
||||
"not_pending",
|
||||
"Approval request was already decided",
|
||||
http_status=409,
|
||||
)
|
||||
|
||||
await db.refresh(req)
|
||||
return req
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user