mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
d807887534
commit
1b0f78100d
4 files changed
+42
-5
No files matched your search
@@ -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<string, unknown>) {
|
||||
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);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
|
||||
@@ -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 },
|
||||
},
|
||||
];
|
||||
|
||||
Reference in new issue
Block a user