Skip to content

feat: Allow configuration of temporary user timeout when connecting to atlas cluster #544

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.

Sign up for GitHub

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 10 commits into from
Sep 12, 2025
Merged

feat: Allow configuration of temporary user timeout when connecting to atlas cluster #544

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
83c9639
feat: Allow configuration of temporary user when connecting to atlas …
jeroenvervaeke Sep 11, 2025
4a9d879
renamed temporaryDatabaseUserLifetimeSeconds -> atlasTemporaryDatabas…
jeroenvervaeke Sep 11, 2025
9581337
split options
jeroenvervaeke Sep 11, 2025
ce905bd
fixed shallow merge issue
jeroenvervaeke Sep 11, 2025
90123d3
fixed typing issue not caught by typescript
jeroenvervaeke Sep 11, 2025
b7e0d1b
updated the readme
jeroenvervaeke Sep 11, 2025
b0e8b94
converted duration numbers to multiplications
jeroenvervaeke Sep 11, 2025
17c15bb
added mergeOptions and options interface
jeroenvervaeke Sep 11, 2025
5f60f73
Merge branch 'main' into MCP-199
jeroenvervaeke Sep 12, 2025
6700878
undo requested PR change requests
jeroenvervaeke Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/common/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const OPTIONS = {
"tlsCertificateSelector",
"tlsDisabledProtocols",
"username",
"temporaryDatabaseUserLifetimeSeconds",
Copy link
Collaborator

@gagik gagik Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this array is duplicated from mongosh and shouldn't be modified.
In the future, we should definetely export it from mongosh and import it here because this is doomed to be confusing.
Maybe let's move this to a separate file now with a comment?

Copy link
Member Author

@jeroenvervaeke jeroenvervaeke Sep 11, 2025

Choose a reason for hiding this comment

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

Could you elaborate on this comment?

Copy link
Collaborator

@himanshusinghs himanshusinghs Sep 11, 2025

Choose a reason for hiding this comment

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

We don't need to (and shouldn't) modify the OPTIONS type as that configuration is for part of code from mongosh which we depend on for connection related work. For general config changes the user UserConfig modification should be enough.

Copy link
Member Author

@jeroenvervaeke jeroenvervaeke Sep 11, 2025

Choose a reason for hiding this comment

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

I only added temporaryDatabaseUserLifetimeSeconds to the list because it didn't work without adding it.

Copy link
Collaborator

@gagik gagik Sep 11, 2025

Choose a reason for hiding this comment

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

that's odd, it shouldn't depend on this.
This object should be only about mongosh-related args

Copy link
Collaborator

@gagik gagik Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should definitely deal with this now. Just create a second object which extends this. It should take a minute.

Copy link
Member Author

@jeroenvervaeke jeroenvervaeke Sep 11, 2025

Choose a reason for hiding this comment

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

Can this PR have a " 👍 ", or do you require any more changes on my side?

Copy link
Collaborator

@gagik gagik Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should definitely deal with this now. Just create a second object which extends this. It should take a minute.

We need to split the object between what is copied of over from mongosh and what is ours.
Something like

const MONGOSH_OPTIONS = [
// copy-paste from that mongosh file
]
const MCP_SERVER_OPTIONS = [
// ...
]
const OPTIONS = [...MONGOSH_OPTIONS, ...MCP_SERVER_OPTIONS]

(this would need to be done per field as otherwise they'd override each other)

Copy link
Member Author

@jeroenvervaeke jeroenvervaeke Sep 11, 2025

Choose a reason for hiding this comment

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

I've split the options like you suggested.

Copy link
Member Author

@jeroenvervaeke jeroenvervaeke Sep 11, 2025

Choose a reason for hiding this comment

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

Fixed the shallow merge issue.

],
boolean: [
"apiDeprecationErrors",
Expand Down Expand Up @@ -161,6 +162,7 @@ export interface UserConfig extends CliOptions {
loggers: Array<"stderr" | "disk" | "mcp">;
idleTimeoutMs: number;
notificationTimeoutMs: number;
temporaryDatabaseUserLifetimeSeconds: number;
}

export const defaultUserConfig: UserConfig = {
Expand All @@ -180,6 +182,7 @@ export const defaultUserConfig: UserConfig = {
idleTimeoutMs: 600000, // 10 minutes
notificationTimeoutMs: 540000, // 9 minutes
httpHeaders: {},
temporaryDatabaseUserLifetimeSeconds: 14400, // 4 hours
};

export const config = setupUserConfig({
Expand Down
3 changes: 1 addition & 2 deletions src/tools/atlas/connect/connectCluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { ensureCurrentIpInAccessList } from "../../../common/atlas/accessListUti
import type { AtlasClusterConnectionInfo } from "../../../common/connectionManager.js";
import { getDefaultRoleFromConfig } from "../../../common/atlas/roles.js";

const EXPIRY_MS = 1000 * 60 * 60 * 12; // 12 hours
const addedIpAccessListMessage =
"Note: Your current IP address has been added to the Atlas project's IP access list to enable secure connection.";

Expand Down Expand Up @@ -77,7 +76,7 @@ export class ConnectClusterTool extends AtlasToolBase {
const username = `mcpUser${Math.floor(Math.random() * 100000)}`;
const password = await generateSecurePassword();

const expiryDate = new Date(Date.now() + EXPIRY_MS);
const expiryDate = new Date(Date.now() + this.config.temporaryDatabaseUserLifetimeSeconds * 1000);
const role = getDefaultRoleFromConfig(this.config);

await this.session.apiClient.createDatabaseUser({
Expand Down
9 changes: 9 additions & 0 deletions tests/unit/common/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ describe("config", () => {
{ envVar: "MDB_MCP_HTTP_HOST", property: "httpHost", value: "localhost" },
{ envVar: "MDB_MCP_IDLE_TIMEOUT_MS", property: "idleTimeoutMs", value: 5000 },
{ envVar: "MDB_MCP_NOTIFICATION_TIMEOUT_MS", property: "notificationTimeoutMs", value: 5000 },
{
envVar: "MDB_MCP_TEMPORARY_DATABASE_USER_LIFETIME_SECONDS",
property: "temporaryDatabaseUserLifetimeSeconds",
value: 12345,
},
] as const;

for (const { envVar, property, value } of testCases) {
Expand Down Expand Up @@ -129,6 +134,10 @@ describe("config", () => {
cli: ["--notificationTimeoutMs", "42"],
expected: { notificationTimeoutMs: "42" },
},
{
cli: ["--temporaryDatabaseUserLifetimeSeconds", "12345"],
expected: { temporaryDatabaseUserLifetimeSeconds: "12345" },
},
{
cli: ["--telemetry", "enabled"],
expected: { telemetry: "enabled" },
Expand Down
Loading