fix(server,client): gate cluster mutations on manage_connections (#207)#217
Merged
Conversation
Unauthorized users could POST to cluster-group and cluster create endpoints; the server silently committed the rows and returned a success status. An audit identified eleven cluster handlers missing the standard admin gate. All now check manage_connections and return 403 on denial. The web client's Add menu hides "Add Cluster Group" and "Add Cluster" for users without the permission. The OpenAPI spec documents 403 Forbidden on the affected paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR enforces the ChangesRBAC Authorization for Cluster Mutations
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant RBACChecker
participant Datastore
Client->>Handler: Mutation request
Handler->>RBACChecker: Check PermManageConnections
RBACChecker-->>Handler: Denied (403) or Allowed
alt Denied
Handler-->>Client: 403 Forbidden
else Allowed
Handler->>Datastore: Read/Mutate
Datastore-->>Handler: Result
Handler-->>Client: Success/Validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high (1 false positive) |
🟢 Metrics 39 complexity · 53 duplication
Metric Results Complexity 39 Duplication 53
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
server/src/internal/api/cluster_handlers.go (1)
597-629: 💤 Low valueOwner-fallback authorization correctly implemented; minor comment clarification.
The owner-fallback logic is sound: admins with
manage_connectionsOR the group's owner can delete. The pattern matchesupdateClusterGroup(lines 548-595).Minor observation: the comment at line 601 states "before any datastore read so denied callers cannot probe group existence," but
GetClusterGroupexecutes at line 616 before the final authorization decision at line 625. This is necessary for ownership verification, but creates a 404 (not found) vs 403 (forbidden) distinction. The comment could be clarified to say "before request decoding" rather than "before any datastore read."This doesn't affect correctness—the code properly implements the spec's owner-fallback requirement.
🤖 Prompt for 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. In `@server/src/internal/api/cluster_handlers.go` around lines 597 - 629, Update the inline comment in deleteClusterGroup to accurately reflect the order of operations: instead of claiming the authorization check runs "before any datastore read", clarify that the owner-fallback authorization (admins with manage_connections or the group's owner) is applied early — before request decoding/response writing — but that GetClusterGroup is necessarily called to verify ownership; modify the comment around the owner-fallback block (referencing deleteClusterGroup, getUserInfoCompat, HasAdminPermission, and GetClusterGroup) to say something like "authorization enforced early (before request decoding/output); datastore read is performed to verify ownership" so the comment matches the code behavior.server/src/internal/api/rbac_issue207_clusters_test.go (1)
90-105: ⚡ Quick win
assertForbiddenWithMessageshould validate the expected permission-denied wording.The helper comment says it checks canonical permission-denied messaging, but it only enforces non-empty text. Verifying the expected message (or stable substring) would better protect the API contract documented for these 403 paths.
🤖 Prompt for 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. In `@server/src/internal/api/rbac_issue207_clusters_test.go` around lines 90 - 105, The test helper assertForbiddenWithMessage currently only checks for a non-empty error string; update it to assert the canonical permission-denied wording by decoding ErrorResponse (already present) and verifying resp.Error contains the expected message or stable substring (e.g., "permission denied" or the project constant if one exists). Modify assertForbiddenWithMessage to import/use the canonical message constant (or define expectedMsg in the test) and replace the resp.Error == "" check with a strings.Contains(resp.Error, expectedMsg) assertion so the test fails if the 403 message no longer matches the documented contract.client/src/components/__tests__/AddMenu.test.tsx (1)
127-135: ⚡ Quick winAdd an explicit “no divider/separator” assertion in denied states.
You already assert gated items are hidden; asserting no
separatoris rendered will lock the full UI contract and prevent subtle regressions.✅ Suggested test hardening
it('renders only Add Server', () => { renderAddMenu(); expect(screen.getByText('Add Server')).toBeInTheDocument(); expect(screen.queryByText('Add Cluster')).not.toBeInTheDocument(); expect( screen.queryByText('Add Cluster Group'), ).not.toBeInTheDocument(); + expect(screen.queryByRole('separator')).not.toBeInTheDocument(); }); ... it('hides gated items when hasPermission always returns false', () => { mockHasPermission.mockReturnValue(false); renderAddMenu(); expect(screen.getByText('Add Server')).toBeInTheDocument(); expect(screen.queryByText('Add Cluster')).not.toBeInTheDocument(); expect( screen.queryByText('Add Cluster Group'), ).not.toBeInTheDocument(); + expect(screen.queryByRole('separator')).not.toBeInTheDocument(); });Also applies to: 159-163
🤖 Prompt for 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. In `@client/src/components/__tests__/AddMenu.test.tsx` around lines 127 - 135, The test "renders only Add Server" should also assert that no UI divider/separator is rendered when gated items are hidden to lock the contract; update the test that uses renderAddMenu() to include an assertion that screen.queryByRole('separator') (or queryAllByRole('separator') === 0) is not in the document, and apply the same additional assertion to the similar test around the 159-163 block so both denied-state tests verify absence of separators in addition to the hidden menu items.
🤖 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/admin-guide/api/openapi.json`:
- Around line 2495-2496: Update the OpenAPI docs for the deleteClusterGroup
endpoint (operationId: deleteClusterGroup) to reflect the owner-fallback rule:
change the endpoint "description" to state that deletion requires the
manage_connections permission or is permitted for the cluster-group owner via
fallback, and adjust the 403 response description to say "Forbidden: requires
manage_connections permission (owners may be allowed via owner-fallback)" so the
contract matches runtime behavior; apply the same wording change to the
duplicate cluster-group deletion entry referenced around lines 2525-2527.
In `@server/src/internal/api/openapi.go`:
- Around line 1697-1705: The OpenAPI entry for the deleteClusterGroup operation
(OperationID "deleteClusterGroup") currently states manage_connections is always
required; update the operation's description and 403 response text to reflect
the server's owner-fallback behavior so the contract is accurate: change the
Description to mention owners may delete without the permission and update the
403 Response (jsonResponse) text to something like "Requires manage_connections
permission (unless requester is the group owner)" so the Security/security
description and Responses map match the server logic for deleteClusterGroup.
In `@server/src/internal/api/rbac_issue207_clusters_test.go`:
- Around line 157-181: Update the test assertion so it fails if the handler
returns either 403 or 401 instead of only 403: in
TestClusterHandler_CreateClusterGroup_Issue207_SuperuserAllowed (and the similar
test around the other occurrence) check rec.Code against both
http.StatusForbidden and http.StatusUnauthorized after calling
handler.createClusterGroup and call t.Fatalf if rec.Code equals either value so
an unexpected 401 (authentication regression) is treated as a test failure.
---
Nitpick comments:
In `@client/src/components/__tests__/AddMenu.test.tsx`:
- Around line 127-135: The test "renders only Add Server" should also assert
that no UI divider/separator is rendered when gated items are hidden to lock the
contract; update the test that uses renderAddMenu() to include an assertion that
screen.queryByRole('separator') (or queryAllByRole('separator') === 0) is not in
the document, and apply the same additional assertion to the similar test around
the 159-163 block so both denied-state tests verify absence of separators in
addition to the hidden menu items.
In `@server/src/internal/api/cluster_handlers.go`:
- Around line 597-629: Update the inline comment in deleteClusterGroup to
accurately reflect the order of operations: instead of claiming the
authorization check runs "before any datastore read", clarify that the
owner-fallback authorization (admins with manage_connections or the group's
owner) is applied early — before request decoding/response writing — but that
GetClusterGroup is necessarily called to verify ownership; modify the comment
around the owner-fallback block (referencing deleteClusterGroup,
getUserInfoCompat, HasAdminPermission, and GetClusterGroup) to say something
like "authorization enforced early (before request decoding/output); datastore
read is performed to verify ownership" so the comment matches the code behavior.
In `@server/src/internal/api/rbac_issue207_clusters_test.go`:
- Around line 90-105: The test helper assertForbiddenWithMessage currently only
checks for a non-empty error string; update it to assert the canonical
permission-denied wording by decoding ErrorResponse (already present) and
verifying resp.Error contains the expected message or stable substring (e.g.,
"permission denied" or the project constant if one exists). Modify
assertForbiddenWithMessage to import/use the canonical message constant (or
define expectedMsg in the test) and replace the resp.Error == "" check with a
strings.Contains(resp.Error, expectedMsg) assertion so the test fails if the 403
message no longer matches the documented contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 500fe738-7035-4b15-8a26-e0ea5a53a6a8
📒 Files selected for processing (11)
.claude/golang-expert/rbac-patterns.md.claude/react-expert/quality-checklist.mdclient/src/components/AddMenu.tsxclient/src/components/__tests__/AddMenu.test.tsxclient/src/components/__tests__/ClusterNavigator.test.tsxdocs/admin-guide/api/openapi.jsondocs/changelog.mdserver/src/internal/api/cluster_handlers.goserver/src/internal/api/cluster_handlers_test.goserver/src/internal/api/openapi.goserver/src/internal/api/rbac_issue207_clusters_test.go
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/internal/api/cluster_handlers_test.go (1)
203-208: ⚡ Quick winExtract the “permission-satisfied” test handler setup.
The same
NewClusterHandler(nil, nil, auth.NewRBACChecker(nil))pattern and explanation now appear in five tests. A tiny helper would centralize the nil-store RBAC assumption and make future auth changes easier to update in one place.Also applies to: 231-234, 259-262, 476-479, 626-629
🤖 Prompt for 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. In `@server/src/internal/api/cluster_handlers_test.go` around lines 203 - 208, Extract the repeated test setup into a small helper that returns the handler pre-configured with a nil-store RBAC checker (e.g. add a function like permissionSatisfiedHandler() that calls NewClusterHandler(nil, nil, auth.NewRBACChecker(nil)) and returns the handler); replace the five direct usages of NewClusterHandler(nil, nil, auth.NewRBACChecker(nil)) in the tests with calls to this helper so the nil-store RBAC assumption is centralized and easy to update (refer to NewClusterHandler and auth.NewRBACChecker(nil) when locating the occurrences to change).
🤖 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.
Nitpick comments:
In `@server/src/internal/api/cluster_handlers_test.go`:
- Around line 203-208: Extract the repeated test setup into a small helper that
returns the handler pre-configured with a nil-store RBAC checker (e.g. add a
function like permissionSatisfiedHandler() that calls NewClusterHandler(nil,
nil, auth.NewRBACChecker(nil)) and returns the handler); replace the five direct
usages of NewClusterHandler(nil, nil, auth.NewRBACChecker(nil)) in the tests
with calls to this helper so the nil-store RBAC assumption is centralized and
easy to update (refer to NewClusterHandler and auth.NewRBACChecker(nil) when
locating the occurrences to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d7b2eb3-eb94-4a75-a4e6-7dab4b446460
📒 Files selected for processing (2)
server/src/internal/api/cluster_handlers_test.goserver/src/internal/api/rbac_issue207_clusters_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/internal/api/rbac_issue207_clusters_test.go
- OpenAPI deleteClusterGroup wording reflects owner-fallback - assertGatePassed also rejects 401 (auth regression) - assertForbiddenWithMessage validates canonical message substring - Clarify deleteClusterGroup datastore-read comment - Extract permissionSatisfiedHandler helper Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per CodeRabbit nit on PR #217: locks the contract so a regression that leaves the divider visible while hiding the items would still fail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #207 — unauthorized users could POST to cluster-group and cluster
create endpoints; the server silently committed the rows and returned a
success status, but the rows were invisible to the creator (only admins
could see them).
An audit triggered by the report found additional unprotected mutating
routes in
cluster_handlers.go. Eleven handlers in total now checkmanage_connectionsand return403 Forbiddenon denial.Server (
cluster_handlers.go)POST /api/v1/cluster-groupsPOST /api/v1/cluster-groups/{id}/clustersPOST /api/v1/clustersPOST /api/v1/clusters/{id}/serversDELETE /api/v1/clusters/{id}/servers/{connectionId}PUT /api/v1/clusters/{id}/connections/{connId}/relationshipsDELETE /api/v1/clusters/{id}/connections/{connId}/relationshipsDELETE /api/v1/clusters/{id}/relationships/{relationshipId}DELETE /api/v1/cluster-groups/{id}clusterstable hasno
owner_usernamecolumn, so a true owner-fallback isn't possiblewithout a schema change):
PUT /api/v1/clusters/{id}DELETE /api/v1/clusters/{id}The denial check fires before request decoding so payload shape
isn't leaked to unauthorized callers.
Client (
AddMenu.tsx)user lacks
manage_connections. "Add Server" is unchanged.OpenAPI
403 Forbiddenresponse with consistentwording (
Requires manage_connections permission).docs/admin-guide/api/openapi.jsonregenerated.Tests
rbac_issue207_clusters_test.gocovering403-on-denied, success-on-grant, owner-fallback, and "denied-before-
decode" assertions.
AddMenu(100% line coverage on AddMenu.tsx).make test-allpasses; lint warning count unchanged from baseline.Test plan
make test-allpasses locallymanage_connectionsmanage_connections, attempt tocreate a cluster group via the API → expect 403, no row inserted
🤖 Generated with Claude Code
Summary by CodeRabbit
Security
New Features / API
UI Updates
Documentation
Tests