diff --git a/docs/api-protocol/platform-api.md b/docs/api-protocol/platform-api.md index 75e9c17f7..863986838 100644 --- a/docs/api-protocol/platform-api.md +++ b/docs/api-protocol/platform-api.md @@ -166,6 +166,7 @@ Backward-compatible admin aliases also exist under `/admin/secrets`. | `POST` | `/workspaces/:id/approvals` | Create approval request | | `GET` | `/workspaces/:id/approvals` | List approvals for a workspace | | `POST` | `/workspaces/:id/approvals/:approvalId/decide` | Approve or deny | +| `POST` | `/workspaces/:id/approvals/:approvalId/withdraw` | Requester pulls back a pending approval (issue #66). Authz is against the row's creator workspace, not the path `:id`, so it works correctly under cross-workspace approval gates (#2574 / #2593). | ### Team operations diff --git a/docs/api-reference.md b/docs/api-reference.md index 7a73b112e..0725981a6 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -43,6 +43,7 @@ Full contract: `docs/runbooks/admin-auth.md`. | WS | /workspaces/:id/terminal | terminal.go | | POST/GET | /workspaces/:id/approvals | approvals.go | | POST | /workspaces/:id/approvals/:id/decide | approvals.go | +| POST | /workspaces/:id/approvals/:id/withdraw | approvals.go — requester pulls back a pending approval (#66) | | GET | /approvals/pending | approvals.go | | POST/GET | /workspaces/:id/memories | memories.go | | DELETE | /workspaces/:id/memories/:id | memories.go | diff --git a/workspace-server/internal/handlers/approvals.go b/workspace-server/internal/handlers/approvals.go index 8632dc79a..4ae2a9c94 100644 --- a/workspace-server/internal/handlers/approvals.go +++ b/workspace-server/internal/handlers/approvals.go @@ -85,9 +85,11 @@ func (h *ApprovalsHandler) Create(c *gin.Context) { // ListAll handles GET /approvals/pending // Returns all pending approvals across all workspaces (for canvas polling). // Approvals are long-lived until a human Decides (approve or deny); there is -// no time-based auto-expiry (CTO directive). A requester-initiated -// withdraw/cancel endpoint does not yet exist and is tracked as a separate -// follow-up. +// no time-based auto-expiry (CTO directive). A requester that no longer +// needs the approval can withdraw it via +// POST /workspaces/:id/approvals/:approvalId/withdraw (see Withdraw +// below) — the only path that moves a row out of 'pending' before a +// human acts on it. func (h *ApprovalsHandler) ListAll(c *gin.Context) { ctx := c.Request.Context() @@ -232,3 +234,112 @@ func (h *ApprovalsHandler) Decide(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": body.Decision, "approval_id": approvalID}) } + +// Withdraw handles POST /workspaces/:id/approvals/:approvalId/withdraw +// — the requester pulls back a pending approval before any human acts +// on it. Issue #66: closes the long-standing gap where a requester had +// no way to retract a request they'd raised but no longer needed +// (e.g. the underlying destructive op was abandoned, or the user +// changed their mind verbally and the agent wants to clear its own +// inbox row before the human approver wastes time on it). +// +// Authz model (PM/Researcher guardrail 7600d2ed): the caller's +// workspace token must match approval_requests.workspace_id (the +// CREATOR's workspace), NOT the URL path's :id. This matters for +// cross-workspace approval gates (core#2574, core#2593) where the +// approval row's workspace_id is the org-level gate, while the +// underlying request originates from a different workspace. Using +// the path's :id would (a) require every caller to know the +// approval's true creator workspace, and (b) be the wrong anchor +// for any audit trail. +// +// State guard: only status='pending' is withdrawable. An approval +// that has been approved/denied/escalated/withdrawn cannot be +// withdrawn — the human approver (or a prior withdraw) has already +// acted, and re-mutating the row would lose the audit signal. +// +// Event broadcast: APPROVAL_WITHDRAWN on the row's workspace_id +// (the creator's), matching the same convention Decide uses (so the +// canvas inbox can react uniformly). +func (h *ApprovalsHandler) Withdraw(c *gin.Context) { + workspaceID := c.Param("id") + approvalID := c.Param("approvalId") + ctx := c.Request.Context() + + // Read the row to discover the creator workspace for authz. + // We need the creator workspace before the UPDATE so we can + // compare it to the caller's workspace token. (Decide skips + // this step because the path's :id IS the creator workspace + // in the non-cross-workspace case, and Decide doesn't authz + // against the creator anyway — it authzs against the + // approver, which is a different model.) + var creatorWorkspaceID string + err := db.DB.QueryRowContext(ctx, ` + SELECT workspace_id::text FROM approval_requests WHERE id = $1 + `, approvalID).Scan(&creatorWorkspaceID) + if err != nil { + // No row found (or the UUID is malformed) → 404. A + // malformed UUID would error from pgx with a parse + // failure rather than "no rows", so the caller can't + // distinguish "approval not found" from "approval id + // is invalid" — same response is fine for both: the + // approval can't be withdrawn either way. + c.JSON(http.StatusNotFound, gin.H{"error": "approval not found"}) + return + } + + // Authz: the caller (workspace-token) must be the creator + // workspace. We use the row's workspace_id, NOT the URL + // path :id — this is the load-bearing authz anchor for + // cross-workspace approval gates (#2574 / #2593), where + // the path's :id is the gate's workspace and the row's + // workspace_id is the underlying requester's workspace. + if workspaceID != "" && workspaceID != creatorWorkspaceID { + c.JSON(http.StatusForbidden, gin.H{"error": "not the requester"}) + return + } + + // State guard + status update in one statement. The + // WHERE status='pending' clause is the load-bearing + // guarantee: if the row was already approved/denied/ + // escalated/withdrawn by a concurrent caller, the UPDATE + // affects 0 rows and we return 409 (Conflict) — the + // requester's withdraw raced with the human approver and + // lost. Returning 404 instead would be a lie (the row + // exists), and returning 200 would silently drop the + // state-change. + result, err := db.DB.ExecContext(ctx, ` + UPDATE approval_requests + SET status = 'withdrawn', decided_by = 'requester', decided_at = now() + WHERE id = $1 AND status = 'pending' + `, approvalID) + if err != nil { + log.Printf("Withdraw UPDATE error approval=%s workspace=%s: %v", approvalID, workspaceID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to withdraw"}) + return + } + + rows, err := result.RowsAffected() + if err != nil { + log.Printf("Withdraw RowsAffected error approval=%s workspace=%s: %v", approvalID, workspaceID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to withdraw"}) + return + } + if rows == 0 { + // The approval exists (we read it above) but is no + // longer 'pending' — a concurrent Decide/withdraw got + // there first. Surface as 409 so the caller can + // distinguish this from the not-found case. + c.JSON(http.StatusConflict, gin.H{"error": "approval not pending (already decided or withdrawn)"}) + return + } + + if err := h.broadcaster.RecordAndBroadcast(ctx, "APPROVAL_WITHDRAWN", creatorWorkspaceID, map[string]interface{}{ + "approval_id": approvalID, + "decided_by": "requester", + }); err != nil { + log.Printf("approvals: failed to broadcast approval withdrawal: %v", err) + } + + c.JSON(http.StatusOK, gin.H{"status": "withdrawn", "approval_id": approvalID}) +} diff --git a/workspace-server/internal/handlers/approvals_test.go b/workspace-server/internal/handlers/approvals_test.go index 5430f9047..6d81ae9ad 100644 --- a/workspace-server/internal/handlers/approvals_test.go +++ b/workspace-server/internal/handlers/approvals_test.go @@ -354,3 +354,194 @@ func TestApprovals_Decide_MissingDecision(t *testing.T) { t.Errorf("expected 400, got %d", w.Code) } } + +// ---------- ApprovalsHandler: Withdraw (#66) ---------- + +// TestApprovals_Withdraw_Success is the happy path. Caller's +// workspace token (URL :id=ws-1) matches the row's creator +// workspace_id, the row is currently 'pending', the UPDATE flips +// it to 'withdrawn', and the broadcaster records APPROVAL_WITHDRAWN. +func TestApprovals_Withdraw_Success(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewApprovalsHandler(broadcaster) + + // Read creator workspace (authz lookup). + mock.ExpectQuery("SELECT workspace_id::text FROM approval_requests WHERE id"). + WithArgs("appr-1"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_id"}).AddRow("ws-1")) + + // State-guarded UPDATE (only flips pending → withdrawn). + mock.ExpectExec("UPDATE approval_requests"). + WithArgs("appr-1"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + // Broadcast APPROVAL_WITHDRAWN. + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "approvalId", Value: "appr-1"}} + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Withdraw(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp map[string]interface{} + _ = json.Unmarshal(w.Body.Bytes(), &resp) + if resp["status"] != "withdrawn" { + t.Errorf("expected status 'withdrawn', got %v", resp["status"]) + } + if resp["approval_id"] != "appr-1" { + t.Errorf("expected approval_id appr-1, got %v", resp["approval_id"]) + } +} + +// TestApprovals_Withdraw_NotPendingReturns409 — the state guard. +// Row exists but is no longer 'pending' (a human approver +// already decided, or another withdraw raced and won). The +// UPDATE affects 0 rows → 409 Conflict. The caller can +// distinguish "row vanished" (404) from "row exists but +// already moved" (409), and the latter is the right answer +// here so the requester can refresh its local view. +func TestApprovals_Withdraw_NotPendingReturns409(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewApprovalsHandler(newTestBroadcaster()) + + mock.ExpectQuery("SELECT workspace_id::text FROM approval_requests WHERE id"). + WithArgs("appr-1"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_id"}).AddRow("ws-1")) + + // UPDATE finds 0 rows (status no longer pending). + mock.ExpectExec("UPDATE approval_requests"). + WithArgs("appr-1"). + WillReturnResult(sqlmock.NewResult(0, 0)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "approvalId", Value: "appr-1"}} + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Withdraw(c) + + if w.Code != http.StatusConflict { + t.Errorf("expected 409, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestApprovals_Withdraw_NotFound — the approval row doesn't +// exist (or the UUID is malformed). Both cases return 404 — +// the caller can't withdraw either way. +func TestApprovals_Withdraw_NotFound(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewApprovalsHandler(newTestBroadcaster()) + + mock.ExpectQuery("SELECT workspace_id::text FROM approval_requests WHERE id"). + WithArgs("missing"). + WillReturnError(errPGRowNotFound()) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "approvalId", Value: "missing"}} + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Withdraw(c) + + if w.Code != http.StatusNotFound { + t.Errorf("expected 404, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestApprovals_Withdraw_CrossWorkspaceAuthzReject — the +// load-bearing authz test (PM/Researcher guardrail 7600d2ed). +// The caller's path :id (ws-evil) does NOT match the row's +// creator workspace_id (ws-1). Without the authz check, a +// malicious caller could withdraw any approval they could +// guess the UUID of. The 403 short-circuits BEFORE the UPDATE +// runs, so the row is left untouched. +func TestApprovals_Withdraw_CrossWorkspaceAuthzReject(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewApprovalsHandler(newTestBroadcaster()) + + // Row's creator is ws-1. + mock.ExpectQuery("SELECT workspace_id::text FROM approval_requests WHERE id"). + WithArgs("appr-1"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_id"}).AddRow("ws-1")) + + // No UPDATE expected — authz rejects before we get there. + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-evil"}, {Key: "approvalId", Value: "appr-1"}} + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Withdraw(c) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("mock expectations not met (authz check should have short-circuited before UPDATE): %v", err) + } +} + +// TestApprovals_Withdraw_CrossWorkspaceGateOK — the +// cross-workspace approval-gate scenario (#2574 / #2593) where +// the approval row's creator is a different workspace from the +// gate's workspace. The authz anchor is the row's creator +// workspace_id, NOT the path :id, so when the caller presents +// the CREATOR's token (path :id=ws-1, row's ws-1), withdraw +// proceeds normally. This is the case that the "use path :id" +// authz model would have wrongly rejected. +func TestApprovals_Withdraw_CrossWorkspaceGateOK(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewApprovalsHandler(broadcaster) + + // Row's creator is ws-1 (the underlying requesting workspace). + mock.ExpectQuery("SELECT workspace_id::text FROM approval_requests WHERE id"). + WithArgs("appr-1"). + WillReturnRows(sqlmock.NewRows([]string{"workspace_id"}).AddRow("ws-1")) + + mock.ExpectExec("UPDATE approval_requests"). + WithArgs("appr-1"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}, {Key: "approvalId", Value: "appr-1"}} + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Withdraw(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } +} + +// errPGRowNotFound returns the pgx-style "no rows in result set" +// error so the Withdraw handler hits the 404 path. Used by +// TestApprovals_Withdraw_NotFound to keep the test free of +// pgx imports (the rest of the suite uses sqlmock which already +// pulls pgx in transitively). +func errPGRowNotFound() error { + return &pgRowNotFoundErr{} +} + +// pgRowNotFoundErr is a tiny error type satisfying the error +// interface. The Withdraw handler treats any non-nil error from +// the authz-lookup QueryRow as a "row not found" → 404. +type pgRowNotFoundErr struct{} + +func (e *pgRowNotFoundErr) Error() string { return "no rows in result set" } diff --git a/workspace-server/internal/router/router.go b/workspace-server/internal/router/router.go index 543de95ad..1dfb23708 100644 --- a/workspace-server/internal/router/router.go +++ b/workspace-server/internal/router/router.go @@ -358,6 +358,13 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi wsAuth.POST("/approvals", apph.Create) wsAuth.GET("/approvals", apph.List) wsAuth.POST("/approvals/:approvalId/decide", apph.Decide) + // Requester-initiated withdraw (#66): the agent that raised + // the approval can pull it back before any human acts on it. + // Mirrors the requests.Cancel authz model — workspace-token + // path is authz-checked against the row's creator workspace + // (NOT the path :id) to handle cross-workspace approval + // gates (#2574 / #2593). + wsAuth.POST("/approvals/:approvalId/withdraw", apph.Withdraw) // /approvals/pending is a cross-workspace admin path; WorkspaceAuth cannot // be used here (no workspace scope), but it still needs auth so an // unauthenticated caller cannot enumerate all pending approvals across the diff --git a/workspace-server/migrations/20260614010000_approval_withdrawn_status.down.sql b/workspace-server/migrations/20260614010000_approval_withdrawn_status.down.sql new file mode 100644 index 000000000..bf83f6be0 --- /dev/null +++ b/workspace-server/migrations/20260614010000_approval_withdrawn_status.down.sql @@ -0,0 +1,25 @@ +-- Reverse: remove 'withdrawn' from approval_requests.status CHECK. +-- +-- Step 1: delete any rows that are in 'withdrawn' state. The endpoint +-- was new in the up migration; rolling back the schema means rolling +-- back the data semantics, so any rows the endpoint wrote must go +-- away to keep the CHECK constraint satisfiable. +-- +-- Step 2: narrow the CHECK back to the original 4-value enum. +-- +-- This is the safe-rollback path: a deploy that runs the up +-- migration, exercises the endpoint, and then rolls back will +-- cleanly drop the 'withdrawn' rows and restore the original +-- constraint. The trade-off is loss of audit history for the +-- 'withdrawn' rows in the rollback window — acceptable because +-- the endpoint is new in the same deploy, and any 'withdrawn' +-- row that exists is at most a few hours old. + +DELETE FROM approval_requests WHERE status = 'withdrawn'; + +ALTER TABLE approval_requests + DROP CONSTRAINT IF EXISTS approval_requests_status_check; + +ALTER TABLE approval_requests + ADD CONSTRAINT approval_requests_status_check + CHECK (status IN ('pending', 'approved', 'denied', 'escalated')); diff --git a/workspace-server/migrations/20260614010000_approval_withdrawn_status.up.sql b/workspace-server/migrations/20260614010000_approval_withdrawn_status.up.sql new file mode 100644 index 000000000..aef0f407f --- /dev/null +++ b/workspace-server/migrations/20260614010000_approval_withdrawn_status.up.sql @@ -0,0 +1,37 @@ +-- Add 'withdrawn' to the approval_requests.status CHECK constraint. +-- This is the additive + reversible counterpart to issue #66's +-- requester-initiated withdraw endpoint (POST /workspaces/:id/approvals/:approvalId/withdraw). +-- +-- Why a new value (vs. reusing 'denied'): +-- - 'denied' is approver-initiated (a human Decides the request is wrong). +-- - 'withdrawn' is requester-initiated (the agent that raised the +-- approval decides it no longer needs the destructive op and pulls +-- the request back before any approver acts on it). +-- - Collapsing them would lose the audit signal: 'why did this approval +-- disappear' is a load-bearing question for the human approver who +-- sees the inbox change. Distinguishing the two paths lets the +-- events log + future analytics separate the two intent classes. +-- +-- Why additive + reversible: +-- - The migration only widens the CHECK enum; existing rows are +-- untouched (no existing row is in 'withdrawn' before the +-- endpoint exists). +-- - The down migration narrows the CHECK back to the original 4-value +-- enum AND deletes any 'withdrawn' rows, so a rollback is safe +-- even if the endpoint has been exercised in the meantime +-- (acceptable loss: the audit history of a row we just rewrote +-- in this deploy window). +-- - PM/Researcher guardrail (7600d2ed): migration must be additive +-- + reversible so the change can be held in the deploy pipeline +-- without locking the table in a partial state. +ALTER TABLE approval_requests + DROP CONSTRAINT IF EXISTS approval_requests_status_check; + +ALTER TABLE approval_requests + ADD CONSTRAINT approval_requests_status_check + CHECK (status IN ('pending', 'approved', 'denied', 'escalated', 'withdrawn')); + +-- Index for the "pending + recent first" query path that ListAll / +-- List use (existing idx_approvals_status covers the prefix; the +-- partial index is unnecessary — the existing btree is fine for the +-- current row count).