Skip to content

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
merged 15 commits into from
Jul 17, 2025
44 changes: 44 additions & 0 deletions src/common/atlas/accessListUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { ApiClient } from "./apiClient.js";
import logger, { LogId } from "../logger.js";
import { ApiClientError } from "./apiClientError.js";

export async function makeCurrentIpAccessListEntry(
apiClient: ApiClient,
projectId: string,
comment: string = "Added by Atlas MCP"
) {
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> {
// Get the current public IP
const entry = await makeCurrentIpAccessListEntry(apiClient, projectId, "Added by MCP pre-run access list helper");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Is that comment helpful externally? If I was a user and saw that, I wouldn't know what it means.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated a bit more, but I think it's helpful to know where the new access list entry came from

try {
await apiClient.createProjectIpAccessList({
params: { path: { groupId: projectId } },
body: [entry],
});
logger.debug(
LogId.atlasIpAccessListAdded,
"atlas-connect-cluster",
`IP access list created: ${JSON.stringify(entry)}`
);
} catch (err) {
if (err instanceof ApiClientError && err.response?.status === 409) {
// 409 Conflict: entry already exists, ignore
return;
}
throw err;
}
}
1 change: 1 addition & 0 deletions src/common/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const LogId = {
atlasConnectAttempt: mongoLogId(1_001_005),
atlasConnectSucceeded: mongoLogId(1_001_006),
atlasApiRevokeFailure: mongoLogId(1_001_007),
atlasIpAccessListAdded: mongoLogId(1_001_008),

telemetryDisabled: mongoLogId(1_002_001),
telemetryEmitFailure: mongoLogId(1_002_002),
Expand Down
15 changes: 15 additions & 0 deletions src/tools/atlas/connect/connectCluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ToolArgs, OperationType } from "../../tool.js";
import { generateSecurePassword } from "../../../helpers/generatePassword.js";
import logger, { LogId } from "../../../common/logger.js";
import { inspectCluster } from "../../../common/atlas/cluster.js";
import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js";

const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours

Expand Down Expand Up @@ -56,6 +57,19 @@ export class ConnectClusterTool extends AtlasToolBase {
"atlas-connect-cluster",
`error querying cluster: ${error.message}`
);

// sometimes the error can be "error querying cluster: bad auth : Authentication failed."
// which just means it's still propagating permissions to the cluster
// in that case, we want to classify this as connecting.
if (error.message.includes("Authentication failed")) {
logger.debug(
LogId.atlasConnectFailure,
"atlas-connect-cluster",
`assuming connecting to cluster: ${this.session.connectedAtlasCluster?.clusterName}`
);
return "connecting";
}

return "unknown";
}
}
Expand Down Expand Up @@ -198,6 +212,7 @@ export class ConnectClusterTool extends AtlasToolBase {
}

