From 015b7e32f366bdaf352e8c2f69e65c9d46854d28 Mon Sep 17 00:00:00 2001 From: Agent Zero Date: Fri, 18 Sep 2026 08:53:54 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20F11=20(Astra=20P1)=20=E2=80=94?= =?UTF-8?q?=20Freigaben=20an=20Entscheider,=20Ablauf=20und=20Atomizitaet?= =?UTF-8?q?=20binden?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- alembic/versions/0146_approval_resolved_by.py | 31 ++++ app/core/approval.py | 131 ++++++++++++++-- app/routes/approvals.py | 56 ++++--- tests/test_s1_security_guards.py | 141 ++++++++++++++++++ 4 files changed, 332 insertions(+), 27 deletions(-) create mode 100644 alembic/versions/0146_approval_resolved_by.py diff --git a/alembic/versions/0146_approval_resolved_by.py b/alembic/versions/0146_approval_resolved_by.py new file mode 100644 index 0000000..73aaa18 --- /dev/null +++ b/alembic/versions/0146_approval_resolved_by.py @@ -0,0 +1,31 @@ +"""Add resolved_by to approval_requests (F11/Astra). + +Separates the assigned approver (approver_id — who the request was +addressed TO) from the actual decider (resolved_by — who decided). +Previously resolve_approval_request overwrote approver_id with the +acting user, destroying the assignment record. + +Revision ID: 0146 +Revises: 0145 +""" + +import sqlalchemy as sa +from sqlalchemy.dialects.postgresql import UUID as PGUUID + +from alembic import op + +revision = "0146" +down_revision = "0145" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "approval_requests", + sa.Column("resolved_by", PGUUID(as_uuid=True), nullable=True), + ) + + +def downgrade() -> None: + op.drop_column("approval_requests", "resolved_by") diff --git a/app/core/approval.py b/app/core/approval.py index 7e8b260..aaeb164 100644 --- a/app/core/approval.py +++ b/app/core/approval.py @@ -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 diff --git a/app/routes/approvals.py b/app/routes/approvals.py index 00b9e78..47faf9a 100644 --- a/app/routes/approvals.py +++ b/app/routes/approvals.py @@ -64,6 +64,7 @@ class ApprovalResponse(BaseModel): requested_by_type: str approver_id: str | None = None approver_group: str | None = None + resolved_by: str | None = None status: str comment: str | None = None created_at: str | None = None @@ -93,6 +94,7 @@ def _to_response(r: ApprovalRequest) -> ApprovalResponse: requested_by_type=r.requested_by_type, approver_id=str(r.approver_id) if r.approver_id else None, approver_group=r.approver_group, + resolved_by=str(r.resolved_by) if r.resolved_by else None, status=r.status, comment=r.comment, created_at=r.created_at.isoformat() if r.created_at else None, @@ -242,16 +244,25 @@ async def approve_approval( """Approve a pending approval request.""" tenant_id = uuid.UUID(current_user["tenant_id"]) rid = _parse_uuid(request_id, "request_id") - req = await resolve_approval_request( - db, - tenant_id, - rid, - decision="approved", - approver_id=uuid.UUID(current_user["user_id"]), - comment=body.comment, - ) + from app.core.approval import ApprovalDecisionError + + try: + req = await resolve_approval_request( + db, + tenant_id, + rid, + decision="approved", + approver_id=uuid.UUID(current_user["user_id"]), + comment=body.comment, + is_system_admin=bool(current_user.get("is_system_admin", False)), + ) + except ApprovalDecisionError as exc: + raise HTTPException( + status_code=exc.http_status, + detail={"detail": str(exc), "code": exc.code}, + ) from exc if req is None: - raise HTTPException(status_code=404, detail="Approval request not found or not pending") + raise HTTPException(status_code=404, detail="Approval request not found") await db.commit() return _to_response(req) @@ -270,16 +281,25 @@ async def reject_approval( """Reject a pending approval request.""" tenant_id = uuid.UUID(current_user["tenant_id"]) rid = _parse_uuid(request_id, "request_id") - req = await resolve_approval_request( - db, - tenant_id, - rid, - decision="rejected", - approver_id=uuid.UUID(current_user["user_id"]), - comment=body.comment, - ) + from app.core.approval import ApprovalDecisionError + + try: + req = await resolve_approval_request( + db, + tenant_id, + rid, + decision="rejected", + approver_id=uuid.UUID(current_user["user_id"]), + comment=body.comment, + is_system_admin=bool(current_user.get("is_system_admin", False)), + ) + except ApprovalDecisionError as exc: + raise HTTPException( + status_code=exc.http_status, + detail={"detail": str(exc), "code": exc.code}, + ) from exc if req is None: - raise HTTPException(status_code=404, detail="Approval request not found or not pending") + raise HTTPException(status_code=404, detail="Approval request not found") await db.commit() return _to_response(req) diff --git a/tests/test_s1_security_guards.py b/tests/test_s1_security_guards.py index 4cdb960..4e1919f 100644 --- a/tests/test_s1_security_guards.py +++ b/tests/test_s1_security_guards.py @@ -263,3 +263,144 @@ class TestF23RestoreRequiresSystemAdmin: "automation:admin) — a tenant admin must not trigger a " "full-database restore" ) + + +@pytest.mark.asyncio +class TestF11ApprovalBinding: + """F11 (Astra P1): approvals bound to decider, expiry and atomicity. + + Acceptance (Astra): + - wrong decider is rejected + - expired request is rejected and marked expired + - a concurrent/duplicate decision does not win twice + - assignment (approver_id) is preserved; resolved_by records the decider + """ + + async def _seed_request(self, db_session, **overrides): + """Create a pending ApprovalRequest row for testing.""" + from datetime import UTC, datetime, timedelta + + from app.core.approval import create_approval_request + + defaults = dict( + entity_type="contact", + entity_id=uuid.uuid4(), + action="tool:send_mail", + requested_by=uuid.uuid4(), + requested_by_type="agent", + ) + defaults.update(overrides) + req = await create_approval_request(db_session, uuid.uuid4(), **defaults) + await db_session.flush() + return req + + async def test_wrong_approver_rejected(self, db_session): + """A request assigned to user A cannot be decided by user B.""" + from app.core.approval import ApprovalDecisionError, resolve_approval_request + + assigned = uuid.uuid4() + req = await self._seed_request(db_session, approver_id=assigned) + other = uuid.uuid4() + + with pytest.raises(ApprovalDecisionError) as exc_info: + await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=other, + ) + assert exc_info.value.code == "wrong_approver" + assert exc_info.value.http_status == 403 + # request stays pending + await db_session.refresh(req) + assert req.status == "pending" + + async def test_assigned_approver_can_decide(self, db_session): + """The assigned approver may decide; assignment survives.""" + from app.core.approval import resolve_approval_request + + assigned = uuid.uuid4() + req = await self._seed_request(db_session, approver_id=assigned) + + result = await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=assigned, + ) + assert result is not None and result.status == "approved" + # F11: assignment preserved, decider recorded separately + assert result.approver_id == assigned + assert result.resolved_by == assigned + + async def test_expired_request_rejected_and_marked(self, db_session): + """An expired request cannot be decided — even by its assignee.""" + from datetime import UTC, datetime, timedelta + + from app.core.approval import ApprovalDecisionError, resolve_approval_request + + assigned = uuid.uuid4() + req = await self._seed_request( + db_session, + approver_id=assigned, + expires_at=datetime.now(UTC) - timedelta(hours=1), + ) + + with pytest.raises(ApprovalDecisionError) as exc_info: + await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=assigned, + ) + assert exc_info.value.code == "expired" + assert exc_info.value.http_status == 410 + await db_session.refresh(req) + assert req.status == "expired" + + async def test_already_decided_rejected(self, db_session): + """A second decision (concurrent or duplicate) is rejected with 409.""" + from app.core.approval import ApprovalDecisionError, resolve_approval_request + + assigned = uuid.uuid4() + req = await self._seed_request(db_session, approver_id=assigned) + + first = await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=assigned, + ) + assert first is not None + + with pytest.raises(ApprovalDecisionError) as exc_info: + await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="rejected", approver_id=assigned, + ) + assert exc_info.value.code == "not_pending" + assert exc_info.value.http_status == 409 + + async def test_unassigned_request_any_decider(self, db_session): + """Unassigned requests (no approver_id, no group) may be decided + by anyone — the approvals:approve permission is enforced by the + route dependency, not the resolver.""" + from app.core.approval import resolve_approval_request + + req = await self._seed_request(db_session) # no assignment + decider = uuid.uuid4() + result = await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=decider, + ) + assert result is not None + assert result.resolved_by == decider + assert result.approver_id is None # assignment untouched + + async def test_system_admin_override_documented(self, db_session): + """System admins may decide assigned requests — documented + operations override (Astra: Ausnahme dokumentiert).""" + from app.core.approval import resolve_approval_request + + assigned = uuid.uuid4() + req = await self._seed_request(db_session, approver_id=assigned) + sysadmin = uuid.uuid4() + result = await resolve_approval_request( + db_session, req.tenant_id, req.id, + decision="approved", approver_id=sysadmin, is_system_admin=True, + ) + assert result is not None + assert result.approver_id == assigned # assignment preserved + assert result.resolved_by == sysadmin # decider recorded