RFC-0002: publish workbench public API contract - #16
Conversation
* feat: orchestrate legacy batch processing * docs: expand public workbench API contract (#18)
* feat: secure worker runtime with OIDC * RFC-0002: reject cross-tenant domain jobs (#15) * fix: reject cross-tenant domain jobs * RFC-0002: publish workbench public API contract (#16) * feat: publish versioned public api contract * RFC-0002: orchestrate legacy batch processing (#17) * feat: orchestrate legacy batch processing * docs: expand public workbench API contract (#18)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98c3a56ea0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| @Get('metrics') | ||
| @Public() |
There was a problem hiding this comment.
With @Public() here, both AccessTokenGuard and PermissionsGuard short-circuit on the public-route metadata, so GET /api/metrics no longer requires a bearer token or observability.read despite the following decorator. In deployments where metrics are intended to be permissioned as before this change, this exposes worker, queue, and legacy batch metrics to unauthenticated callers.
Useful? React with 👍 / 👎.
| 'LEGACY_EXPORT', | ||
| 'LEGACY_IMPORT', |
There was a problem hiding this comment.
CreateJobV1Dto and JobsController.validateCreateJob both accept anything in JOB_TYPES, so adding LEGACY_EXPORT/LEGACY_IMPORT here lets callers with only workbench.jobs.write submit internal legacy batch worker jobs through /api/jobs. In tenants where job submitters are not compliance managers, they can trigger processing of an existing queued legacy batch by knowing its batch_id, bypassing the compliance.manage gate on LegacyBatchesController.
Useful? React with 👍 / 👎.
| return this.runtime.getManager().markDelivered( | ||
| req.tenantId, batchId, body.authority_reference, | ||
| body.delivered_at ? new Date(body.delivered_at) : undefined, | ||
| ).then((receipt) => this.present(receipt)); |
There was a problem hiding this comment.
For POST /api/regulatory-exports/:batchId/delivery, the handler requires Idempotency-Key but then calls markDelivered without passing the key or a request hash into persistence. When a client retries a timed-out delivery request, or reuses the same key with a different delivery body, the legacy batch store has no way to replay or reject based on that key, unlike the export/import creation paths that persist idempotency inputs.
Useful? React with 👍 / 👎.
|
|
||
| private async enqueue(batchId: string, tenantId: string, type: 'LEGACY_EXPORT' | 'LEGACY_IMPORT'): Promise<void> { | ||
| await this.jobs.enqueue({ | ||
| job_id: `legacy-${batchId}`, queue: 'legacy', type, tenant_id: tenantId, |
There was a problem hiding this comment.
This makes the legacy worker job ID legacy-${batchId}. I checked src/controllers/jobs.controller.ts, and GET /api/jobs/:jobId reads store.get(jobId) without comparing the job's tenant to the requester, so any principal with workbench.read and a leaked or known batch UUID can call /api/jobs/legacy-<uuid> and read status/result/error details for another tenant's compliance batch. Keeping these job IDs opaque or scoping job reads by tenant avoids turning the batch ID into a global lookup key.
Useful? React with 👍 / 👎.
| catch (error) { | ||
| if (error instanceof BadRequestException) throw error; | ||
| if (error instanceof LegacyBatchConflictError || error instanceof LegacyBatchStateError) throw new ConflictException(error.message); | ||
| throw new BadRequestException((error as Error).message); |
There was a problem hiding this comment.
When the legacy store or connector layer fails unexpectedly in production, this catch-all maps every non-conflict exception to 400 Bad Request. That makes server-side outages or database errors look like client contract errors, which can suppress retries and alerting and leaves callers unable to distinguish malformed input from an unavailable service; unknown exceptions should be allowed to surface as 5xx while only validation errors become 400s.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Stacked on the RFC-0002 tenant isolation branch.