fix: atlas connectCluster defaults to readOnly DB roles - MCP-125 #471
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR extracts role determination logic from the ConnectClusterTool into a reusable function that defaults to read-only database roles unless all tools are enabled. The change improves security by being more restrictive with database permissions.
- Extracted role determination logic into a reusable
getDefaultRoleFromConfigfunction - Changed default behavior to assign read-only roles when any write tools are disabled
- Added comprehensive unit tests for the new role determination logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/common/atlas/roles.ts | New module containing the extracted role determination logic |
| src/tools/atlas/connect/connectCluster.ts | Refactored to use the new role determination function |
| tests/unit/common/roles.test.ts | Unit tests covering various role determination scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Pull Request Test Coverage Report for Build 17161333946Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
src/common/atlas/roles.ts
Outdated
| }; | ||
| } | ||
|
|
||
| // If all write tools are enabled, use readWriteAnyDatabase |
There was a problem hiding this comment.
This is a bit hard to follow due to the double negation, but I don't think this is the right condition to check - if any write tool is enabled, we should default to readWrite, otherwise we're risking some tools to not work as intended. For example, if I want to be able to insert new data, but not delete existing documents, I may disable delete tools, but I'll still need the readWriteAnyDatabase user.
There was a problem hiding this comment.
This is a bit hard to follow due to the double negation, but I don't think this is the right condition to check - if any write tool is enabled, we should default to readWrite, otherwise we're risking some tools to not work as intended.
Oh, got it! I thought that we were trying to avoid any writes if any of them are disabled.
For example, if I want to be able to insert new data, but not delete existing documents, I may disable delete tools, but I'll still need the readWriteAnyDatabase user.
Fair, I thought we wanted to be more strict, but I agree that this would just cause people to enable all tools
Proposed changes
Checklist