Skip to content

feat(slack): Slack-button approvals for Cedar HITL gates (#112) #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
flamingquaks wants to merge 1 commit into from
Closed

feat(slack): Slack-button approvals for Cedar HITL gates (#112) #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cdk/src/constructs/slack-integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ export interface SlackIntegrationProps {
/** The DynamoDB task events table (must have DynamoDB Streams enabled). */
readonly taskEventsTable: dynamodb.ITable;

/** The Cedar HITL approvals table (optional — enables Slack-button
* approve/deny on `approval_requested` messages; issue #112). */
readonly taskApprovalsTable?: dynamodb.ITable;

/** The DynamoDB repo config table (optional — for repo onboarding checks). */
readonly repoTable?: dynamodb.ITable;

Expand Down Expand Up @@ -305,6 +309,19 @@ export class SlackIntegration extends Construct {
slackInteractionsFn.addToRolePolicy(readSlackSecretsPolicy);
props.taskTable.grantReadWriteData(slackInteractionsFn);
this.userMappingTable.grantReadData(slackInteractionsFn);
// Slack-button HITL approvals (issue #112): the interactions handler
// runs the same decision core as ApproveTaskFn/DenyTaskFn, which
// needs the approvals table (decision row + rate-limit counter) and
// the events table (approval_decision_recorded audit). Without the
// approvals table, approval clicks degrade to an ephemeral
// "not configured" message.
if (props.taskApprovalsTable) {
slackInteractionsFn.addEnvironment('TASK_APPROVALS_TABLE_NAME', props.taskApprovalsTable.tableName);
slackInteractionsFn.addEnvironment('TASK_EVENTS_TABLE_NAME', props.taskEventsTable.tableName);
slackInteractionsFn.addEnvironment('TASK_RETENTION_DAYS', String(props.taskRetentionDays ?? 90));
props.taskApprovalsTable.grantReadWriteData(slackInteractionsFn);
props.taskEventsTable.grantWriteData(slackInteractionsFn);
}

// --- Slash Command Acknowledger ---
const slackCommandsFn = new lambda.NodejsFunction(this, 'SlackCommandsFn', {
Expand Down
248 changes: 49 additions & 199 deletions cdk/src/handlers/approve-task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,46 +17,33 @@
* SOFTWARE.
*/

import { DynamoDBClient, TransactionCanceledException } from '@aws-sdk/client-dynamodb';
import { DynamoDBDocumentClient, PutCommand, TransactWriteCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb';
import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
import { DynamoDBDocumentClient } from '@aws-sdk/lib-dynamodb';
import type { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda';
import { ulid } from 'ulid';
import { approvalDecisionConfigFromEnv, processApprovalDecision } from './shared/approval-decision';
import { VALID_APPROVAL_SCOPE_PREFIXES, parseApprovalScope } from './shared/approval-scope';
import { extractUserId } from './shared/gateway';
import { logger } from './shared/logger';
import { formatMinuteBucket } from './shared/rate-limit';
import { ErrorCode, errorResponse, successResponse } from './shared/response';
import type { ApprovalRequest, ApprovalResponse, ApprovalScope } from './shared/types';

const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({}));
const TASK_TABLE_NAME = process.env.TASK_TABLE_NAME;
const TASK_APPROVALS_TABLE_NAME = process.env.TASK_APPROVALS_TABLE_NAME;
const EVENTS_TABLE_NAME = process.env.TASK_EVENTS_TABLE_NAME;
if (!TASK_TABLE_NAME || !TASK_APPROVALS_TABLE_NAME || !EVENTS_TABLE_NAME) {
throw new Error(
'approve-task handler requires TASK_TABLE_NAME, TASK_APPROVALS_TABLE_NAME, and TASK_EVENTS_TABLE_NAME env vars',
);
}
const APPROVE_RATE_LIMIT_PER_MINUTE = Number(process.env.APPROVE_RATE_LIMIT_PER_MINUTE ?? '30');
const AUDIT_EVENT_RETENTION_DAYS = Number(process.env.TASK_RETENTION_DAYS ?? '90');
// Validates the required table env vars at module load so a broken
// deployment fails loudly at cold start, matching the old behavior.
const DECISION_CONFIG = approvalDecisionConfigFromEnv();

/**
* POST /v1/tasks/{task_id}/approve — User-in-the-loop approval decision.
*
* Flow (design §7.1):
* 1. Auth — Cognito JWT `sub` → `caller_user_id` (verbatim).
* 2. Parse + validate body (`request_id`, optional `scope`).
* 3. Per-user per-minute rate limit (30/min).
* 4. Atomic cross-table `TransactWriteItems`:
* - Update approval row: `status PENDING → APPROVED`, guarded by
* `user_id = :caller` (ownership) AND `status = :pending`.
* - No-op update on TaskTable guarded by
* `status = AWAITING_APPROVAL AND awaiting_approval_request_id = :rid`.
* 5. On `TransactionCanceledException`, inspect per-item reasons to
* distinguish 404 vs 409 variants (prevents enumeration via the
* 404 collapse on ownership mismatch — finding #6).
* 6. Write `approval_decision_recorded` audit event to
* `TaskEventsTable` (IMPL-6).
* 3–6. Rate limit, atomic transition, cancel classification, and the
* audit event live in `shared/approval-decision.ts` —
* `processApprovalDecision` — shared with the Slack-button
* approvals path (issue #112) so the trust-critical core cannot
* drift between surfaces.
*
* Returns 202 with `{task_id, request_id, status: APPROVED, scope,
* decided_at}` on success.
Expand Down Expand Up @@ -118,125 +105,47 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
);
}

const nowIso = new Date().toISOString();
const nowEpoch = Math.floor(Date.now() / 1000);

// 3. Per-user per-minute rate limit. Uses a synthetic row in the
// approvals table keyed on `RATE#<user_id>#MINUTE#<yyyymmddhhmm>`
// so the existing grantReadWriteData wiring carries forward; TTL
// reaps the counter after ~120s.
const minuteBucket = formatMinuteBucket(new Date());
try {
await ddb.send(new UpdateCommand({
TableName: TASK_APPROVALS_TABLE_NAME,
Key: {
task_id: `RATE#${callerUserId}#APPROVE`,
request_id: `MINUTE#${minuteBucket}`,
},
UpdateExpression: 'ADD #count :one SET #ttl = :ttl',
ConditionExpression: 'attribute_not_exists(#count) OR #count < :max',
ExpressionAttributeNames: {
'#count': 'count',
'#ttl': 'ttl',
},
ExpressionAttributeValues: {
':one': 1,
':max': APPROVE_RATE_LIMIT_PER_MINUTE,
':ttl': nowEpoch + 120,
},
}));
} catch (err: unknown) {
const name = (err as { name?: string })?.name;
if (name === 'ConditionalCheckFailedException') {
return errorResponse(
429,
ErrorCode.RATE_LIMIT_EXCEEDED,
`Rate limit exceeded: at most ${APPROVE_RATE_LIMIT_PER_MINUTE} approve/deny decisions per minute.`,
requestId,
);
}
throw err;
// 3–5. Shared decision core (rate limit + atomic transition + audit).
const outcome = await processApprovalDecision(ddb, DECISION_CONFIG, {
taskId,
requestId: request_id,
callerUserId,
decision: 'approve',
scope,
});
if (outcome.kind === 'rate_limited') {
return errorResponse(
429,
ErrorCode.RATE_LIMIT_EXCEEDED,
`Rate limit exceeded: at most ${outcome.limit} approve/deny decisions per minute.`,
requestId,
);
}

// 4. Cross-table atomic transition (§7.1 pseudocode).
try {
await ddb.send(new TransactWriteCommand({
TransactItems: [
{
Update: {
TableName: TASK_APPROVALS_TABLE_NAME,
Key: { task_id: taskId, request_id },
UpdateExpression:
'SET #status = :approved, decided_at = :now, #scope = :scope',
ConditionExpression:
'attribute_exists(request_id) AND #status = :pending AND user_id = :caller',
ExpressionAttributeNames: {
'#status': 'status',
'#scope': 'scope',
},
ExpressionAttributeValues: {
':approved': 'APPROVED',
':pending': 'PENDING',
':now': nowIso,
':scope': scope,
':caller': callerUserId,
},
},
},
{
Update: {
TableName: TASK_TABLE_NAME,
Key: { task_id: taskId },
// No-op update on TaskTable; the purpose is the condition guard.
UpdateExpression: 'SET last_decision_at = :now',
ConditionExpression:
'#status = :awaiting AND awaiting_approval_request_id = :rid',
ExpressionAttributeNames: { '#status': 'status' },
ExpressionAttributeValues: {
':awaiting': 'AWAITING_APPROVAL',
':rid': request_id,
':now': nowIso,
},
},
},
],
}));
} catch (err: unknown) {
if (err instanceof TransactionCanceledException) {
return classifyCancel(err, requestId);
}
throw err;
if (outcome.kind === 'not_found') {
return errorResponse(
404,
ErrorCode.REQUEST_NOT_FOUND,
'Approval request not found or not owned by caller.',
requestId,
);
}

// 5. Audit event (IMPL-6). Failure to write the audit is logged
// but does not fail the request — the decision is already
// committed on TaskApprovalsTable and the agent will see it on its
// next poll regardless.
try {
await ddb.send(new PutCommand({
TableName: EVENTS_TABLE_NAME,
Item: {
task_id: taskId,
event_id: ulid(),
event_type: 'approval_decision_recorded',
timestamp: nowIso,
ttl: nowEpoch + AUDIT_EVENT_RETENTION_DAYS * 86400,
metadata: {
request_id,
status: 'APPROVED',
scope,
decided_at: nowIso,
caller_user_id: callerUserId,
},
},
}));
} catch (auditErr) {
logger.warn('approval_decision_recorded audit write failed (decision already committed)', {
task_id: taskId,
request_id,
error: auditErr instanceof Error ? auditErr.message : String(auditErr),
});
if (outcome.kind === 'not_awaiting') {
return errorResponse(
409,
ErrorCode.TASK_NOT_AWAITING_APPROVAL,
'Task is not currently awaiting approval for this request.',
requestId,
);
}
if (outcome.kind === 'transaction_unknown') {
return errorResponse(
503,
ErrorCode.SERVICE_UNAVAILABLE,
'Approval transaction cancelled for unknown reason.',
requestId,
);
}
const nowIso = outcome.decidedAt;

logger.info('Approval recorded', {
task_id: taskId,
Expand All @@ -262,62 +171,3 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
return errorResponse(500, ErrorCode.INTERNAL_ERROR, 'Internal server error.', requestId);
}
}

/**
* Map a `TransactionCanceledException` to the correct 4xx response.
*
* Per §7.1, the cancellation reasons are per-item (index 0 is the
* approvals-row Update, index 1 is the task-row Update). We read them
* to distinguish:
* - approvals item cancelled:
* - via `attribute_exists` failure → row missing → 404
* - via `user_id = :caller` failure → wrong owner → 404 (no oracle)
* - via `status = :pending` failure → already decided → 409
* - task-row item cancelled only → task not in AWAITING_APPROVAL → 409
*
* DDB does not return which sub-clause of the `ConditionExpression`
* failed, so we infer from whichever row was cancelled. If ONLY the
* approvals Update tripped, it could be any of {missing, wrong owner,
* wrong status}; we conservatively return 404 to prevent the existence
* oracle. The more-specific 409 ALREADY_DECIDED path requires
* additional information we do not have from the reason array alone;
* implementations that want the stronger distinction need to do a
* subsequent GetItem, which re-introduces the race the transaction
* eliminates. v1 accepts the less-granular 404 on ownership drift.
*/
function classifyCancel(
err: TransactionCanceledException,
requestId: string,
): APIGatewayProxyResult {
const reasons = err.CancellationReasons ?? [];
const approvalsReason = reasons[0]?.Code;
const taskReason = reasons[1]?.Code;

if (approvalsReason === 'ConditionalCheckFailed') {
// Collapse all approvals-row failures into REQUEST_NOT_FOUND per
// §7.1 finding #6 (no existence oracle). The less-specific code
// avoids leaking whether the row exists, belongs to a different
// user, or has already been decided.
return errorResponse(
404,
ErrorCode.REQUEST_NOT_FOUND,
'Approval request not found or not owned by caller.',
requestId,
);
}
if (taskReason === 'ConditionalCheckFailed') {
return errorResponse(
409,
ErrorCode.TASK_NOT_AWAITING_APPROVAL,
'Task is not currently awaiting approval for this request.',
requestId,
);
}
// Defensive fallback — propagate 503 since the cause is unexplained.
return errorResponse(
503,
ErrorCode.SERVICE_UNAVAILABLE,
'Approval transaction cancelled for unknown reason.',
requestId,
);
}
Loading
Loading