feat: add the subgraph-viewer role
#2022
Conversation
WalkthroughA new RBAC role, Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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). (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
controlplane/test/feature-subgraph/create-feature-subgraph.test.ts (1)
433-433: Fix the test descriptionThe test description says "feature flag" but this test is actually verifying feature subgraph creation permissions.
- ])('%s should not be able to create feature flag', async (role) => { + ])('%s should not be able to create feature subgraph', async (role) => {controlplane/test/feature-subgraph/delete-feature-subgraph.test.ts (1)
197-197: Fix the test descriptionThe test description says "feature flag" but this test is actually verifying feature subgraph deletion permissions.
- ])('%s should not be able to create feature flag', async (role) => { + ])('%s should not be able to delete feature subgraph', async (role) => {controlplane/src/core/repositories/SubgraphRepository.ts (1)
1395-1397: Consider enhancing the deprecation comment with migration guidance.While marking the method as deprecated is appropriate, consider adding context about why it's deprecated and what developers should use instead.
/** + * @deprecated Use getSubgraphMembersByTargetId instead. This method will be removed in a future version. - * @deprecated */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
controlplane/migrations/0126_certain_puppet_master.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/subgraph/getSubgraphMembers.ts(2 hunks)controlplane/src/core/bufservices/subgraph/getSubgraphSDLFromLatestComposition.ts(2 hunks)controlplane/src/core/repositories/FeatureFlagRepository.ts(2 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(3 hunks)controlplane/src/core/services/RBACEvaluator.ts(1 hunks)controlplane/src/db/schema.ts(1 hunks)controlplane/test/api-keys.test.ts(3 hunks)controlplane/test/cache-warmer/configure-cache-warmer.test.ts(1 hunks)controlplane/test/cache-warmer/delete-cache-operation.test.ts(1 hunks)controlplane/test/cache-warmer/push-cache-operation.test.ts(1 hunks)controlplane/test/check-federated-graph.test.ts(1 hunks)controlplane/test/check-subgraph-schema.test.ts(1 hunks)controlplane/test/feature-flag/create-feature-flag.test.ts(1 hunks)controlplane/test/feature-flag/delete-feature-flag.test.ts(1 hunks)controlplane/test/feature-flag/update-feature-flag.test.ts(1 hunks)controlplane/test/feature-subgraph/create-feature-subgraph.test.ts(1 hunks)controlplane/test/feature-subgraph/delete-feature-subgraph.test.ts(1 hunks)controlplane/test/feature-subgraph/get-feature-subgraphs.test.ts(1 hunks)controlplane/test/feature-subgraph/update-feature-subgraph.test.ts(1 hunks)controlplane/test/federated-graph/version.test.ts(2 hunks)controlplane/test/monograph/version.test.ts(2 hunks)controlplane/test/namespace/get-namespaces.test.ts(5 hunks)controlplane/test/proposal/create-proposal.test.ts(1 hunks)controlplane/test/proposal/update-proposal.test.ts(1 hunks)controlplane/test/rbac-evaluator.test.ts(2 hunks)controlplane/test/subgraph/create-subgraph.test.ts(3 hunks)controlplane/test/subgraph/get-subgraphs.test.ts(1 hunks)controlplane/test/subgraph/publish-subgraph.test.ts(2 hunks)controlplane/test/subgraph/update-subgraph.test.ts(1 hunks)studio/src/lib/constants.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
controlplane/test/feature-subgraph/create-feature-subgraph.test.ts (1)
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: docker-compose.yml:102-103
Timestamp: 2025-07-03T10:33:38.291Z
Learning: In the wundergraph/cosmo project, anonymous admin access in Grafana (GF_AUTH_ANONYMOUS_ENABLED=true and GF_AUTH_ANONYMOUS_ORG_ROLE=Admin) is intentionally configured for development environments in docker-compose.yml files.
controlplane/test/subgraph/create-subgraph.test.ts (1)
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: docker-compose.yml:102-103
Timestamp: 2025-07-03T10:33:38.291Z
Learning: In the wundergraph/cosmo project, anonymous admin access in Grafana (GF_AUTH_ANONYMOUS_ENABLED=true and GF_AUTH_ANONYMOUS_ORG_ROLE=Admin) is intentionally configured for development environments in docker-compose.yml files.
controlplane/test/rbac-evaluator.test.ts (1)
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: docker-compose.yml:102-103
Timestamp: 2025-07-03T10:33:38.291Z
Learning: In the wundergraph/cosmo project, anonymous admin access in Grafana (GF_AUTH_ANONYMOUS_ENABLED=true and GF_AUTH_ANONYMOUS_ORG_ROLE=Admin) is intentionally configured for development environments in docker-compose.yml files.
controlplane/test/feature-subgraph/delete-feature-subgraph.test.ts (1)
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: docker-compose.yml:102-103
Timestamp: 2025-07-03T10:33:38.291Z
Learning: In the wundergraph/cosmo project, anonymous admin access in Grafana (GF_AUTH_ANONYMOUS_ENABLED=true and GF_AUTH_ANONYMOUS_ORG_ROLE=Admin) is intentionally configured for development environments in docker-compose.yml files.
🧬 Code Graph Analysis (2)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
controlplane/src/db/schema.ts (1)
namespaces(461-482)
controlplane/test/rbac-evaluator.test.ts (1)
controlplane/src/core/test-util.ts (2)
createTestGroup(181-198)createTestRBACEvaluator(173-175)
⏰ 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_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (42)
controlplane/migrations/meta/_journal.json (1)
887-893: LGTM! Properly formatted migration journal entry.The migration entry follows the standard Drizzle format and correctly tracks the database schema change for adding the
subgraph-viewerrole to the PostgreSQL enum.controlplane/test/feature-subgraph/get-feature-subgraphs.test.ts (1)
191-191: LGTM! Appropriate read access for viewer role.Adding
subgraph-viewerto this test correctly validates that the new role has read access to feature subgraphs from allowed namespaces, which aligns with the expected viewer permissions.controlplane/test/subgraph/update-subgraph.test.ts (1)
128-128: LGTM! Correct write access restriction for viewer role.Properly restricts the
subgraph-viewerrole from update operations, ensuring it remains read-only as intended for a viewer role.controlplane/test/proposal/create-proposal.test.ts (1)
272-272: LGTM! Appropriate restriction on proposal creation.Correctly prevents the
subgraph-viewerrole from creating proposals, maintaining its read-only nature and ensuring proper separation of permissions.controlplane/test/cache-warmer/delete-cache-operation.test.ts (1)
260-260: LGTM! Proper restriction on cache operation deletion.Appropriately restricts the
subgraph-viewerrole from deleting cache operations, maintaining consistency with its read-only permissions and ensuring proper access control.controlplane/test/feature-flag/update-feature-flag.test.ts (1)
340-340: LGTM: Consistent RBAC implementationThe addition of
'subgraph-viewer'to the unauthorized roles list correctly enforces the read-only nature of this role for feature flag operations.controlplane/test/cache-warmer/push-cache-operation.test.ts (1)
655-655: LGTM: Proper access control implementationThe inclusion of
'subgraph-viewer'in the unauthorized roles list correctly prevents this read-only role from configuring cache warmer operations.controlplane/migrations/0126_certain_puppet_master.sql (1)
1-1: LGTM: Clean database migrationThe migration correctly adds the new
'subgraph-viewer'enum value to the PostgreSQLorganization_roletype using standard syntax.controlplane/test/feature-subgraph/create-feature-subgraph.test.ts (1)
432-432: LGTM: Correct role authorizationThe addition of
'subgraph-viewer'to the unauthorized roles list properly enforces read-only permissions for feature subgraph creation.controlplane/test/feature-subgraph/delete-feature-subgraph.test.ts (1)
196-196: LGTM: Consistent access controlThe addition of
'subgraph-viewer'correctly restricts this read-only role from deleting feature subgraphs.controlplane/test/feature-flag/create-feature-flag.test.ts (1)
228-228: LGTM! Appropriate restriction for viewer role.The addition of
'subgraph-viewer'to the list of roles that cannot create feature flags is consistent with the expected read-only permissions of a viewer role.controlplane/test/monograph/version.test.ts (2)
155-155: LGTM! Correct access restriction for subgraph-focused role.Adding
'subgraph-viewer'to the restricted roles is appropriate since monograph operations are outside the scope of a subgraph-focused viewer role.
439-440: LGTM! Appropriate write operation restriction.The addition of
'subgraph-viewer'to roles that cannot update router compatibility versions correctly enforces the read-only nature of viewer roles.controlplane/test/proposal/update-proposal.test.ts (1)
185-185: LGTM! Correct restriction for proposal state management.Adding
'subgraph-viewer'to the list of roles that cannot update proposal states is appropriate, as proposal approval is a write operation that should be restricted to roles with elevated permissions.controlplane/src/db/schema.ts (1)
1349-1349: LGTM! New subgraph-viewer role properly added to schema.The addition of
'subgraph-viewer'to theorganizationRoleEnumis correctly positioned and follows the established naming convention. This change aligns with the PR objective and is consistent with the broader implementation of the new role across the codebase.controlplane/test/feature-flag/delete-feature-flag.test.ts (1)
128-128: Correct authorization test coverage for subgraph-viewer role.The addition of
'subgraph-viewer'to the list of roles that should not be able to delete feature flags is appropriate. This aligns with the read-only nature of viewer roles and ensures proper test coverage for the new role's authorization constraints.controlplane/test/feature-subgraph/update-feature-subgraph.test.ts (1)
253-253: Appropriate test coverage for subgraph-viewer update restrictions.Adding
'subgraph-viewer'to the roles that should not be able to update feature subgraphs is correct. This maintains the read-only access pattern for viewer roles and ensures comprehensive authorization testing.controlplane/test/cache-warmer/configure-cache-warmer.test.ts (1)
201-201: Consistent authorization enforcement for cache warmer configuration.The inclusion of
'subgraph-viewer'in the roles that cannot configure the cache warmer is appropriate. This maintains the read-only access model for viewer roles and ensures proper authorization boundaries are tested.controlplane/test/api-keys.test.ts (1)
172-172: LGTM! Consistent RBAC implementation for the new viewer role.The addition of
'subgraph-viewer'to the test cases that verify unauthorized access for API key operations is correct. As a read-only role,subgraph-viewershould not have permissions to create, update, or delete API keys.Also applies to: 208-208, 253-253
controlplane/test/subgraph/publish-subgraph.test.ts (1)
103-103: LGTM! Proper enforcement of read-only permissions for the viewer role.The inclusion of
'subgraph-viewer'in the test cases that verify unauthorized access for subgraph publishing operations is appropriate. This ensures the viewer role maintains read-only access and cannot perform write operations.Also applies to: 240-240
controlplane/test/subgraph/get-subgraphs.test.ts (1)
192-192: LGTM! Appropriate read access granted to the viewer role.The addition of
'subgraph-viewer'to the test that verifies the ability to list subgraphs from allowed namespaces is correct. This ensures the viewer role has the necessary read permissions while maintaining the namespace-based access control.controlplane/src/core/bufservices/subgraph/getSubgraphSDLFromLatestComposition.ts (2)
13-13: LGTM! Proper import for authorization error handling.The import of
UnauthorizedErroris necessary for the new authorization check implemented below.
41-43: LGTM! Well-implemented authorization check.The authorization logic is correctly placed and implemented:
- Validates user permissions after confirming resource existence
- Uses the appropriate RBAC method
hasSubGraphReadAccess- Throws the correct error type for unauthorized access
- Prevents unauthorized SDL retrieval while maintaining proper error handling flow
controlplane/test/check-subgraph-schema.test.ts (1)
248-248: LGTM! Consistent restriction of write operations for the viewer role.The addition of
'subgraph-viewer'to the test case that verifies unauthorized access for schema operations (create, publish, check) is appropriate. This maintains the read-only nature of the viewer role across all subgraph operations.controlplane/test/check-federated-graph.test.ts (1)
372-373: LGTM! Proper test coverage for new roles.The addition of
'subgraph-publisher'and'subgraph-viewer'to the list of roles that cannot check graphs for composition errors is appropriate. These roles should have limited scopes -subgraph-publisherfor publishing subgraph schemas only, andsubgraph-viewerfor read-only access to subgraphs.studio/src/lib/constants.ts (1)
276-281: LGTM! Well-structured role definition.The new
subgraph-viewerrole is properly defined with all required properties and follows the established patterns. The description clearly communicates the read-only nature of this role.controlplane/src/core/bufservices/subgraph/getSubgraphMembers.ts (1)
11-11: LGTM! Proper authorization implementation.The authorization check is correctly implemented:
- Placed after existence verification to avoid information leakage
- Uses the appropriate RBAC method
hasSubGraphReadAccess- Throws the correct error type for unauthorized access
- Follows security best practices
Also applies to: 38-40
controlplane/test/federated-graph/version.test.ts (2)
154-154: LGTM! Appropriate test coverage for subgraph-viewer role.Correctly adding
'subgraph-viewer'to roles that cannot read federated graph versions. This role should be limited to subgraph read access only.
412-412: LGTM! Appropriate test coverage for subgraph-publisher role.Correctly adding
'subgraph-publisher'to roles that cannot update router compatibility versions. This role should be limited to publishing subgraph schemas.controlplane/src/core/services/RBACEvaluator.ts (1)
221-224: LGTM! Proper RBAC implementation for subgraph read access.The modification correctly implements explicit read access checking by including all relevant subgraph roles:
'subgraph-admin','subgraph-publisher', and the new'subgraph-viewer'. This approach is more maintainable and clear than relying on write access methods for read permissions.controlplane/test/namespace/get-namespaces.test.ts (3)
25-34: LGTM! Comprehensive test coverage for the new subgraph-viewer role.The addition of
'subgraph-viewer'to the namespace retrieval tests ensures that the new role has appropriate read access to namespaces, which is essential for a viewer role.
76-82: Consistent test coverage across namespace access scenarios.The systematic addition of
'subgraph-publisher'and'subgraph-viewer'roles across all namespace access test scenarios provides comprehensive validation of the new RBAC permissions.Also applies to: 110-114, 145-149
209-213: Critical test coverage for subgraph-viewer namespace access.Adding
'subgraph-viewer'to this test ensures that viewers can properly access namespaces containing subgraphs they're permitted to view, which is fundamental to the role's functionality.controlplane/src/core/repositories/SubgraphRepository.ts (1)
592-614: RBAC logic correctly implements subgraph-viewer role.The implementation follows the established pattern for role handling, properly extending query conditions to include namespaces and resources accessible to the
subgraph-viewerrole. This ensures consistent read access permissions across the subgraph repository.controlplane/test/subgraph/create-subgraph.test.ts (2)
186-187: Correct permissions and improved test description.Adding
'subgraph-viewer'to the roles that cannot create subgraphs is appropriate for a read-only viewer role. The test description fix also improves clarity and readability.
413-415: Consistent RBAC permission model across test suites.The role permission alignment between regular and RBAC-enabled tests ensures consistent behavior. Restricting
'subgraph-publisher'and'subgraph-viewer'from creating subgraphs maintains the appropriate permission hierarchy.Also applies to: 444-445
controlplane/src/core/repositories/FeatureFlagRepository.ts (2)
255-256: LGTM: Correctly addssubgraph-viewerrole to RBAC evaluation.The implementation follows the established pattern for role checking and correctly includes the new role in the condition that determines if any subgraph-related roles are present.
273-276: LGTM: Proper handling ofsubgraph-viewernamespaces and resources.The implementation correctly follows the established pattern for collecting namespaces and resources from the role, ensuring proper RBAC scope application for feature flag queries.
controlplane/test/rbac-evaluator.test.ts (4)
19-19: LGTM: Properly defines test group for the new role.The
subgraph-viewertest group follows the established pattern and correctly sets up the role for testing.
671-695: LGTM: Comprehensive test for global read-only access.The test correctly verifies that
subgraph-viewerhas only read access to subgraphs while ensuring all other permissions are denied. The assertions properly validate the read-only nature of this role.
697-724: LGTM: Proper validation of namespace-scoped read access.The test correctly verifies that
subgraph-viewerwith namespace restrictions only has read access to subgraphs within the granted namespace, while maintaining denial of all other permissions.
726-754: LGTM: Accurate testing of resource-scoped read access.The test correctly validates that
subgraph-viewerwith resource restrictions only has read access to specifically granted subgraph resources, while properly denying access to non-granted resources and all other permissions.
…-for-subgraphs # Conflicts: # controlplane/migrations/meta/0126_snapshot.json # controlplane/migrations/meta/_journal.json
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Checklist