From 1b0f78100d74b19b8759d69019f9debffb542b54 Mon Sep 17 00:00:00 2001 From: Svetlana Zolotenkova <69031404+MrBlackTongue@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:43:02 +0300 Subject: [PATCH] fix(access): return membershipRole in the company user directory (#15183) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents often check a human approval. The check must confirm the role of the approving user in the company. > - An agent can read the company user directory. The directory removes the membership role from the server record. > - The admin member list requires `users:manage_permissions`. An approval check must not need that permission. > - This pull request returns `membershipRole` in the existing user directory and keeps all access limits. > - The benefit is that agents can verify an approver's role without admin permissions. ## Linked Issues or Issue Description **What happened?** `GET /api/companies/{companyId}/user-directory` returns active users without `membershipRole`. An agent that checks the authority of an approving user cannot tell an `owner` from other roles. `/members` requires `users:manage_permissions`. **Expected behavior** The directory returns the exact `membershipRole` from `companyMemberships`. A missing role stays `null`. Reading this field does not require admin permissions. Member management keeps its current limits. **Steps to reproduce** 1. Use a company with an active `owner` user and an active user without a role. 2. Call `/user-directory` with an agent key. 3. Before this change, neither record has `membershipRole`. After this change, the field is `owner` and `null`. 4. The same agent still cannot read `/members` or change a member. ## What Changed - Add `membershipRole` to the select and result of `loadCompanyUserDirectory`. - Test the exact `owner`/`null` values, agent read access, denied admin read and change, denied other company, and denied unauthenticated request. - Keep the existing `assertCompanyAccess` and the company, user type, and active filters. - The client type includes a nullable `membershipRole`. The management test rejects only `users:manage_permissions` and checks the exact calls. The first review findings are fixed in a0d1ad3. ## Verification - `pnpm --filter @paperclipai/server typecheck`: pass. - `pnpm --filter @paperclipai/server build`: pass. - `pnpm --filter @paperclipai/ui typecheck`: pass. - `pnpm exec vitest run server/src/__tests__/company-user-directory-route.test.ts server/src/__tests__/access-service.test.ts server/src/__tests__/access-routes-hidden-floor.test.ts`: 16 passed, 17 skipped. `ui/src/lib/company-members.test.ts` is added. - The regression test fails on the old `access.ts` because `membershipRole` is missing. It passes after the change. - `git diff --check`: pass. ## Risks The directory response gets one more field. There are no migrations. The role comes directly from the server record, with no `owner` fallback. User management permissions do not change. ## Model Used OpenAI Codex, GPT-6, with shell tools, code editing, and test runs. The environment does not expose the exact model identifier. ## 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 --- .../company-user-directory-route.test.ts | 43 ++++++++++++++++--- server/src/routes/access.ts | 2 + ui/src/api/access.ts | 1 + ui/src/lib/company-members.test.ts | 1 + 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/company-user-directory-route.test.ts b/server/src/__tests__/company-user-directory-route.test.ts index c56ea4e0d49..783e6c63754 100644 --- a/server/src/__tests__/company-user-directory-route.test.ts +++ b/server/src/__tests__/company-user-directory-route.test.ts @@ -4,11 +4,13 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { accessRoutes } from "../routes/access.js"; import { errorHandler } from "../middleware/index.js"; +const { hasPermission } = vi.hoisted(() => ({ hasPermission: vi.fn() })); + vi.mock("../services/index.js", () => ({ accessService: () => ({ isInstanceAdmin: vi.fn(), canUser: vi.fn(), - hasPermission: vi.fn(), + hasPermission, }), agentService: () => ({ getById: vi.fn(), @@ -26,8 +28,8 @@ vi.mock("../services/index.js", () => ({ function createDbStub() { const activeMemberships = [ - { principalId: "user-2", status: "active" as const }, - { principalId: "user-1", status: "active" as const }, + { principalId: "user-2", status: "active" as const, membershipRole: null }, + { principalId: "user-1", status: "active" as const, membershipRole: "owner" }, ]; const users = [ { id: "user-1", name: "Dotta", email: "dotta@example.com", image: "https://example.com/dotta.png" }, @@ -48,7 +50,7 @@ function createDbStub() { "updatedAt" in table; return { - select() { + select(fields: Record) { return { from(table: unknown) { if (isCompanyMembershipsTable(table)) { @@ -57,7 +59,9 @@ function createDbStub() { return query; }, orderBy() { - return Promise.resolve(activeMemberships); + return Promise.resolve(activeMemberships.map((member) => Object.fromEntries( + Object.keys(fields).map((key) => [key, member[key as keyof typeof member]]), + ))); }, }; return query; @@ -119,14 +123,43 @@ describe("GET /companies/:companyId/user-directory", () => { { principalId: "user-2", status: "active", + membershipRole: null, user: { id: "user-2", name: null, email: "alex@example.com", image: null }, }, { principalId: "user-1", status: "active", + membershipRole: "owner", user: { id: "user-1", name: "Dotta", email: "dotta@example.com", image: "https://example.com/dotta.png" }, }, ], }); }); + + it("lets a company agent read roles without allowing membership management", async () => { + hasPermission.mockImplementation(async (_companyId, _principalType, _principalId, permissionKey) => permissionKey !== "users:manage_permissions"); + const app = createApp({ type: "agent", agentId: "agent-1", companyId: "company-1", source: "agent_key" }); + const res = await request(app).get("/api/companies/company-1/user-directory"); + expect(res.status).toBe(200); + expect(res.body.users.map(({ principalId, status, membershipRole }: any) => ({ principalId, status, membershipRole }))).toEqual([ + { principalId: "user-2", status: "active", membershipRole: null }, + { principalId: "user-1", status: "active", membershipRole: "owner" }, + ]); + expect((await request(app).get("/api/companies/company-1/members")).status).toBe(403); + expect((await request(app).patch("/api/companies/company-1/members/member-1").send({ membershipRole: "owner" })).status).toBe(403); + expect(hasPermission.mock.calls).toEqual([ + ["company-1", "agent", "agent-1", "users:manage_permissions"], + ["company-1", "agent", "agent-1", "users:manage_permissions"], + ]); + }); + + it("rejects another company and unauthenticated callers", async () => { + for (const actor of [ + { type: "agent", agentId: "agent-1", companyId: "company-2", source: "agent_key" } as const, + { type: "none", source: "none" } as const, + ]) { + const res = await request(createApp(actor)).get("/api/companies/company-1/user-directory"); + expect(res.status).toBe(actor.type === "none" ? 401 : 403); + } + }); }); diff --git a/server/src/routes/access.ts b/server/src/routes/access.ts index e1fe2566c3f..3279a1ec780 100644 --- a/server/src/routes/access.ts +++ b/server/src/routes/access.ts @@ -1382,6 +1382,7 @@ async function loadCompanyUserDirectory(db: Db, companyId: string) { .select({ principalId: companyMemberships.principalId, status: companyMemberships.status, + membershipRole: companyMemberships.membershipRole, }) .from(companyMemberships) .where( @@ -1399,6 +1400,7 @@ async function loadCompanyUserDirectory(db: Db, companyId: string) { return members.map((member) => ({ principalId: member.principalId, status: "active" as const, + membershipRole: member.membershipRole, user: userMap.get(member.principalId) ?? null, })); } diff --git a/ui/src/api/access.ts b/ui/src/api/access.ts index 35c76898e0f..c77b212d3e1 100644 --- a/ui/src/api/access.ts +++ b/ui/src/api/access.ts @@ -149,6 +149,7 @@ export type CompanyMembersResponse = { export type CompanyUserDirectoryEntry = { principalId: string; status: "active"; + membershipRole: string | null; user: { id: string; email: string | null; name: string | null; image: string | null } | null; }; diff --git a/ui/src/lib/company-members.test.ts b/ui/src/lib/company-members.test.ts index c8b1e630ba6..f2231d491a6 100644 --- a/ui/src/lib/company-members.test.ts +++ b/ui/src/lib/company-members.test.ts @@ -130,6 +130,7 @@ describe("company-members helpers", () => { { principalId: "user-1", status: "active", + membershipRole: null, user: { id: "user-1", name: "Taylor", email: "taylor@example.com", image: null }, }, ];