BCH-1342: Scope ssp-export-offering subscribe to an instance-level downstream allow-list (Phase 6)#452
Conversation
…wnstream allow-list (Phase 6) Adds an allowed-downstreams allow-list per export offering, managed via a new admin CRUD surface and enforced by a handler-level check in Subscribe. An offering with no allow-list keeps today's type-level default (any downstream with the contributor role may subscribe); once an allow-list exists, only listed downstream SSPs may subscribe, even holding that role. Ships as a handler-level check rather than generic PDP/manifest-driven ABAC because BCH-1319's C1/C2 resource-attribute resolution isn't honoured by any shipped driver yet (no resource-side PIP, ScopeParam is C0-only and unwired, neither driver reads Resource.Props) — documented in the manifest and code comments so it isn't mistaken for that engine capability landing.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds an OSCAL export-offering downstream allow-list model, CRUD API, OpenAPI documentation, subscription enforcement, migration wiring, and unit/integration tests covering default, allowed, denied, idempotent, and invalid cases. ChangesExport offering downstream allow-list
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Subscribe
participant AllowListCheck
participant Database
Client->>Subscribe: Submit offering subscription
Subscribe->>AllowListCheck: Check downstream membership
AllowListCheck->>Database: Query offering allow-list
Database-->>AllowListCheck: Return matching entries
AllowListCheck-->>Subscribe: Allow or deny
Subscribe-->>Client: Return subscription response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs.go`:
- Around line 20841-20899: Add a swaggo `@Description` annotation to the Go
handler implementing DELETE
/oscal/system-security-plans/{id}/export-offerings/{offeringId}/allowed-downstreams/{downstreamSspId},
alongside its existing annotations, then re-run swag init to regenerate the docs
artifact with the description field.
In `@internal/api/handler/oscal/ssp_export_offering_allowlist_test.go`:
- Around line 112-120: Add a test alongside
TestRemoveAllowedDownstreamNotFoundReturns404 named
TestRemoveAllowedDownstreamInvalidUUIDReturns400, using newSSPLeverageTestDB,
newLeverageFixture, and NewSSPExportOfferingHandler; construct the DELETE
context with downstreamSspId set to "not-a-uuid", invoke
RemoveAllowedDownstream, and assert a 400 Bad Request response.
In `@internal/api/handler/oscal/ssp_export_offerings.go`:
- Around line 776-785: Replace uuid.MustParse in the handler constructing
SSPExportOfferingAllowedDownstream with uuid.Parse, handle its returned error by
returning an appropriate client error response, and use the parsed UUID for
DownstreamSSPID. Prefer reusing a parsed UUID from validate() if that method can
return one, while preserving the existing database error handling and success
response.
In `@internal/api/handler/oscal/ssp_leverage.go`:
- Around line 445-452: Move the isDownstreamAllowed check from the
pre-transaction handler flow into the write transaction callback, using tx
rather than h.db so the allow-list read and subsequent write are atomic.
Propagate a dedicated sentinel error from the callback when downstreamAllowed is
false, then translate that sentinel into the existing HTTP 403 response while
preserving other errors as 500 responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5daae63-fd6e-4023-a1db-9874b16186eb
📒 Files selected for processing (13)
docs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/api/handler/oscal/ssp_export_offering_allowlist_test.gointernal/api/handler/oscal/ssp_export_offerings.gointernal/api/handler/oscal/ssp_leverage.gointernal/api/handler/oscal/ssp_leverage_allowlist_integration_test.gointernal/api/handler/oscal/ssp_leverage_allowlist_test.gointernal/api/handler/oscal/ssp_leverage_unit_test.gointernal/authz/manifest.yamlinternal/service/migrator.gointernal/service/relational/ssp_offerings.gointernal/tests/migrate.go
… panic risk, and doc/test gaps Moves the allow-list check inside Subscribe's write transaction so the read and the leverage-link write are atomic, closing a window where a concurrent allow-list change could race an in-flight subscribe. Also replaces a MustParse with proper error handling in AddAllowedDownstream, documents RemoveAllowedDownstream's swagger description, and adds the missing invalid-UUID test case for it.
1 participant
Summary
SSPExportOfferingAllowedDownstream, a per-offering allow-list of downstream SSPs permitted to subscribe.GET/POST/DELETE .../export-offerings/:offeringId/allowed-downstreams[/:downstreamSspId]), gated by the existingssp:exportguard.Subscribenow denies (403) a downstream SSP not on a non-empty allow-list, even if that user holds the contributor role. An offering with no allow-list rows keeps today's type-level default (backwards compatible).allowed_downstreams: set<uuid>attribute is declared ininternal/authz/manifest.yamlfor documentation/future engine wiring, but is not enforced by the PDP — BCH-1319's C1/C2 resource-attribute resolution isn't honoured by any shipped driver yet (no resource-side PIP,ScopeParamis C0-only and unwired, neither driver readsResource.Props). Enforcement is a handler-level check inSubscribe(isDownstreamAllowed), mirroring the existingauthorizeDownstreamUpdatead-hoc pattern already in that same handler. This is called out explicitly in the manifest and code comments.Test plan
go build ./...,go vet ./...(incl.-tags integration),golangci-lint run ./...,go mod tidyall cleango test ./...(unit) passes, including new CRUD round-trip/idempotency/validation and allow-list enforcement testsgo test -tags integration ./internal/api/handler/oscal/...passes (real Postgres via testcontainers), through the real Cedar-driven PEP — proves the AC's literal "even with the contributor role" wording, an offering with no allow-list still allows any downstream, and an allow-listed downstream succeedsmake swagdocs regenerated for the 3 new endpointsSummary by CodeRabbit
New Features
Documentation
Tests