This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7384-fceef70e32260c9dc29762a2f70d87f8fef03f3b in repository https://gitbox.apache.org/repos/asf/texera.git
commit e328232b436b3c57936f1ef8618a14b5e87ba559 Author: Xinyuan Lin <[email protected]> AuthorDate: Sun Aug 9 18:48:03 2026 -0700 test(agent-service): cover the auth, workflow, and backend API clients (#7384) ### What changes were proposed in this PR? Three of the four API clients under `agent-service/src/api` had no spec. `auth-api.ts` is the one that decides whether a request is authenticated at all, and none of its decisions were pinned. Adds 30 tests across three spec files, following the `fetch`-spy pattern already established by `compile-api.spec.ts`. **auth-api** — several of these are policy choices that read like oversights, so the tests state the intent rather than just the behaviour: | Input | Result | |---|---| | token with no `exp` | valid — tokens minted without an expiry never expire | | malformed token | invalid — the decode error is swallowed and reported as expired, not thrown | | payload with no `role` | `REGULAR`, so absent means least privilege | | `bearer` / `BEARER` | accepted; the scheme is matched case-insensitively | | two-segment token whose payload parses | rejected | **workflow-api** — the workflow `content` round-trips as a nested JSON **string**: the request sends `JSON.stringify(content)` and the response is re-parsed when it comes back as a string. Sending the object directly is the obvious-looking mistake and the backend rejects it, so both directions are pinned, along with the empty-description default and the error text on a refused save or a missing workflow. **backend-api** — the endpoint set, the defensive copy of the module-level config, and the two failure paths of the metadata fetch. **Verified by mutation**, all reverted (production diff empty): | Mutation | Result | |---|---| | default a missing role to `ADMIN` | red | | remove the three-segment check | red | | treat a token with no `exp` as expired | red | | compare `exp` as milliseconds instead of seconds | red | | make the Bearer scheme case-sensitive | red | | report a malformed token as valid | red | | send `content` as a nested object | red | | drop the empty-description default | red | | stop re-parsing a stringified response `content` | red | | drop the wid from the retrieve URL | red | | return the shared config by reference | red | The three-segment mutation initially **survived**: the test used `"only.two"`, whose payload fails `JSON.parse` regardless, so the segment check was never actually exercised. Replaced with a two-segment token carrying a valid payload — an unsigned token — which is the case the check exists for. No production file is touched. ### Any related issues, documentation, discussions? Closes #7381 ### How was this PR tested? ``` bun test ``` ``` 232 pass 0 fail Ran 232 tests across 18 files. ``` `bun run typecheck` and `bun run format:check` both pass. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- agent-service/src/api/auth-api.spec.ts | 137 ++++++++++++++++++++++++++ agent-service/src/api/backend-api.spec.ts | 78 +++++++++++++++ agent-service/src/api/workflow-api.spec.ts | 150 +++++++++++++++++++++++++++++ 3 files changed, 365 insertions(+) diff --git a/agent-service/src/api/auth-api.spec.ts b/agent-service/src/api/auth-api.spec.ts new file mode 100644 index 0000000000..0037790ffc --- /dev/null +++ b/agent-service/src/api/auth-api.spec.ts @@ -0,0 +1,137 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { describe, expect, test } from "bun:test"; +import { createAuthHeaders, extractBearerToken, extractUserFromToken, validateToken } from "./auth-api"; + +/** + * Builds a well-formed three-segment token carrying the given payload. All three segments are + * required — `decodeJWT` rejects anything that is not exactly three — but only the payload one is + * decoded, so the header and signature just have to be present. + */ +function tokenWith(payload: Record<string, unknown>): string { + const encoded = Buffer.from(JSON.stringify(payload), "utf-8").toString("base64"); + return `header.${encoded}.signature`; +} + +/** `exp` is a UNIX second count, so expiries are built in seconds throughout. */ +function nowInSeconds(): number { + return Math.floor(Date.now() / 1000); +} + +describe("extractUserFromToken", () => { + test("maps the JWT payload onto the user record", () => { + const token = tokenWith({ userId: 42, sub: "ada", email: "[email protected]", role: "ADMIN" }); + + expect(extractUserFromToken(token)).toEqual({ + uid: 42, + name: "ada", + email: "[email protected]", + role: "ADMIN", + }); + }); + + test("defaults a missing email to empty and a missing role to REGULAR", () => { + // Role defaulting is a privilege decision: absent must mean least privilege, never admin. + const user = extractUserFromToken(tokenWith({ userId: 7, sub: "grace" })); + + expect(user.email).toBe(""); + expect(user.role).toBe("REGULAR"); + }); + + test("rejects a token missing its signature segment even when the payload decodes", () => { + // The payload here is perfectly good JSON, so only the segment-count check stands between an + // unsigned token and a populated user record. + const payload = Buffer.from(JSON.stringify({ userId: 1, sub: "ada" }), "utf-8").toString("base64"); + + expect(() => extractUserFromToken(`header.${payload}`)).toThrow(/Failed to decode JWT/); + }); + + test("rejects a token with too many segments", () => { + const payload = Buffer.from(JSON.stringify({ userId: 1, sub: "ada" }), "utf-8").toString("base64"); + + expect(() => extractUserFromToken(`header.${payload}.signature.extra`)).toThrow(/Failed to decode JWT/); + }); + + test("rejects a token whose payload is not JSON", () => { + const notJson = Buffer.from("<html>", "utf-8").toString("base64"); + expect(() => extractUserFromToken(`header.${notJson}.signature`)).toThrow(/Failed to decode JWT/); + }); +}); + +describe("validateToken", () => { + test("accepts a token whose expiry is still in the future", () => { + expect(validateToken(tokenWith({ userId: 1, exp: nowInSeconds() + 60 }))).toBe(true); + }); + + test("rejects a token whose expiry has passed", () => { + expect(validateToken(tokenWith({ userId: 1, exp: nowInSeconds() - 60 }))).toBe(false); + }); + + test("treats a token with no expiry as valid forever", () => { + // Deliberate: tokens minted without `exp` never expire. Pinned because it is a policy choice + // that reads like an oversight, and flipping it would lock out every non-expiring token. + expect(validateToken(tokenWith({ userId: 1 }))).toBe(true); + }); + + test("rejects a malformed token rather than letting the decode failure escape", () => { + // isTokenExpired swallows the decode error and reports "expired", so callers get false here + // instead of an exception. A caller that only catches would otherwise let a junk token through. + expect(validateToken("not-a-token")).toBe(false); + }); + + test("expiry is compared in seconds, not milliseconds", () => { + // exp is a UNIX second count; reading it as milliseconds would place every real token in 1970 + // and reject it. A far-future second count must still validate. + expect(validateToken(tokenWith({ exp: nowInSeconds() + 3600 }))).toBe(true); + }); +}); + +describe("extractBearerToken", () => { + test("returns the token from a Bearer header", () => { + expect(extractBearerToken("Bearer abc123")).toBe("abc123"); + }); + + test("accepts the scheme in any case", () => { + expect(extractBearerToken("bearer abc123")).toBe("abc123"); + expect(extractBearerToken("BEARER abc123")).toBe("abc123"); + }); + + test("ignores a non-Bearer scheme", () => { + expect(extractBearerToken("Basic abc123")).toBeUndefined(); + }); + + test("returns undefined for a missing header", () => { + expect(extractBearerToken(undefined)).toBeUndefined(); + }); + + test("returns undefined when the scheme carries no token", () => { + expect(extractBearerToken("Bearer")).toBeUndefined(); + expect(extractBearerToken("Bearer ")).toBeUndefined(); + }); +}); + +describe("createAuthHeaders", () => { + test("sends the token as a Bearer credential alongside a JSON content type", () => { + expect(createAuthHeaders("abc123")).toEqual({ + Authorization: "Bearer abc123", + "Content-Type": "application/json", + }); + }); +}); diff --git a/agent-service/src/api/backend-api.spec.ts b/agent-service/src/api/backend-api.spec.ts new file mode 100644 index 0000000000..1ce1cf0557 --- /dev/null +++ b/agent-service/src/api/backend-api.spec.ts @@ -0,0 +1,78 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { fetchOperatorMetadata, getBackendConfig, type OperatorMetadata } from "./backend-api"; + +const metadata: OperatorMetadata = { operators: [], groups: [] }; + +describe("getBackendConfig", () => { + test("exposes the four service endpoints", () => { + const config = getBackendConfig(); + + expect(Object.keys(config).sort()).toEqual([ + "apiEndpoint", + "compileEndpoint", + "executionEndpoint", + "modelsEndpoint", + ]); + }); + + test("hands out a copy, so a caller cannot repoint the service endpoints", () => { + // The config is module-level state shared by every API client; returning the live object would + // let one caller's edit silently redirect everyone else's requests. + const first = getBackendConfig(); + first.apiEndpoint = "http://evil.example.com";; + + expect(getBackendConfig().apiEndpoint).not.toBe("http://evil.example.com";); + }); +}); + +describe("fetchOperatorMetadata", () => { + afterEach(() => { + mock.restore(); + }); + + test("reads the metadata from the dashboard service", async () => { + const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify(metadata), { status: 200 }) + ); + + const result = await fetchOperatorMetadata(); + + expect(result).toEqual(metadata); + const [url] = fetchSpy.mock.calls[0] as [string]; + expect(url).toBe(`${getBackendConfig().apiEndpoint}/api/resources/operator-metadata`); + }); + + test("throws with the status when the metadata cannot be fetched", async () => { + spyOn(globalThis, "fetch").mockResolvedValue( + new Response("down", { status: 503, statusText: "Service Unavailable" }) + ); + + await expect(fetchOperatorMetadata()).rejects.toThrow(/Failed to fetch operator metadata: 503 Service Unavailable/); + }); + + test("lets a network failure surface rather than returning empty metadata", async () => { + // A silent empty result here would leave the agent believing the cluster has zero operators. + spyOn(globalThis, "fetch").mockRejectedValue(new Error("network down")); + + await expect(fetchOperatorMetadata()).rejects.toThrow(/network down/); + }); +}); diff --git a/agent-service/src/api/workflow-api.spec.ts b/agent-service/src/api/workflow-api.spec.ts new file mode 100644 index 0000000000..6ddaea515b --- /dev/null +++ b/agent-service/src/api/workflow-api.spec.ts @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { persistWorkflow, retrieveWorkflow, type Workflow } from "./workflow-api"; +import type { WorkflowContent } from "../types/workflow"; + +const TOKEN = "tok-123"; + +const content = { + operators: [{ operatorID: "opX", operatorType: "CSVFileScan" }], + links: [], +} as unknown as WorkflowContent; + +/** The backend stores `content` as a JSON string, so responses echo it back in that form. */ +function storedWorkflow(overrides: Partial<Workflow> = {}): Record<string, unknown> { + return { wid: 5, name: "flow", content: JSON.stringify(content), ...overrides }; +} + +describe("persistWorkflow", () => { + afterEach(() => { + mock.restore(); + }); + + test("POSTs the workflow with the caller's bearer token", async () => { + const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify(storedWorkflow()), { status: 200 }) + ); + + await persistWorkflow(TOKEN, 5, "flow", content, "a description"); + + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + expect(url).toMatch(/\/api\/workflow\/persist$/); + expect(init.method).toBe("POST"); + expect(init.headers).toEqual({ + Authorization: `Bearer ${TOKEN}`, + "Content-Type": "application/json", + }); + }); + + test("serializes the content as a nested JSON string, not a nested object", async () => { + // The persist endpoint takes `content` as a string field. Sending the object directly is the + // obvious-looking mistake and the backend rejects it, so the double encoding is pinned here. + const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify(storedWorkflow()), { status: 200 }) + ); + + await persistWorkflow(TOKEN, 5, "flow", content, "a description"); + + const body = JSON.parse((fetchSpy.mock.calls[0] as [string, RequestInit])[1].body as string); + expect(typeof body.content).toBe("string"); + expect(JSON.parse(body.content)).toEqual(content); + expect(body).toMatchObject({ wid: 5, name: "flow", description: "a description", isPublic: false }); + }); + + test("sends an empty description when none is given", async () => { + const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify(storedWorkflow()), { status: 200 }) + ); + + await persistWorkflow(TOKEN, 5, "flow", content); + + const body = JSON.parse((fetchSpy.mock.calls[0] as [string, RequestInit])[1].body as string); + expect(body.description).toBe(""); + }); + + test("parses the stringified content on the way back out", async () => { + spyOn(globalThis, "fetch").mockResolvedValue(new Response(JSON.stringify(storedWorkflow()), { status: 200 })); + + const saved = await persistWorkflow(TOKEN, 5, "flow", content); + + expect(saved.content).toEqual(content); + }); + + test("leaves an already-parsed content object alone", async () => { + spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ wid: 5, name: "flow", content }), { status: 200 }) + ); + + const saved = await persistWorkflow(TOKEN, 5, "flow", content); + + expect(saved.content).toEqual(content); + }); + + test("reports the status and the server's message when the save is refused", async () => { + spyOn(globalThis, "fetch").mockResolvedValue( + new Response("workflow is read-only", { status: 403, statusText: "Forbidden" }) + ); + + await expect(persistWorkflow(TOKEN, 5, "flow", content)).rejects.toThrow( + /Failed to persist workflow: 403 Forbidden - workflow is read-only/ + ); + }); +}); + +describe("retrieveWorkflow", () => { + afterEach(() => { + mock.restore(); + }); + + test("GETs the workflow by id with the caller's bearer token", async () => { + const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify(storedWorkflow()), { status: 200 }) + ); + + await retrieveWorkflow(TOKEN, 5); + + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + expect(url).toMatch(/\/api\/workflow\/5$/); + expect(init.method).toBe("GET"); + expect(init.headers).toEqual({ + Authorization: `Bearer ${TOKEN}`, + "Content-Type": "application/json", + }); + }); + + test("parses the stringified content on the way back out", async () => { + spyOn(globalThis, "fetch").mockResolvedValue(new Response(JSON.stringify(storedWorkflow()), { status: 200 })); + + const loaded = await retrieveWorkflow(TOKEN, 5); + + expect(loaded.content).toEqual(content); + }); + + test("reports the status and the server's message when the workflow is missing", async () => { + spyOn(globalThis, "fetch").mockResolvedValue( + new Response("no such workflow", { status: 404, statusText: "Not Found" }) + ); + + await expect(retrieveWorkflow(TOKEN, 5)).rejects.toThrow( + /Failed to retrieve workflow: 404 Not Found - no such workflow/ + ); + }); +});
