diff --git a/docs/security/defer-decisions.md b/docs/security/defer-decisions.md index 3829362..a6d4599 100644 --- a/docs/security/defer-decisions.md +++ b/docs/security/defer-decisions.md @@ -185,9 +185,10 @@ context template. When the agent runs with `--with slack` and a deferred tool's `to` is `channel:slack:`, Forge **delivers the approval natively**: on a deferral the Slack adapter posts a **Block Kit** message with **Approve / -Reject** buttons to that channel, and a click resolves the deferral — -the tool proceeds or fails and the message updates with the outcome + -approver. +Reject** buttons to that channel. **Approve** resolves the deferral +immediately; **Reject** opens a modal that prompts for a reason (recorded +as the decision `note`). Either way the tool then proceeds or fails and +the message updates with the outcome + approver. ```yaml security: @@ -228,31 +229,86 @@ doesn't implement interactive approvals (`channels.ApprovalDeliverer`) can't be a target. Telegram / MS Teams interactive approvals are a follow-up (same interface). -> ⚠️ **The approval authority is channel membership.** Any user who can -> see the message and click a button resolves the deferred call. Forge -> records **who** clicked (in the audit event) but does **not** yet check -> that they are *authorized* to approve — so **the target channel's -> membership IS the approval ACL.** Consequences: +> ⚠️ **Without an `approvers` allowlist, the approval authority is channel +> membership.** Any user who can see the message and click a button +> resolves the deferred call. So unless you set `approvers` (below), +> **the target channel's membership IS the approval ACL.** Consequences: > > - Route approvals to a **tightly-scoped private channel**, not a broad > `#oncall` that includes guests, contractors, bots, or integrations. > - A **compromised member account** grants approval authority over every > agent action routed there — treat channel membership as a privileged > grant. -> - There is no requester≠approver (four-eyes) or per-tool approver -> restriction today. A per-tool, email-based approver allowlist is -> tracked in #313. +> - There is no requester≠approver (four-eyes) gate yet. > -> **No rejection reason is captured from Slack today.** A button click -> carries no free text, so a Slack `reject` records an empty `note`; if -> you need a documented reason, resolve via `POST /tasks/{id}/decisions` -> with a `note`, or wait for the follow-up that opens a reason modal. +> **Reject captures a reason.** A button click carries no free text, so +> clicking **Reject** opens a modal that prompts for a justification; the +> typed reason is recorded as the decision `note` (and surfaces in the +> `task_deferred_decision` audit event and the `defer: rejected by …` +> tool error). If the modal can't be opened (a transient `views.open` +> failure), Forge falls back to a reason-less reject so the decision is +> never lost. **Approve** resolves immediately, with no modal. **If the target adapter isn't running**, Forge warns at startup (e.g. `security.defer` routes `cli_execute` to `channel:slack:…` but `slack` is not active — start with `--with slack`). The deferral still holds; the approval just won't be delivered until an approver POSTs directly. +### Approver allowlist (`approvers`) + +To require that only specific people can approve — rather than anyone in +the channel — set a per-tool (or default) **email** allowlist: + +```yaml +security: + defer: + default_approvers: [sec-lead@corp.com] # applied when a tool omits its own + tools: + cli_execute: + to: channel:slack:#sec-approvals + timeout: 5m + approvers: [alice@corp.com, oncall-lead@corp.com] # only these may resolve +``` + +- **Email, not platform ids** — a portable identity that works the same + across Slack / Teams / a web console and matches Forge's OIDC model. +- **The Slack adapter resolves the clicker's email** via `users.info` + (cached) — this needs the bot's **`users:read.email`** scope. +- **Enforced in the shared runtime** at `POST /tasks/{id}/decisions` + (adapter-agnostic — a direct `curl` must send `approver_email` too), and + **fails closed**: an approver whose email isn't listed — or can't be + resolved (guest without email, missing scope) — is refused with `403` + and the **deferral stays pending** for a real approver. The refusal is + audited (`task_deferred_decision` with `authorized: false`). +- Empty `approvers` (and empty `default_approvers`) → no allowlist; channel + membership is the ACL (see the warning above). + +> 🔑 **The allowlist authorizes an *asserted* email — its strength depends on +> who asserts it.** +> +> - **Slack path** (the one #311 left open): the runtime resolves the real +> email from `users.info`, bound to the authenticated Slack identity. Here +> the allowlist genuinely restricts *which channel members* can approve. +> - **Direct `POST /tasks/{id}/decisions`**: the endpoint trusts the +> `approver_email` the caller supplies. Whoever holds the runtime bearer +> token can assert any listed email — so for direct posts the real gate is +> the endpoint's **authentication**, and the allowlist is advisory on top. +> That's acceptable (the token holder is already trusted), but it means +> **`--no-auth` + an allowlist does not hold**: with no auth, anyone who can +> reach the port can assert any email. Never rely on the allowlist without +> the runner's auth middleware enabled. +> +> A **malformed** approver entry — a bare handle (`alice`), a missing `@` +> (`alicecorp.com`), or a no-dot domain (`alice@corp`) — can never match and +> silently locks that person out; fail-closed, not insecure. Forge warns at +> startup for any `approvers`/`default_approvers` entry that isn't +> email-shaped so the mistake is caught before it's needed. (This is a +> structural check only — it can't catch a *valid-but-wrong* address like a +> `corp`↔`crop` domain typo, which needs a directory to detect.) + +Group/role-based approver policies and a requester≠approver (four-eyes) +gate are future work. + > ⏱ **Approval window for channel-initiated conversations.** A conversation > that arrives *through* a channel adapter (Slack/Telegram → agent) is > served synchronously: the channel router holds the request open for diff --git a/forge-cli/cmd/run.go b/forge-cli/cmd/run.go index f7672d9..b04bb2d 100644 --- a/forge-cli/cmd/run.go +++ b/forge-cli/cmd/run.go @@ -399,6 +399,15 @@ func runRun(cmd *cobra.Command, args []string) error { for _, w := range deferTimeoutWarnings(cfg.Security.Defer, channels.SyncRequestTimeout) { fmt.Fprintln(os.Stderr, " Warning: "+w) } + // #315 review (finding 2): approver emails are free strings, so a + // malformed entry (a bare handle, a missing `@`, a no-dot domain) silently + // never matches and locks that person out — fail-closed, so not insecure, + // just a confusing lockout. Warn on any approver entry that isn't + // email-shaped so the mistake is caught at startup. (Structural only — a + // valid-but-wrong address like a `corp`↔`crop` typo can't be caught here.) + for _, w := range deferApproverWarnings(cfg.Security.Defer) { + fmt.Fprintln(os.Stderr, " Warning: "+w) + } return runner.Run(ctx) } @@ -464,6 +473,46 @@ func deferTimeoutWarnings(deferCfg types.DeferConfig, syncWait time.Duration) [] return warns } +// deferApproverWarnings returns a warning for each DEFER approver entry +// (per-tool `approvers` or `default_approvers`) that isn't email-shaped. The +// allowlist matches on email (#313), so a non-email entry can never match and +// silently locks that approver out — fail-closed, but a startup warning turns a +// 2 a.m. lockout into a config typo caught at boot. Pure + testable. +func deferApproverWarnings(deferCfg types.DeferConfig) []string { + if !deferCfg.Enabled { + return nil + } + var warns []string + check := func(where string, list []string) { + for _, a := range list { + if !looksLikeEmail(a) { + warns = append(warns, fmt.Sprintf( + "security.defer %s lists approver %q, which is not email-shaped — the approver allowlist matches on email, so this entry will never authorize anyone (missing @, bare handle, or no-dot domain)", + where, a)) + } + } + } + for tool, tc := range deferCfg.Tools { + check(fmt.Sprintf("tool %q", tool), tc.Approvers) + } + check("default_approvers", deferCfg.DefaultApprovers) + sort.Strings(warns) + return warns +} + +// looksLikeEmail is a deliberately loose check — one "@" with non-empty local +// and domain parts, and a dot in the domain. Enough to catch the common typos +// (missing @, bare username) without rejecting valid addresses. +func looksLikeEmail(s string) bool { + s = strings.TrimSpace(s) + at := strings.IndexByte(s, '@') + if at <= 0 || at != strings.LastIndexByte(s, '@') || at == len(s)-1 { + return false + } + domain := s[at+1:] + return strings.Contains(domain, ".") && !strings.HasPrefix(domain, ".") && !strings.HasSuffix(domain, ".") +} + // parseDeferTarget parses a DEFER `to:` value of the form // "channel::" (e.g. "channel:slack:#oncall") into its adapter // and target. Returns ok=false for any other shape. @@ -480,9 +529,10 @@ func parseDeferTarget(to string) (adapter, target string, ok bool) { // whether the channel adapter runs in-process or as a separate process. func postDeferDecision(ctx context.Context, agentURL, token string, d corechannels.ApprovalDecision) error { body, _ := json.Marshal(map[string]string{ - "decision": d.Decision, - "approver": d.Approver, - "note": d.Note, + "decision": d.Decision, + "approver": d.Approver, + "approver_email": d.ApproverEmail, // #313 — the runtime allowlist check keys on this + "note": d.Note, }) url := fmt.Sprintf("%s/tasks/%s/decisions", agentURL, d.TaskID) req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) diff --git a/forge-cli/cmd/run_test.go b/forge-cli/cmd/run_test.go index 9988911..8e89a6f 100644 --- a/forge-cli/cmd/run_test.go +++ b/forge-cli/cmd/run_test.go @@ -181,6 +181,62 @@ func TestDeferTimeoutWarnings(t *testing.T) { }) } +// TestDeferApproverWarnings covers the config-hygiene warning (#315 finding 2): +// a non-email approver entry (per-tool or default) can never match the +// email-keyed allowlist, so it's flagged at startup. +func TestDeferApproverWarnings(t *testing.T) { + cfg := types.DeferConfig{ + Enabled: true, + Tools: map[string]types.DeferToolConfig{ + "ok_tool": {Approvers: []string{"alice@corp.com"}}, // valid → no warn + "bad_tool": {Approvers: []string{"bob", "carol@corp.com"}}, // "bob" → warn + "typo": {Approvers: []string{"dave@corp"}}, // no dot in domain → warn + }, + DefaultApprovers: []string{"@handle"}, // bare handle → warn + } + + warns := deferApproverWarnings(cfg) + if len(warns) != 3 { // bob + dave@corp + @handle + t.Fatalf("expected 3 approver warnings, got %d: %v", len(warns), warns) + } + joined := strings.Join(warns, "\n") + for _, want := range []string{`"bob"`, `"dave@corp"`, `"@handle"`, "default_approvers"} { + if !strings.Contains(joined, want) { + t.Errorf("approver warning missing %q:\n%s", want, joined) + } + } + if strings.Contains(joined, "alice@corp.com") { + t.Errorf("must not warn for a valid email:\n%s", joined) + } + + t.Run("disabled → no warnings", func(t *testing.T) { + off := cfg + off.Enabled = false + if w := deferApproverWarnings(off); w != nil { + t.Errorf("disabled defer must not warn, got %v", w) + } + }) + + t.Run("looksLikeEmail edge cases", func(t *testing.T) { + for in, want := range map[string]bool{ + "a@b.com": true, + "a.b@c.co.uk": true, + "bob": false, + "a@b": false, // no dot in domain + "@b.com": false, // empty local + "a@": false, // empty domain + "a@@b.com": false, // two @ + "a@.com": false, // domain starts with dot + "a@b.": false, // domain ends with dot + " a@b.com ": true, // trimmed + } { + if got := looksLikeEmail(in); got != want { + t.Errorf("looksLikeEmail(%q) = %v, want %v", in, got, want) + } + } + }) +} + // TestSyncRequestTimeout_IsReferenced guards the single-source-of-truth: the // warning threshold uses the exported router constant. func TestSyncRequestTimeout_IsReferenced(t *testing.T) { diff --git a/forge-cli/runtime/defer.go b/forge-cli/runtime/defer.go index 2d5a6cf..296da00 100644 --- a/forge-cli/runtime/defer.go +++ b/forge-cli/runtime/defer.go @@ -140,11 +140,12 @@ func (r *Runner) registerDeferHook(hooks *coreruntime.HookRegistry, store TaskSt CorrelationID: hctx.CorrelationID, TaskID: hctx.TaskID, Fields: map[string]any{ - "tool": hctx.ToolName, - "decision": string(res.Decision), - "approver": res.Approver, - "note": res.Note, - "wait_ms": waitMs, + "tool": hctx.ToolName, + "decision": string(res.Decision), + "approver": res.Approver, + "approver_email": res.ApproverEmail, + "note": res.Note, + "wait_ms": waitMs, }, }) return nil @@ -154,11 +155,12 @@ func (r *Runner) registerDeferHook(hooks *coreruntime.HookRegistry, store TaskSt CorrelationID: hctx.CorrelationID, TaskID: hctx.TaskID, Fields: map[string]any{ - "tool": hctx.ToolName, - "decision": string(res.Decision), - "approver": res.Approver, - "note": res.Note, - "wait_ms": waitMs, + "tool": hctx.ToolName, + "decision": string(res.Decision), + "approver": res.Approver, + "approver_email": res.ApproverEmail, + "note": res.Note, + "wait_ms": waitMs, }, }) return fmt.Errorf("defer: rejected by %s: %s", res.Approver, res.Note) @@ -204,11 +206,31 @@ func resolveDeferSpec(cfg types.DeferConfig, tool types.DeferToolConfig, hctx *c "{tool}", hctx.ToolName, "{args}", hctx.ToolInput, ).Replace(ctxTemplate) + approvers := tool.Approvers + if len(approvers) == 0 { + approvers = cfg.DefaultApprovers + } return deferengine.Spec{ To: to, Timeout: timeout, ContextForApprover: rendered, + Approvers: normalizeApprovers(approvers), + } +} + +// normalizeApprovers lowercases + trims the allowlist emails and drops blanks, +// so the engine's case-insensitive membership check is a plain compare. +func normalizeApprovers(in []string) []string { + if len(in) == 0 { + return nil + } + out := make([]string, 0, len(in)) + for _, e := range in { + if e = strings.ToLower(strings.TrimSpace(e)); e != "" { + out = append(out, e) + } } + return out } // truncateForAudit caps context strings on the audit event so a @@ -245,7 +267,7 @@ func (r *Runner) registerDecisionsEndpoint(srv *server.Server, auditLogger *core if r.deferEngine == nil { return } - srv.RegisterHTTPHandler("POST /tasks/{id}/decisions", makeDecisionsHandler(r.deferEngine)) + srv.RegisterHTTPHandler("POST /tasks/{id}/decisions", makeDecisionsHandler(r.deferEngine, auditLogger)) } // makeDecisionsHandler returns the http.HandlerFunc for @@ -254,7 +276,7 @@ func (r *Runner) registerDecisionsEndpoint(srv *server.Server, auditLogger *core // status-code paths (400 malformed / 404 no-pending-deferral / // 409 Peek-vs-Resolve race / 200 resolved) without spinning up // the full a2a server. -func makeDecisionsHandler(engine *deferengine.Engine) http.HandlerFunc { +func makeDecisionsHandler(engine *deferengine.Engine, auditLogger *coreruntime.AuditLogger) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { taskID := req.PathValue("id") if taskID == "" { @@ -262,9 +284,10 @@ func makeDecisionsHandler(engine *deferengine.Engine) http.HandlerFunc { return } var body struct { - Decision string `json:"decision"` - Approver string `json:"approver"` - Note string `json:"note"` + Decision string `json:"decision"` + Approver string `json:"approver"` + ApproverEmail string `json:"approver_email"` // #313 — for the allowlist check + Note string `json:"note"` } if err := json.NewDecoder(req.Body).Decode(&body); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid body: " + err.Error()}) @@ -277,16 +300,43 @@ func makeDecisionsHandler(engine *deferengine.Engine) http.HandlerFunc { }) return } - if _, ok := engine.Peek(taskID); !ok { + handle, ok := engine.Peek(taskID) + if !ok { writeJSON(w, http.StatusNotFound, map[string]string{ "error": "no pending deferral for task", }) return } + // #313 — approver allowlist. When the tool declares `approvers`, the + // resolved email must be present and listed; fail closed (an empty + // email against a non-empty allowlist is refused). The deferral stays + // pending so a real approver can still act. No-op when no allowlist is + // configured (empty Approvers → IsApprover always true). + if !handle.IsApprover(body.ApproverEmail) { + if auditLogger != nil { + auditLogger.Emit(coreruntime.AuditEvent{ + Event: coreruntime.AuditTaskDeferredDecision, + TaskID: taskID, + Fields: map[string]any{ + "tool": handle.Tool(), + "decision": "denied", + "reason": "unauthorized_approver", + "authorized": false, + "approver": body.Approver, + "approver_email": body.ApproverEmail, + }, + }) + } + writeJSON(w, http.StatusForbidden, map[string]string{ + "error": "approver not authorized to resolve this deferral", + }) + return + } if err := engine.Resolve(taskID, deferengine.Resolution{ - Decision: decision, - Approver: body.Approver, - Note: body.Note, + Decision: decision, + Approver: body.Approver, + ApproverEmail: body.ApproverEmail, + Note: body.Note, }); err != nil { // Race: another decision landed in the window between // Peek and Resolve. 409 is the honest signal. diff --git a/forge-cli/runtime/defer_test.go b/forge-cli/runtime/defer_test.go index 3d9b137..b76919e 100644 --- a/forge-cli/runtime/defer_test.go +++ b/forge-cli/runtime/defer_test.go @@ -305,7 +305,7 @@ func TestDecisionsEndpoint_HappyPath(t *testing.T) { req.SetPathValue("id", "task-http") rec := httptest.NewRecorder() - handler := makeDecisionsHandler(engine) + handler := makeDecisionsHandler(engine, coreruntime.NewAuditLogger(discardWriter{})) handler(rec, req) if rec.Code != 200 { @@ -328,7 +328,7 @@ func TestDecisionsEndpoint_NotFound(t *testing.T) { req := httptest.NewRequest("POST", "/tasks/nope/decisions", strings.NewReader(string(body))) req.SetPathValue("id", "nope") rec := httptest.NewRecorder() - makeDecisionsHandler(engine)(rec, req) + makeDecisionsHandler(engine, coreruntime.NewAuditLogger(discardWriter{}))(rec, req) if rec.Code != 404 { t.Errorf("status: got %d want 404", rec.Code) } @@ -343,7 +343,7 @@ func TestDecisionsEndpoint_BadDecision(t *testing.T) { req := httptest.NewRequest("POST", "/tasks/task-x/decisions", strings.NewReader(string(body))) req.SetPathValue("id", "task-x") rec := httptest.NewRecorder() - makeDecisionsHandler(engine)(rec, req) + makeDecisionsHandler(engine, coreruntime.NewAuditLogger(discardWriter{}))(rec, req) if rec.Code != 400 { t.Errorf("status: got %d want 400", rec.Code) } @@ -419,3 +419,85 @@ func TestDeferHook_NotifierFailureNonFatal(t *testing.T) { } var errTestDelivery = fmt.Errorf("simulated delivery failure") + +// TestDecisionsEndpoint_ApproverAllowlist covers #313 enforcement at the +// decisions endpoint: allowlisted email resolves (case-insensitive), +// non-listed is refused (deferral stays pending), empty email fails closed, +// and no allowlist allows anyone. +func TestDecisionsEndpoint_ApproverAllowlist(t *testing.T) { + post := func(engine *deferengine.Engine, taskID, email string) *httptest.ResponseRecorder { + body, _ := json.Marshal(map[string]string{"decision": "approve", "approver": "who", "approver_email": email}) + req := httptest.NewRequest("POST", "/tasks/"+taskID+"/decisions", strings.NewReader(string(body))) + req.SetPathValue("id", taskID) + rec := httptest.NewRecorder() + makeDecisionsHandler(engine, coreruntime.NewAuditLogger(discardWriter{}))(rec, req) + return rec + } + allow := deferengine.Spec{Timeout: 5 * time.Second, Approvers: []string{"alice@corp.com"}} + + t.Run("allowlisted email resolves (case-insensitive)", func(t *testing.T) { + engine := deferengine.New() + _, _ = engine.Register("t1", "cli_execute", allow) + if rec := post(engine, "t1", "Alice@Corp.com"); rec.Code != 200 { + t.Errorf("allowlisted approver: got %d want 200; %s", rec.Code, rec.Body.String()) + } + }) + + t.Run("non-listed refused, deferral stays pending", func(t *testing.T) { + engine := deferengine.New() + _, _ = engine.Register("t2", "cli_execute", allow) + if rec := post(engine, "t2", "mallory@evil.com"); rec.Code != 403 { + t.Errorf("non-listed approver: got %d want 403", rec.Code) + } + if _, ok := engine.Peek("t2"); !ok { + t.Error("deferral must stay pending after a refused approval") + } + _ = engine.Resolve("t2", deferengine.Resolution{Decision: deferengine.DecisionTimeout}) + }) + + t.Run("empty email fails closed", func(t *testing.T) { + engine := deferengine.New() + _, _ = engine.Register("t3", "cli_execute", allow) + if rec := post(engine, "t3", ""); rec.Code != 403 { + t.Errorf("empty email vs allowlist must fail closed: got %d want 403", rec.Code) + } + _ = engine.Resolve("t3", deferengine.Resolution{Decision: deferengine.DecisionTimeout}) + }) + + t.Run("no allowlist allows anyone", func(t *testing.T) { + engine := deferengine.New() + _, _ = engine.Register("t4", "cli_execute", deferengine.Spec{Timeout: 5 * time.Second}) + if rec := post(engine, "t4", ""); rec.Code != 200 { + t.Errorf("no allowlist: got %d want 200", rec.Code) + } + }) +} + +// TestResolveDeferSpec_Approvers pins normalization + the default fallback. +func TestResolveDeferSpec_Approvers(t *testing.T) { + hctx := &coreruntime.HookContext{ToolName: "cli_execute", ToolInput: "{}"} + + t.Run("per-tool, normalized", func(t *testing.T) { + spec := resolveDeferSpec(types.DeferConfig{}, types.DeferToolConfig{ + Approvers: []string{" Alice@Corp.com ", "BOB@corp.com", " "}, + }, hctx) + want := []string{"alice@corp.com", "bob@corp.com"} + if len(spec.Approvers) != 2 || spec.Approvers[0] != want[0] || spec.Approvers[1] != want[1] { + t.Errorf("approvers = %v, want %v (lowercased, trimmed, blanks dropped)", spec.Approvers, want) + } + }) + + t.Run("falls back to default_approvers", func(t *testing.T) { + spec := resolveDeferSpec(types.DeferConfig{DefaultApprovers: []string{"OnCall@corp.com"}}, + types.DeferToolConfig{}, hctx) + if len(spec.Approvers) != 1 || spec.Approvers[0] != "oncall@corp.com" { + t.Errorf("approvers = %v, want [oncall@corp.com]", spec.Approvers) + } + }) + + t.Run("empty → no allowlist", func(t *testing.T) { + if spec := resolveDeferSpec(types.DeferConfig{}, types.DeferToolConfig{}, hctx); spec.Approvers != nil { + t.Errorf("no approvers → nil, got %v", spec.Approvers) + } + }) +} diff --git a/forge-core/channels/plugin.go b/forge-core/channels/plugin.go index d0ee3a0..90cf209 100644 --- a/forge-core/channels/plugin.go +++ b/forge-core/channels/plugin.go @@ -78,8 +78,13 @@ type ApprovalRequest struct { type ApprovalDecision struct { TaskID string // must match the ApprovalRequest.TaskID Decision string // "approve" | "reject" - Approver string // who acted (platform user id / name) - Note string // optional justification + Approver string // who acted (platform user id / name), for audit + // ApproverEmail is the approver's resolved email (#313). Adapters populate + // it (e.g. Slack via users.info) so the runtime's approver allowlist check + // is adapter-agnostic. Empty when the adapter couldn't resolve it — the + // runtime fails closed against a configured allowlist. + ApproverEmail string + Note string // optional justification } // ApprovalResolver is invoked by an adapter when an approver acts on a diff --git a/forge-core/security/deferpolicy/defer.go b/forge-core/security/deferpolicy/defer.go index 338bc5f..ba815e9 100644 --- a/forge-core/security/deferpolicy/defer.go +++ b/forge-core/security/deferpolicy/defer.go @@ -23,6 +23,8 @@ import ( "context" "errors" "fmt" + "slices" + "strings" "sync" "time" ) @@ -41,10 +43,11 @@ const ( // deferral. Emitted onto Handle.wait, consumed by the executor // goroutine when it resumes. type Resolution struct { - Decision Decision - Approver string // free-form; typically a user id or email - Note string // optional operator-supplied justification - At time.Time + Decision Decision + Approver string // free-form; typically a user id or name + ApproverEmail string // resolved approver email (#313), when available + Note string // optional operator-supplied justification + At time.Time } // Handle represents a pending deferral. The executor obtains one @@ -73,6 +76,26 @@ type Spec struct { To string Timeout time.Duration ContextForApprover string + // Approvers is the per-tool approver allowlist (#313), normalized to + // lowercase emails. Empty → no allowlist (any approver may resolve). + // Non-empty → the decisions endpoint requires the approver's email to + // be present and in this set (fail-closed). + Approvers []string +} + +// IsApprover reports whether email is authorized to resolve this deferral. +// An empty allowlist authorizes anyone (pre-#313 behavior). A non-empty +// allowlist requires a non-empty email that is a member — fail-closed. +// Comparison is case-insensitive. +func (h *Handle) IsApprover(email string) bool { + if len(h.spec.Approvers) == 0 { + return true + } + email = strings.ToLower(strings.TrimSpace(email)) + if email == "" { + return false + } + return slices.Contains(h.spec.Approvers, email) } // TaskID returns the task the deferral is for. Used by the runner diff --git a/forge-core/security/deferpolicy/defer_test.go b/forge-core/security/deferpolicy/defer_test.go index c21d29c..5798535 100644 --- a/forge-core/security/deferpolicy/defer_test.go +++ b/forge-core/security/deferpolicy/defer_test.go @@ -236,3 +236,26 @@ func TestDefaultTimeout(t *testing.T) { h.Deadline(), e.now()) } } + +// TestHandle_IsApprover pins the #313 allowlist membership check: empty +// allowlist authorizes anyone; a non-empty one requires a listed email +// (case-insensitive) and fails closed on an empty email. +func TestHandle_IsApprover(t *testing.T) { + e := New() + + noList, _ := e.Register("t-none", "cli_execute", Spec{Timeout: time.Minute}) + if !noList.IsApprover("") || !noList.IsApprover("anyone@x.com") { + t.Error("empty allowlist must authorize anyone") + } + + list, _ := e.Register("t-list", "cli_execute", Spec{Timeout: time.Minute, Approvers: []string{"alice@corp.com"}}) + if !list.IsApprover("alice@corp.com") || !list.IsApprover(" ALICE@Corp.com ") { + t.Error("listed email (any case/whitespace) must be authorized") + } + if list.IsApprover("mallory@evil.com") { + t.Error("non-listed email must be refused") + } + if list.IsApprover("") { + t.Error("empty email against a non-empty allowlist must fail closed") + } +} diff --git a/forge-core/types/config.go b/forge-core/types/config.go index f7b00bd..2ebff84 100644 --- a/forge-core/types/config.go +++ b/forge-core/types/config.go @@ -366,6 +366,11 @@ type DeferConfig struct { // legal — the deferral fires and the audit event carries "" for // operators to route on downstream. DefaultTo string `yaml:"default_to,omitempty"` + + // DefaultApprovers is the approver email allowlist applied when a + // tool's Approvers is unset (#313). Empty → no allowlist (any approver + // may resolve). See DeferToolConfig.Approvers. + DefaultApprovers []string `yaml:"default_approvers,omitempty"` } // Validate returns an error when the config would silently no-op or @@ -399,6 +404,14 @@ type DeferToolConfig struct { // at hook time. When empty, the payload is // `"tool={tool} args={args}"`. ContextTemplate string `yaml:"context_template,omitempty"` + + // Approvers is the email allowlist authorized to resolve this tool's + // deferrals (#313). Empty → falls back to DeferConfig.DefaultApprovers; + // if that is also empty there is NO allowlist and any approver may + // resolve (channel membership is the ACL). Non-empty → the decisions + // endpoint requires the approver's resolved email to be in this set + // (case-insensitive) and FAILS CLOSED when the email can't be resolved. + Approvers []string `yaml:"approvers,omitempty"` } // ObservabilityConfig groups telemetry-related sub-blocks. Today it diff --git a/forge-plugins/channels/slack/approvals.go b/forge-plugins/channels/slack/approvals.go index b745cac..406c87c 100644 --- a/forge-plugins/channels/slack/approvals.go +++ b/forge-plugins/channels/slack/approvals.go @@ -25,6 +25,14 @@ var _ channels.ApprovalDeliverer = (*Plugin)(nil) const ( approveActionID = "forge_defer_approve" rejectActionID = "forge_defer_reject" + + // rejectCallbackID identifies the reason-capture modal opened on a Reject + // click. A button click carries no free text, so Reject opens a modal + // (views.open) that prompts for a justification; the view_submission comes + // back with this callback_id and the typed reason becomes the decision Note. + rejectCallbackID = "forge_defer_reject_modal" + reasonBlockID = "forge_reject_reason_block" + reasonActionID = "forge_reject_reason" ) // SetApprovalResolver wires the callback invoked when an approver clicks a @@ -208,8 +216,9 @@ func buildApprovalPayload(req channels.ApprovalRequest) map[string]any { // slackInteraction is the subset of Slack's block_actions payload we consume. type slackInteraction struct { - Type string `json:"type"` - User struct { + Type string `json:"type"` + TriggerID string `json:"trigger_id"` // opens the reject-reason modal (views.open) + User struct { ID string `json:"id"` Username string `json:"username"` Name string `json:"name"` @@ -230,14 +239,14 @@ type slackInteraction struct { // block_actions payload. Pure + testable. Returns ok=false for any payload // that isn't one of our approval buttons (other apps' interactions, non-button // types) so the caller ignores it. `channelID`/`msgTS` locate the message for -// the outcome update. -func parseApprovalInteraction(payload []byte) (dec channels.ApprovalDecision, channelID, msgTS string, ok bool) { +// the outcome update; `triggerID` opens the reject-reason modal. +func parseApprovalInteraction(payload []byte) (dec channels.ApprovalDecision, userID, channelID, msgTS, triggerID string, ok bool) { var in slackInteraction if err := json.Unmarshal(payload, &in); err != nil { - return dec, "", "", false + return dec, "", "", "", "", false } if in.Type != "block_actions" || len(in.Actions) == 0 { - return dec, "", "", false + return dec, "", "", "", "", false } a := in.Actions[0] var decision string @@ -247,10 +256,10 @@ func parseApprovalInteraction(payload []byte) (dec channels.ApprovalDecision, ch case rejectActionID: decision = "reject" default: - return dec, "", "", false // not our button + return dec, "", "", "", "", false // not our button } if a.Value == "" { - return dec, "", "", false + return dec, "", "", "", "", false } approver := in.User.Username if approver == "" { @@ -263,16 +272,126 @@ func parseApprovalInteraction(payload []byte) (dec channels.ApprovalDecision, ch TaskID: a.Value, Decision: decision, Approver: approver, - }, in.Channel.ID, in.Message.TS, true + }, in.User.ID, in.Channel.ID, in.Message.TS, in.TriggerID, true +} + +// resolveUserEmail resolves a Slack user id to their email via users.info +// (cached), for the DEFER approver allowlist (#313). Requires the +// users:read.email scope. Returns an error when the email can't be determined +// (missing scope, guest without an email) — the caller leaves ApproverEmail +// empty and the runtime fails closed against a configured allowlist. +func (p *Plugin) resolveUserEmail(ctx context.Context, userID string) (string, error) { + if userID == "" { + return "", fmt.Errorf("empty user id") + } + p.userMu.Lock() + if e, ok := p.userEmailCache[userID]; ok { + p.userMu.Unlock() + return e, nil + } + p.userMu.Unlock() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.apiBase+"/users.info?user="+url.QueryEscape(userID), nil) + if err != nil { + return "", err + } + req.Header.Set("Authorization", "Bearer "+p.botToken) + resp, err := p.client.Do(req) + if err != nil { + return "", err + } + defer func() { _ = resp.Body.Close() }() + var out struct { + OK bool `json:"ok"` + Error string `json:"error"` + User struct { + Profile struct { + Email string `json:"email"` + } `json:"profile"` + } `json:"user"` + } + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + return "", fmt.Errorf("users.info decode: %w", err) + } + if !out.OK { + return "", fmt.Errorf("users.info: %s", out.Error) + } + email := strings.ToLower(strings.TrimSpace(out.User.Profile.Email)) + if email == "" { + return "", fmt.Errorf("no email on profile (guest, or missing users:read.email scope)") + } + p.userMu.Lock() + if p.userEmailCache == nil { + p.userEmailCache = map[string]string{} + } + p.userEmailCache[userID] = email + p.userMu.Unlock() + return email, nil } -// handleInteractive resolves an approval button click. No-op for interactions -// that aren't ours. Best-effort updates the source message with the outcome. +// handleInteractive dispatches an interactive envelope. Approve resolves the +// deferral immediately; Reject opens a reason-capture modal whose submission +// (view_submission) resolves it with the typed justification. No-op for +// interactions that aren't ours. func (p *Plugin) handleInteractive(ctx context.Context, payload []byte) error { - dec, channelID, msgTS, ok := parseApprovalInteraction(payload) + var probe struct { + Type string `json:"type"` + } + if err := json.Unmarshal(payload, &probe); err != nil { + return nil + } + switch probe.Type { + case "view_submission": + return p.handleRejectSubmission(ctx, payload) + default: // block_actions (and anything else falls through to the no-op) + return p.handleBlockAction(ctx, payload) + } +} + +// handleBlockAction routes an Approve/Reject button click. Approve resolves +// straight away; Reject opens the reason modal (falling back to a +// reason-less reject only if the modal can't be opened, so a reject is never +// lost to a transient views.open failure). +func (p *Plugin) handleBlockAction(ctx context.Context, payload []byte) error { + dec, userID, channelID, msgTS, triggerID, ok := parseApprovalInteraction(payload) if !ok { return nil // not a forge approval interaction; ignore quietly } + if dec.Decision == "reject" { + meta := rejectMeta{TaskID: dec.TaskID, ChannelID: channelID, MsgTS: msgTS} + if err := p.openRejectModal(ctx, triggerID, meta); err != nil { + p.logWarn("could not open reject-reason modal; rejecting without a reason", + map[string]any{"task": dec.TaskID, "error": err.Error()}) + return p.resolveDecision(ctx, dec, userID, channelID, msgTS) + } + return nil // resolution happens on modal submit + } + return p.resolveDecision(ctx, dec, userID, channelID, msgTS) +} + +// handleRejectSubmission resolves the deferral from a reject-modal submission, +// carrying the typed reason as the decision Note. +func (p *Plugin) handleRejectSubmission(ctx context.Context, payload []byte) error { + dec, userID, meta, ok := parseRejectSubmission(payload) + if !ok { + return nil // not our modal + } + return p.resolveDecision(ctx, dec, userID, meta.ChannelID, meta.MsgTS) +} + +// resolveDecision resolves the email for the runtime allowlist (#313), invokes +// the wired resolver, and best-effort updates the source message with the +// outcome. Shared by the approve, reject-with-reason, and reason-less-reject +// fallback paths. +func (p *Plugin) resolveDecision(ctx context.Context, dec channels.ApprovalDecision, userID, channelID, msgTS string) error { + // Best effort: on failure ApproverEmail stays empty and the runtime fails + // closed if the tool declares an allowlist. We still log it so a missing + // users:read.email scope is visible. + if email, err := p.resolveUserEmail(ctx, userID); err == nil { + dec.ApproverEmail = email + } else { + p.logWarn("could not resolve approver email", map[string]any{"user": userID, "error": err.Error()}) + } if p.approvalResolver == nil { return fmt.Errorf("approval click for task %s but no resolver wired", dec.TaskID) } @@ -286,6 +405,132 @@ func (p *Plugin) handleInteractive(ctx context.Context, payload []byte) error { return nil } +// rejectMeta is round-tripped through the modal's private_metadata so the +// view_submission can locate the deferral (task id) and the source message. +type rejectMeta struct { + TaskID string `json:"task_id"` + ChannelID string `json:"channel_id"` + MsgTS string `json:"msg_ts"` +} + +// viewSubmission is the subset of Slack's view_submission payload we consume. +type viewSubmission struct { + Type string `json:"type"` + User struct { + ID string `json:"id"` + Username string `json:"username"` + Name string `json:"name"` + } `json:"user"` + View struct { + CallbackID string `json:"callback_id"` + PrivateMetadata string `json:"private_metadata"` + State struct { + Values map[string]map[string]struct { + Value string `json:"value"` + } `json:"values"` + } `json:"state"` + } `json:"view"` +} + +// parseRejectSubmission extracts the reject decision + reason from a +// view_submission. Pure + testable. ok=false for any submission that isn't our +// reject modal (wrong callback_id, unparseable metadata) so the caller ignores +// it. +func parseRejectSubmission(payload []byte) (dec channels.ApprovalDecision, userID string, meta rejectMeta, ok bool) { + var in viewSubmission + if err := json.Unmarshal(payload, &in); err != nil { + return dec, "", meta, false + } + if in.Type != "view_submission" || in.View.CallbackID != rejectCallbackID { + return dec, "", meta, false + } + if err := json.Unmarshal([]byte(in.View.PrivateMetadata), &meta); err != nil || meta.TaskID == "" { + return dec, "", meta, false + } + var reason string + if block, ok := in.View.State.Values[reasonBlockID]; ok { + reason = strings.TrimSpace(block[reasonActionID].Value) + } + approver := in.User.Username + if approver == "" { + approver = in.User.Name + } + if approver == "" { + approver = in.User.ID + } + return channels.ApprovalDecision{ + TaskID: meta.TaskID, + Decision: "reject", + Approver: approver, + Note: reason, + }, in.User.ID, meta, true +} + +// openRejectModal opens the reason-capture modal (views.open) using the click's +// trigger_id. The deferral stays pending until the modal is submitted. +func (p *Plugin) openRejectModal(ctx context.Context, triggerID string, meta rejectMeta) error { + if triggerID == "" { + return fmt.Errorf("empty trigger_id") + } + metaJSON, err := json.Marshal(meta) + if err != nil { + return err + } + body, _ := json.Marshal(map[string]any{ + "trigger_id": triggerID, + "view": buildRejectModal(string(metaJSON)), + }) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, p.apiBase+"/views.open", bytes.NewReader(body)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+p.botToken) + resp, err := p.client.Do(req) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + var out struct { + OK bool `json:"ok"` + Error string `json:"error"` + } + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + return fmt.Errorf("views.open decode: %w", err) + } + if !out.OK { + return fmt.Errorf("views.open: %s", out.Error) + } + return nil +} + +// buildRejectModal renders the views.open modal (minus trigger_id). Pure so +// tests can assert the shape. `privateMetadata` round-trips the rejectMeta so +// the submission can locate the deferral and source message. The reason input +// is required so a reject always carries a justification. +func buildRejectModal(privateMetadata string) map[string]any { + return map[string]any{ + "type": "modal", + "callback_id": rejectCallbackID, + "private_metadata": privateMetadata, + "title": map[string]any{"type": "plain_text", "text": "Reject action"}, + "submit": map[string]any{"type": "plain_text", "text": "Reject"}, + "close": map[string]any{"type": "plain_text", "text": "Cancel"}, + "blocks": []any{ + map[string]any{ + "type": "input", + "block_id": reasonBlockID, + "label": map[string]any{"type": "plain_text", "text": "Reason for rejection"}, + "element": map[string]any{ + "type": "plain_text_input", + "action_id": reasonActionID, + "multiline": true, + }, + }, + }, + } +} + // approvalOutcomeText is the message body that replaces the buttons after a // decision. func approvalOutcomeText(d channels.ApprovalDecision) string { diff --git a/forge-plugins/channels/slack/approvals_test.go b/forge-plugins/channels/slack/approvals_test.go index 834b846..6cbdda1 100644 --- a/forge-plugins/channels/slack/approvals_test.go +++ b/forge-plugins/channels/slack/approvals_test.go @@ -52,8 +52,9 @@ func TestBuildApprovalPayload(t *testing.T) { func interactionJSON(actionID, taskID, username string) []byte { m := map[string]any{ - "type": "block_actions", - "user": map[string]any{"id": "U1", "username": username, "name": "Alice N"}, + "type": "block_actions", + "trigger_id": "trig-1", + "user": map[string]any{"id": "U1", "username": username, "name": "Alice N"}, "actions": []any{map[string]any{ "action_id": actionID, "value": taskID, "type": "button", }}, @@ -68,38 +69,44 @@ func interactionJSON(actionID, taskID, username string) []byte { // the ignore paths (not our buttons / wrong shape). func TestParseApprovalInteraction(t *testing.T) { t.Run("approve", func(t *testing.T) { - d, ch, ts, ok := parseApprovalInteraction(interactionJSON(approveActionID, "task-7", "alice")) + d, uid, ch, ts, _, ok := parseApprovalInteraction(interactionJSON(approveActionID, "task-7", "alice")) if !ok || d.Decision != "approve" || d.TaskID != "task-7" || d.Approver != "alice" { t.Fatalf("got %+v ok=%v", d, ok) } + if uid != "U1" { + t.Errorf("user id = %q, want U1 (needed for email resolution)", uid) + } if ch != "C1" || ts != "1700000000.000100" { t.Errorf("message locator wrong: ch=%q ts=%q", ch, ts) } }) - t.Run("reject", func(t *testing.T) { - d, _, _, ok := parseApprovalInteraction(interactionJSON(rejectActionID, "task-7", "bob")) + t.Run("reject carries trigger id for the modal", func(t *testing.T) { + d, _, _, _, trigger, ok := parseApprovalInteraction(interactionJSON(rejectActionID, "task-7", "bob")) if !ok || d.Decision != "reject" { t.Fatalf("got %+v ok=%v", d, ok) } + if trigger != "trig-1" { + t.Errorf("trigger_id = %q, want trig-1 (needed to open the reason modal)", trigger) + } }) t.Run("approver falls back to name then id", func(t *testing.T) { - d, _, _, _ := parseApprovalInteraction(interactionJSON(approveActionID, "t", "")) // no username + d, _, _, _, _, _ := parseApprovalInteraction(interactionJSON(approveActionID, "t", "")) // no username if d.Approver != "Alice N" { t.Errorf("approver fallback = %q, want name", d.Approver) } }) t.Run("not our button ignored", func(t *testing.T) { - if _, _, _, ok := parseApprovalInteraction(interactionJSON("some_other_app_action", "t", "x")); ok { + if _, _, _, _, _, ok := parseApprovalInteraction(interactionJSON("some_other_app_action", "t", "x")); ok { t.Error("a non-forge action_id must be ignored") } }) t.Run("wrong type ignored", func(t *testing.T) { - if _, _, _, ok := parseApprovalInteraction([]byte(`{"type":"view_submission"}`)); ok { + if _, _, _, _, _, ok := parseApprovalInteraction([]byte(`{"type":"view_submission"}`)); ok { t.Error("non-block_actions must be ignored") } }) t.Run("empty task id ignored", func(t *testing.T) { - if _, _, _, ok := parseApprovalInteraction(interactionJSON(approveActionID, "", "x")); ok { + if _, _, _, _, _, ok := parseApprovalInteraction(interactionJSON(approveActionID, "", "x")); ok { t.Error("empty value (task id) must be ignored") } }) @@ -277,3 +284,219 @@ func TestDeliverApproval_EmptyTarget(t *testing.T) { t.Fatal("expected an error for an empty target channel") } } + +// TestResolveUserEmail covers users.info resolution (#313): success + cache, +// no-email, and missing-scope error. +func TestResolveUserEmail(t *testing.T) { + t.Run("resolves lowercased + caches", func(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/users.info") { + calls++ + _, _ = w.Write([]byte(`{"ok":true,"user":{"profile":{"email":"Alice@Corp.com"}}}`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + p := New() + p.apiBase = srv.URL + p.botToken = "xoxb-test" + for range 2 { + email, err := p.resolveUserEmail(context.Background(), "U1") + if err != nil || email != "alice@corp.com" { + t.Fatalf("resolveUserEmail = (%q,%v), want alice@corp.com", email, err) + } + } + if calls != 1 { + t.Errorf("users.info called %d times, want 1 (cached)", calls) + } + }) + + t.Run("no email fails", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"ok":true,"user":{"profile":{"email":""}}}`)) + })) + defer srv.Close() + p := New() + p.apiBase = srv.URL + p.botToken = "x" + if _, err := p.resolveUserEmail(context.Background(), "U2"); err == nil { + t.Error("expected an error when no email is on the profile") + } + }) + + t.Run("missing scope surfaces", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"ok":false,"error":"missing_scope"}`)) + })) + defer srv.Close() + p := New() + p.apiBase = srv.URL + p.botToken = "x" + if _, err := p.resolveUserEmail(context.Background(), "U3"); err == nil || !strings.Contains(err.Error(), "missing_scope") { + t.Errorf("expected the missing_scope error, got %v", err) + } + }) +} + +// TestHandleInteractive_AttachesEmail: a click resolves the approver's email +// and attaches it to the decision passed to the runtime (#313). +func TestHandleInteractive_AttachesEmail(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/users.info") { + _, _ = w.Write([]byte(`{"ok":true,"user":{"profile":{"email":"carol@corp.com"}}}`)) + return + } + w.WriteHeader(http.StatusOK) // chat.update + })) + defer srv.Close() + + var got channels.ApprovalDecision + p := New() + p.apiBase = srv.URL + p.botToken = "xoxb-test" + p.SetApprovalResolver(func(_ context.Context, d channels.ApprovalDecision) error { got = d; return nil }) + + if err := p.handleInteractive(context.Background(), interactionJSON(approveActionID, "task-9", "carol")); err != nil { + t.Fatalf("handleInteractive: %v", err) + } + if got.ApproverEmail != "carol@corp.com" { + t.Errorf("ApproverEmail = %q, want carol@corp.com", got.ApproverEmail) + } +} + +// rejectSubmissionJSON builds a view_submission payload for the reject modal. +func rejectSubmissionJSON(callbackID, meta, reason string) []byte { + m := map[string]any{ + "type": "view_submission", + "user": map[string]any{"id": "U1", "username": "dave"}, + "view": map[string]any{ + "callback_id": callbackID, + "private_metadata": meta, + "state": map[string]any{ + "values": map[string]any{ + reasonBlockID: map[string]any{ + reasonActionID: map[string]any{"type": "plain_text_input", "value": reason}, + }, + }, + }, + }, + } + b, _ := json.Marshal(m) + return b +} + +// TestBuildRejectModal pins the modal shape: our callback_id, the round-tripped +// private_metadata, and the required reason input. +func TestBuildRejectModal(t *testing.T) { + raw, _ := json.Marshal(buildRejectModal(`{"task_id":"task-5"}`)) + blob := string(raw) + for _, want := range []string{ + `"type":"modal"`, + `"callback_id":"` + rejectCallbackID + `"`, + `"private_metadata":"{\"task_id\":\"task-5\"}"`, + `"block_id":"` + reasonBlockID + `"`, + `"action_id":"` + reasonActionID + `"`, + `"multiline":true`, + } { + if !strings.Contains(blob, want) { + t.Errorf("reject modal missing %q\n%s", want, blob) + } + } +} + +// TestParseRejectSubmission covers the reason + metadata extraction and the +// ignore paths (wrong callback, unparseable/empty metadata). +func TestParseRejectSubmission(t *testing.T) { + meta := `{"task_id":"task-5","channel_id":"C1","msg_ts":"111.222"}` + t.Run("extracts reason and meta", func(t *testing.T) { + d, uid, m, ok := parseRejectSubmission(rejectSubmissionJSON(rejectCallbackID, meta, " too risky for prod ")) + if !ok || d.Decision != "reject" || d.TaskID != "task-5" || d.Approver != "dave" { + t.Fatalf("got %+v ok=%v", d, ok) + } + if d.Note != "too risky for prod" { + t.Errorf("note = %q, want trimmed reason", d.Note) + } + if uid != "U1" || m.ChannelID != "C1" || m.MsgTS != "111.222" { + t.Errorf("locator wrong: uid=%q meta=%+v", uid, m) + } + }) + t.Run("wrong callback ignored", func(t *testing.T) { + if _, _, _, ok := parseRejectSubmission(rejectSubmissionJSON("someone_elses_modal", meta, "x")); ok { + t.Error("a foreign callback_id must be ignored") + } + }) + t.Run("bad metadata ignored", func(t *testing.T) { + if _, _, _, ok := parseRejectSubmission(rejectSubmissionJSON(rejectCallbackID, "not-json", "x")); ok { + t.Error("unparseable private_metadata must be ignored") + } + }) +} + +// TestHandleBlockAction_RejectOpensModal: a Reject click opens the reason modal +// (views.open) and does NOT resolve the deferral yet. +func TestHandleBlockAction_RejectOpensModal(t *testing.T) { + var openedModal bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/views.open") { + openedModal = true + body, _ := io.ReadAll(r.Body) + if !strings.Contains(string(body), `"trigger_id":"trig-1"`) { + t.Errorf("views.open missing trigger id: %s", body) + } + } + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer srv.Close() + + var resolved bool + p := New() + p.apiBase = srv.URL + p.botToken = "xoxb-test" + p.SetApprovalResolver(func(context.Context, channels.ApprovalDecision) error { resolved = true; return nil }) + + if err := p.handleInteractive(context.Background(), interactionJSON(rejectActionID, "task-5", "dave")); err != nil { + t.Fatalf("handleInteractive: %v", err) + } + if !openedModal { + t.Error("expected a views.open to prompt for the reject reason") + } + if resolved { + t.Error("the deferral must NOT resolve until the modal is submitted") + } +} + +// TestHandleRejectSubmission: submitting the reason modal resolves the deferral +// as a reject carrying the typed reason and the resolved approver email. +func TestHandleRejectSubmission(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/users.info") { + _, _ = w.Write([]byte(`{"ok":true,"user":{"profile":{"email":"dave@corp.com"}}}`)) + return + } + w.WriteHeader(http.StatusOK) // chat.update + })) + defer srv.Close() + + var got channels.ApprovalDecision + p := New() + p.apiBase = srv.URL + p.botToken = "xoxb-test" + p.SetApprovalResolver(func(_ context.Context, d channels.ApprovalDecision) error { got = d; return nil }) + + meta := `{"task_id":"task-5","channel_id":"C1","msg_ts":"111.222"}` + payload := rejectSubmissionJSON(rejectCallbackID, meta, "denied: touches prod data") + if err := p.handleInteractive(context.Background(), payload); err != nil { + t.Fatalf("handleInteractive: %v", err) + } + if got.Decision != "reject" || got.TaskID != "task-5" { + t.Fatalf("resolver got %+v", got) + } + if got.Note != "denied: touches prod data" { + t.Errorf("note = %q, want the typed reason", got.Note) + } + if got.ApproverEmail != "dave@corp.com" { + t.Errorf("ApproverEmail = %q, want dave@corp.com", got.ApproverEmail) + } +} diff --git a/forge-plugins/channels/slack/slack.go b/forge-plugins/channels/slack/slack.go index 329bc47..4906cef 100644 --- a/forge-plugins/channels/slack/slack.go +++ b/forge-plugins/channels/slack/slack.go @@ -55,6 +55,11 @@ type Plugin struct { // (#310) so a repeated deferral to the same channel skips conversations.list. chanMu sync.Mutex chanIDCache map[string]string + + // userEmailCache memoizes resolved user id → email for the DEFER approver + // allowlist (#313) so a repeated approver skips users.info. + userMu sync.Mutex + userEmailCache map[string]string } // SetLogger wires a structured ops logger (channels.LoggerAware). Optional. @@ -73,10 +78,11 @@ func (p *Plugin) logWarn(msg string, fields map[string]any) { // New creates an uninitialised Slack plugin. func New() *Plugin { return &Plugin{ - client: &http.Client{Timeout: 30 * time.Second}, - apiBase: slackAPIBase, - dedupCache: make(map[string]time.Time), - chanIDCache: make(map[string]string), + client: &http.Client{Timeout: 30 * time.Second}, + apiBase: slackAPIBase, + dedupCache: make(map[string]time.Time), + chanIDCache: make(map[string]string), + userEmailCache: make(map[string]string), } }