mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 16:51:28 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The board selects a company before it loads that company's inbox and tasks. > - Instance administrators can list companies where they have no membership. > - The board treated that directory as a list of companies the user could enter. > - This pull request gives navigation a list based on the existing company access check. > - Users can select their companies without landing on an inbox that rejects their access. ## Linked Issues or Issue Description Fixes #6090. Refs #4855 for the related account-recovery case; this PR does not grant company membership. **What happened?** An instance administrator can select a company where they have no membership. Its inbox then shows “User does not have access to this company.” Company directory visibility and access to company contents use different rules. **Expected behavior** Company navigation should show only companies the current user can enter. A stored selection for an inaccessible company should fall back to an accessible company. A direct link to an inaccessible company should use the existing unavailable-company page. **Steps to reproduce** 1. Create Company A and Company B with separate owners. 2. Sign in as an instance administrator who belongs only to Company A. 3. Select Company B through a stored selection or a link with its prefix. 4. Observe that the board accepts the company selection, but company-scoped requests return 403. Related: #10524 lets cloud users enter additional companies where they hold memberships. This fix preserves that access and excludes companies where they have no membership. ## What Changed - Added `scope=accessible` to `GET /api/companies`, using the existing `hasCompanyAccess` predicate. - Changed the board navigation list to request that scope. Instance Access uses a separate unscoped, account-keyed directory so administrators can manage all companies. Membership edits refresh navigation. - Reject empty, unknown, and repeated scope values with 400. Directory loading errors offer a retry before access controls are shown. - Added route tests for cloud, session, board-key, local trusted, non-member, and agent access. - Added client and component tests for navigation/admin request isolation, grants outside the navigation list, self-membership refresh, directory failure recovery, and forbidden administration. - Updated the API guide and OpenAPI document. ## Verification - Latest commit: 36 focused UI tests passed. The broader UI shard passed all 281 files / 2,533 tests after correcting an asynchronous test assertion. - Server authorization and OpenAPI regression suites: 31 tests passed. - UI typecheck and `pnpm check:token-gates`: passed. - `pnpm -r typecheck` and `pnpm build`: passed after review fixes. - Full local test runner: exercised the supported shards. Several unrelated suites hit embedded PostgreSQL startup failures or startup timeouts under local load. The UI regression issue found in the broad run was corrected and its full UI shard passed. These local limitations are not reported as a green full-suite result. - [GitHub CI](https://github.com/paperclipai/paperclip/actions/runs/34233416473): all checks green on `4e1698cd4` — all server and workspace test shards, all browser end-to-end shards, typecheck/release registry, build, canary dry run, policy, and Docker context integrity. Security checks also passed. - Greptile: 5/5 on the latest commit; both initial findings addressed and all review threads resolved. ## Risks - The UI now excludes companies visible only through instance administrator status. Company membership continues to control access to contents. - Additional companies with active memberships remain available. - The client and server changes must ship together. An older server ignores the new query parameter and retains the previous behavior. - No database migration or permission grant changes. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, repository inspection, code editing, and test execution. The exact served model identifier and context window are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
140 lines
6.4 KiB
TypeScript
140 lines
6.4 KiB
TypeScript
import { QueryClient } from "@tanstack/react-query";
|
|
import { describe, expect, it, vi } from "vitest";
|
|
import { companyDirectoryQueryOptions, companyListQueryOptions, resolveAccountUserId } from "./companies-query";
|
|
import { ApiError } from "./client";
|
|
import { queryKeys } from "../lib/queryKeys";
|
|
|
|
const mockAuthApi = vi.hoisted(() => ({
|
|
getSession: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("./auth", () => ({
|
|
authApi: mockAuthApi,
|
|
}));
|
|
|
|
const mockCompaniesApi = vi.hoisted(() => ({
|
|
list: vi.fn(),
|
|
detachInflightList: vi.fn(),
|
|
directory: vi.fn(),
|
|
detachInflightDirectory: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("./companies", () => ({
|
|
companiesApi: mockCompaniesApi,
|
|
}));
|
|
|
|
describe("companyListQueryOptions", () => {
|
|
it.each([401, 403])("treats %s company-list failures as unauthorized bootstrap state", async (status) => {
|
|
mockCompaniesApi.list.mockRejectedValueOnce(new ApiError("Board access required", status, { error: "Board access required" }));
|
|
|
|
await expect(companyListQueryOptions("user-1").queryFn()).resolves.toEqual({
|
|
companies: [],
|
|
unauthorized: true,
|
|
});
|
|
});
|
|
});
|
|
|
|
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,
|
|
);
|
|
expect(companyListQueryOptions(null).queryKey).toEqual(companyListQueryOptions(null).queryKey);
|
|
});
|
|
|
|
it("stays under the `companies` prefix so existing invalidations still reach it", () => {
|
|
expect(companyListQueryOptions("user-1").queryKey.slice(0, 1)).toEqual(queryKeys.companies.all);
|
|
});
|
|
|
|
// Coalescing keys on the request path alone, so a `/companies` request issued
|
|
// under the previous session would otherwise answer this one and land the
|
|
// wrong account's list in an account-keyed entry.
|
|
it("refuses to join an in-flight request issued for another account", async () => {
|
|
mockCompaniesApi.list.mockResolvedValueOnce([]);
|
|
await companyListQueryOptions("user-2").queryFn();
|
|
expect(mockCompaniesApi.detachInflightList).toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
// 19 call sites invalidate `queryKeys.companies.all` after mutating a company.
|
|
// Keying the list deeper only stays safe while that prefix still matches it, so
|
|
// this asserts the matching itself rather than the shape of the key.
|
|
describe("existing invalidations still reach the account-keyed list", () => {
|
|
it("marks the list stale when the `companies` prefix is invalidated", async () => {
|
|
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
|
const key = companyListQueryOptions("user-1").queryKey;
|
|
queryClient.setQueryData(key, { companies: [], unauthorized: false });
|
|
expect(queryClient.getQueryState(key)?.isInvalidated).toBe(false);
|
|
|
|
await queryClient.invalidateQueries({ queryKey: queryKeys.companies.all });
|
|
|
|
expect(queryClient.getQueryState(key)?.isInvalidated).toBe(true);
|
|
});
|
|
});
|
|
|
|
// A session request that fails answers nothing. `authApi.getSession` returns
|
|
// null only for a 401 — a real "signed out" — and throws otherwise, so treating
|
|
// a failure as null would key a credentialed response to `anonymous` and hand
|
|
// it to every signed-out reader. The original bug, rebuilt for anonymous.
|
|
describe("an unavailable session is not an anonymous session", () => {
|
|
it("does not resolve an identity from an errored session entry", async () => {
|
|
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
|
mockAuthApi.getSession.mockRejectedValueOnce(new Error("network down"));
|
|
await queryClient
|
|
.fetchQuery({ queryKey: queryKeys.auth.session, queryFn: () => mockAuthApi.getSession(), retry: false })
|
|
.catch(() => undefined);
|
|
expect(queryClient.getQueryState(queryKeys.auth.session)?.status).toBe("error");
|
|
|
|
// Asked again rather than assumed anonymous.
|
|
mockAuthApi.getSession.mockResolvedValueOnce({ user: { id: "user-9" } });
|
|
await expect(resolveAccountUserId(queryClient)).resolves.toBe("user-9");
|
|
});
|
|
|
|
it("fetches the session when the cache has no answer, instead of defaulting to anonymous", async () => {
|
|
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
|
mockAuthApi.getSession.mockResolvedValueOnce({ user: { id: "user-7" } });
|
|
|
|
await expect(resolveAccountUserId(queryClient)).resolves.toBe("user-7");
|
|
expect(mockAuthApi.getSession).toHaveBeenCalled();
|
|
});
|
|
|
|
it("still treats a confirmed 401 as the anonymous identity", async () => {
|
|
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
|
// `getSession` resolves null for a 401 — signed out is an answer.
|
|
mockAuthApi.getSession.mockResolvedValueOnce(null);
|
|
|
|
await expect(resolveAccountUserId(queryClient)).resolves.toBeNull();
|
|
});
|
|
});
|
|
|
|
|
|
// Invalidation is the app saying "that answer belongs to the previous account".
|
|
// The entry stays `success` with the old user until a refetch lands, so a status
|
|
// check alone would key the new account's list to the old account's id.
|
|
describe("an invalidated session is not a current identity", () => {
|
|
it("re-reads the session rather than trusting an invalidated success", async () => {
|
|
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
|
mockAuthApi.getSession.mockResolvedValueOnce({ user: { id: "user-1" } });
|
|
await queryClient.fetchQuery({
|
|
queryKey: queryKeys.auth.session,
|
|
queryFn: () => mockAuthApi.getSession(),
|
|
retry: false,
|
|
});
|
|
|
|
// Signed in as somebody else; the invite page invalidates and asks at once.
|
|
await queryClient.invalidateQueries({ queryKey: queryKeys.auth.session });
|
|
expect(queryClient.getQueryState(queryKeys.auth.session)?.status).toBe("success");
|
|
mockAuthApi.getSession.mockResolvedValueOnce({ user: { id: "user-2" } });
|
|
|
|
await expect(resolveAccountUserId(queryClient)).resolves.toBe("user-2");
|
|
});
|
|
});
|