protected async execute({ projectId, clusterName }: ToolArgs<typeof this.argsShape>): Promise<CallToolResult> {
await ensureCurrentIpInAccessList(this.session.apiClient, projectId);
for (let i = 0; i < 60; i++) {
const state = await this.queryConnection(projectId, clusterName);
switch (state) {
Expand Down
12 changes: 6 additions & 6 deletions src/tools/atlas/create/createAccessList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { z } from "zod";
import { CallToolResult } from "@modelcontextprotocol/sdk/types.js";
import { AtlasToolBase } from "../atlasTool.js";
import { ToolArgs, OperationType } from "../../tool.js";
import { makeCurrentIpAccessListEntry } from "../../../common/atlas/accessListUtils.js";

const DEFAULT_COMMENT = "Added by Atlas MCP";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated now - should we move it to accessListUtils?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated!


Expand Down Expand Up @@ -38,12 +39,11 @@ export class CreateAccessListTool extends AtlasToolBase {
}));

if (currentIpAddress) {
const currentIp = await this.session.apiClient.getIpInfo();
const input = {
groupId: projectId,
ipAddress: currentIp.currentIpv4Address,
comment: comment || DEFAULT_COMMENT,
};
const input = await makeCurrentIpAccessListEntry(
this.session.apiClient,
projectId,
comment || DEFAULT_COMMENT
);
ipInputs.push(input);
}

Expand Down
2 changes: 2 additions & 0 deletions src/tools/atlas/create/createDBUser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { AtlasToolBase } from "../atlasTool.js";
import { ToolArgs, OperationType } from "../../tool.js";
import { CloudDatabaseUser, DatabaseUserRole } from "../../../common/atlas/openapi.js";
import { generateSecurePassword } from "../../../helpers/generatePassword.js";
import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js";

export class CreateDBUserTool extends AtlasToolBase {
public name = "atlas-create-db-user";
Expand Down Expand Up @@ -44,6 +45,7 @@ export class CreateDBUserTool extends AtlasToolBase {
roles,
clusters,
}: ToolArgs<typeof this.argsShape>): Promise<CallToolResult> {
await ensureCurrentIpInAccessList(this.session.apiClient, projectId);
const shouldGeneratePassword = !password;
if (shouldGeneratePassword) {
password = await generateSecurePassword();
Expand Down
2 changes: 2 additions & 0 deletions src/tools/atlas/create/createFreeCluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { CallToolResult } from "@modelcontextprotocol/sdk/types.js";
import { AtlasToolBase } from "../atlasTool.js";
import { ToolArgs, OperationType } from "../../tool.js";
import { ClusterDescription20240805 } from "../../../common/atlas/openapi.js";
import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUtils.js";

export class CreateFreeClusterTool extends AtlasToolBase {
public name = "atlas-create-free-cluster";
Expand Down Expand Up @@ -37,6 +38,7 @@ export class CreateFreeClusterTool extends AtlasToolBase {
terminationProtectionEnabled: false,
} as unknown as ClusterDescription20240805;

await ensureCurrentIpInAccessList(this.session.apiClient, projectId);
await this.session.apiClient.createCluster({
params: {
path: {
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/tools/atlas/accessLists.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,5 +95,23 @@
}
});
});

describe("ensureCurrentIpInAccessList helper", () => {
it("should add the current IP to the access list and be idempotent", async () => {
const apiClient = integration.mcpServer().session.apiClient;
const projectId = getProjectId();
const ipInfo = await apiClient.getIpInfo();
// First call should add the IP
await expect(ensureCurrentIpInAccessList(apiClient, projectId)).resolves.not.toThrow();

Check failure on line 105 in tests/integration/tools/atlas/accessLists.test.ts

View workflow job for this annotation

GitHub Actions / check-style

Cannot find name 'ensureCurrentIpInAccessList'.

Check failure on line 105 in tests/integration/tools/atlas/accessLists.test.ts

View workflow job for this annotation

GitHub Actions / Run Atlas tests

tests/integration/tools/atlas/accessLists.test.ts > atlas > ip access lists > project > with project > ensureCurrentIpInAccessList helper > should add the current IP to the access list and be idempotent

ReferenceError: ensureCurrentIpInAccessList is not defined ❯ tests/integration/tools/atlas/accessLists.test.ts:105:23
// Second call should be a no-op (idempotent)
await expect(ensureCurrentIpInAccessList(apiClient, projectId)).resolves.not.toThrow();

Check failure on line 107 in tests/integration/tools/atlas/accessLists.test.ts

View workflow job for this annotation

GitHub Actions / check-style

Cannot find name 'ensureCurrentIpInAccessList'.
// Check that the IP is present in the access list
const accessList = await apiClient.listProjectIpAccessLists({
params: { path: { groupId: projectId } },
});
const found = accessList.results?.some((entry) => entry.ipAddress === ipInfo.currentIpv4Address);
expect(found).toBe(true);
});
});
});
});
13 changes: 11 additions & 2 deletions tests/integration/tools/atlas/clusters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,20 +82,29 @@
expect(createFreeCluster.inputSchema.properties).toHaveProperty("region");
});

it("should create a free cluster", async () => {
it("should create a free cluster and add current IP to access list", async () => {
const projectId = getProjectId();
const session = integration.mcpServer().session;
const ipInfo = await session.apiClient.getIpInfo();

const response = (await integration.mcpClient().callTool({
name: "atlas-create-free-cluster",
arguments: {
projectId,
name: clusterName,
name: clusterName + "-iptest",
region: "US_EAST_1",
},
})) as CallToolResult;
expect(response.content).toBeInstanceOf(Array);
expect(response.content).toHaveLength(2);
expect(response.content[0]?.text).toContain("has been created");

// Check that the current IP is present in the access list
const accessList = await session.apiClient.listProjectIpAccessLists({
params: { path: { groupId: projectId } },
});
const found = accessList.results?.some((entry) => entry.ipAddress === ipInfo.currentIpv4Address);
expect(found).toBe(true);
});
});

