feat: introduce the subgraph-checker role
#2198
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds a new organization role "subgraph-checker", wires it into RBAC evaluation and subgraph-check authorization, updates DB enum/policy and studio role constants, adds a migration and snapshot/journal entries, and extends multiple tests to cover the new role. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
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: 1
🧹 Nitpick comments (1)
controlplane/test/rbac-evaluator.test.ts (1)
697-697: Rename test titles: say “subgraph” instead of “graph” for clarity.Avoid confusion with federated graphs.
Apply this diff:
- test('Should have check access to every graph', () => { + test('Should have check access to every subgraph', () => { - test('Should have check access to every graph in granted namespace', () => { + test('Should have check access to every subgraph in granted namespace', () => { - test('Should have check access to granted graphs', () => { + test('Should have check access to granted subgraphs', () => {Also applies to: 724-724, 755-755
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
controlplane/migrations/0129_fluffy_dagger.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(1 hunks)controlplane/src/core/services/RBACEvaluator.ts(7 hunks)controlplane/src/db/schema.ts(1 hunks)controlplane/test/rbac-evaluator.test.ts(25 hunks)studio/src/lib/constants.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/db/schema.tscontrolplane/test/rbac-evaluator.test.ts
📚 Learning: 2025-09-08T20:57:07.923Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.923Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/test/rbac-evaluator.test.ts
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/test/rbac-evaluator.test.ts
🧬 Code graph analysis (2)
controlplane/test/rbac-evaluator.test.ts (1)
controlplane/src/core/test-util.ts (2)
createTestGroup(181-198)createTestRBACEvaluator(173-175)
controlplane/src/core/services/RBACEvaluator.ts (3)
studio/src/lib/constants.ts (1)
OrganizationRole(290-290)connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
Namespace(16203-16241)FeatureFlag(18281-18355)controlplane/src/db/models.ts (1)
Target(25-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
controlplane/migrations/0129_fluffy_dagger.sql (1)
1-1: PG ≥12 support confirmed
The default Postgres image in bothdocker-compose.ymlanddocker-compose.full.ymlis pinned to version 15.3, which fully supportsALTER TYPE … ADD VALUE … BEFORE. No additional version guard is needed. Optionally, you can addIF NOT EXISTSfor idempotency (supported in PG 15.3+).controlplane/src/db/schema.ts (1)
1352-1354: Enum addition aligns with migration ordering
'subgraph-checker'is correctly placed between'subgraph-publisher'and'subgraph-viewer', matching the SQL migration. LGTM.controlplane/migrations/meta/_journal.json (1)
907-914: Journal entry looks consistentEntry idx 129 with tag
0129_fluffy_daggerand version "7" is correctly appended. No issues.controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts (1)
135-141: Authorization now honors ‘subgraph-checker’ — confirm behavior for non-existent subgraphsSwitch to
hasSubGraphCheckAccess(subgraph)is correct for existing subgraphs. For non-existent subgraphs, access still requirescanCreateSubGraph(namespace). If checkers should be able to run checks for new (not-yet-created) subgraphs, we’ll need a separate path (e.g., namespace-scoped “check” right). Please confirm intended product behavior.controlplane/src/core/services/RBACEvaluator.ts (3)
168-178: Subgraph read now includes checker — good
hasSubGraphCheckAccess+subgraph-viewerfallback makes read semantics clear and minimal. LGTM.
118-122: Creation/deletion refactors look correct; owner override retainedCreation uses namespace-scoped admin roles; deletion permits owner override via
isTargetOwnedByUser. Matches prior behavior with clearer helpers. LGTM.Also applies to: 144-148, 154-160
58-75: Approve use of Object.groupBy
The project’s engines.node is set to ≥22.11.0, which includes native support forObject.groupBy, and it’s already used elsewhere without issues.controlplane/test/rbac-evaluator.test.ts (5)
19-19: Add subgraphChecker test group — looks good.Matches existing test helpers and role naming.
44-44: Matrix assertions for hasSubGraphCheckAccess are consistent across roles.Behavior matches the intended policy: admins/developers/publishers have check access; viewers, graph and namespace roles do not; publisher/admin scope respects namespaces/resources; read is granted where check is granted. Good coverage.
Also applies to: 110-110, 139-139, 168-168, 197-197, 226-226, 256-256, 285-285, 314-314, 344-344, 378-378, 409-409, 439-439, 469-469, 498-498, 527-527, 560-562, 595-596, 626-626, 656-657, 689-693, 812-812, 841-841
696-786: Solid new suite for subgraph-checker.Covers default (all), namespace-scoped, and resource-scoped access, and validates non-escalation to write. Nicely done.
696-786: Incorrect description suggestion
Thesubgraph-checkerentry in studio/src/lib/constants.ts has no description field (onlydisplayName: "Checker"), so there’s no “publish access” label to correct.Likely an incorrect or invalid review comment.
720-722: Check implies read access by design
hasSubGraphReadAccess delegates to hasSubGraphCheckAccess, so any target with “check” permission also grants “read.” The existing tests asserting read=true when check=true are correct and no changes are needed.
…eck-with-read-permission # Conflicts: # controlplane/migrations/meta/0129_snapshot.json # controlplane/migrations/meta/_journal.json # controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controlplane/migrations/meta/0131_snapshot.json (1)
2801-2821: Composite-unique index on linked_schema_checksGood change to allow multiple linked entries per schema_check_id while preventing duplicate pairs. Ensure the SQL migration (0131) actually drops the old unique constraint and creates this index; otherwise your runtime will still enforce the old uniqueness.
If this runs on sizable tables in production, consider CREATE INDEX CONCURRENTLY in a separate, non-transactional step to avoid long locks.
Also applies to: 2882-2882
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controlplane/migrations/0131_known_stepford_cuckoos.sql(1 hunks)controlplane/migrations/meta/0131_snapshot.json(4 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(1 hunks)controlplane/src/db/schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
- controlplane/migrations/meta/_journal.json
- controlplane/src/db/schema.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/migrations/meta/0131_snapshot.json
📚 Learning: 2025-08-31T18:51:32.185Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/bufservices/check/getCheckSummary.ts:0-0
Timestamp: 2025-08-31T18:51:32.185Z
Learning: In the SchemaCheckRepository.getLinkedSchemaCheck method, organization-level security is implemented through post-query validation by checking `check.subgraphs[0].namespace.organizationId !== organizationId` and returning undefined if the linked check doesn't belong to the caller's organization, preventing cross-tenant data leakage.
Applied to files:
controlplane/migrations/meta/0131_snapshot.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (3)
controlplane/migrations/0131_known_stepford_cuckoos.sql (1)
1-1: Ensure Postgres version compatibility and non-transactional execution
“ADD VALUE … BEFORE …” is only supported on sufficiently recent Postgres versions and, on some PG releases, cannot run inside a transaction. Verify locally that your control-plane Postgres server supports this syntax and that your SQL migrations are not wrapped in BEGIN/COMMIT in all environments. Please run:psql -tAc 'SHOW server_version;'and inspect your migration-runner configuration for transactional wrapping of
.sqlfiles.controlplane/migrations/meta/0131_snapshot.json (2)
2-3: Generated IDs updatedLooks like standard snapshot churn; nothing to do.
8441-8443: RBAC surfaces updated; verify DB RLS policies
TS enums (controlplane/src/db/schema.ts), Studio role constants (studio/src/lib/constants.ts), and RBACEvaluator helpers now include “subgraph-checker” immediately before “subgraph-viewer” and use hasSubGraphCheckAccess. No “CREATE POLICY” or RLS definitions referencing organization_role or linked_subgraphs were found—please confirm any DB-level policies are updated for the new role.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JivusAyrus
left a comment
JivusAyrus
left a comment
There was a problem hiding this comment.
Can we write one test in check-subgraph-schema.test to evaluate the new change
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/check-subgraph-schema.test.ts (1)
52-54: Addsubgraph-checkerto allowed roles — correct; nit: tighten test titleThe role switch happens only before the check, not during create/publish. Consider renaming the test title to avoid implying broader privileges.
Apply this minimal edit:
- '%s should be able to create a subgraph, publish the schema and then check with new schema', + '%s can check subgraph schema (creation/publish done as admin)',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/test/check-subgraph-schema.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/test/check-subgraph-schema.test.ts
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/test/check-subgraph-schema.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (2)
controlplane/test/check-subgraph-schema.test.ts (2)
161-163: Namespace-scoped allowance forsubgraph-checker— LGTMIncluding
subgraph-checkeralongside subgraph-scoped roles for namespace-restricted checks aligns with the new RBAC surface.
250-251: Explicitly denyinggraph-checkerfor subgraph checks — LGTMThis enforces clear separation between graph- vs subgraph-level permissions and matches the intent of the new role.
…heck-with-read-permission' into wilson/eng-8027-allow-subgraph-check-with-read-permission
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/check-subgraph-schema.test.ts (1)
52-106: Allow check access for subgraph-checker — consider centralizing role listsGranting "subgraph-checker" check permissions is correct. To avoid role drift across tests, consider extracting shared role arrays (allowed/denied) into a test util constant and reusing them here and in publish/create/update tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controlplane/test/check-subgraph-schema.test.ts(2 hunks)controlplane/test/subgraph/create-subgraph.test.ts(3 hunks)controlplane/test/subgraph/publish-subgraph.test.ts(3 hunks)controlplane/test/subgraph/update-subgraph.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/test/subgraph/update-subgraph.test.tscontrolplane/test/check-subgraph-schema.test.tscontrolplane/test/subgraph/create-subgraph.test.tscontrolplane/test/subgraph/publish-subgraph.test.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
controlplane/test/check-subgraph-schema.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
controlplane/test/subgraph/update-subgraph.test.ts (1)
120-131: RBAC: subgraph-checker correctly denied update accessAdding "subgraph-checker" to the not-authorized roles for update aligns with the role’s intended scope (check-only).
controlplane/test/check-subgraph-schema.test.ts (1)
161-241: RBAC: namespace-scoped check access includes subgraph-checkerGood coverage: authorized when scoped to the namespace, and ERROR_NOT_AUTHORIZED when scope mismatches.
controlplane/test/subgraph/publish-subgraph.test.ts (3)
130-139: RBAC: subgraph-checker correctly cannot publish existing regular subgraphsMatches the role’s check-only intent.
266-276: RBAC: subgraph-checker correctly cannot create-and-publish regular subgraphsExpectation updated to ERROR_NOT_AUTHORIZED is appropriate.
733-743: RBAC: subgraph-checker correctly cannot create-and-publish plugin subgraphsConsistent with role semantics.
controlplane/test/subgraph/create-subgraph.test.ts (3)
185-195: RBAC: subgraph-checker correctly cannot create regular subgraphsNegative case looks good.
455-465: RBAC: subgraph-checker correctly cannot create EDG subgraphsConsistent with check-only permissions.
752-762: RBAC: subgraph-checker correctly cannot create plugin subgraphsTest expectations are correct.
wilsonrivera
deleted the
wilson/eng-8027-allow-subgraph-check-with-read-permission
branch
Summary by CodeRabbit
New Features
Refactor
Tests
Database
Checklist