Skip to content

Commit 226c375

Browse files
Fixes for #8900 (#8901)
* fix(mcp): Fix operator precedence in auth fallback logic Add parentheses to ensure skipAutoAuth check happens before fallback to "Application Default Credentials" string. Also add unit tests to cover this logic and ensure correct behavior. * refactor: Remove variable reference for unimportant sinon stub * Fix esm style imports and add changelog --------- Co-authored-by: Kevin Partington <[email protected]>
1 parent dd6c8b0 commit 226c375

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

57 files changed

+303
-239
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Fixed an issue where the MCP server could not successfully use Application Default Credentials. (#8896)

src/mcp/CONTRIBUTING.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ Tool files should be named `<product>/<foo_tool>`. The tool will then be listed
8888

8989
```typescript
9090
import { z } from "zod";
91-
import { tool } from "../../tool.js";
92-
import { mcpError, toContent } from "../../util.js";
91+
import { tool } from "../../tool";
92+
import { mcpError, toContent } from "../../util";
9393

9494
export const foo_bar = tool(
9595
{
@@ -156,7 +156,7 @@ export const <product>Tools = [
156156
If this is the first tool for this product, also go to `src/mcp/tools/index.ts` and add your product:
157157

158158
```typescript
159-
import { <product>Tools } from "./<product>/index.js"
159+
import { <product>Tools } from "./<product>/index"
160160

161161
const tools: Record<ServerFeature, ServerTool[]> = {
162162
// Exisitng tools here...

src/mcp/errors.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { CallToolResult } from "@modelcontextprotocol/sdk/types";
1+
import { CallToolResult } from "@modelcontextprotocol/sdk/types.js";
22
import { commandExistsSync, mcpError } from "./util";
33

44
export const NO_PROJECT_ERROR = mcpError(

src/mcp/index.spec.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { expect } from "chai";
2+
import * as sinon from "sinon";
3+
import { FirebaseMcpServer } from "./index";
4+
import * as requireAuthModule from "../requireAuth";
5+
6+
describe("FirebaseMcpServer.getAuthenticatedUser", () => {
7+
let server: FirebaseMcpServer;
8+
let requireAuthStub: sinon.SinonStub;
9+
10+
beforeEach(() => {
11+
// Mock the methods that may cause hanging BEFORE creating the instance
12+
sinon.stub(FirebaseMcpServer.prototype, "detectProjectRoot").resolves("/test/project");
13+
sinon.stub(FirebaseMcpServer.prototype, "detectActiveFeatures").resolves([]);
14+
15+
server = new FirebaseMcpServer({});
16+
17+
// Mock the resolveOptions method to avoid dependency issues
18+
sinon.stub(server, "resolveOptions").resolves({});
19+
20+
requireAuthStub = sinon.stub(requireAuthModule, "requireAuth");
21+
});
22+
23+
afterEach(() => {
24+
sinon.restore();
25+
});
26+
27+
it("should return email when authenticated user is present", async () => {
28+
const testEmail = "[email protected]";
29+
requireAuthStub.resolves(testEmail);
30+
31+
const result = await server.getAuthenticatedUser();
32+
33+
expect(result).to.equal(testEmail);
34+
expect(requireAuthStub.calledOnce).to.be.true;
35+
});
36+
37+
it("should return null when no user and skipAutoAuth is true", async () => {
38+
requireAuthStub.resolves(null);
39+
40+
const result = await server.getAuthenticatedUser(true);
41+
42+
expect(result).to.be.null;
43+
expect(requireAuthStub.calledOnce).to.be.true;
44+
});
45+
46+
it("should return 'Application Default Credentials' when no user and skipAutoAuth is false", async () => {
47+
requireAuthStub.resolves(null);
48+
49+
const result = await server.getAuthenticatedUser(false);
50+
51+
expect(result).to.equal("Application Default Credentials");
52+
expect(requireAuthStub.calledOnce).to.be.true;
53+
});
54+
55+
it("should return null when requireAuth throws an error", async () => {
56+
requireAuthStub.rejects(new Error("Auth failed"));
57+
58+
const result = await server.getAuthenticatedUser();
59+
60+
expect(result).to.be.null;
61+
expect(requireAuthStub.calledOnce).to.be.true;
62+
});
63+
});

src/mcp/index.ts

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,27 +9,27 @@ import {
99
ListToolsRequestSchema,
1010
CallToolResult,
1111
} from "@modelcontextprotocol/sdk/types.js";
12-
import { checkFeatureActive, mcpError } from "./util.js";
13-
import { ClientConfig, SERVER_FEATURES, ServerFeature } from "./types.js";
14-
import { availableTools } from "./tools/index.js";
15-
import { ServerTool, ServerToolContext } from "./tool.js";
16-
import { configstore } from "../configstore.js";
17-
import { Command } from "../command.js";
18-
import { requireAuth } from "../requireAuth.js";
19-
import { Options } from "../options.js";
20-
import { getProjectId } from "../projectUtils.js";
21-
import { mcpAuthError, NO_PROJECT_ERROR, mcpGeminiError } from "./errors.js";
22-
import { trackGA4 } from "../track.js";
23-
import { Config } from "../config.js";
24-
import { loadRC } from "../rc.js";
25-
import { EmulatorHubClient } from "../emulator/hubClient.js";
26-
import { Emulators } from "../emulator/types.js";
12+
import { checkFeatureActive, mcpError } from "./util";
13+
import { ClientConfig, SERVER_FEATURES, ServerFeature } from "./types";
14+
import { availableTools } from "./tools/index";
15+
import { ServerTool, ServerToolContext } from "./tool";
16+
import { configstore } from "../configstore";
17+
import { Command } from "../command";
18+
import { requireAuth } from "../requireAuth";
19+
import { Options } from "../options";
20+
import { getProjectId } from "../projectUtils";
21+
import { mcpAuthError, NO_PROJECT_ERROR, mcpGeminiError } from "./errors";
22+
import { trackGA4 } from "../track";
23+
import { Config } from "../config";
24+
import { loadRC } from "../rc";
25+
import { EmulatorHubClient } from "../emulator/hubClient";
26+
import { Emulators } from "../emulator/types";
2727
import { existsSync } from "node:fs";
28-
import { ensure, check } from "../ensureApiEnabled.js";
29-
import * as api from "../api.js";
30-
import { LoggingStdioServerTransport } from "./logging-transport.js";
31-
import { isFirebaseStudio } from "../env.js";
32-
import { timeoutFallback } from "../timeout.js";
28+
import { ensure, check } from "../ensureApiEnabled";
29+
import * as api from "../api";
30+
import { LoggingStdioServerTransport } from "./logging-transport";
31+
import { isFirebaseStudio } from "../env";
32+
import { timeoutFallback } from "../timeout";
3333

3434
const SERVER_VERSION = "0.2.0";
3535

@@ -233,7 +233,7 @@ export class FirebaseMcpServer {
233233
this.log("debug", `calling requireAuth`);
234234
const email = await requireAuth(await this.resolveOptions(), skipAutoAuth);
235235
this.log("debug", `detected authenticated account: ${email || "<none>"}`);
236-
return email ?? skipAutoAuth ? null : "Application Default Credentials";
236+
return email ?? (skipAutoAuth ? null : "Application Default Credentials");
237237
} catch (e) {
238238
this.log("debug", `error in requireAuth: ${e}`);
239239
return null;

src/mcp/tool.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { CallToolResult } from "@modelcontextprotocol/sdk/types";
1+
import { CallToolResult } from "@modelcontextprotocol/sdk/types.js";
22
import { z, ZodTypeAny } from "zod";
33
import { zodToJsonSchema } from "zod-to-json-schema";
44
import type { FirebaseMcpServer } from "./index";

src/mcp/tools/apphosting/fetch_logs.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import { z } from "zod";
2-
import { tool } from "../../tool.js";
3-
import { toContent } from "../../util.js";
4-
import { Backend, getBackend, getTraffic, listBuilds, Traffic } from "../../../gcp/apphosting.js";
5-
import { last } from "../../../utils.js";
6-
import { FirebaseError } from "../../../error.js";
7-
import { fetchServiceLogs } from "../../../gcp/run.js";
8-
import { listEntries } from "../../../gcp/cloudlogging.js";
2+
import { tool } from "../../tool";
3+
import { toContent } from "../../util";
4+
import { Backend, getBackend, getTraffic, listBuilds, Traffic } from "../../../gcp/apphosting";
5+
import { last } from "../../../utils";
6+
import { FirebaseError } from "../../../error";
7+
import { fetchServiceLogs } from "../../../gcp/run";
8+
import { listEntries } from "../../../gcp/cloudlogging";
99

1010
export const fetch_logs = tool(
1111
{

src/mcp/tools/apphosting/list_backends.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from "zod";
2-
import { tool } from "../../tool.js";
3-
import { toContent } from "../../util.js";
2+
import { tool } from "../../tool";
3+
import { toContent } from "../../util";
44
import {
55
Backend,
66
Domain,
@@ -9,7 +9,7 @@ import {
99
listDomains,
1010
parseBackendName,
1111
Traffic,
12-
} from "../../../gcp/apphosting.js";
12+
} from "../../../gcp/apphosting";
1313

1414
export const list_backends = tool(
1515
{

src/mcp/tools/auth/disable_user.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { z } from "zod";
2-
import { tool } from "../../tool.js";
3-
import { toContent } from "../../util.js";
4-
import { disableUser } from "../../../gcp/auth.js";
2+
import { tool } from "../../tool";
3+
import { toContent } from "../../util";
4+
import { disableUser } from "../../../gcp/auth";
55

66
export const disable_user = tool(
77
{

src/mcp/tools/auth/get_user.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { z } from "zod";
2-
import { tool } from "../../tool.js";
3-
import { mcpError, toContent } from "../../util.js";
4-
import { findUser } from "../../../gcp/auth.js";
2+
import { tool } from "../../tool";
3+
import { mcpError, toContent } from "../../util";
4+
import { findUser } from "../../../gcp/auth";
55

66
export const get_user = tool(
77
{

0 commit comments

Comments
 (0)