Expand All @@ -120,7 +129,7 @@
})) as CallToolResult;
expect(response.content).toBeInstanceOf(Array);
expect(response.content).toHaveLength(1);
expect(response.content[0]?.text).toContain(`${clusterName} | `);

Check failure on line 132 in tests/integration/tools/atlas/clusters.test.ts

View workflow job for this annotation

GitHub Actions / Run Atlas tests

tests/integration/tools/atlas/clusters.test.ts > atlas > clusters > project > with project > atlas-inspect-cluster > returns cluster data

AssertionError: expected 'Error running atlas-inspect-cluster: …' to contain 'ClusterTest-68765008d21f2dc5bfd7589e …' Expected: "ClusterTest-68765008d21f2dc5bfd7589e | " Received: "Error running atlas-inspect-cluster: [404 Not Found] error calling Atlas API: Not Found; No cluster named ClusterTest-68765008d21f2dc5bfd7589e exists in group 68765009115de670563f13ab." ❯ tests/integration/tools/atlas/clusters.test.ts:132:51
});
});

Expand All @@ -142,7 +151,7 @@
.callTool({ name: "atlas-list-clusters", arguments: { projectId } })) as CallToolResult;
expect(response.content).toBeInstanceOf(Array);
expect(response.content).toHaveLength(2);
expect(response.content[1]?.text).toContain(`${clusterName} | `);

Check failure on line 154 in tests/integration/tools/atlas/clusters.test.ts

View workflow job for this annotation

GitHub Actions / Run Atlas tests

tests/integration/tools/atlas/clusters.test.ts > atlas > clusters > project > with project > atlas-list-clusters > returns clusters by project

AssertionError: expected 'Cluster Name | Cluster Type | Tier | …' to contain 'ClusterTest-68765008d21f2dc5bfd7589e …' - Expected + Received - ClusterTest-68765008d21f2dc5bfd7589e | + Cluster Name | Cluster Type | Tier | State | MongoDB Version | Connection String + ----------------|----------------|----------------|----------------|----------------|---------------- + ClusterTest-68765008d21f2dc5bfd7589e-iptest | FREE | N/A | CREATING | 8.0.11 | N/A ❯ tests/integration/tools/atlas/clusters.test.ts:154:51
});
});

Expand Down
12 changes: 12 additions & 0 deletions tests/integration/tools/atlas/dbUsers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,18 @@ describeWithAtlas("db users", (integration) => {
expect(elements[0]?.text).toContain(userName);
expect(elements[0]?.text).toContain("with password: `");
});

it("should add current IP to access list when creating a database user", async () => {
const projectId = getProjectId();
const session = integration.mcpServer().session;
const ipInfo = await session.apiClient.getIpInfo();
await createUserWithMCP();
const accessList = await session.apiClient.listProjectIpAccessLists({
params: { path: { groupId: projectId } },
});
const found = accessList.results?.some((entry) => entry.ipAddress === ipInfo.currentIpv4Address);
expect(found).toBe(true);
});
});
describe("atlas-list-db-users", () => {
it("should have correct metadata", async () => {
Expand Down
41 changes: 41 additions & 0 deletions tests/unit/accessListUtils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { describe, it, expect, vi } from "vitest";
import { ApiClient } from "../../src/common/atlas/apiClient.js";
import { ensureCurrentIpInAccessList } 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");
expect(apiClient.createProjectIpAccessList).toHaveBeenCalledWith({
params: { path: { groupId: "projectId" } },
body: [
{ groupId: "projectId", ipAddress: "127.0.0.1", comment: "Added by MCP pre-run access list helper" },
],
});
});

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");
expect(apiClient.createProjectIpAccessList).toHaveBeenCalledWith({
params: { path: { groupId: "projectId" } },
body: [
{ groupId: "projectId", ipAddress: "127.0.0.1", comment: "Added by MCP pre-run access list helper" },
],
});
});
});
Loading