-
Notifications
You must be signed in to change notification settings - Fork 97
chore: Improve access list and connect experienece [MCP-5] #372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3d079c6
WIP
blva 43cf255
MCP-5: Improve accesslist experience
blva a34de86
remove file
blva 46e2e5f
fix test
blva b373956
Merge remote-tracking branch 'origin/main' into MCP-5
blva 39cc667
update test
blva ccb61ac
Revert change
blva 1dda709
update
blva cdb37b9
update test
blva 0f5cb6b
update logs
blva 4389e6c
lint
blva 3c1d3fc
Merge branch 'main' into MCP-5
blva 66ec84f
address comments
blva e2f0d21
update
blva 25e2b58
fix: address comment
fmenezes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { ApiClient } from "./apiClient.js"; | ||
import logger, { LogId } from "../logger.js"; | ||
import { ApiClientError } from "./apiClientError.js"; | ||
|
||
export const DEFAULT_ACCESS_LIST_COMMENT = "Added by MongoDB MCP Server to enable tool access"; | ||
|
||
export async function makeCurrentIpAccessListEntry( | ||
apiClient: ApiClient, | ||
projectId: string, | ||
comment: string = DEFAULT_ACCESS_LIST_COMMENT | ||
) { | ||
const { currentIpv4Address } = await apiClient.getIpInfo(); | ||
return { | ||
groupId: projectId, | ||
ipAddress: currentIpv4Address, | ||
comment, | ||
}; | ||
} | ||
|
||
/** | ||
* Ensures the current public IP is in the access list for the given Atlas project. | ||
* If the IP is already present, this is a no-op. | ||
* @param apiClient The Atlas API client instance | ||
* @param projectId The Atlas project ID | ||
*/ | ||
export async function ensureCurrentIpInAccessList(apiClient: ApiClient, projectId: string): Promise<void> { | ||
const entry = await makeCurrentIpAccessListEntry(apiClient, projectId, DEFAULT_ACCESS_LIST_COMMENT); | ||
try { | ||
await apiClient.createProjectIpAccessList({ | ||
params: { path: { groupId: projectId } }, | ||
body: [entry], | ||
}); | ||
logger.debug( | ||
LogId.atlasIpAccessListAdded, | ||
"accessListUtils", | ||
`IP access list created: ${JSON.stringify(entry)}` | ||
); | ||
} catch (err) { | ||
if (err instanceof ApiClientError && err.response?.status === 409) { | ||
// 409 Conflict: entry already exists, log info | ||
logger.debug( | ||
LogId.atlasIpAccessListAdded, | ||
"accessListUtils", | ||
`IP address ${entry.ipAddress} is already present in the access list for project ${projectId}.` | ||
); | ||
return; | ||
} | ||
logger.debug( | ||
LogId.atlasIpAccessListAddFailure, | ||
"accessListUtils", | ||
`Error adding IP access list: ${err instanceof Error ? err.message : String(err)}` | ||
); | ||
} | ||
} |
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import { describe, it, expect, vi } from "vitest"; | ||
import { ApiClient } from "../../src/common/atlas/apiClient.js"; | ||
import { ensureCurrentIpInAccessList, DEFAULT_ACCESS_LIST_COMMENT } from "../../src/common/atlas/accessListUtils.js"; | ||
import { ApiClientError } from "../../src/common/atlas/apiClientError.js"; | ||
|
||
describe("accessListUtils", () => { | ||
it("should add the current IP to the access list", async () => { | ||
const apiClient = { | ||
getIpInfo: vi.fn().mockResolvedValue({ currentIpv4Address: "127.0.0.1" } as never), | ||
createProjectIpAccessList: vi.fn().mockResolvedValue(undefined as never), | ||
} as unknown as ApiClient; | ||
await ensureCurrentIpInAccessList(apiClient, "projectId"); | ||
// eslint-disable-next-line @typescript-eslint/unbound-method | ||
expect(apiClient.createProjectIpAccessList).toHaveBeenCalledWith({ | ||
params: { path: { groupId: "projectId" } }, | ||
body: [{ groupId: "projectId", ipAddress: "127.0.0.1", comment: DEFAULT_ACCESS_LIST_COMMENT }], | ||
}); | ||
}); | ||
|
||
it("should not fail if the current IP is already in the access list", async () => { | ||
const apiClient = { | ||
getIpInfo: vi.fn().mockResolvedValue({ currentIpv4Address: "127.0.0.1" } as never), | ||
createProjectIpAccessList: vi | ||
.fn() | ||
.mockRejectedValue( | ||
ApiClientError.fromError( | ||
{ status: 409, statusText: "Conflict" } as Response, | ||
{ message: "Conflict" } as never | ||
) as never | ||
), | ||
} as unknown as ApiClient; | ||
await ensureCurrentIpInAccessList(apiClient, "projectId"); | ||
// eslint-disable-next-line @typescript-eslint/unbound-method | ||
expect(apiClient.createProjectIpAccessList).toHaveBeenCalledWith({ | ||
params: { path: { groupId: "projectId" } }, | ||
body: [{ groupId: "projectId", ipAddress: "127.0.0.1", comment: DEFAULT_ACCESS_LIST_COMMENT }], | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is silently ignoring the error - does it make sense that we do that? I assume that this could give users the false impression everything is working and then hit an issue with the IP address config at a later point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for now, there are different configurations where user does not have access to add access list but has accsess to proceed with the tool call, I'd like to avoid causing errors and having the logs to troubleshoot for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear - I'm not sure about those scenarios but was trying to avoid causing disruption. If you think the risk is too low I can throw the error back. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure either, just not ideal that this function seems like a prerequisite for many features, but we're not communicating to the user that something might have gone wrong. I would say this PR is a clear improvement to the status quo, and the worst case of this failing and us logging it is no worse than what we currently have, so fine with keeping it as is. I guess we can at least bump the log level to warn so that it's a bit more prominent.