diff --git a/cdk/src/constructs/jira-integration.ts b/cdk/src/constructs/jira-integration.ts index 101da31a..7f63de99 100644 --- a/cdk/src/constructs/jira-integration.ts +++ b/cdk/src/constructs/jira-integration.ts @@ -51,14 +51,8 @@ const WEBHOOK_PROCESSOR_TIMEOUT_SECONDS = 300; /** * Marker key embedded in the auto-generated stack-wide webhook-secret - * placeholder. The CLI (`bgagent jira setup`) recognizes a secret carrying - * this key as "never configured" and seeds the operator's value over it. - * - * MUST stay in sync with `JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY` in - * `cli/src/commands/jira.ts`. Unlike Linear (whose real secrets always start - * with `lin_wh_`), Atlassian webhook signing secrets are operator-chosen bare - * strings with no fixed shape, so the *placeholder* — not the real value — is - * the thing we make recognizable. See #368. + * placeholder. `bgagent jira setup` replaces the complete placeholder with + * the sole active tenant's admin-console webhook secret. */ const JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY = 'abca_jira_webhook_placeholder'; @@ -187,24 +181,18 @@ export class JiraIntegration extends Construct { // --- Webhook signing secret (placeholder, populated by `bgagent jira setup`) --- // Per-tenant OAuth tokens live in `bgagent-jira-oauth-` secrets - // created by the CLI at runtime — not here. This stack-wide secret is - // a back-compat fallback for single-tenant installs predating per- - // tenant signing. + // created by the CLI at runtime — not here. Jira admin-console webhooks + // omit cloudId, so a single-tenant install verifies them against this + // stack-wide copy of the tenant's signing secret. // - // The initial value is an explicit JSON placeholder carrying - // `JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY`. Without `generateSecretString`, - // CDK seeds a BARE random string — which the CLI's placeholder heuristic - // mistook for an already-configured secret, so `setup` never seeded the - // operator's value and every admin-UI webhook delivery (whose payload has - // no `cloudId`, forcing stack-wide verification) failed HMAC with 401, - // silently (#368). Making the placeholder explicit lets the CLI reliably - // tell "never configured" from an operator-set value. + // The generated JSON is a non-operational initial value. Setup + // unconditionally replaces the complete value before the webhook is enabled. this.webhookSecret = new secretsmanager.Secret(this, 'WebhookSecret', { description: 'Jira webhook signing secret — populate via `bgagent jira setup`', removalPolicy, generateSecretString: { - // Yields `{"abca_jira_webhook_placeholder":true,"value":""}`: - // a JSON object (starts with `{`) with an explicit marker key. + // Yields `{"abca_jira_webhook_placeholder":true,"value":""}`. + // No runtime code interprets the marker key. secretStringTemplate: JSON.stringify({ [JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY]: true }), generateStringKey: 'value', }, diff --git a/cdk/src/handlers/jira-link.ts b/cdk/src/handlers/jira-link.ts index 72eaa67e..d7b1a417 100644 --- a/cdk/src/handlers/jira-link.ts +++ b/cdk/src/handlers/jira-link.ts @@ -67,6 +67,7 @@ export async function handler(event: APIGatewayProxyEvent): Promise { ProjectionExpression: 'jira_cloud_id, #s', ExpressionAttributeNames: { '#s': 'status' }, ExclusiveStartKey: lastKey, + ConsistentRead: true, })); for (const item of page.Items ?? []) { if (item.status === 'active' && typeof item.jira_cloud_id === 'string') { @@ -302,6 +303,16 @@ export async function handler(event: ProcessorEvent): Promise { }); return; } + const commentProjectKey = issue.fields?.project?.key; + if (!commentProjectKey) { + logger.info('Jira comment issue has no project.key — skipping (cannot establish onboarding)', { + issue_key: issue.key, + }); + return; + } + if (!await getActiveProjectMapping(cloudId, commentProjectKey, issue.key)) { + return; + } await handleCommentTrigger(payload, issue, cloudId); return; } @@ -311,11 +322,6 @@ export async function handler(event: ProcessorEvent): Promise { logger.info('Jira issue has no project.key — skipping (cannot route to a repo)', { issue_key: issue.key, }); - await safeReportIssueFailure( - issue.key, - cloudId, - "❌ This Jira issue isn't in a project — ABCA needs a Jira project to route the task to a repo. Move the issue into a project and re-apply the trigger label.", - ); return; } @@ -331,25 +337,12 @@ export async function handler(event: ProcessorEvent): Promise { return; } - const projectIdentity = `${cloudId}#${projectKey}`; - const mapping = await ddb.send(new GetCommand({ - TableName: PROJECT_MAPPING_TABLE, - Key: { jira_project_identity: projectIdentity }, - })); - if (!mapping.Item || mapping.Item.status !== 'active') { - logger.info('Jira project is not onboarded or is removed — skipping', { - jira_project_identity: projectIdentity, - issue_key: issue.key, - }); - await safeReportIssueFailure( - issue.key, - cloudId, - `❌ This Jira project isn't onboarded to ABCA. An admin can onboard it with \`bgagent jira map ${cloudId} ${projectKey} --repo /\` (add \`--label \` to change the trigger label).`, - ); + const mapping = await getActiveProjectMapping(cloudId, projectKey, issue.key); + if (!mapping) { return; } - const repo = mapping.Item.repo as string; - const labelFilter = (mapping.Item.label_filter as string | undefined) ?? DEFAULT_LABEL_FILTER; + const repo = mapping.repo as string; + const labelFilter = (mapping.label_filter as string | undefined) ?? DEFAULT_LABEL_FILTER; if (!shouldTrigger(payload, labelFilter)) { logger.info('Jira webhook does not match trigger criteria', { @@ -362,13 +355,24 @@ export async function handler(event: ProcessorEvent): Promise { return; } - const accountId = payload.user?.accountId - ?? issue.fields?.reporter?.accountId - ?? issue.fields?.creator?.accountId; + const actorAccountId = payload.user?.accountId; + const reporterAccountId = issue.fields?.reporter?.accountId; + const creatorAccountId = issue.fields?.creator?.accountId; + const accountId = actorAccountId ?? reporterAccountId ?? creatorAccountId; + const accountSource = actorAccountId + ? 'webhook_user' + : reporterAccountId + ? 'issue_reporter' + : creatorAccountId + ? 'issue_creator' + : undefined; if (!accountId) { logger.warn('Jira webhook missing user.accountId — cannot attribute task', { issue_key: issue.key, jira_cloud_id: cloudId, + jira_actor_account_id: actorAccountId, + jira_reporter_account_id: reporterAccountId, + jira_creator_account_id: creatorAccountId, }); await safeReportIssueFailure( issue.key, @@ -383,12 +387,18 @@ export async function handler(event: ProcessorEvent): Promise { logger.warn('Jira account has no linked platform user — skipping task creation', { jira_cloud_id: cloudId, jira_account_id: accountId, + jira_account_source: accountSource, + jira_actor_account_id: actorAccountId, + jira_reporter_account_id: reporterAccountId, + jira_creator_account_id: creatorAccountId, + jira_identity_lookup_key: `${cloudId}#${accountId}`, issue_key: issue.key, }); await safeReportIssueFailure( issue.key, cloudId, - "❌ This Jira user isn't linked to a platform user. Run `bgagent jira link ` from a Cognito-authenticated CLI session to complete linking.", + `❌ The Jira user for this trigger isn't linked to a platform user (accountId: \`${accountId}\`). ` + + `Ask an admin to run \`bgagent jira invite-user ${cloudId} ${accountId}\`, then redeem the generated link code.`, ); return; } @@ -408,8 +418,8 @@ export async function handler(event: ProcessorEvent): Promise { // admin configured `bgagent jira map ... --status-on-start/--status-on-pr`, // stamp them so the agent's best-effort transition helpers prefer these // status names over the built-in statusCategory / "In Review" heuristics. - const statusOnStart = mapping.Item.status_on_start as string | undefined; - const statusOnPr = mapping.Item.status_on_pr as string | undefined; + const statusOnStart = mapping.status_on_start as string | undefined; + const statusOnPr = mapping.status_on_pr as string | undefined; if (statusOnStart) { channelMetadata.jira_status_on_start = statusOnStart; } @@ -579,9 +589,9 @@ export async function handler(event: ProcessorEvent): Promise { /** * Handle `comment_created` independently of the label-trigger path. * - * The prior task is the routing source of truth: comments do not require the - * trigger label to still be present or the Jira project mapping to remain - * active. This preserves reviewer follow-ups after the original run. + * The prior task is the routing source of truth after the caller establishes + * that the Jira project mapping is still active. Comments do not require the + * trigger label to remain present. */ async function handleCommentTrigger( payload: JiraIssueEvent, @@ -1021,7 +1031,38 @@ async function lookupPlatformUser(cloudId: string, accountId: string): Promise | null> { + const projectIdentity = `${cloudId}#${projectKey}`; + const mapping = await ddb.send(new GetCommand({ + TableName: PROJECT_MAPPING_TABLE, + Key: { jira_project_identity: projectIdentity }, + ConsistentRead: true, + })); + if (!mapping.Item || mapping.Item.status !== 'active') { + // Jira admin-console webhooks fire site-wide. An unmapped project has not + // opted into ABCA, so it must remain a true no-op for every event type. + logger.info('Jira project is not onboarded or is removed — skipping silently', { + jira_project_identity: projectIdentity, + issue_key: issueKey, + }); + return null; + } + return mapping.Item; } diff --git a/cdk/src/handlers/jira-webhook.ts b/cdk/src/handlers/jira-webhook.ts index bb9f6832..a8603b14 100644 --- a/cdk/src/handlers/jira-webhook.ts +++ b/cdk/src/handlers/jira-webhook.ts @@ -122,10 +122,10 @@ export async function handler(event: APIGatewayProxyEvent): Promise`. Webhook subscriptions are - * tenant-scoped, so a single stack-wide signing secret cannot verify - * events from multiple tenants. The webhook receiver looks this up by - * `cloudId` at verify time. + * events with `X-Hub-Signature: sha256=`. The receiver uses this copy + * when the payload carries cloudId. Jira admin-console payloads omit + * cloudId, so the sole active tenant also synchronizes its value to the + * stack-wide verifier. * * Optional for back-compat: tokens written before per-tenant signing - * was wired up won't have it, and the receiver falls back to the - * stack-wide `JIRA_WEBHOOK_SECRET_ARN` for those installs. */ + * was wired up won't have it, so the receiver uses the stack-wide + * `JIRA_WEBHOOK_SECRET_ARN` verifier for those installs. */ readonly webhook_signing_secret?: string; } @@ -341,8 +341,8 @@ export async function resolveJiraOauthToken( * Strict variant of {@link getRegistryRow}: throws on infra error * (DDB throttle, network) instead of returning null. Use this from the * webhook signature-verification path where a `null` return would let - * a transient throttle silently downgrade per-tenant verification to - * the stack-wide fallback secret. + * a transient throttle silently bypass per-tenant verification by using + * the stack-wide verifier. */ export async function getRegistryRowStrict( ddb: DynamoDBDocumentClient, @@ -355,6 +355,7 @@ export async function getRegistryRowStrict( const result = await ddb.send(new GetCommand({ TableName: tableName, Key: { jira_cloud_id: cloudId }, + ConsistentRead: true, })); return parseRegistryRow(result.Item, cloudId); } @@ -372,6 +373,7 @@ export async function getRegistryRow( result = await ddb.send(new GetCommand({ TableName: tableName, Key: { jira_cloud_id: cloudId }, + ConsistentRead: true, })); } catch (err) { logger.error('Failed to fetch Jira workspace registry row', { diff --git a/cdk/src/handlers/shared/jira-verify.ts b/cdk/src/handlers/shared/jira-verify.ts index cf01f915..34851091 100644 --- a/cdk/src/handlers/shared/jira-verify.ts +++ b/cdk/src/handlers/shared/jira-verify.ts @@ -191,8 +191,9 @@ export async function verifyJiraRequest( * - `'revoked'` — registry row exists but status is not `active`. * Reject; do NOT fall back. * - `'no-per-tenant-secret'` — no registry row, OR the stored secret - * has no `webhook_signing_secret`. Caller should fall back to the - * stack-wide secret for back-compat with single-tenant installs. + * has no `webhook_signing_secret`. Caller should use the stack-wide + * verifier. This is also the normal path for admin-console payloads, which + * omit cloudId before tenant selection is possible. * * Strict lookups (throw on infra errors) are used so a transient DDB or * SM error doesn't silently downgrade a per-tenant-secured tenant to diff --git a/cdk/test/constructs/jira-integration.test.ts b/cdk/test/constructs/jira-integration.test.ts index 72ece28b..31eecf9f 100644 --- a/cdk/test/constructs/jira-integration.test.ts +++ b/cdk/test/constructs/jira-integration.test.ts @@ -58,15 +58,13 @@ describe('JiraIntegration construct', () => { }); }); - // #368: the webhook secret MUST seed an explicit JSON placeholder so the CLI - // can distinguish "never configured" from an operator-set value. A bare - // generated string (CDK's default with no GenerateSecretString) caused - // `bgagent jira setup` to skip seeding, leaving every admin-UI webhook - // delivery to fail HMAC verification with 401. - test('webhook secret seeds a JSON placeholder carrying the explicit marker key (#368)', () => { + // A structured initial value avoids seeding a bare signing-key-shaped + // secret. Setup unconditionally replaces the complete value; no runtime + // code interprets the marker key. + test('webhook secret seeds a non-signing JSON placeholder', () => { template.hasResourceProperties('AWS::SecretsManager::Secret', { GenerateSecretString: Match.objectLike({ - // secretStringTemplate is the JSON object carrying the marker key. + // secretStringTemplate pins the non-operational initial JSON shape. SecretStringTemplate: Match.stringLikeRegexp('abca_jira_webhook_placeholder'), GenerateStringKey: 'value', }), diff --git a/cdk/test/handlers/jira-link.test.ts b/cdk/test/handlers/jira-link.test.ts index 75d9d247..c784e7ce 100644 --- a/cdk/test/handlers/jira-link.test.ts +++ b/cdk/test/handlers/jira-link.test.ts @@ -95,6 +95,8 @@ describe('jira-link handler', () => { const result = await handler(makeEvent({ code: 'link-3f8b4a2c' }, 'cognito-user-1')); expect(result.statusCode).toBe(200); + const getCall = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'Get'); + expect(getCall![0].input.ConsistentRead).toBe(true); const putCall = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'Put'); expect(putCall).toBeTruthy(); expect(putCall![0].input.Item.jira_identity).toBe('cloud-1#acc-1'); diff --git a/cdk/test/handlers/jira-webhook-processor.test.ts b/cdk/test/handlers/jira-webhook-processor.test.ts index 3ec35ab2..47e3cd93 100644 --- a/cdk/test/handlers/jira-webhook-processor.test.ts +++ b/cdk/test/handlers/jira-webhook-processor.test.ts @@ -213,10 +213,34 @@ describe('jira-webhook-processor handler', () => { }, }; + function mockCommentDdb( + userMapping?: Record, + projectMapping: Record = { + repo: 'org/repo', + status: 'active', + label_filter: 'bgagent', + }, + ): void { + ddbSend.mockImplementation((command: { input: { TableName?: string } }) => { + if (command.input.TableName === 'JiraProjects') { + return Promise.resolve({ Item: projectMapping }); + } + if (command.input.TableName === 'JiraUsers') { + return Promise.resolve({ Item: userMapping }); + } + return Promise.resolve({}); + }); + } + + beforeEach(() => { + mockCommentDdb(); + }); + test('ADF @bgagent comment creates a PR iteration for the linked comment author', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ - Item: { platform_user_id: 'linked-reviewer', status: 'active' }, + mockCommentDdb({ + platform_user_id: 'linked-reviewer', + status: 'active', }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: '{}' }); @@ -256,16 +280,17 @@ describe('jira-webhook-processor handler', () => { 'ENG-42', '👀 ABCA accepted this follow-up and is updating PR #42.', ); - // Comment triggers route from the prior task, not the current project - // mapping or label state. The only DDB Get is author attribution. - expect(ddbSend.mock.calls).toHaveLength(1); + expect(ddbSend.mock.calls).toHaveLength(2); expect(ddbSend.mock.calls[0][0].input.Key) + .toEqual({ jira_project_identity: 'cloud-1#ENG' }); + expect(ddbSend.mock.calls[0][0].input.ConsistentRead).toBe(true); + expect(ddbSend.mock.calls[1][0].input.Key) .toEqual({ jira_identity: 'cloud-1#reviewer-1' }); + expect(ddbSend.mock.calls[1][0].input.ConsistentRead).toBe(true); }); test('ADF mention node creates a PR iteration', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: '{}' }); const payload = comment(); (payload.comment as Record).body = { @@ -292,7 +317,6 @@ describe('jira-webhook-processor handler', () => { test('preserves ADF hard breaks in a multiline instruction', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: '{}' }); const payload = comment(); (payload.comment as Record).body = { @@ -322,7 +346,6 @@ describe('jira-webhook-processor handler', () => { pr_number: undefined, pr_url: 'https://github.com/org/repo/pull/73', }); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: '{}' }); const payload = comment(); (payload.comment as Record).body = '@bgagent rename the flag'; @@ -337,7 +360,6 @@ describe('jira-webhook-processor handler', () => { test('bare mention uses the latest-review fallback instruction', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: '{}' }); const payload = comment(); (payload.comment as Record).body = '@bgagent'; @@ -360,6 +382,30 @@ describe('jira-webhook-processor handler', () => { expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); + test.each([ + ['unmapped', undefined], + ['removed', { repo: 'org/repo', status: 'removed' }], + ])('keeps @bgagent comments in %s projects silent', async (_state, projectMapping) => { + mockCommentDdb(undefined, projectMapping ?? {}); + + await handler(eventWith(comment())); + + expect(resolveTaskByJiraIssueMock).not.toHaveBeenCalled(); + expect(createTaskCoreMock).not.toHaveBeenCalled(); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + + test('keeps @bgagent comments without a project key silent', async () => { + const payload = comment(); + delete (payload.issue as { fields: { project: Record } }).fields.project.key; + + await handler(eventWith(payload)); + + expect(resolveTaskByJiraIssueMock).not.toHaveBeenCalled(); + expect(createTaskCoreMock).not.toHaveBeenCalled(); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + test.each([ { author: { accountId: 'app-1', accountType: 'app' }, @@ -404,7 +450,6 @@ describe('jira-webhook-processor handler', () => { ...priorTask, user_id: undefined, }); - ddbSend.mockResolvedValueOnce({ Item: undefined }); await handler(eventWith(comment())); @@ -414,7 +459,6 @@ describe('jira-webhook-processor handler', () => { test('idempotent replay creates no duplicate task acknowledgement', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 200, body: '{}' }); await handler(eventWith(comment())); @@ -425,7 +469,6 @@ describe('jira-webhook-processor handler', () => { test('task admission failure is reported instead of acknowledged', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 400, body: JSON.stringify({ @@ -444,7 +487,6 @@ describe('jira-webhook-processor handler', () => { test('transient admission failure tells the reviewer to post a new comment', async () => { resolveTaskByJiraIssueMock.mockResolvedValueOnce(priorTask); - ddbSend.mockResolvedValueOnce({ Item: undefined }); createTaskCoreMock.mockResolvedValueOnce({ statusCode: 503, body: '{}' }); await handler(eventWith(comment())); @@ -509,6 +551,7 @@ describe('jira-webhook-processor handler', () => { createTaskCoreMock.mockResolvedValue({ task_id: 'T1' }); await handler(eventWith(payload)); expect(createTaskCoreMock).toHaveBeenCalled(); + expect(ddbSend.mock.calls[0][0].input.ConsistentRead).toBe(true); }); // ─── Stack-wide-verified deliveries: cloudId is not trusted from the body ── @@ -565,12 +608,25 @@ describe('jira-webhook-processor handler', () => { ddbSend.mockResolvedValueOnce({ Item: undefined }); await handler(eventWith(issue())); expect(createTaskCoreMock).not.toHaveBeenCalled(); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); test('skips when project mapping is removed', async () => { ddbSend.mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'removed' } }); await handler(eventWith(issue())); expect(createTaskCoreMock).not.toHaveBeenCalled(); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + + test('keeps an ordinary event in an unmapped project completely silent', async () => { + ddbSend.mockResolvedValueOnce({ Item: undefined }); + const payload = issue(); + (payload.issue as { fields: Record }).fields.labels = ['other']; + + await handler(eventWith(payload)); + + expect(createTaskCoreMock).not.toHaveBeenCalled(); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); test('skips when trigger label is absent on create', async () => { @@ -615,10 +671,15 @@ describe('jira-webhook-processor handler', () => { expect(createTaskCoreMock).not.toHaveBeenCalled(); }); - test('skips when accountId has no linked platform user', async () => { + test.each([ + ['missing', undefined], + ['pending', { platform_user_id: 'cognito-user-1', status: 'pending' }], + ['revoked', { platform_user_id: 'cognito-user-1', status: 'revoked' }], + ['active but missing platform_user_id', { platform_user_id: '', status: 'active' }], + ])('skips when the user mapping is %s', async (_case, userMapping) => { ddbSend .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) - .mockResolvedValueOnce({ Item: undefined }); + .mockResolvedValueOnce({ Item: userMapping }); await handler(eventWith(issue())); expect(createTaskCoreMock).not.toHaveBeenCalled(); }); @@ -708,6 +769,7 @@ describe('jira-webhook-processor handler', () => { const getCall = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'Get'); expect(getCall![0].input.Key.jira_project_identity).toBe('cloud-1#ENG'); + expect(getCall![0].input.ConsistentRead).toBe(true); }); test('fires on update when changelog labels diff newly contains the trigger', async () => { @@ -766,6 +828,7 @@ describe('jira-webhook-processor handler', () => { const userGetCall = ddbSend.mock.calls.filter(([cmd]) => cmd._type === 'Get')[1]; expect(userGetCall[0].input.Key.jira_identity).toBe('cloud-1#reporter-acc'); + expect(userGetCall[0].input.ConsistentRead).toBe(true); }); test('drops event when tenant resolves to null (registry miss / inactive / unreadable secret)', async () => { @@ -781,47 +844,32 @@ describe('jira-webhook-processor handler', () => { }); describe('user-visible feedback on silent-failure paths', () => { - test('posts comment when issue has no project.key', async () => { + test('keeps an issue with no project.key silent because onboarding cannot be established', async () => { const payload = issue(); delete (payload.issue as { fields: { project: Record } }).fields.project.key; await handler(eventWith(payload)); - expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); - const [ctx, issueIdOrKey, message] = reportIssueFailureMock.mock.calls[0]; - expect(ctx).toEqual({ - cloudId: 'cloud-1', - registryTableName: process.env.JIRA_WORKSPACE_REGISTRY_TABLE_NAME, - }); - expect(issueIdOrKey).toBe('ENG-42'); - expect(message).toContain("isn't in a project"); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); - test('posts feedback when project is not onboarded', async () => { + test('keeps a trigger-labeled issue in an unmapped project silent', async () => { ddbSend.mockResolvedValueOnce({ Item: undefined }); await handler(eventWith(issue())); - expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); - const [, issueKey, message] = reportIssueFailureMock.mock.calls[0]; - expect(issueKey).toBe('ENG-42'); - expect(message).toContain("isn't onboarded"); - // The suggested command must be the real one (`map`) with the required - // cloud-id + project-key positionals, not the non-existent - // `onboard-project`. - expect(message).toContain('bgagent jira map cloud-1 ENG --repo'); - expect(message).not.toContain('onboard-project'); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); - test('posts feedback when project mapping is removed', async () => { + test('keeps a trigger-labeled issue with a removed mapping silent', async () => { ddbSend.mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'removed' } }); await handler(eventWith(issue())); - expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); - test('posts feedback when accountId has no linked platform user', async () => { + test('posts actionable feedback for the selected Jira actor in an onboarded project', async () => { ddbSend .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) .mockResolvedValueOnce({ Item: undefined }); @@ -831,7 +879,8 @@ describe('jira-webhook-processor handler', () => { expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); const [, , message] = reportIssueFailureMock.mock.calls[0]; expect(message).toContain("isn't linked to a platform user"); - expect(message).toContain('bgagent jira link'); + expect(message).toContain('accountId: `acc-1`'); + expect(message).toContain('bgagent jira invite-user cloud-1 acc-1'); }); test('surfaces guardrail block message on createTaskCore 400', async () => { @@ -905,19 +954,21 @@ describe('jira-webhook-processor handler', () => { reportIssueFailureMock.mockImplementationOnce(() => { throw new Error('synthetic synchronous throw'); }); - const payload = issue(); - delete (payload.issue as { fields: { project: Record } }).fields.project.key; + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: undefined }); - await expect(handler(eventWith(payload))).resolves.toBeUndefined(); + await expect(handler(eventWith(issue()))).resolves.toBeUndefined(); expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); }); test('safeReportIssueFailure: async rejection from reportIssueFailure does not propagate', async () => { reportIssueFailureMock.mockRejectedValueOnce(new Error('async failure')); - const payload = issue(); - delete (payload.issue as { fields: { project: Record } }).fields.project.key; + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: undefined }); - await expect(handler(eventWith(payload))).resolves.toBeUndefined(); + await expect(handler(eventWith(issue()))).resolves.toBeUndefined(); expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); }); }); diff --git a/cdk/test/handlers/shared/jira-oauth-resolver.test.ts b/cdk/test/handlers/shared/jira-oauth-resolver.test.ts index a3a04847..1767951d 100644 --- a/cdk/test/handlers/shared/jira-oauth-resolver.test.ts +++ b/cdk/test/handlers/shared/jira-oauth-resolver.test.ts @@ -122,6 +122,7 @@ describe('resolveJiraOauthToken', () => { const result = await resolveJiraOauthToken('cloud-uuid-1', REGISTRY_TABLE, clients); + expect(clients.ddbSend.mock.calls[0][0].input.ConsistentRead).toBe(true); expect(result).toEqual({ accessToken: 'jira_oauth_happy', scope: stored.scope, @@ -838,6 +839,7 @@ describe('getRegistryRow / parseRegistryRow', () => { const send = jest.fn().mockResolvedValue({ Item: undefined }); const row = await getRegistryRow(asDdb(send), REGISTRY_TABLE, 'cloud-x'); expect(row).toBeNull(); + expect(send.mock.calls[0][0].input.ConsistentRead).toBe(true); }); test('returns null and logs when DDB throws (non-strict swallows error)', async () => { @@ -871,6 +873,7 @@ describe('getRegistryRow / parseRegistryRow', () => { await expect(getRegistryRowStrict(asDdb(send), REGISTRY_TABLE, 'cloud-x')).rejects.toThrow( 'DDB throttle', ); + expect(send.mock.calls[0][0].input.ConsistentRead).toBe(true); }); }); diff --git a/cli/README.md b/cli/README.md index f3cb5291..39121512 100644 --- a/cli/README.md +++ b/cli/README.md @@ -302,9 +302,9 @@ With no flags, writes to the platform default `GitHubTokenSecretArn` stack outpu Configure the preview-deploy screenshot pipeline webhook. See [Deploy preview screenshots guide](../docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md). -### `bgagent jira app-template` / `setup` / `app-setup` / `map` / `invite-user` / `link` +### `bgagent jira app-template` / `setup` / `update-webhook-secret` / `app-setup` / `map` / `invite-user` / `link` -Manage the Jira Cloud integration. `setup` authorizes a tenant via OAuth (3LO) for inbound reads and human lookup. `app-setup` verifies and stores the signed Forge proxy used for outbound comments and transitions as the dedicated `bgagent` app actor. `map` routes a Jira project to a GitHub repo; the two-step `invite-user` → `link` handshake links a teammate's Jira identity to their platform user. See the [Jira setup guide](../docs/guides/JIRA_SETUP_GUIDE.md) for Forge deployment, secret handling, permissions, and the full walkthrough. +Manage the Jira Cloud integration. `setup` authorizes a tenant via OAuth (3LO) for inbound reads and human lookup. `update-webhook-secret` rotates the admin-console webhook secret in both required Secrets Manager locations without repeating OAuth. `app-setup` verifies and stores the signed Forge proxy used for outbound comments and transitions as the dedicated `bgagent` app actor. `map` routes a Jira project to a GitHub repo; the two-step `invite-user` → `link` handshake links a teammate's Jira identity to their platform user. See the [Jira setup guide](../docs/guides/JIRA_SETUP_GUIDE.md) for Forge deployment, secret handling, permissions, and the full walkthrough. ``` bgagent jira app-template @@ -313,6 +313,10 @@ bgagent jira setup \ --region \ --stack-name +bgagent jira update-webhook-secret \ + --region \ + --stack-name + bgagent jira app-setup \ --proxy-url https://.webtrigger.atlassian.app/public/ \ --region \ diff --git a/cli/src/commands/jira.ts b/cli/src/commands/jira.ts index 6427c738..9319ae6e 100644 --- a/cli/src/commands/jira.ts +++ b/cli/src/commands/jira.ts @@ -28,8 +28,10 @@ import { SecretsManagerClient, } from '@aws-sdk/client-secrets-manager'; import { + DynamoDBDocumentClient, GetCommand, PutCommand, + ScanCommand, UpdateCommand, } from '@aws-sdk/lib-dynamodb'; import { Command } from 'commander'; @@ -246,78 +248,100 @@ export async function upsertOauthSecret( } } -/** - * Marker key embedded in the CDK-generated stack-wide webhook-secret - * placeholder. A secret whose JSON carries this key has never been - * configured by an operator, so `setup` is free to seed the real value. - * - * MUST stay in sync with `JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY` in - * `cdk/src/constructs/jira-integration.ts`. See #368. - */ -export const JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY = 'abca_jira_webhook_placeholder'; +async function listActiveJiraTenantIds( + ddb: DynamoDBDocumentClient, + tableName: string, +): Promise { + const ids = new Set(); + let lastKey: Record | undefined; + do { + const page = await ddb.send(new ScanCommand({ + TableName: tableName, + ProjectionExpression: 'jira_cloud_id, #s', + FilterExpression: '#s = :active', + ExpressionAttributeNames: { '#s': 'status' }, + ExpressionAttributeValues: { ':active': 'active' }, + ExclusiveStartKey: lastKey, + ConsistentRead: true, + })); + for (const item of page.Items ?? []) { + if (typeof item.jira_cloud_id === 'string' && item.jira_cloud_id) { + ids.add(item.jira_cloud_id); + } + } + lastKey = page.LastEvaluatedKey; + } while (lastKey); + return [...ids]; +} + +function multiTenantWebhookError(cloudIds: readonly string[]): CliError { + if (cloudIds.length === 0) { + return new CliError( + 'Could not confirm a sole active Jira tenant in the workspace registry. ' + + 'Re-run `bgagent jira setup` before configuring or rotating this webhook.', + ); + } + return new CliError( + 'Jira admin-console webhooks omit cloudId, so one webhook URL cannot select among ' + + `multiple tenant secrets (active tenants: ${cloudIds.join(', ')}). ` + + 'This setup supports exactly one active Jira tenant. Remove or revoke the other ' + + 'tenant before configuring or rotating this webhook.', + ); +} /** - * Check whether the JiraWebhookSecret already holds a real, operator-set - * signing secret (vs the CDK-generated placeholder). Used to decide whether - * to seed the stack-wide secret on a `setup` run. + * Persist one signing secret to both locations required by Jira + * admin-console webhooks. These payloads omit cloudId, so the receiver uses + * the stack-wide verifier; the per-tenant copy is retained for payloads that + * do carry cloudId. * - * Atlassian's generic-webhook signing secrets are operator-chosen — they have - * no fixed prefix like Linear's `lin_wh_`, so we cannot positively recognize a - * *real* value by shape. Instead we recognize the *placeholder*: the CDK - * construct seeds an explicit JSON object carrying - * `JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY`. Anything that is not that placeholder - * is treated as an operator value. - * - * NOTE (#368 migration): stacks deployed before the explicit-placeholder fix - * seeded a *bare random string* placeholder, which is indistinguishable from - * an operator value and so is (conservatively) reported as configured. Such - * installs must redeploy the CDK stack — which regenerates the secret with the - * JSON placeholder — before `setup` will seed it. + * The writes are not atomic. If the process exits between them, re-run the + * command to converge both copies. */ -export async function isWebhookSecretConfigured( - client: SecretsManagerClient, - secretArn: string, -): Promise { +export async function synchronizeJiraWebhookSecrets( + sm: SecretsManagerClient, + oauthSecretArn: string, + stackWideSecretArn: string, + stored: StoredJiraOauthToken, + webhookSigningSecret: string, +): Promise { + const merged: StoredJiraOauthToken = { + ...stored, + webhook_signing_secret: webhookSigningSecret, + updated_at: new Date().toISOString(), + }; + const originalSecretString = JSON.stringify(stored); + + await sm.send(new PutSecretValueCommand({ + SecretId: oauthSecretArn, + SecretString: JSON.stringify(merged), + })); try { - const result = await client.send(new GetSecretValueCommand({ SecretId: secretArn })); - const value = result.SecretString; - if (typeof value !== 'string' || value.length === 0) return false; - return !isWebhookSecretPlaceholder(value); + await sm.send(new PutSecretValueCommand({ + SecretId: stackWideSecretArn, + SecretString: webhookSigningSecret, + })); } catch (err) { - const errorName = (err as { name?: string }).name; - if (errorName === 'ResourceNotFoundException') { - return false; + try { + await sm.send(new PutSecretValueCommand({ + SecretId: oauthSecretArn, + SecretString: originalSecretString, + })); + } catch (rollbackErr) { + throw new CliError( + 'Failed to update the stack-wide Jira webhook secret and failed to restore the tenant bundle: ' + + `${err instanceof Error ? err.message : String(err)}; rollback: ` + + `${rollbackErr instanceof Error ? rollbackErr.message : String(rollbackErr)}. ` + + 'Manually synchronize both Secrets Manager values before re-enabling the Jira webhook.', + ); } - const message = err instanceof Error ? err.message : String(err); throw new CliError( - `Failed to read Jira webhook secret '${secretArn}': ${errorName ?? 'Error'}: ${message}. ` - + 'Likely IAM permission gap — confirm your CLI principal has ' - + '`secretsmanager:GetSecretValue` on this ARN.', + 'Failed to update the stack-wide Jira webhook secret, but restored the tenant bundle ' + + `to its previous value: ${err instanceof Error ? err.message : String(err)}. ` + + 'Both secrets remain consistent at the previous value; rotation can be safely retried.', ); } -} - -/** - * True when `value` is the CDK-generated placeholder — a JSON object carrying - * the {@link JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY} marker. A non-JSON value, or - * JSON without the marker, is an operator-set secret. - */ -function isWebhookSecretPlaceholder(value: string): boolean { - const trimmed = value.trim(); - // Fast reject: real Atlassian signing secrets are bare strings. - if (!trimmed.startsWith('{')) return false; - try { - const parsed: unknown = JSON.parse(trimmed); - return ( - typeof parsed === 'object' - && parsed !== null - && JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY in (parsed as Record) - ); - } catch { - // Starts with `{` but isn't valid JSON — not our placeholder. Treat as a - // (malformed) operator value rather than silently re-seeding over it. - return false; // nosemgrep: ts-silent-success-masking -- unparseable secret is conservatively treated as operator-set (not the placeholder), so setup never overwrites it - } + return merged; } interface JiraUserSearchResult { @@ -674,6 +698,17 @@ export function makeJiraCommand(): Command { console.log(` cloud_id: ${cloudId}`); console.log(` site_url: ${siteUrl}`); + // Jira Settings → System → Webhooks payloads omit cloudId, so the + // receiver must use one stack-wide verifier. Refuse a second active + // tenant before writing its OAuth or registry state rather than + // presenting an apparently complete but unverifiable setup. + const ddb = makeDocClient({ region }); + const activeTenantIds = await listActiveJiraTenantIds(ddb, workspaceRegistryTable!); + const otherActiveTenantIds = activeTenantIds.filter((id) => id !== cloudId); + if (otherActiveTenantIds.length > 0) { + throw multiTenantWebhookError([...otherActiveTenantIds, cloudId]); + } + // ─── Step 4: Persist token to per-tenant Secrets Manager ───────── process.stdout.write(' → Storing OAuth token...'); const sm = makeClient(SecretsManagerClient, { region }); @@ -696,7 +731,6 @@ export function makeJiraCommand(): Command { console.log(` ✓ (${secretName})`); // ─── Step 5: Persist registry row ──────────────────────────────── - const ddb = makeDocClient({ region }); // Update instead of replacing the row so re-running OAuth setup keeps // app-actor audit metadata written by `jira app-setup`. await ddb.send(new UpdateCommand({ @@ -724,20 +758,12 @@ export function makeJiraCommand(): Command { })); console.log(' ✓ Recorded tenant in registry'); - // ─── Step 6: Webhook signing secret (per-tenant primary) ───────── + // ─── Step 6: Webhook signing secret ────────────────────────────── // // Atlassian doesn't auto-generate webhook signing secrets — they're - // operator-chosen at webhook-create time in the Jira admin UI, and - // each tenant's webhook is configured independently with its OWN - // secret. So we always prompt for THIS tenant's secret and store it - // on the per-tenant OAuth bundle — the primary verification path. - // - // We deliberately do NOT copy an existing stack-wide secret into a - // new tenant's bundle (the old behavior): that would make tenant A's - // secret verify per-tenant for tenant B, and a holder of the - // stack-wide secret could then forge per-tenant-signed events for any - // tenant. The stack-wide secret is only seeded once, from the FIRST - // tenant's secret, as the single-tenant back-compat fallback. + // operator-chosen in the Jira admin UI. Those webhook payloads omit + // cloudId, so the sole active tenant's value must be kept in both its + // OAuth bundle and the stack-wide verifier. const apiBaseUrl = config.api_url.replace(/\/+$/, ''); console.log(); console.log(' Webhook signing secret needed for THIS tenant.'); @@ -746,33 +772,29 @@ export function makeJiraCommand(): Command { console.log(' Events: Issue: created, Issue: updated, Comment: created'); console.log(' Secret: choose a strong random value (e.g. `openssl rand -hex 32`)'); console.log(); - const webhookSigningSecret = await promptSecret('Webhook signing secret: '); + const webhookSigningSecret = (await promptSecret('Webhook signing secret: ')).trim(); if (!webhookSigningSecret) { throw new CliError('Webhook signing secret is required.'); } - const merged: StoredJiraOauthToken = { - ...stored, - webhook_signing_secret: webhookSigningSecret, - updated_at: new Date().toISOString(), - }; - await upsertOauthSecret(sm, secretName, merged, cloudId); - console.log(' ✓ Stored signing secret on the per-tenant OAuth bundle'); - - // Seed the stack-wide fallback only if it has never been set, so a - // single-tenant install (no per-tenant routing) still verifies. Once - // a second tenant onboards, its secret is per-tenant only — the - // stack-wide secret stays pinned to the first tenant. - const stackWideAlreadyConfigured = await isWebhookSecretConfigured(sm, webhookSecretArn!); - if (stackWideAlreadyConfigured) { - console.log(' ✓ Stack-wide fallback already configured (leaving as-is)'); - } else { - await sm.send(new PutSecretValueCommand({ - SecretId: webhookSecretArn!, - SecretString: webhookSigningSecret, - })); - console.log(' ✓ Seeded stack-wide fallback for single-tenant back-compat'); - } + // upsertOauthSecret preserves Forge app-actor fields on setup reruns. + // Read back that persisted bundle so synchronization does not replace + // it with the fresh OAuth-only object assembled above. + const persistedOauthSecret = await sm.send(new GetSecretValueCommand({ + SecretId: oauthSecretArn, + })); + const persistedStored = parseStoredJiraOauthToken( + persistedOauthSecret.SecretString, + oauthSecretArn, + ); + await synchronizeJiraWebhookSecrets( + sm, + oauthSecretArn, + webhookSecretArn!, + persistedStored, + webhookSigningSecret, + ); + console.log(' ✓ Synchronized tenant and stack-wide signing secrets'); // ─── Done ───────────────────────────────────────────────────────── console.log(); @@ -827,6 +849,7 @@ export function makeJiraCommand(): Command { const registry = await ddb.send(new GetCommand({ TableName: registryTableName, Key: { jira_cloud_id: cloudId }, + ConsistentRead: true, })); const row = registry.Item; if (!row || row.status !== 'active' || typeof row.oauth_secret_arn !== 'string') { @@ -922,6 +945,78 @@ export function makeJiraCommand(): Command { }), ); + // ─── update-webhook-secret ──────────────────────────────────────────────── + jira.addCommand( + new Command('update-webhook-secret') + .description('Rotate the Jira admin-console webhook signing secret (single active tenant)') + .argument('', 'Atlassian tenant cloudId (UUID)') + .option('--region ', 'AWS region (defaults to configured region)') + .option('--stack-name ', 'CloudFormation stack name', 'backgroundagent-dev') + .action(async (cloudId: string, opts) => { + const config = loadConfig(); + const region = opts.region || config.region; + const stackName = opts.stackName; + const [workspaceRegistryTable, webhookSecretArn] = await Promise.all([ + getStackOutput(region, stackName, 'JiraWorkspaceRegistryTableName'), + getStackOutput(region, stackName, 'JiraWebhookSecretArn'), + ]); + const missing: string[] = []; + if (!workspaceRegistryTable) missing.push('JiraWorkspaceRegistryTableName'); + if (!webhookSecretArn) missing.push('JiraWebhookSecretArn'); + if (missing.length > 0) { + throw new CliError( + `Stack '${stackName}' is missing outputs ${missing.join(', ')}. ` + + 'Re-deploy with the JiraIntegration CDK changes (mise //cdk:deploy).', + ); + } + + const ddb = makeDocClient({ region }); + const registry = await ddb.send(new GetCommand({ + TableName: workspaceRegistryTable!, + Key: { jira_cloud_id: cloudId }, + ConsistentRead: true, + })); + const registryRow = registry.Item; + if (!registryRow || registryRow.status !== 'active') { + throw new CliError( + `Jira tenant '${cloudId}' is not in the registry (or status != 'active'). ` + + 'Run `bgagent jira setup` for that tenant first.', + ); + } + const oauthSecretArn = registryRow.oauth_secret_arn as string | undefined; + if (!oauthSecretArn) { + throw new CliError( + `Jira tenant '${cloudId}' registry row is missing oauth_secret_arn. ` + + 'Re-run `bgagent jira setup`.', + ); + } + + const activeTenantIds = await listActiveJiraTenantIds(ddb, workspaceRegistryTable!); + if (activeTenantIds.length !== 1 || activeTenantIds[0] !== cloudId) { + throw multiTenantWebhookError(activeTenantIds); + } + + const webhookSigningSecret = (await promptSecret('New webhook signing secret: ')).trim(); + if (!webhookSigningSecret) { + throw new CliError('Webhook signing secret is required.'); + } + + const sm = makeClient(SecretsManagerClient, { region }); + const oauthSecret = await sm.send(new GetSecretValueCommand({ SecretId: oauthSecretArn })); + const stored = parseStoredJiraOauthToken(oauthSecret.SecretString, oauthSecretArn); + await synchronizeJiraWebhookSecrets( + sm, + oauthSecretArn, + webhookSecretArn!, + stored, + webhookSigningSecret, + ); + + console.log(`✓ Updated Jira webhook signing secret for tenant '${cloudId}'.`); + console.log(' Tenant OAuth bundle and stack-wide verifier are synchronized.'); + }), + ); + // ─── invite-user ────────────────────────────────────────────────────────── jira.addCommand( new Command('invite-user') @@ -961,6 +1056,7 @@ export function makeJiraCommand(): Command { const registry = await ddb.send(new GetCommand({ TableName: workspaceRegistryTable!, Key: { jira_cloud_id: cloudId }, + ConsistentRead: true, })); const registryRow = registry.Item; if (!registryRow || registryRow.status !== 'active') { diff --git a/cli/src/jira-oauth.ts b/cli/src/jira-oauth.ts index fe028867..29ce8e6b 100644 --- a/cli/src/jira-oauth.ts +++ b/cli/src/jira-oauth.ts @@ -113,14 +113,15 @@ export interface StoredJiraOauthToken { /** * Per-tenant Jira webhook signing secret. * - * Atlassian's "Generic webhooks" support a per-webhook secret that - * signs events with `X-Hub-Signature: sha256=`. Webhook - * subscriptions are tenant-scoped, so a single stack-wide signing - * secret cannot verify events from multiple tenants. + * Atlassian's "Generic webhooks" support a per-webhook secret that signs + * events with `X-Hub-Signature: sha256=`. The receiver uses this copy + * when the payload carries cloudId. Jira admin-console payloads omit + * cloudId, so the sole active tenant also synchronizes its value to the + * stack-wide verifier. * * Optional for back-compat: tokens written before per-tenant signing - * was wired up won't have it, and the receiver falls back to the - * stack-wide `JIRA_WEBHOOK_SECRET_ARN` for those installs. + * was wired up won't have it, so the receiver uses the stack-wide + * `JIRA_WEBHOOK_SECRET_ARN` verifier for those installs. */ readonly webhook_signing_secret?: string; } diff --git a/cli/test/commands/jira.test.ts b/cli/test/commands/jira.test.ts index 4fc64d5b..7014876d 100644 --- a/cli/test/commands/jira.test.ts +++ b/cli/test/commands/jira.test.ts @@ -23,14 +23,18 @@ import { PutSecretValueCommand, ResourceExistsException, } from '@aws-sdk/client-secrets-manager'; -import { GetCommand, PutCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb'; +import { + GetCommand, + PutCommand, + ScanCommand, + UpdateCommand, +} from '@aws-sdk/lib-dynamodb'; import { ApiClient } from '../../src/api-client'; import { - isWebhookSecretConfigured, - JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY, makeJiraCommand, openBrowser, renderJiraAppTemplate, + synchronizeJiraWebhookSecrets, upsertOauthSecret, } from '../../src/commands/jira'; import * as config from '../../src/config'; @@ -134,6 +138,7 @@ describe('makeJiraCommand', () => { 'app-setup', 'app-template', 'setup', + 'update-webhook-secret', 'invite-user', 'link', 'map', @@ -386,6 +391,205 @@ describe('jira link action', () => { }); }); +describe('jira update-webhook-secret action', () => { + let loadConfigSpy: jest.SpiedFunction; + let logSpy: jest.SpiedFunction; + + beforeEach(() => { + ddbSend.mockReset(); + smSend.mockReset(); + promptSecretMock.mockReset().mockResolvedValue('new-signing-secret'); + cfnSend.mockReset().mockResolvedValue({ + Stacks: [{ + Outputs: [ + { OutputKey: 'JiraWorkspaceRegistryTableName', OutputValue: 'JiraRegistryTable' }, + { OutputKey: 'JiraWebhookSecretArn', OutputValue: 'arn:stack-wide' }, + ], + }], + }); + loadConfigSpy = jest.spyOn(config, 'loadConfig').mockReturnValue({ + region: 'us-west-2', + } as ReturnType); + logSpy = jest.spyOn(console, 'log').mockImplementation(); + }); + + afterEach(() => { + loadConfigSpy.mockRestore(); + logSpy.mockRestore(); + }); + + async function runUpdate(cloudId = 'cloud-123'): Promise { + const program = makeJiraCommand(); + await program.parseAsync(['node', 'bgagent', 'update-webhook-secret', cloudId]); + } + + test('preserves OAuth fields and synchronizes tenant plus stack-wide secrets', async () => { + ddbSend + .mockResolvedValueOnce({ + Item: { + jira_cloud_id: 'cloud-123', + oauth_secret_arn: 'arn:jira-oauth', + status: 'active', + }, + }) + .mockResolvedValueOnce({ + Items: [{ jira_cloud_id: 'cloud-123', status: 'active' }], + }); + smSend + .mockResolvedValueOnce({ + SecretString: JSON.stringify(sampleToken({ + cloud_id: 'cloud-123', + webhook_signing_secret: 'old-signing-secret', + })), + }) + .mockResolvedValueOnce({}) + .mockResolvedValueOnce({}); + + await runUpdate(); + + expect(ddbSend.mock.calls[0][0]).toBeInstanceOf(GetCommand); + expect((ddbSend.mock.calls[0][0] as GetCommand).input.ConsistentRead).toBe(true); + expect(ddbSend.mock.calls[1][0]).toBeInstanceOf(ScanCommand); + expect(smSend.mock.calls[0][0]).toBeInstanceOf(GetSecretValueCommand); + + const tenantWrite = smSend.mock.calls[1][0] as PutSecretValueCommand; + expect(tenantWrite.input.SecretId).toBe('arn:jira-oauth'); + expect(JSON.parse(tenantWrite.input.SecretString as string)).toMatchObject({ + access_token: 'access-xyz', + refresh_token: 'refresh-xyz', + client_secret: 'client-secret', + webhook_signing_secret: 'new-signing-secret', + }); + + const globalWrite = smSend.mock.calls[2][0] as PutSecretValueCommand; + expect(globalWrite.input).toEqual({ + SecretId: 'arn:stack-wide', + SecretString: 'new-signing-secret', + }); + }); + + test('refuses rotation when more than one Jira tenant is active', async () => { + ddbSend + .mockResolvedValueOnce({ + Item: { + jira_cloud_id: 'cloud-123', + oauth_secret_arn: 'arn:jira-oauth', + status: 'active', + }, + }) + .mockResolvedValueOnce({ + Items: [ + { jira_cloud_id: 'cloud-123', status: 'active' }, + { jira_cloud_id: 'cloud-456', status: 'active' }, + ], + }); + + await expect(runUpdate()).rejects.toThrow(/multiple tenant secrets/); + + expect(promptSecretMock).not.toHaveBeenCalled(); + expect(smSend).not.toHaveBeenCalled(); + }); + + test('refuses rotation when the active-tenant scan is empty', async () => { + ddbSend + .mockResolvedValueOnce({ + Item: { + jira_cloud_id: 'cloud-123', + oauth_secret_arn: 'arn:jira-oauth', + status: 'active', + }, + }) + .mockResolvedValueOnce({ Items: [] }); + + await expect(runUpdate()).rejects.toThrow(/Could not confirm a sole active Jira tenant/); + + expect(promptSecretMock).not.toHaveBeenCalled(); + expect(smSend).not.toHaveBeenCalled(); + }); + + test('paginates the active-tenant scan before rotating', async () => { + ddbSend + .mockResolvedValueOnce({ + Item: { + jira_cloud_id: 'cloud-123', + oauth_secret_arn: 'arn:jira-oauth', + status: 'active', + }, + }) + .mockResolvedValueOnce({ + LastEvaluatedKey: { jira_cloud_id: 'cursor' }, + }) + .mockResolvedValueOnce({ + Items: [{ jira_cloud_id: 'cloud-123', status: 'active' }], + }); + smSend + .mockResolvedValueOnce({ + SecretString: JSON.stringify(sampleToken({ cloud_id: 'cloud-123' })), + }) + .mockResolvedValueOnce({}) + .mockResolvedValueOnce({}); + + await runUpdate(); + + expect(ddbSend).toHaveBeenCalledTimes(3); + const secondScan = ddbSend.mock.calls[2][0] as ScanCommand; + expect(secondScan.input.ExclusiveStartKey).toEqual({ jira_cloud_id: 'cursor' }); + }); + + test('refuses rotation for an inactive or unknown tenant', async () => { + ddbSend.mockResolvedValueOnce({ + Item: { jira_cloud_id: 'cloud-123', status: 'revoked' }, + }); + + await expect(runUpdate()).rejects.toThrow(/not in the registry/); + + expect(ddbSend).toHaveBeenCalledTimes(1); + expect(smSend).not.toHaveBeenCalled(); + }); + + test('reports all missing stack outputs before accessing Jira state', async () => { + cfnSend.mockResolvedValue({ Stacks: [{ Outputs: [] }] }); + + await expect(runUpdate()).rejects.toThrow( + /JiraWorkspaceRegistryTableName, JiraWebhookSecretArn/, + ); + + expect(ddbSend).not.toHaveBeenCalled(); + expect(smSend).not.toHaveBeenCalled(); + }); + + test('refuses rotation when the registry row has no OAuth secret ARN', async () => { + ddbSend.mockResolvedValueOnce({ + Item: { jira_cloud_id: 'cloud-123', status: 'active' }, + }); + + await expect(runUpdate()).rejects.toThrow(/missing oauth_secret_arn/); + + expect(ddbSend).toHaveBeenCalledTimes(1); + expect(promptSecretMock).not.toHaveBeenCalled(); + expect(smSend).not.toHaveBeenCalled(); + }); + + test('refuses a blank replacement signing secret', async () => { + ddbSend + .mockResolvedValueOnce({ + Item: { + jira_cloud_id: 'cloud-123', + oauth_secret_arn: 'arn:jira-oauth', + status: 'active', + }, + }) + .mockResolvedValueOnce({ + Items: [{ jira_cloud_id: 'cloud-123', status: 'active' }], + }); + promptSecretMock.mockResolvedValueOnce(' '); + + await expect(runUpdate()).rejects.toThrow(/Webhook signing secret is required/); + + expect(smSend).not.toHaveBeenCalled(); + }); +}); + describe('jira invite-user action', () => { const originalFetch = global.fetch; let loadConfigSpy: jest.SpiedFunction; @@ -471,6 +675,7 @@ describe('jira invite-user action', () => { expect(getCmd.input).toMatchObject({ TableName: 'JiraRegistryTable', Key: { jira_cloud_id: 'cloud-123' }, + ConsistentRead: true, }); const searchUrl = new URL(String(fetchMock.mock.calls[0][0])); @@ -820,6 +1025,11 @@ describe('jira setup action', () => { let loadConfigSpy: jest.SpiedFunction; beforeEach(() => { + ddbSend.mockReset(); + smSend.mockReset(); + promptSecretMock.mockReset(); + awaitOauthCallbackMock.mockReset(); + execFileMock.mockReset(); cfnSend.mockReset(); loadConfigSpy = jest.spyOn(config, 'loadConfig').mockReturnValue({ region: 'us-west-2' } as ReturnType); }); @@ -836,6 +1046,190 @@ describe('jira setup action', () => { ).rejects.toThrow(/missing outputs .*JiraWorkspaceRegistryTableName.*JiraWebhookSecretArn/s); }); + test.each([ + ['a first install', []], + ['a same-tenant setup rerun', [{ jira_cloud_id: 'cloud-123', status: 'active' }]], + ])('completes OAuth setup and synchronizes webhook secrets for %s', async (_case, activeTenants) => { + cfnSend.mockResolvedValue({ + Stacks: [{ + Outputs: [ + { OutputKey: 'JiraWorkspaceRegistryTableName', OutputValue: 'RegTable' }, + { OutputKey: 'JiraWebhookSecretArn', OutputValue: 'arn:webhook' }, + ], + }], + }); + loadConfigSpy.mockReturnValue({ + region: 'us-west-2', + api_url: 'https://api.example.test/v1/', + } as ReturnType); + const credsSpy = jest.spyOn(config, 'loadCredentials').mockReturnValue({ + id_token: fakeIdToken('cognito-sub-123'), + } as ReturnType); + const logSpy = jest.spyOn(console, 'log').mockImplementation(); + const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => true); + const fetchSpy = jest.spyOn(global, 'fetch') + .mockResolvedValueOnce(new Response(JSON.stringify({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read:jira-work write:jira-work read:jira-user', + }), { status: 200, headers: { 'Content-Type': 'application/json' } })) + .mockResolvedValueOnce(new Response(JSON.stringify([{ + id: 'cloud-123', + name: 'Acme', + url: 'https://acme.atlassian.net', + scopes: ['read:jira-work'], + }]), { status: 200, headers: { 'Content-Type': 'application/json' } })); + + let completeOauth!: (value: { + kind: 'direct-oauth'; + code: string; + state: string; + }) => void; + awaitOauthCallbackMock.mockReturnValueOnce(new Promise((resolve) => { + completeOauth = resolve; + })); + execFileMock.mockImplementationOnce( + (_command: string, args: string[], callback: (err: Error | null) => void) => { + const state = new URL(args[0]).searchParams.get('state'); + completeOauth({ kind: 'direct-oauth', code: 'auth-code', state: state! }); + callback(null); + }, + ); + promptSecretMock.mockResolvedValueOnce('new-signing-secret'); + ddbSend.mockImplementation((command: unknown) => { + if (command instanceof ScanCommand) { + return Promise.resolve({ + Items: activeTenants, + }); + } + return Promise.resolve({}); + }); + smSend + .mockResolvedValueOnce({ ARN: 'arn:jira-oauth' }) + .mockResolvedValueOnce({ + SecretString: JSON.stringify(sampleToken({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + cloud_id: 'cloud-123', + })), + }) + .mockResolvedValueOnce({}) + .mockResolvedValueOnce({}); + + try { + const program = makeJiraCommand(); + await program.parseAsync([ + 'node', + 'bgagent', + 'setup', + '--client-id', + 'client-id', + '--client-secret', + 'client-secret', + ]); + + expect(ddbSend.mock.calls[0][0]).toBeInstanceOf(ScanCommand); + expect(ddbSend.mock.calls[1][0]).toBeInstanceOf(UpdateCommand); + expect(smSend.mock.calls[0][0]).toBeInstanceOf(CreateSecretCommand); + expect(smSend.mock.calls[1][0]).toBeInstanceOf(GetSecretValueCommand); + + const tenantWrite = smSend.mock.calls[2][0] as PutSecretValueCommand; + expect(tenantWrite.input.SecretId).toBe('arn:jira-oauth'); + expect(JSON.parse(tenantWrite.input.SecretString as string)).toMatchObject({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + webhook_signing_secret: 'new-signing-secret', + }); + const stackWideWrite = smSend.mock.calls[3][0] as PutSecretValueCommand; + expect(stackWideWrite.input).toEqual({ + SecretId: 'arn:webhook', + SecretString: 'new-signing-secret', + }); + } finally { + credsSpy.mockRestore(); + logSpy.mockRestore(); + writeSpy.mockRestore(); + fetchSpy.mockRestore(); + } + }); + + test('refuses a second active tenant before writing OAuth or registry state', async () => { + cfnSend.mockResolvedValue({ + Stacks: [{ + Outputs: [ + { OutputKey: 'JiraWorkspaceRegistryTableName', OutputValue: 'RegTable' }, + { OutputKey: 'JiraWebhookSecretArn', OutputValue: 'arn:webhook' }, + ], + }], + }); + const credsSpy = jest.spyOn(config, 'loadCredentials').mockReturnValue({ + id_token: fakeIdToken('cognito-sub-123'), + } as ReturnType); + const logSpy = jest.spyOn(console, 'log').mockImplementation(); + const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => true); + const fetchSpy = jest.spyOn(global, 'fetch') + .mockResolvedValueOnce(new Response(JSON.stringify({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read:jira-work write:jira-work read:jira-user', + }), { status: 200, headers: { 'Content-Type': 'application/json' } })) + .mockResolvedValueOnce(new Response(JSON.stringify([{ + id: 'cloud-123', + name: 'Acme', + url: 'https://acme.atlassian.net', + scopes: ['read:jira-work'], + }]), { status: 200, headers: { 'Content-Type': 'application/json' } })); + + let completeOauth!: (value: { + kind: 'direct-oauth'; + code: string; + state: string; + }) => void; + awaitOauthCallbackMock.mockReturnValueOnce(new Promise((resolve) => { + completeOauth = resolve; + })); + execFileMock.mockImplementationOnce( + (_command: string, args: string[], callback: (err: Error | null) => void) => { + const state = new URL(args[0]).searchParams.get('state'); + completeOauth({ kind: 'direct-oauth', code: 'auth-code', state: state! }); + callback(null); + }, + ); + ddbSend.mockResolvedValueOnce({ + Items: [{ jira_cloud_id: 'cloud-456', status: 'active' }], + }); + + try { + const program = makeJiraCommand(); + await expect(program.parseAsync([ + 'node', + 'bgagent', + 'setup', + '--client-id', + 'client-id', + '--client-secret', + 'client-secret', + ])).rejects.toThrow(/multiple tenant secrets/); + + expect(ddbSend).toHaveBeenCalledTimes(1); + expect(ddbSend.mock.calls[0][0]).toBeInstanceOf(ScanCommand); + expect( + ddbSend.mock.calls.some(([command]) => command instanceof UpdateCommand), + ).toBe(false); + expect(smSend).not.toHaveBeenCalled(); + expect(promptSecretMock).not.toHaveBeenCalled(); + } finally { + credsSpy.mockRestore(); + logSpy.mockRestore(); + writeSpy.mockRestore(); + fetchSpy.mockRestore(); + } + }); + test('aborts on OAuth state mismatch after generating PKCE/state (covers randomState)', async () => { cfnSend.mockResolvedValue({ Stacks: [{ @@ -1009,6 +1403,10 @@ describe('jira app-setup action', () => { cloudId: 'cloud-123', siteUrl: 'https://acme.atlassian.net', }); + const registryGet = ddbSend.mock.calls + .map((call) => call[0]) + .find((command) => command instanceof GetCommand) as GetCommand; + expect(registryGet.input.ConsistentRead).toBe(true); const put = smSend.mock.calls .map((call) => call[0]) .find((command) => command instanceof PutSecretValueCommand) as PutSecretValueCommand; @@ -1226,76 +1624,85 @@ describe('upsertOauthSecret', () => { }); }); -describe('isWebhookSecretConfigured', () => { +describe('synchronizeJiraWebhookSecrets', () => { const mockSend = jest.fn(); - const mockClient = { send: mockSend } as unknown as Parameters[0]; + const mockClient = { send: mockSend } as unknown as Parameters< + typeof synchronizeJiraWebhookSecrets + >[0]; beforeEach(() => { mockSend.mockReset(); }); - test('returns false for the CDK-generated JSON placeholder (the #368 case)', async () => { - // Mirrors what the JiraIntegration construct seeds via generateSecretString: - // a JSON object carrying the explicit placeholder marker key. - const placeholder = JSON.stringify({ - [JIRA_WEBHOOK_SECRET_PLACEHOLDER_KEY]: true, - value: 'abcdEFGH1234random', + test('preserves Forge app actor fields while rotating both signing-secret copies', async () => { + const stored = sampleToken({ + webhook_signing_secret: 'old-secret', + app_actor_proxy_url: 'https://install.webtrigger.atlassian.app/public/trigger', + app_actor_shared_secret: 's'.repeat(64), + app_actor_account_id: 'app-account-1', + app_actor_display_name: 'bgagent', + app_actor_configured_at: '2026-07-23T00:00:00.000Z', }); - mockSend.mockResolvedValueOnce({ SecretString: placeholder }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(false); - }); + mockSend.mockResolvedValueOnce({}).mockResolvedValueOnce({}); - test('returns true for an operator-set bare-string secret', async () => { - // Atlassian signing secrets are operator-chosen bare strings, no fixed prefix. - mockSend.mockResolvedValueOnce({ SecretString: 'operator-chosen-signing-secret' }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(true); - }); - - test('returns true for the legacy bare-string CDK placeholder (pre-#368, indistinguishable from real)', async () => { - // Stacks deployed before the explicit-placeholder fix seeded a bare random - // string. It is conservatively reported as configured — such installs must - // redeploy the stack (regenerating the JSON placeholder) before setup seeds. - mockSend.mockResolvedValueOnce({ SecretString: 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef' }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(true); - }); - - test('returns true for an operator value that happens to start with "{" but is not the placeholder', async () => { - mockSend.mockResolvedValueOnce({ SecretString: '{not really json' }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(true); - }); + const updated = await synchronizeJiraWebhookSecrets( + mockClient, + 'arn:tenant', + 'arn:global', + stored, + 'new-secret', + ); - test('returns true for JSON that lacks the placeholder marker key', async () => { - mockSend.mockResolvedValueOnce({ SecretString: '{"value":"abcd"}' }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(true); + expect(updated).toMatchObject({ + webhook_signing_secret: 'new-secret', + app_actor_proxy_url: stored.app_actor_proxy_url, + app_actor_shared_secret: stored.app_actor_shared_secret, + app_actor_account_id: stored.app_actor_account_id, + app_actor_display_name: stored.app_actor_display_name, + app_actor_configured_at: stored.app_actor_configured_at, + }); + const tenantWrite = mockSend.mock.calls[0][0] as PutSecretValueCommand; + expect(JSON.parse(tenantWrite.input.SecretString as string)).toMatchObject(updated); }); - test('returns false on ResourceNotFoundException (secret not created yet)', async () => { - const err = new Error('Secrets Manager cannot find the specified secret.'); - err.name = 'ResourceNotFoundException'; - mockSend.mockRejectedValueOnce(err); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(false); - }); + test('rolls the tenant bundle back when the stack-wide write fails', async () => { + const stored = sampleToken({ webhook_signing_secret: 'old-secret' }); + mockSend + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new Error('stack-wide write failed')) + .mockResolvedValueOnce({}); - test('throws on AccessDenied so operators see the IAM gap instead of a confusing re-prompt', async () => { - const err = new Error('User is not authorized to perform: secretsmanager:GetSecretValue'); - err.name = 'AccessDeniedException'; - mockSend.mockRejectedValueOnce(err); - await expect(isWebhookSecretConfigured(mockClient, 'arn:secret')).rejects.toThrow(/IAM permission gap/); - }); + await expect(synchronizeJiraWebhookSecrets( + mockClient, + 'arn:tenant', + 'arn:global', + stored, + 'new-secret', + )).rejects.toMatchObject({ + name: 'CliError', + message: expect.stringMatching( + /restored the tenant bundle.*Both secrets remain consistent.*safely retried/s, + ), + }); - test('returns false when SecretString is missing', async () => { - mockSend.mockResolvedValueOnce({}); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(false); + expect(mockSend).toHaveBeenCalledTimes(3); + const rollback = mockSend.mock.calls[2][0] as PutSecretValueCommand; + expect(rollback.input.SecretId).toBe('arn:tenant'); + expect(JSON.parse(rollback.input.SecretString as string)).toEqual(stored); }); - test('returns false when SecretString is empty', async () => { - mockSend.mockResolvedValueOnce({ SecretString: '' }); - expect(await isWebhookSecretConfigured(mockClient, 'arn:secret')).toBe(false); - }); + test('reports both failures when updating global and restoring the tenant both fail', async () => { + mockSend + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new Error('global failed')) + .mockRejectedValueOnce(new Error('rollback failed')); - test('wraps a non-Error rejection with the IAM-gap guidance', async () => { - // A thrown non-Error value exercises the `?? \'Error\'` / String(err) branches. - mockSend.mockRejectedValueOnce('boom'); - await expect(isWebhookSecretConfigured(mockClient, 'arn:secret')).rejects.toThrow(/IAM permission gap/); + await expect(synchronizeJiraWebhookSecrets( + mockClient, + 'arn:tenant', + 'arn:global', + sampleToken(), + 'new-secret', + )).rejects.toThrow(/global failed; rollback: rollback failed/); }); }); diff --git a/docs/decisions/ADR-015-jira-integration.md b/docs/decisions/ADR-015-jira-integration.md index 5d9b2577..e3c656f0 100644 --- a/docs/decisions/ADR-015-jira-integration.md +++ b/docs/decisions/ADR-015-jira-integration.md @@ -2,6 +2,7 @@ **Status:** accepted **Date:** 2026-06-08 +**Revised:** 2026-08-05 by [#709](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/709) ## Context @@ -41,14 +42,16 @@ Web-trigger URLs have no Forge-managed caller authentication. ABCA signs `timest These are the points where blindly copying Linear would have been wrong: 1. **Label-add detection on updates.** Jira's `jira:issue_updated` payload reports label changes in `changelog.items[]` (`field: "labels"`, `fromString` / `toString`) — it does *not* re-send the full label list. The processor diffs the changelog, not `issue.fields.labels`, so re-saving an issue that already carries the label does not re-trigger. -2. **Webhook signing secret is operator-chosen.** Atlassian does not auto-generate a per-subscription signing secret the way Linear does. The operator picks one at webhook-create time and pastes it during `bgagent jira setup`; ABCA stores it on the per-tenant OAuth bundle. The stack-wide secret is seeded only once (from the first tenant) for single-tenant back-compat — it is **not** copied into later tenants' bundles (see *Multi-tenant signature binding* below). +2. **Webhook signing secret is operator-chosen and synchronized.** Atlassian does not auto-generate a per-subscription signing secret the way Linear does. The operator picks one at webhook-create time and pastes it during `bgagent jira setup`; ABCA stores the same value on the sole active tenant's OAuth bundle and in the stack-wide verifier. Jira admin-console webhook payloads omit `cloudId`, so the channel supports exactly one active Jira tenant. 3. **Signature scheme.** Atlassian signs with HMAC-SHA256 over the *raw* request body, delivered as `X-Hub-Signature: sha256=`. Verification uses a constant-time compare over the unparsed bytes. 4. **ADF descriptions.** Jira issue descriptions are Atlassian Document Format, not markdown. The processor extracts text/headings/lists (and external `media` image URLs) into markdown for the task description rather than rolling a full ADF converter. 5. **Dedup key.** `{issueKey}#{webhookEvent}#{timestamp}` with an 8-hour TTL, rather than keying on event type alone — so two distinct label-adds in quick succession aren't collapsed, while retries of one delivery (same timestamp) are. Jira retries far less aggressively than Linear, so 8 hours is safe parity. A timestamp-less delivery collapses to `…#unknown` and skips the (advisory, unsigned) replay-window check, which is logged rather than treated as fatal. -### Multi-tenant signature binding +### Admin-console webhook tenant binding -The per-tenant signing secret proves which tenant signed a delivery, so a per-tenant-verified webhook's body `cloudId` is trusted for routing. The **stack-wide fallback secret is not bound to any `cloudId`**, so a delivery verified that way cannot trust a body-supplied `cloudId`. The receiver flags stack-wide verifications (`verified_via_stack_wide`) to the processor, which then ignores the body `cloudId` and binds the event to the **sole active tenant**, dropping when zero or multiple tenants are active. This preserves the fail-closed multi-tenant guarantee: a holder of the stack-wide secret cannot steer a webhook at an arbitrary tenant's mappings. +Jira admin-console webhook payloads omit `cloudId`, so one webhook URL cannot select among multiple tenant secrets. `bgagent jira setup` therefore refuses to onboard a second active Jira tenant and synchronizes the sole active tenant's signing secret to both its OAuth bundle and the stack-wide verifier. + +Payloads that do carry `cloudId` can still use the per-tenant copy. For stack-wide verifications, the receiver flags the delivery (`verified_via_stack_wide`) and the processor ignores any body-supplied `cloudId`, binding the event to the sole active tenant. It drops the event when zero or multiple tenants are active, so a holder of the stack-wide secret cannot steer a webhook at arbitrary tenant mappings. ### Token refresh ownership @@ -60,17 +63,19 @@ Atlassian **rotates the `refresh_token` on every use**. Only trusted Lambda code - (+) Jira comments and workflow history identify the dedicated `bgagent` app instead of the OAuth setup user. - (+) Inbound human attribution remains independent: `JiraUserMappingTable` still controls task ownership, concurrency, cost, and audit. - (+) One identity-selection rule covers Lambda and agent writes, and a configured app failure cannot silently change actor. -- (+) Per-tenant credential isolation, signature binding, and the changelog-diff trigger keep the trust and re-trigger semantics correct for multi-tenant installs. +- (+) Synchronizing the tenant and stack-wide signing-secret copies keeps admin-console webhook verification deterministic. - (-) Operators deploy and install a small Forge app per Atlassian environment and manage one additional HMAC secret. +- (-) The Jira channel supports exactly one active tenant because admin-console webhook payloads provide no tenant-routing key. - (-) Forge web-trigger and invocation limits become part of the outbound path. - (-) ADF→markdown is lossy by design (text/headings/lists + external image URLs only); rich content in descriptions is flattened, and `file`-type attachment media (needing a Jira API round-trip) are skipped. - (!) `cloudId` must be used consistently as the tenant key. Indexing on domain or site name anywhere would break tenant resolution. -- (!) The webhook signing secret lives on the per-tenant OAuth bundle; rotating it in Jira without re-running `bgagent jira setup` causes silent 401s on every delivery. +- (!) The webhook signing secret lives in both the per-tenant OAuth bundle and the stack-wide verifier; after rotating it in Jira, run `bgagent jira update-webhook-secret ` to synchronize both copies and avoid silent 401s. ## References - Issue: [#288 — Jira Cloud integration (parity with Linear)](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/288) - Issue: [#642 — give Jira outbound actions a bgagent app identity](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/642) +- Issue: [#709 — repair Jira webhook admission and secret rotation](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/709) - [JIRA_SETUP_GUIDE.md](../guides/JIRA_SETUP_GUIDE.md) — operational walkthrough - [LINEAR_SETUP_GUIDE.md](../guides/LINEAR_SETUP_GUIDE.md) — the analog integration this mirrors - Reference implementation: `cdk/src/constructs/jira-integration.ts`, `cdk/src/handlers/jira-*.ts`, `cdk/src/handlers/shared/jira-{verify,oauth-resolver,feedback}.ts`, `agent/src/jira_reactions.py`, `agent/src/channel_mcp.py` diff --git a/docs/guides/JIRA_SETUP_GUIDE.md b/docs/guides/JIRA_SETUP_GUIDE.md index 2a185889..4a27e1de 100644 --- a/docs/guides/JIRA_SETUP_GUIDE.md +++ b/docs/guides/JIRA_SETUP_GUIDE.md @@ -145,7 +145,19 @@ This runs the OAuth 3LO dance: - **Events** — *Issue: created*, *Issue: updated*, and *Comment: created* - **Secret** — a strong random value, e.g. `openssl rand -hex 32` -Paste that same secret value back at the `Webhook signing secret:` prompt. ABCA stores it on the per-tenant OAuth bundle and seeds the stack-wide single-tenant fallback only when it is still unset. The receiver looks up the tenant value to verify `X-Hub-Signature` on each delivery. +Paste that same secret value back at the `Webhook signing secret:` prompt. ABCA stores it on the per-tenant OAuth bundle and synchronizes the stack-wide verifier. The receiver looks up that value to verify `X-Hub-Signature` on each delivery. + +> **One active tenant for Jira admin-console webhooks.** Webhooks created under **Settings → System → Webhooks** do not include `cloudId` in their payload, so the receiver cannot select among multiple tenant secrets. `jira setup` therefore refuses to configure a second active Jira tenant through this flow. For the sole active tenant it always synchronizes the tenant bundle and stack-wide verifier, including on setup reruns. + +When recreating the webhook or rotating its secret later, update the secret in Jira and then run: + +```bash +bgagent jira update-webhook-secret +``` + +The command prompts for the new value and updates both required Secrets Manager values without repeating OAuth. Keep the Jira webhook disabled until the command succeeds. + +The operator role running this command needs `cloudformation:DescribeStacks`; `dynamodb:GetItem` and `dynamodb:Scan` on `JiraWorkspaceRegistryTable`; `secretsmanager:GetSecretValue` and `secretsmanager:PutSecretValue` on the tenant's `bgagent-jira-oauth-` secret; and `secretsmanager:PutSecretValue` on the stack-wide ARN from `JiraWebhookSecretArn`. ### 4. Install the dedicated outbound app @@ -293,7 +305,7 @@ Re-running `bgagent jira setup` preserves an existing app-actor configuration. O Atlassian signs each delivery with HMAC-SHA256 over the **raw request body**, delivered as `X-Hub-Signature: sha256=`. The receiver: 1. Computes `HMAC-SHA256(rawBody, secret)` and compares it constant-time against the header value (tolerating a pasted value with or without the `sha256=` prefix). -2. Prefers the **per-tenant** signing secret stored on `bgagent-jira-oauth-`; falls back to the stack-wide `JiraWebhookSecret` for installs that predate per-tenant storage. +2. Uses the per-tenant signing secret when the payload carries `cloudId`. Admin-console payloads omit it, so they use the synchronized stack-wide `JiraWebhookSecret` and bind to the sole active tenant. 3. Rejects with 401 on mismatch. The body must be verified as the *raw unparsed bytes* — never parsed-and-restringified JSON, which would change the byte sequence and break the HMAC. @@ -304,6 +316,7 @@ The body must be verified as the *raw unparsed bytes* — never parsed-and-restr - **`jira:issue_updated`** — triggers only if the label was **newly added** in this update. Jira reports label changes in `changelog.items[]` (`field: "labels"`, with `fromString` / `toString`), *not* by re-sending the full label list. The processor diffs the changelog rather than inspecting `issue.fields.labels`, so re-saving an issue that already has the label does not re-trigger. - **`comment_created`** — triggers only when the new comment contains a token-bounded `@bgagent` mention and the issue has a prior ABCA pull request. - All other event types get a silent `200`. +- Issues outside active project mappings are always silent, even if they use the same label. This includes explicit `@bgagent` follow-ups after a project mapping is removed: offboarding ends all ABCA interaction for that project. Site-wide Jira activity must not cause ABCA comments in projects that were never onboarded or are no longer connected. ## Comment-triggered PR iteration @@ -382,7 +395,7 @@ The receiver dedupes issue events on `{issueKey}#{webhookEvent}#{timestamp}` and ### Webhook signature verification fails repeatedly (401) -The signing secret stored for this tenant doesn't match what Jira is sending. Most often the value pasted at the `Webhook signing secret:` prompt differs from the one entered in Jira's webhook config (or the webhook secret was rotated in Jira). Re-run `bgagent jira setup` for the tenant and re-enter matching values. To inspect what's stored: +The signing secret stored for this tenant doesn't match what Jira is sending. Most often the value entered in Jira differs from ABCA's copy, or the webhook was recreated with a new secret. Run `bgagent jira update-webhook-secret ` and enter the exact value configured in Jira. This synchronizes both locations required by admin-console webhooks. To inspect what's stored: ```bash aws secretsmanager get-secret-value \ @@ -390,6 +403,17 @@ aws secretsmanager get-secret-value \ --query SecretString --output text | jq .webhook_signing_secret ``` +### Linking succeeds but a trigger says the Jira user is unlinked + +Jira attributes a trigger to the account in the webhook's `user.accountId`. That may differ from the issue reporter, creator, or the OAuth account ABCA uses to post comments. The name shown above an ABCA comment is therefore not proof that the same account triggered the event. + +Check the webhook-processor warning for `jira_account_id`, `jira_account_source`, and `jira_identity_lookup_key`. The failure comment on an onboarded, explicitly triggered issue also prints the selected account ID. Invite and link that exact account: + +```bash +bgagent jira invite-user +bgagent jira link +``` + ### `setup` hangs at "Waiting for browser callback…" The consent redirect never reached the CLI's localhost listener — see the note under [Step 2](#2-authorize-the-app-on-the-tenant). Ctrl-C and re-run `bgagent jira setup`; re-running is safe. diff --git a/docs/src/content/docs/decisions/Adr-015-jira-integration.md b/docs/src/content/docs/decisions/Adr-015-jira-integration.md index 428e0d3f..d7eee09e 100644 --- a/docs/src/content/docs/decisions/Adr-015-jira-integration.md +++ b/docs/src/content/docs/decisions/Adr-015-jira-integration.md @@ -6,6 +6,7 @@ title: Adr 015 jira integration **Status:** accepted **Date:** 2026-06-08 +**Revised:** 2026-08-05 by [#709](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/709) ## Context @@ -45,14 +46,16 @@ Web-trigger URLs have no Forge-managed caller authentication. ABCA signs `timest These are the points where blindly copying Linear would have been wrong: 1. **Label-add detection on updates.** Jira's `jira:issue_updated` payload reports label changes in `changelog.items[]` (`field: "labels"`, `fromString` / `toString`) — it does *not* re-send the full label list. The processor diffs the changelog, not `issue.fields.labels`, so re-saving an issue that already carries the label does not re-trigger. -2. **Webhook signing secret is operator-chosen.** Atlassian does not auto-generate a per-subscription signing secret the way Linear does. The operator picks one at webhook-create time and pastes it during `bgagent jira setup`; ABCA stores it on the per-tenant OAuth bundle. The stack-wide secret is seeded only once (from the first tenant) for single-tenant back-compat — it is **not** copied into later tenants' bundles (see *Multi-tenant signature binding* below). +2. **Webhook signing secret is operator-chosen and synchronized.** Atlassian does not auto-generate a per-subscription signing secret the way Linear does. The operator picks one at webhook-create time and pastes it during `bgagent jira setup`; ABCA stores the same value on the sole active tenant's OAuth bundle and in the stack-wide verifier. Jira admin-console webhook payloads omit `cloudId`, so the channel supports exactly one active Jira tenant. 3. **Signature scheme.** Atlassian signs with HMAC-SHA256 over the *raw* request body, delivered as `X-Hub-Signature: sha256=`. Verification uses a constant-time compare over the unparsed bytes. 4. **ADF descriptions.** Jira issue descriptions are Atlassian Document Format, not markdown. The processor extracts text/headings/lists (and external `media` image URLs) into markdown for the task description rather than rolling a full ADF converter. 5. **Dedup key.** `{issueKey}#{webhookEvent}#{timestamp}` with an 8-hour TTL, rather than keying on event type alone — so two distinct label-adds in quick succession aren't collapsed, while retries of one delivery (same timestamp) are. Jira retries far less aggressively than Linear, so 8 hours is safe parity. A timestamp-less delivery collapses to `…#unknown` and skips the (advisory, unsigned) replay-window check, which is logged rather than treated as fatal. -### Multi-tenant signature binding +### Admin-console webhook tenant binding -The per-tenant signing secret proves which tenant signed a delivery, so a per-tenant-verified webhook's body `cloudId` is trusted for routing. The **stack-wide fallback secret is not bound to any `cloudId`**, so a delivery verified that way cannot trust a body-supplied `cloudId`. The receiver flags stack-wide verifications (`verified_via_stack_wide`) to the processor, which then ignores the body `cloudId` and binds the event to the **sole active tenant**, dropping when zero or multiple tenants are active. This preserves the fail-closed multi-tenant guarantee: a holder of the stack-wide secret cannot steer a webhook at an arbitrary tenant's mappings. +Jira admin-console webhook payloads omit `cloudId`, so one webhook URL cannot select among multiple tenant secrets. `bgagent jira setup` therefore refuses to onboard a second active Jira tenant and synchronizes the sole active tenant's signing secret to both its OAuth bundle and the stack-wide verifier. + +Payloads that do carry `cloudId` can still use the per-tenant copy. For stack-wide verifications, the receiver flags the delivery (`verified_via_stack_wide`) and the processor ignores any body-supplied `cloudId`, binding the event to the sole active tenant. It drops the event when zero or multiple tenants are active, so a holder of the stack-wide secret cannot steer a webhook at arbitrary tenant mappings. ### Token refresh ownership @@ -64,17 +67,19 @@ Atlassian **rotates the `refresh_token` on every use**. Only trusted Lambda code - (+) Jira comments and workflow history identify the dedicated `bgagent` app instead of the OAuth setup user. - (+) Inbound human attribution remains independent: `JiraUserMappingTable` still controls task ownership, concurrency, cost, and audit. - (+) One identity-selection rule covers Lambda and agent writes, and a configured app failure cannot silently change actor. -- (+) Per-tenant credential isolation, signature binding, and the changelog-diff trigger keep the trust and re-trigger semantics correct for multi-tenant installs. +- (+) Synchronizing the tenant and stack-wide signing-secret copies keeps admin-console webhook verification deterministic. - (-) Operators deploy and install a small Forge app per Atlassian environment and manage one additional HMAC secret. +- (-) The Jira channel supports exactly one active tenant because admin-console webhook payloads provide no tenant-routing key. - (-) Forge web-trigger and invocation limits become part of the outbound path. - (-) ADF→markdown is lossy by design (text/headings/lists + external image URLs only); rich content in descriptions is flattened, and `file`-type attachment media (needing a Jira API round-trip) are skipped. - (!) `cloudId` must be used consistently as the tenant key. Indexing on domain or site name anywhere would break tenant resolution. -- (!) The webhook signing secret lives on the per-tenant OAuth bundle; rotating it in Jira without re-running `bgagent jira setup` causes silent 401s on every delivery. +- (!) The webhook signing secret lives in both the per-tenant OAuth bundle and the stack-wide verifier; after rotating it in Jira, run `bgagent jira update-webhook-secret ` to synchronize both copies and avoid silent 401s. ## References - Issue: [#288 — Jira Cloud integration (parity with Linear)](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/288) - Issue: [#642 — give Jira outbound actions a bgagent app identity](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/642) +- Issue: [#709 — repair Jira webhook admission and secret rotation](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/709) - [JIRA_SETUP_GUIDE.md](/sample-autonomous-cloud-coding-agents/using/jira-setup-guide) — operational walkthrough - [LINEAR_SETUP_GUIDE.md](/sample-autonomous-cloud-coding-agents/using/linear-setup-guide) — the analog integration this mirrors - Reference implementation: `cdk/src/constructs/jira-integration.ts`, `cdk/src/handlers/jira-*.ts`, `cdk/src/handlers/shared/jira-{verify,oauth-resolver,feedback}.ts`, `agent/src/jira_reactions.py`, `agent/src/channel_mcp.py` diff --git a/docs/src/content/docs/using/Jira-setup-guide.md b/docs/src/content/docs/using/Jira-setup-guide.md index fa37ee68..5ec3b3bc 100644 --- a/docs/src/content/docs/using/Jira-setup-guide.md +++ b/docs/src/content/docs/using/Jira-setup-guide.md @@ -149,7 +149,19 @@ This runs the OAuth 3LO dance: - **Events** — *Issue: created*, *Issue: updated*, and *Comment: created* - **Secret** — a strong random value, e.g. `openssl rand -hex 32` -Paste that same secret value back at the `Webhook signing secret:` prompt. ABCA stores it on the per-tenant OAuth bundle and seeds the stack-wide single-tenant fallback only when it is still unset. The receiver looks up the tenant value to verify `X-Hub-Signature` on each delivery. +Paste that same secret value back at the `Webhook signing secret:` prompt. ABCA stores it on the per-tenant OAuth bundle and synchronizes the stack-wide verifier. The receiver looks up that value to verify `X-Hub-Signature` on each delivery. + +> **One active tenant for Jira admin-console webhooks.** Webhooks created under **Settings → System → Webhooks** do not include `cloudId` in their payload, so the receiver cannot select among multiple tenant secrets. `jira setup` therefore refuses to configure a second active Jira tenant through this flow. For the sole active tenant it always synchronizes the tenant bundle and stack-wide verifier, including on setup reruns. + +When recreating the webhook or rotating its secret later, update the secret in Jira and then run: + +```bash +bgagent jira update-webhook-secret +``` + +The command prompts for the new value and updates both required Secrets Manager values without repeating OAuth. Keep the Jira webhook disabled until the command succeeds. + +The operator role running this command needs `cloudformation:DescribeStacks`; `dynamodb:GetItem` and `dynamodb:Scan` on `JiraWorkspaceRegistryTable`; `secretsmanager:GetSecretValue` and `secretsmanager:PutSecretValue` on the tenant's `bgagent-jira-oauth-` secret; and `secretsmanager:PutSecretValue` on the stack-wide ARN from `JiraWebhookSecretArn`. ### 4. Install the dedicated outbound app @@ -297,7 +309,7 @@ Re-running `bgagent jira setup` preserves an existing app-actor configuration. O Atlassian signs each delivery with HMAC-SHA256 over the **raw request body**, delivered as `X-Hub-Signature: sha256=`. The receiver: 1. Computes `HMAC-SHA256(rawBody, secret)` and compares it constant-time against the header value (tolerating a pasted value with or without the `sha256=` prefix). -2. Prefers the **per-tenant** signing secret stored on `bgagent-jira-oauth-`; falls back to the stack-wide `JiraWebhookSecret` for installs that predate per-tenant storage. +2. Uses the per-tenant signing secret when the payload carries `cloudId`. Admin-console payloads omit it, so they use the synchronized stack-wide `JiraWebhookSecret` and bind to the sole active tenant. 3. Rejects with 401 on mismatch. The body must be verified as the *raw unparsed bytes* — never parsed-and-restringified JSON, which would change the byte sequence and break the HMAC. @@ -308,6 +320,7 @@ The body must be verified as the *raw unparsed bytes* — never parsed-and-restr - **`jira:issue_updated`** — triggers only if the label was **newly added** in this update. Jira reports label changes in `changelog.items[]` (`field: "labels"`, with `fromString` / `toString`), *not* by re-sending the full label list. The processor diffs the changelog rather than inspecting `issue.fields.labels`, so re-saving an issue that already has the label does not re-trigger. - **`comment_created`** — triggers only when the new comment contains a token-bounded `@bgagent` mention and the issue has a prior ABCA pull request. - All other event types get a silent `200`. +- Issues outside active project mappings are always silent, even if they use the same label. This includes explicit `@bgagent` follow-ups after a project mapping is removed: offboarding ends all ABCA interaction for that project. Site-wide Jira activity must not cause ABCA comments in projects that were never onboarded or are no longer connected. ## Comment-triggered PR iteration @@ -386,7 +399,7 @@ The receiver dedupes issue events on `{issueKey}#{webhookEvent}#{timestamp}` and ### Webhook signature verification fails repeatedly (401) -The signing secret stored for this tenant doesn't match what Jira is sending. Most often the value pasted at the `Webhook signing secret:` prompt differs from the one entered in Jira's webhook config (or the webhook secret was rotated in Jira). Re-run `bgagent jira setup` for the tenant and re-enter matching values. To inspect what's stored: +The signing secret stored for this tenant doesn't match what Jira is sending. Most often the value entered in Jira differs from ABCA's copy, or the webhook was recreated with a new secret. Run `bgagent jira update-webhook-secret ` and enter the exact value configured in Jira. This synchronizes both locations required by admin-console webhooks. To inspect what's stored: ```bash aws secretsmanager get-secret-value \ @@ -394,6 +407,17 @@ aws secretsmanager get-secret-value \ --query SecretString --output text | jq .webhook_signing_secret ``` +### Linking succeeds but a trigger says the Jira user is unlinked + +Jira attributes a trigger to the account in the webhook's `user.accountId`. That may differ from the issue reporter, creator, or the OAuth account ABCA uses to post comments. The name shown above an ABCA comment is therefore not proof that the same account triggered the event. + +Check the webhook-processor warning for `jira_account_id`, `jira_account_source`, and `jira_identity_lookup_key`. The failure comment on an onboarded, explicitly triggered issue also prints the selected account ID. Invite and link that exact account: + +```bash +bgagent jira invite-user +bgagent jira link +``` + ### `setup` hangs at "Waiting for browser callback…" The consent redirect never reached the CLI's localhost listener — see the note under [Step 2](#2-authorize-the-app-on-the-tenant). Ctrl-C and re-run `bgagent jira setup`; re-running is safe.