diff --git a/docs/api/companies.md b/docs/api/companies.md index 00e7ab66b8..5ee6adc81b 100644 --- a/docs/api/companies.md +++ b/docs/api/companies.md @@ -11,7 +11,18 @@ Manage companies within your Paperclip instance. GET /api/companies ``` -Returns all companies the current user/agent has access to. +Requires a board user. Returns companies where the user has active membership. +Instance administrators and the local trusted board can list all companies. + +For navigation and company selectors, use `GET /api/companies?scope=accessible`. +This returns only companies the caller can enter through company-scoped routes, +including for instance administrators. Instance administrator status alone does +not grant access to a company's contents. The local trusted board can still +enter all companies. The board UI uses this scope for its company list, so it +does not select companies the user cannot open. +The Instance Access screen uses the unscoped directory so administrators can +manage membership for all companies. A supplied `scope` must be a single +`accessible` value; empty, unknown, or repeated values return `400`. ## Get Company diff --git a/server/src/__tests__/companies-route-cross-company-authz.test.ts b/server/src/__tests__/companies-route-cross-company-authz.test.ts index 4a92461f9c..58adaa4aa5 100644 --- a/server/src/__tests__/companies-route-cross-company-authz.test.ts +++ b/server/src/__tests__/companies-route-cross-company-authz.test.ts @@ -224,6 +224,72 @@ describe.sequential("company route cross-company authorization", () => { resetMockDefaults(); }); + it.each(["session", "board_key", "cloud_tenant"])( + "limits navigable companies to memberships for a %s instance admin", + async (source) => { + mockCompanyService.list.mockResolvedValue([createCompany(companyBId), createCompany(companyAId)]); + const app = await createApp(boardActor({ + userId: "owner-a", + source, + isInstanceAdmin: true, + companyIds: [companyAId], + })); + + const navigation = await request(app).get("/api/companies?scope=accessible").expect(200); + expect(navigation.body.map((company: { id: string }) => company.id)).toEqual([companyAId]); + await request(app).get(`/api/companies/${companyAId}`).expect(200); + await request(app).get(`/api/companies/${companyBId}`).expect(403); + + // The existing directory remains available for instance administration. + const directory = await request(app).get("/api/companies").expect(200); + expect(directory.body.map((company: { id: string }) => company.id)).toEqual([companyBId, companyAId]); + }, + ); + + it.each([false, true])("returns no navigable companies without memberships (admin=%s)", async (isInstanceAdmin) => { + mockCompanyService.list.mockResolvedValue([createCompany(companyAId)]); + const app = await createApp(boardActor({ userId: "outsider", isInstanceAdmin })); + const res = await request(app).get("/api/companies?scope=accessible").expect(200); + expect(res.body).toEqual([]); + }); + + it.each(["scope=accessible&scope=accessible", "scope=accessible&scope=all", "scope=all", "scope="])( + "rejects malformed list scope without loading the directory: %s", + async (query) => { + const app = await createApp(boardActor({ userId: "admin", isInstanceAdmin: true })); + await request(app).get(`/api/companies?${query}`).expect(400); + expect(mockCompanyService.list).not.toHaveBeenCalled(); + }, + ); + + it("includes additional companies where a cloud user has membership", async () => { + mockCompanyService.list.mockResolvedValue([createCompany(companyAId), createCompany(companyBId)]); + const app = await createApp(boardActor({ + userId: "owner-of-both", + source: "cloud_tenant", + companyIds: [companyAId, companyBId], + })); + const res = await request(app).get("/api/companies?scope=accessible").expect(200); + expect(res.body.map((company: { id: string }) => company.id)).toEqual([companyAId, companyBId]); + await request(app).get(`/api/companies/${companyBId}`).expect(200); + }); + + it("keeps all companies navigable for the local trusted board", async () => { + mockCompanyService.list.mockResolvedValue([createCompany(companyAId), createCompany(companyBId)]); + const app = await createApp(boardActor({ userId: "local-board", source: "local_implicit" })); + const res = await request(app).get("/api/companies?scope=accessible").expect(200); + expect(res.body.map((company: { id: string }) => company.id)).toEqual([companyAId, companyBId]); + }); + + it.each([{ type: "none", source: "none" }, companyACeoActor()])( + "rejects navigation list requests from a $type actor", + async (actor) => { + const app = await createApp(actor); + await request(app).get("/api/companies?scope=accessible").expect(403); + expect(mockCompanyService.list).not.toHaveBeenCalled(); + }, + ); + it.each([ { label: "GET /api/companies/:companyId", diff --git a/server/src/__tests__/openapi-routes.test.ts b/server/src/__tests__/openapi-routes.test.ts index 98f67f5a4a..29a6047b8a 100644 --- a/server/src/__tests__/openapi-routes.test.ts +++ b/server/src/__tests__/openapi-routes.test.ts @@ -190,6 +190,14 @@ describe("openapi routes", () => { expect(res.body.paths["/api/health"].get.security).toEqual([]); expect(res.body.paths["/mcp/gateways/{gatewayPublicId}"].post.security).toEqual([]); expect(res.body.paths["/api/mcp/gateways/{gatewayPublicId}"]).toBeUndefined(); + expect(res.body.paths["/api/companies"].get.parameters).toContainEqual({ + name: "scope", + in: "query", + required: false, + schema: { type: "string", enum: ["accessible"] }, + }); + expect(res.body.paths["/api/companies"].get.responses["403"]).toBeDefined(); + expect(res.body.paths["/api/companies"].get.responses["400"]).toBeDefined(); expect(res.body.paths["/api/companies"].post.responses["201"]).toBeDefined(); expect(res.body.paths["/api/companies"].post.requestBody.content["application/json"].schema).toMatchObject({ type: "object", diff --git a/server/src/routes/companies.ts b/server/src/routes/companies.ts index 0c7306a15c..dc048a4403 100644 --- a/server/src/routes/companies.ts +++ b/server/src/routes/companies.ts @@ -376,7 +376,18 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan router.get("/", async (req, res) => { assertBoard(req); + const scope = req.query.scope; + if (scope !== undefined && scope !== "accessible") { + throw badRequest("scope must be a single accessible value when provided"); + } const result = await svc.list(); + // Navigation needs the same membership scope as company detail routes. + // Instance admins can inspect the directory without membership, but that + // visibility alone does not let them open a company's inbox or tasks. + if (scope === "accessible") { + res.json(result.filter((company) => hasCompanyAccess(req, company.id))); + return; + } if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) { res.json(result); return; diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index e08fff3636..538a6919a8 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -1342,7 +1342,11 @@ registry.registerPath({ path: "/api/companies", tags: ["companies"], summary: "List companies", - responses: { 200: r.ok(), 401: r.unauthorized }, + description: "Requires a board user. Instance admins can list the full directory; scope=accessible limits the list to companies the caller can enter.", + request: { + query: z.object({ scope: z.enum(["accessible"]).optional() }), + }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 403: r.forbidden }, }); registry.registerPath({ diff --git a/ui/src/api/companies-query.test.ts b/ui/src/api/companies-query.test.ts index 865f0718f8..66b8fd4447 100644 --- a/ui/src/api/companies-query.test.ts +++ b/ui/src/api/companies-query.test.ts @@ -1,6 +1,6 @@ import { QueryClient } from "@tanstack/react-query"; import { describe, expect, it, vi } from "vitest"; -import { companyListQueryOptions, resolveAccountUserId } from "./companies-query"; +import { companyDirectoryQueryOptions, companyListQueryOptions, resolveAccountUserId } from "./companies-query"; import { ApiError } from "./client"; import { queryKeys } from "../lib/queryKeys"; @@ -15,6 +15,8 @@ vi.mock("./auth", () => ({ const mockCompaniesApi = vi.hoisted(() => ({ list: vi.fn(), detachInflightList: vi.fn(), + directory: vi.fn(), + detachInflightDirectory: vi.fn(), })); vi.mock("./companies", () => ({ @@ -33,6 +35,14 @@ describe("companyListQueryOptions", () => { }); describe("company list cache keys", () => { + it("isolates the administration directory from navigation and other accounts", async () => { + const options = companyDirectoryQueryOptions("admin"); + expect(options.queryKey).not.toEqual(companyListQueryOptions("admin").queryKey); + expect(options.queryKey).not.toEqual(companyDirectoryQueryOptions("other-admin").queryKey); + mockCompaniesApi.directory.mockResolvedValueOnce([{ id: "non-member-company" }]); + await expect(options.queryFn()).resolves.toEqual([{ id: "non-member-company" }]); + expect(mockCompaniesApi.detachInflightDirectory).toHaveBeenCalled(); + }); it("gives each account its own entry, and a stable one for signed-out", () => { expect(companyListQueryOptions("user-1").queryKey).not.toEqual( companyListQueryOptions("user-2").queryKey, diff --git a/ui/src/api/companies-query.ts b/ui/src/api/companies-query.ts index 057cdb89f5..606d1d2bd5 100644 --- a/ui/src/api/companies-query.ts +++ b/ui/src/api/companies-query.ts @@ -40,6 +40,18 @@ export function companyListQueryOptions(userId: string | null) { } as const; } +/** The administration directory has its own account-scoped cache and request. */ +export function companyDirectoryQueryOptions(userId: string | null) { + return { + queryKey: queryKeys.companies.directory(userId), + queryFn: async () => { + companiesApi.detachInflightDirectory(); + return companiesApi.directory(); + }, + retry: false, + } as const; +} + const sessionQueryOptions = { queryKey: queryKeys.auth.session, queryFn: () => authApi.getSession(), diff --git a/ui/src/api/companies.test.ts b/ui/src/api/companies.test.ts new file mode 100644 index 0000000000..c512ce215b --- /dev/null +++ b/ui/src/api/companies.test.ts @@ -0,0 +1,53 @@ +import { afterEach, expect, it, vi } from "vitest"; +import { companiesApi } from "./companies"; + +afterEach(() => vi.unstubAllGlobals()); + +it("keeps directory requests separate from navigation and previous accounts", async () => { + let resolveOldDirectory!: (response: Response) => void; + const oldDirectory = new Promise((resolve) => { resolveOldDirectory = resolve; }); + const fetchMock = vi.fn() + .mockReturnValueOnce(oldDirectory) + .mockResolvedValueOnce(Response.json([{ id: "member-company" }])) + .mockResolvedValueOnce(Response.json([{ id: "directory-company" }])); + vi.stubGlobal("fetch", fetchMock); + const previousAccount = companiesApi.directory(); + const navigation = companiesApi.list(); + companiesApi.detachInflightDirectory(); + const currentDirectory = companiesApi.directory(); + try { + await expect(navigation).resolves.toEqual([{ id: "member-company" }]); + await expect(currentDirectory).resolves.toEqual([{ id: "directory-company" }]); + expect(fetchMock.mock.calls.map(([url]) => url)).toEqual([ + "/api/companies", "/api/companies?scope=accessible", "/api/companies", + ]); + } finally { + resolveOldDirectory(Response.json([])); + await previousAccount; + } +}); + +it("requests navigable companies and detaches the same request when accounts change", async () => { + let resolveOldRequest!: (response: Response) => void; + const oldRequest = new Promise((resolve) => { resolveOldRequest = resolve; }); + const currentCompanies = [{ id: "current-company" }]; + const fetchMock = vi.fn() + .mockReturnValueOnce(oldRequest) + .mockResolvedValueOnce(Response.json(currentCompanies)); + vi.stubGlobal("fetch", fetchMock); + + const previousAccount = companiesApi.list(); + companiesApi.detachInflightList(); + const currentAccount = companiesApi.list(); + try { + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(fetchMock.mock.calls.map(([url]) => url)).toEqual([ + "/api/companies?scope=accessible", + "/api/companies?scope=accessible", + ]); + await expect(currentAccount).resolves.toEqual(currentCompanies); + } finally { + resolveOldRequest(Response.json([{ id: "previous-company" }])); + await previousAccount; + } +}); diff --git a/ui/src/api/companies.ts b/ui/src/api/companies.ts index ea6e3da839..f46d8be012 100644 --- a/ui/src/api/companies.ts +++ b/ui/src/api/companies.ts @@ -23,7 +23,10 @@ import { } from "@paperclipai/shared/company-import-transfer"; import { api, detachInflightGet, type RequestOptions } from "./client"; -const COMPANIES_LIST_PATH = "/companies"; +// The board navigates only into companies the user can enter. The unscoped +// directory also includes companies visible solely through instance admin. +const COMPANIES_LIST_PATH = "/companies?scope=accessible"; +const COMPANIES_DIRECTORY_PATH = "/companies"; export type CompanyStats = Record; @@ -78,6 +81,8 @@ export interface CompanyImportJobStatus { export const companiesApi = { list: () => api.get(COMPANIES_LIST_PATH), + directory: () => api.get(COMPANIES_DIRECTORY_PATH), + detachInflightDirectory: () => detachInflightGet(COMPANIES_DIRECTORY_PATH), /** * Call before re-reading the list for a different account: an in-flight * `/companies` GET issued under the previous session would otherwise be diff --git a/ui/src/lib/queryKeys.ts b/ui/src/lib/queryKeys.ts index 407c00ef80..36efbd6c9e 100644 --- a/ui/src/lib/queryKeys.ts +++ b/ui/src/lib/queryKeys.ts @@ -16,6 +16,8 @@ export const queryKeys = { */ list: (userId: string | null) => ["companies", "list", userId ?? "anonymous"] as const, + directory: (userId: string | null) => + ["companies", "directory", userId ?? "anonymous"] as const, detail: (id: string) => ["companies", id] as const, stats: ["companies", "stats"] as const, exportFidelity: (companyId: string) => diff --git a/ui/src/pages/InstanceAccess.test.tsx b/ui/src/pages/InstanceAccess.test.tsx new file mode 100644 index 0000000000..cdffc0a9ba --- /dev/null +++ b/ui/src/pages/InstanceAccess.test.tsx @@ -0,0 +1,127 @@ +// @vitest-environment jsdom + +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ApiError } from "@/api/client"; +import { CompanyProvider, useCompany } from "@/context/CompanyContext"; +import { InstanceAccess } from "./InstanceAccess"; + +const mocks = vi.hoisted(() => ({ + list: vi.fn(), directory: vi.fn(), detachInflightList: vi.fn(), detachInflightDirectory: vi.fn(), + getSession: vi.fn(), searchAdminUsers: vi.fn(), getUserCompanyAccess: vi.fn(), + setUserCompanyAccess: vi.fn(), setBreadcrumbs: vi.fn(), pushToast: vi.fn(), +})); +vi.mock("@/api/companies", () => ({ companiesApi: mocks })); +vi.mock("@/api/auth", () => ({ authApi: mocks })); +vi.mock("@/api/access", () => ({ accessApi: mocks })); +vi.mock("@/context/BreadcrumbContext", () => ({ useBreadcrumbs: () => mocks })); +vi.mock("@/context/ToastContext", () => ({ useToast: () => mocks })); + +const companyA = { id: "company-a", name: "Company A", issuePrefix: "CPA", status: "active" }; +const companyB = { id: "company-b", name: "Company B", issuePrefix: "CPB", status: "active" }; +const user = { id: "admin", name: "Admin", email: "admin@example.com", isInstanceAdmin: true }; +const membershipA = { + id: "membership-a", companyId: companyA.id, companyName: companyA.name, + status: "active", membershipRole: "owner", updatedAt: "2020-01-01T00:00:00Z", +}; + +let container: HTMLDivElement; +let root: Root; +let client: QueryClient; + +function NavigationProbe() { + const { companies } = useCompany(); + return ; +} + +async function renderPage() { + await act(async () => { + root.render( + + + , + ); + }); +} + +async function eventually(assertion: () => void) { + await vi.waitFor(async () => { + await act(async () => { await new Promise((resolve) => setTimeout(resolve, 0)); }); + assertion(); + }); +} + +function button(text: string) { + return [...container.querySelectorAll("button")].find((element) => element.textContent === text); +} + +beforeEach(() => { + vi.resetAllMocks(); + Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + localStorage.clear(); + mocks.getSession.mockResolvedValue({ session: { id: "session-admin", userId: user.id }, user }); + mocks.list.mockResolvedValue([companyA]); + mocks.directory.mockResolvedValue([companyA, companyB]); + mocks.searchAdminUsers.mockResolvedValue([user]); + mocks.getUserCompanyAccess.mockResolvedValue({ user, companyAccess: [membershipA] }); + mocks.setUserCompanyAccess.mockResolvedValue({}); + client = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(async () => { + await act(async () => root.unmount()); + client.clear(); + container.remove(); +}); + +describe("InstanceAccess company directory", () => { + it("lets admins grant access outside their navigation list and refreshes their own navigation", async () => { + await renderPage(); + await eventually(() => { + expect(button("Save organization access")).toBeDefined(); + expect(container.querySelector("nav")?.textContent).toBe("Company A"); + }); + const otherCompany = [...container.querySelectorAll("label")].find((label) => label.textContent?.includes("Company B")); + const checkbox = otherCompany?.querySelector('[role="checkbox"]'); + expect(checkbox?.getAttribute("aria-checked")).toBe("false"); + mocks.setUserCompanyAccess.mockImplementation(async () => { + mocks.list.mockResolvedValue([companyA, companyB]); + mocks.getUserCompanyAccess.mockResolvedValue({ user, companyAccess: [membershipA, { + ...membershipA, id: "membership-b", companyId: companyB.id, companyName: companyB.name, + }] }); + return {}; + }); + await act(async () => checkbox!.click()); + await act(async () => button("Save organization access")!.click()); + await eventually(() => { + expect(mocks.setUserCompanyAccess).toHaveBeenCalledWith(user.id, [companyA.id, companyB.id]); + expect(container.querySelector("nav")?.textContent).toBe("Company A, Company B"); + }); + }); + + it("prevents editing an incomplete directory and lets the admin retry", async () => { + mocks.directory.mockRejectedValue(new Error("Unavailable")); + await renderPage(); + await eventually(() => { + expect(container.textContent).toContain("Failed to load organizations."); + expect(container.querySelector("nav")?.textContent).toBe("Company A"); + }); + expect(button("Save organization access")).toBeUndefined(); + expect(mocks.setUserCompanyAccess).not.toHaveBeenCalled(); + mocks.directory.mockResolvedValue([companyA, companyB]); + await act(async () => button("Try again")!.click()); + await eventually(() => expect(button("Save organization access")).toBeDefined()); + }); + + it("does not request the directory when instance administration is forbidden", async () => { + mocks.searchAdminUsers.mockRejectedValue(new ApiError("Forbidden", 403, {})); + await renderPage(); + await eventually(() => expect(container.textContent).toContain("Instance admin access is required")); + expect(mocks.directory).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/src/pages/InstanceAccess.tsx b/ui/src/pages/InstanceAccess.tsx index ddfa8b1f3c..459191fe64 100644 --- a/ui/src/pages/InstanceAccess.tsx +++ b/ui/src/pages/InstanceAccess.tsx @@ -7,12 +7,12 @@ import { Button } from "@/components/ui/button"; import { Checkbox } from "@/components/ui/checkbox"; import { useBreadcrumbs } from "@/context/BreadcrumbContext"; import { Card } from "@/components/ui/card"; -import { useCompany } from "@/context/CompanyContext"; +import { companyDirectoryQueryOptions, useAccountIdentity } from "@/api/companies-query"; import { useToast } from "@/context/ToastContext"; import { queryKeys } from "@/lib/queryKeys"; export function InstanceAccess() { - const { companies } = useCompany(); + const { userId: accountUserId, settled: accountSettled } = useAccountIdentity(); const { setBreadcrumbs } = useBreadcrumbs(); const { pushToast } = useToast(); const queryClient = useQueryClient(); @@ -33,6 +33,12 @@ export function InstanceAccess() { queryFn: () => accessApi.searchAdminUsers(search), }); + const companiesQuery = useQuery({ + ...companyDirectoryQueryOptions(accountUserId), + enabled: accountSettled && usersQuery.isSuccess, + }); + const companies = companiesQuery.data ?? []; + const selectedUser = useMemo( () => usersQuery.data?.find((user) => user.id === selectedUserId) ?? null, [selectedUserId, usersQuery.data], @@ -66,6 +72,7 @@ export function InstanceAccess() { onSuccess: async () => { await queryClient.invalidateQueries({ queryKey: queryKeys.access.userCompanyAccess(selectedUserId!) }); await queryClient.invalidateQueries({ queryKey: queryKeys.access.adminUsers(search) }); + await queryClient.invalidateQueries({ queryKey: queryKeys.companies.all }); pushToast({ title: "Organization access updated", tone: "success" }); }, }); @@ -85,8 +92,8 @@ export function InstanceAccess() { }, }); - if (usersQuery.isLoading) { - return
Loading instance users…
; + if (usersQuery.isLoading || !accountSettled || (usersQuery.isSuccess && companiesQuery.isPending)) { + return
Loading instance access…
; } if (usersQuery.error) { @@ -99,6 +106,15 @@ export function InstanceAccess() { return
{message}
; } + if (companiesQuery.error) { + return ( +
+

Failed to load organizations. Try again before changing access.

+ +
+ ); + } + return (