From 4568bce1b1fbcad781976c8b76c9933e4da732c7 Mon Sep 17 00:00:00 2001 From: kaseywright Date: Thu, 17 Sep 2026 13:06:41 -0400 Subject: [PATCH 1/3] feat(organizations): org-level role management for Org Managers (#337) Implements API-2 per Product decisions: - D1: /users manages org-level roles only; PATCH accepts 'Org Manager' (promote) and 'Org Member' (demote). Demotion removes non-anchor org-scoped grants while preserving the Org Member anchor and all project-scoped grants. - D2: self-role-change and self-removal both return 403, guaranteeing at least one Org Manager always remains. - Grant ROLE_ASSIGN_ORG_MANAGER to the Org Manager role in the RBAC seed so OMs can promote/demote within their own org (deploy note: re-run the RBAC seed). - PATCH /organizations/{orgId}/users/{userId} with requireUserAccess(USER_UPDATE) + canAssignRole(org-scoped) checks; idempotent on unchanged role. - Block self-removal in DELETE /organizations/{orgId}/users/{userId}. - requireSuperAdmin now explicitly requires a global grant, since role:assign:org_manager is no longer SuperAdmin-exclusive. - Remove dead role-stripping code from PATCH /users/:id (role changes now go through org/project role endpoints only). - Update org-onboarding plan + ticket docs for D1-D3. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- docs/features/org-onboarding/plan.md | 28 +++--- ...-org-manager-self-service-prerequisites.md | 57 +++++------ src/db/seeds/rbac.ts | 1 + .../users/org-users.repository.ts | 75 ++++++++++++++- .../organizations/users/org-users.route.ts | 83 +++++++++++++++- .../users/org-users.service.test.ts | 95 +++++++++++++++++++ .../organizations/users/org-users.service.ts | 42 ++++++++ .../organizations/users/org-users.types.ts | 11 +++ src/domains/users/users.route.ts | 15 --- .../services/permissions/authorize.test.ts | 23 +++++ src/middlewares/role-auth.test.ts | 53 +++++++++++ src/middlewares/role-auth.ts | 5 +- 12 files changed, 419 insertions(+), 69 deletions(-) create mode 100644 src/domains/organizations/users/org-users.service.test.ts create mode 100644 src/domains/organizations/users/org-users.service.ts create mode 100644 src/domains/organizations/users/org-users.types.ts create mode 100644 src/middlewares/role-auth.test.ts diff --git a/docs/features/org-onboarding/plan.md b/docs/features/org-onboarding/plan.md index d6a9278b..99e10cad 100644 --- a/docs/features/org-onboarding/plan.md +++ b/docs/features/org-onboarding/plan.md @@ -132,35 +132,31 @@ No code change expected. Add a route-level or middleware-level test proving the ## Ticket API-2: Prerequisites for fluent-web #489 (Org Manager self-service on the Users page) +**Implemented on `feat/org-manager-self-service`.** Product decisions (2026-09-16): **D1 = Option (b)** — org-level roles only on the Users page (`Org Manager` to promote, `Org Member` to demote); no org-level PM, so Task 3 was dropped. **D2** — self-change block is the last-OM guard; DELETE also rejects self-removal. **D3** — role display precedence is web-side. + Full task detail, including tests, lives in `fluent-web/docs/features/org-manager-users-page/plan.md` Phase A. Summary: ### Task 1: Org Manager may assign Org Manager -- [ ] `rbac.ts`: add `{ roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER }`. -- [ ] `authorize.test.ts`: org-scoped Org Manager can `canAssignRole(…, ORG_MANAGER, ORG, null)`; still cannot assign `SuperAdmin`. -- [ ] Update the comment in `requireSuperAdmin` (`role-auth.ts:121-123`): the permission is no longer SuperAdmin-exclusive, but the check still holds because it also requires a _global_ grant. Add a test that an Org Manager with the new permission is still rejected by `requireSuperAdmin`. +- [x] `rbac.ts`: added `{ roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER }`. +- [x] `authorize.test.ts`: org-scoped OM can assign OM (not SuperAdmin, not cross-org); project-pinned `role:assign:org_manager` can't satisfy org scope. +- [x] `requireSuperAdmin` comment updated; new `role-auth.test.ts` proves an OM holding the org-scoped permission is still rejected. ### Task 2: Org-level role change ``` -PATCH /organizations/{orgId}/users/{userId} body { roleName: 'Org Manager' | 'Project Manager'* } -200 userResponseSchema | 400 role not allowed at org level | 403 self-change or !canAssignRole | 404 not a member +PATCH /organizations/{orgId}/users/{userId} body { roleName: 'Org Manager' | 'Org Member' } +200 userResponseSchema | 400 not a member | 403 self-change or !canAssignRole ``` -\* `Project Manager` only if decision D1 in the #489 plan lands on "org-level PM". - -- [ ] Service `updateOrgUserRole(callerId, orgId, userId, roleName)` in `src/domains/organizations/users/org-users.service.ts`: forbid `callerId === userId`; in a transaction delete `user_roles` rows `(userId, orgId, projectId IS NULL, role ≠ Org Member)` then insert the new grant; return `usersService.getUserById(userId)`. -- [ ] Route in `org-users.route.ts`, mirroring `PATCH /projects/{projectId}/users/{userId}`. -- [ ] Tests: self-change → 403; non-member → 404; anchor + project-scoped grants preserved; idempotent when unchanged. - -### Task 3 (conditional on D1): Org-level Project Manager grant +- [x] `org-users.service.ts` + `repo.updateOrgUserRole`: forbid `callerId === userId`; require org membership (`findUserIdsInOrg`); transaction replaces the non-anchor org-level grant (or deletes it on demote), keeping the `Org Member` anchor and all project-scoped grants; idempotent when unchanged. +- [x] Route in `org-users.route.ts` with `requirePermission(USER_UPDATE, org-scope)` + `canAssignRole` in the handler. DELETE org-user now also blocks `caller.id === userId` (self-removal → 403). -- [ ] `canAssignRole` branch 4: `PROJECT_MANAGER` with `projectId === null` requires `ROLE_ASSIGN_PROJECT` at org scope. Translator/Observer remain project-only. -- [ ] Revisit the TEMP bypass in `projects.route.ts:90-104`. +### Task 3: ~~Org-level Project Manager grant~~ — dropped per D1 (b) -### Task 4: Optional cleanup +### Task 4: Cleanup -- [ ] `PATCH /users/:id` handler (`users.route.ts:465-475`) strips `role` after the zod schema has already dropped it — dead code. Remove or add `role` to the schema deliberately (recommend remove; role changes go through the org/project endpoints). +- [x] Removed the dead `updates.role` strip (and now-unused `authorize` import) from `PATCH /users/:id`. --- diff --git a/docs/features/org-onboarding/tickets/2026-09-16-org-manager-self-service-prerequisites.md b/docs/features/org-onboarding/tickets/2026-09-16-org-manager-self-service-prerequisites.md index 8d5f41f4..5486014d 100644 --- a/docs/features/org-onboarding/tickets/2026-09-16-org-manager-self-service-prerequisites.md +++ b/docs/features/org-onboarding/tickets/2026-09-16-org-manager-self-service-prerequisites.md @@ -1,6 +1,6 @@ # Org Manager self-service prerequisites for fluent-web #489 -> **Status: NOT STARTED** — awaiting go-ahead to implement, and Product decision D1 (see below). +> **Status: IMPLEMENTED (local)** — on `feat/org-manager-self-service`, awaiting review before push. > GitHub: [fluent-api#337](https://github.com/eten-tech-foundation/fluent-api/issues/337) **Parent feature:** [`org-onboarding`](../plan.md) — Ticket API-2. Task-level detail with tests also in `fluent-web/docs/features/org-manager-users-page/plan.md` Phase A. @@ -16,14 +16,12 @@ fluent-web#489 lets an Org Manager add/edit Org Managers and Project Managers fr 2. `PATCH /users/:id` ignores `role` — not in `updateUserRequestSchema`, and stripped again in `users.repository.update()`. The Edit User dialog "saves" a role that never changes. No org-level role-change endpoint exists; only `PATCH /projects/{projectId}/users/{userId}`. 3. `Project Manager` exists only as a project-pinned grant; `canAssignRole` returns `false` for PM with `projectId === null` (`authorize.ts:82-90`). "Org-scoped Project Manager" as written in #489 is not in the model. -## Product decision D1 (open) +## Product decisions (resolved 2026-09-16) -How should "Project Manager" behave on the org-level Users page? - -- **(a) Org-level PM grant** — allow `Project Manager` with `projectId = null`. Matches #489's wording. `grant-utils.isProjectManager()` on the web already treats a null-project manager grant as managing every project in the org. New RBAC concept; touches `canAssignRole` and the TEMP bypass in `projects.route.ts:90-104`. -- **(b) Org Manager only** — the Users page offers only `Org Manager`; PMs stay per-project via Add Project User. Deviates from #489. - -Task 3 below exists only under (a). +- **D1 → Option (b):** Project roles stay project-scoped. The Users page manages org-level roles only — `Org Manager` (promote) and `Org Member` (demote, removes the org-level role while keeping the anchor + project grants). There is no org-level Project Manager; Task 3 is dropped. +- **D2 → Self-change block is the guard:** an Org Manager cannot change their own org-level role; they can only be demoted by a different Org Manager, which keeps the org with ≥1 OM. Self-removal via DELETE is blocked for the same reason. +- **D3 → display order** is a web concern (member → org-level role → project role priority PM > Translator > Observer); no API change. +- **Removal notice:** `DELETE /organizations/{orgId}/users/{userId}` already clears chapter assignments + all org grants; the "user has assignments" warning is rendered web-side via `GET /users/{userId}/chapter-assignments`. ## Tasks @@ -31,43 +29,32 @@ Task 3 below exists only under (a). Files: `src/db/seeds/rbac.ts`, `src/lib/services/permissions/authorize.test.ts`, `src/middlewares/role-auth.ts` (+ test) -- [ ] Add `{ roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER }` to the Org Manager block. -- [ ] Tests: org-scoped Org Manager → `canAssignRole(…, ORG_MANAGER, ORG, null) === true`; same caller cannot assign `SuperAdmin`. -- [ ] `requireSuperAdmin` (`role-auth.ts:121-123`) comments call this permission SuperAdmin-exclusive; it no longer is. The check still holds because it also requires a _global_ grant. Fix the comment and add a test that an Org Manager holding the new permission is still rejected. +- [x] Add `{ roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER }` to the Org Manager block. +- [x] Tests: org-scoped Org Manager → `canAssignRole(…, ORG_MANAGER, ORG, null) === true`; same caller cannot assign `SuperAdmin`; project-pinned `role:assign:org_manager` cannot satisfy org scope. +- [x] `requireSuperAdmin` comment updated (permission no longer exclusive; global-grant requirement keeps the check). New `src/middlewares/role-auth.test.ts` proves an org-scoped OM holding the permission is still rejected. ### 2. Org-level role change endpoint -Create: `src/domains/organizations/users/org-users.service.ts` (+ `.test.ts`), `org-users.types.ts`. Modify: `org-users.route.ts`. +Created: `org-users.service.ts`, `org-users.service.test.ts`, `org-users.types.ts`. Modified: `org-users.route.ts`, `org-users.repository.ts`. ``` -PATCH /organizations/{orgId}/users/{userId} body { roleName: 'Org Manager' | 'Project Manager'* } -middleware: authenticateUser, requireUserAccess(USER_ACTIONS.UPDATE, 'userId') -200 userResponseSchema (orgGrants refreshed) -400 roleName not allowed at org level -403 caller === target (self-change blocked, same as project route) OR !canAssignRole(caller, roleName, orgId, null) -404 target is not a member of orgId +PATCH /organizations/{orgId}/users/{userId} body { roleName: 'Org Manager' | 'Org Member' } +middleware: authenticateUser, requirePermission(USER_UPDATE, orgId-from-param) +handler: canAssignRole(caller, roleName, orgId, null) → 403 +200 userResponseSchema (grants refreshed) +400 target is not a member of orgId (USER_NOT_IN_ORGANIZATION, per getHttpStatus) +403 caller === target (self-change, D2) OR !canAssignRole ``` -\* only under D1 (a). - -- [ ] Failing service tests: self-change → `FORBIDDEN`; non-member → `USER_NOT_FOUND`; replaces the existing org-level non-anchor grant, keeps the `Org Member` anchor and all project-scoped grants; idempotent when unchanged. -- [ ] Service `updateOrgUserRole(callerId, orgId, userId, roleName)`: `getRoleId`; in one transaction delete `user_roles` where `(userId, orgId, projectId IS NULL, role ≠ Org Member)`, insert the new grant; return `usersService.getUserById(userId)`. -- [ ] Route mirrors `updateProjectUserRoleRoute` in `project-users.route.ts:185-240`; body schema `z.object({ roleName: z.enum([...allowedOrgRoles]) })`. -- [ ] Unit tests for `canAssignRole` at scope `{ orgId, projectId: null }` for each allowed role. - -### 3. Org-level Project Manager grant — only under D1 (a) - -Files: `src/lib/services/permissions/authorize.ts` (+ test), `src/domains/projects/projects.route.ts` - -- [ ] `canAssignRole` branch 4: `PROJECT_MANAGER && projectId === null` → require `ROLE_ASSIGN_PROJECT` at `{ orgId, projectId: null }`. Translator/Observer remain project-only. -- [ ] Tests: Org Manager can assign org-level PM; project-pinned PM cannot (grant not applicable at org scope); Translator/Observer at org scope still `false`. -- [ ] Revisit the TEMP bypass comment in `projects.route.ts:90-104` — an org-level PM satisfies the normal `authorize()` path. Keep the bypass for legacy project-pinned PMs or remove in a follow-up; document the choice. +- [x] Service tests: self-change → `FORBIDDEN` (repo untouched); non-member → `USER_NOT_IN_ORGANIZATION`; success returns refreshed user; repo failure propagates. +- [x] `repo.updateOrgUserRole` in one transaction: `roleId === orgMemberRoleId` → delete non-anchor org-level rows (demote); otherwise replace the non-anchor org-level row set with the new grant (idempotent when unchanged). Anchor + project-scoped grants always preserved. +- [x] DELETE org-user now also rejects `caller.id === userId` (self-removal) → 403, consistent with D2. -Under D1 (b): skip, and restrict Task 2's enum to `Org Manager`. +### 3. ~~Org-level Project Manager grant~~ — dropped per D1 (b) -### 4. Optional cleanup +### 4. Cleanup -- [ ] `PATCH /users/:id` handler (`users.route.ts:465-475`) deletes `updates.role` after zod already dropped it — dead code. Remove it; role changes go through org/project endpoints. +- [x] Removed dead `updates.role` strip + unused `authorize`/`hasGrantManagement` block from `PATCH /users/:id` (`users.route.ts`). Role changes go through the org/project endpoints only. ## Verification diff --git a/src/db/seeds/rbac.ts b/src/db/seeds/rbac.ts index 2c46ef76..9dc374f7 100644 --- a/src/db/seeds/rbac.ts +++ b/src/db/seeds/rbac.ts @@ -38,6 +38,7 @@ const ROLE_PERMISSION_MAP = [ { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.CONTENT_UPDATE }, { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.MEMBERSHIP_REVOKE }, { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_PROJECT }, + { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER }, { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.USER_VIEW }, { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.USER_CREATE }, { roleName: ROLES.ORG_MANAGER, permissionName: PERMISSIONS.USER_UPDATE }, diff --git a/src/domains/organizations/users/org-users.repository.ts b/src/domains/organizations/users/org-users.repository.ts index 07b8ecde..4f811aad 100644 --- a/src/domains/organizations/users/org-users.repository.ts +++ b/src/domains/organizations/users/org-users.repository.ts @@ -1,10 +1,12 @@ -import { and, eq, inArray, sql } from 'drizzle-orm'; +import { and, eq, inArray, isNull, sql } from 'drizzle-orm'; import type { Result } from '@/lib/types'; import { db } from '@/db'; import { chapter_assignments, project_units, projects, user_roles } from '@/db/schema'; +import { getRoleId } from '@/domains/user-roles/user-roles.service'; import { logger } from '@/lib/logger'; +import { ROLES } from '@/lib/roles'; import { err, ErrorCode, ok } from '@/lib/types'; /** @@ -82,3 +84,74 @@ export async function removeOrgUser(orgId: number, userId: number): Promise> { + try { + const orgMemberRoleId = await getRoleId(ROLES.ORG_MEMBER); + + return await db.transaction(async (tx) => { + const nonAnchorOrgScope = and( + eq(user_roles.userId, userId), + eq(user_roles.orgId, orgId), + isNull(user_roles.projectId), + sql`${user_roles.roleId} != ${orgMemberRoleId}` + ); + + if (roleId === orgMemberRoleId) { + await tx.delete(user_roles).where(nonAnchorOrgScope); + return ok(undefined); + } + + const existing = await tx + .select({ id: user_roles.id, roleId: user_roles.roleId }) + .from(user_roles) + .where(nonAnchorOrgScope); + + if (existing.length === 1 && existing[0].roleId === roleId) { + return ok(undefined); + } + + if (existing.length > 0) { + await tx.delete(user_roles).where( + inArray( + user_roles.id, + existing.map((r) => r.id) + ) + ); + } + + await tx + .insert(user_roles) + .values({ userId, orgId, projectId: null, roleId, createdBy }) + .onConflictDoNothing(); + + return ok(undefined); + }); + } catch (error) { + logger.error({ + cause: error, + message: 'Failed to update org-level role for user', + context: { orgId, userId, roleId }, + }); + return err(ErrorCode.INTERNAL_ERROR); + } +} diff --git a/src/domains/organizations/users/org-users.route.ts b/src/domains/organizations/users/org-users.route.ts index 0fe3a508..205b5dc0 100644 --- a/src/domains/organizations/users/org-users.route.ts +++ b/src/domains/organizations/users/org-users.route.ts @@ -1,17 +1,21 @@ import { createRoute, z } from '@hono/zod-openapi'; import * as HttpStatusCodes from 'stoker/http-status-codes'; import * as HttpStatusPhrases from 'stoker/http-status-phrases'; -import { jsonContent } from 'stoker/openapi/helpers'; +import { jsonContent, jsonContentRequired } from 'stoker/openapi/helpers'; import { createMessageObjectSchema } from 'stoker/openapi/schemas'; +import { getRoleId } from '@/domains/user-roles/user-roles.service'; import * as usersService from '@/domains/users/users.service'; import { userResponseSchema } from '@/domains/users/users.types'; import { PERMISSIONS } from '@/lib/permissions'; +import { canAssignRole } from '@/lib/services/permissions/authorize'; import { getHttpStatus } from '@/lib/types'; import { authenticateUser, requirePermission } from '@/middlewares/role-auth'; import { server } from '@/server/server'; import { removeOrgUser } from './org-users.repository'; +import { updateOrgUserRole } from './org-users.service'; +import { updateOrgUserRoleBodySchema } from './org-users.types'; // ── Shared param schema ──────────────────────────────────────────────────────── @@ -138,9 +142,86 @@ const removeOrgUserRoute = createRoute({ server.openapi(removeOrgUserRoute, async (c) => { const { orgId, userId } = c.req.valid('param'); + const caller = c.get('user')!; + + // Self-removal is blocked for the same reason self-role-change is (D2): an + // Org Manager who could remove themselves could leave the org with no OM. + if (caller.id === userId) { + return c.json( + { message: 'You cannot remove yourself from the organization.' }, + HttpStatusCodes.FORBIDDEN + ); + } const result = await removeOrgUser(orgId, userId); if (result.ok) return c.body(null, HttpStatusCodes.NO_CONTENT); return c.json({ message: result.error.message }, getHttpStatus(result.error) as never); }); + +// ─── PATCH /organizations/:orgId/users/:userId ───────────────────────────────── + +const updateOrgUserRoleRoute = createRoute({ + tags: ['Organizations - Users'], + method: 'patch', + path: '/organizations/{orgId}/users/{userId}', + middleware: [ + authenticateUser, + requirePermission(PERMISSIONS.USER_UPDATE, (c) => { + const orgId = Number(c.req.param('orgId')); + return Number.isFinite(orgId) ? { orgId } : {}; + }), + ] as const, + request: { + params: orgUserParamSchema, + body: jsonContentRequired( + updateOrgUserRoleBodySchema, + 'New org-level role: "Org Manager" to grant it, "Org Member" to remove the org-level role (demote).' + ), + }, + responses: { + [HttpStatusCodes.OK]: jsonContent( + userResponseSchema, + 'The updated user with refreshed role grants' + ), + [HttpStatusCodes.BAD_REQUEST]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.BAD_REQUEST), + 'User is not a member of this org' + ), + [HttpStatusCodes.UNAUTHORIZED]: jsonContent( + createMessageObjectSchema('Unauthorized'), + 'Authentication required' + ), + [HttpStatusCodes.FORBIDDEN]: jsonContent( + createMessageObjectSchema('Forbidden'), + 'Insufficient privileges to assign this role, or self-role-change attempted' + ), + [HttpStatusCodes.INTERNAL_SERVER_ERROR]: jsonContent( + createMessageObjectSchema(HttpStatusPhrases.INTERNAL_SERVER_ERROR), + 'Internal server error' + ), + }, + summary: 'Update org-level role for a user', + description: + 'Changes the org-level role of an existing org member. Org-level roles only (D1): project roles remain project-scoped and are never touched. "Org Member" demotes — it deletes the org-level role row while keeping the membership anchor and all project-scoped grants. A caller may not change their own role (D2).', +}); + +server.openapi(updateOrgUserRoleRoute, async (c) => { + const { orgId, userId } = c.req.valid('param'); + const { roleName } = c.req.valid('json'); + const caller = c.get('user')!; + + const policyUser = { id: caller.id, grants: caller.grants }; + if (!canAssignRole(policyUser, roleName, orgId, null)) { + return c.json( + { message: 'Forbidden: Insufficient privileges to assign this role.' }, + HttpStatusCodes.FORBIDDEN + ); + } + + const roleId = await getRoleId(roleName); + const result = await updateOrgUserRole(caller.id, orgId, userId, roleId); + if (result.ok) return c.json(result.data, HttpStatusCodes.OK); + + return c.json({ message: result.error.message }, getHttpStatus(result.error) as never); +}); diff --git a/src/domains/organizations/users/org-users.service.test.ts b/src/domains/organizations/users/org-users.service.test.ts new file mode 100644 index 00000000..412d4bda --- /dev/null +++ b/src/domains/organizations/users/org-users.service.test.ts @@ -0,0 +1,95 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { findUserIdsInOrg } from '@/domains/user-roles/user-roles.repository'; +import * as usersService from '@/domains/users/users.service'; +import { ErrorCode } from '@/lib/types'; + +import * as repo from './org-users.repository'; +import { updateOrgUserRole } from './org-users.service'; + +vi.mock('@/domains/user-roles/user-roles.repository', () => ({ + findUserIdsInOrg: vi.fn(), +})); + +vi.mock('@/domains/users/users.service', () => ({ + getUserById: vi.fn(), +})); + +vi.mock('./org-users.repository', () => ({ + updateOrgUserRole: vi.fn(), + removeOrgUser: vi.fn(), +})); + +vi.mock('@/lib/logger', () => ({ + logger: { debug: vi.fn(), error: vi.fn(), info: vi.fn(), warn: vi.fn() }, +})); + +const ORG = 10; +const CALLER = 1; +const TARGET = 2; +const ROLE_ID = 7; + +const updatedUser = { + id: TARGET, + username: 'target', + firstName: 'Target', + lastName: 'User', + email: 'target@example.com', + status: 'verified' as const, + createdAt: new Date('2026-09-16T12:00:00.000Z'), + updatedAt: new Date('2026-09-16T12:00:00.000Z'), + createdBy: null, + grants: [], +}; + +describe('org-users service', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('updateOrgUserRole', () => { + it('rejects a caller changing their own org-level role (D2)', async () => { + const result = await updateOrgUserRole(CALLER, ORG, CALLER, ROLE_ID); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.code).toBe(ErrorCode.FORBIDDEN); + expect(repo.updateOrgUserRole).not.toHaveBeenCalled(); + expect(findUserIdsInOrg).not.toHaveBeenCalled(); + }); + + it('rejects when the target user is not a member of the org', async () => { + vi.mocked(findUserIdsInOrg).mockResolvedValue(new Set()); + + const result = await updateOrgUserRole(CALLER, ORG, TARGET, ROLE_ID); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.code).toBe(ErrorCode.USER_NOT_IN_ORGANIZATION); + expect(repo.updateOrgUserRole).not.toHaveBeenCalled(); + }); + + it('delegates to the repository and returns the refreshed user', async () => { + vi.mocked(findUserIdsInOrg).mockResolvedValue(new Set([TARGET])); + vi.mocked(repo.updateOrgUserRole).mockResolvedValue({ ok: true, data: undefined }); + vi.mocked(usersService.getUserById).mockResolvedValue({ ok: true, data: updatedUser }); + + const result = await updateOrgUserRole(CALLER, ORG, TARGET, ROLE_ID); + + expect(repo.updateOrgUserRole).toHaveBeenCalledWith(ORG, TARGET, ROLE_ID, CALLER); + expect(usersService.getUserById).toHaveBeenCalledWith(TARGET); + expect(result).toEqual({ ok: true, data: updatedUser }); + }); + + it('propagates repository failures without fetching the user', async () => { + vi.mocked(findUserIdsInOrg).mockResolvedValue(new Set([TARGET])); + vi.mocked(repo.updateOrgUserRole).mockResolvedValue({ + ok: false, + error: { code: ErrorCode.INTERNAL_ERROR, message: 'boom' }, + }); + + const result = await updateOrgUserRole(CALLER, ORG, TARGET, ROLE_ID); + + expect(result.ok).toBe(false); + expect(usersService.getUserById).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/domains/organizations/users/org-users.service.ts b/src/domains/organizations/users/org-users.service.ts new file mode 100644 index 00000000..6c258d7f --- /dev/null +++ b/src/domains/organizations/users/org-users.service.ts @@ -0,0 +1,42 @@ +import type { UserResponse } from '@/domains/users/users.types'; +import type { Result } from '@/lib/types'; + +import { findUserIdsInOrg } from '@/domains/user-roles/user-roles.repository'; +import * as usersService from '@/domains/users/users.service'; +import { err, ErrorCode } from '@/lib/types'; + +import * as repo from './org-users.repository'; + +/** + * Changes a member's org-level role. + * + * Callers must pass the authorization checks before reaching this service + * (org-scoped user:update middleware + canAssignRole in the route). This layer + * enforces the remaining invariants: + * + * - A user may not change their own org-level role (D2). An Org Manager can + * only be demoted by a *different* Org Manager, which guarantees the org + * always retains at least one. + * - The target must already belong to the org (any grant row counts as + * membership, matching addProjectUsers' check). + */ +export async function updateOrgUserRole( + callerId: number, + orgId: number, + userId: number, + roleId: number +): Promise> { + if (callerId === userId) { + return err(ErrorCode.FORBIDDEN); + } + + const memberIds = await findUserIdsInOrg(orgId, [userId]); + if (!memberIds.has(userId)) { + return err(ErrorCode.USER_NOT_IN_ORGANIZATION); + } + + const result = await repo.updateOrgUserRole(orgId, userId, roleId, callerId); + if (!result.ok) return result; + + return usersService.getUserById(userId); +} diff --git a/src/domains/organizations/users/org-users.types.ts b/src/domains/organizations/users/org-users.types.ts new file mode 100644 index 00000000..5cb2dad0 --- /dev/null +++ b/src/domains/organizations/users/org-users.types.ts @@ -0,0 +1,11 @@ +import { z } from '@hono/zod-openapi'; + +import { ROLES } from '@/lib/roles'; + +// Org-level roles only (D1): project-scoped roles are managed on the project. +// Org Member = demotion — removes the org-level role but keeps membership. +export const updateOrgUserRoleBodySchema = z.object({ + roleName: z.enum([ROLES.ORG_MANAGER, ROLES.ORG_MEMBER]), +}); + +export type UpdateOrgUserRoleBody = z.infer; diff --git a/src/domains/users/users.route.ts b/src/domains/users/users.route.ts index 3af6d43c..fd803520 100644 --- a/src/domains/users/users.route.ts +++ b/src/domains/users/users.route.ts @@ -15,7 +15,6 @@ import { createUserWithInvitation, inviteExistingUserToOrg, } from '@/lib/services/auth/auth.service'; -import { authorize } from '@/lib/services/permissions/authorize'; import { ErrorCode, ErrorMessages, getHttpStatus } from '@/lib/types'; import { authenticateUser, orgFromBody, requirePermission } from '@/middlewares/role-auth'; import { server } from '@/server/server'; @@ -441,8 +440,6 @@ const updateUserRoute = createRoute({ server.openapi(updateUserRoute, async (c) => { const { id } = c.req.valid('param'); const updates = c.req.valid('json'); - const currentUser = c.get('user')!; - const targetUser = c.get('targetUser')!; if (Object.keys(updates).length === 0) { return c.json( @@ -463,18 +460,6 @@ server.openapi(updateUserRoute, async (c) => { ); } - // Strip role update if user lacks MEMBERSHIP_REVOKE - const targetOrgIds = await findOrgIdsForUser(targetUser.id); - const hasGrantManagement = targetOrgIds.some((orgId) => - authorize({ id: currentUser.id, grants: currentUser.grants }, PERMISSIONS.MEMBERSHIP_REVOKE, { - orgId, - }) - ); - - if (!hasGrantManagement) { - delete (updates as Record).role; - } - const result = await userService.updateUser(id, updates); if (result.ok) { diff --git a/src/lib/services/permissions/authorize.test.ts b/src/lib/services/permissions/authorize.test.ts index 914934c3..79d17924 100644 --- a/src/lib/services/permissions/authorize.test.ts +++ b/src/lib/services/permissions/authorize.test.ts @@ -124,6 +124,7 @@ describe('authorize', () => { PERMISSIONS.CONTENT_UPDATE, PERMISSIONS.MEMBERSHIP_REVOKE, PERMISSIONS.ROLE_ASSIGN_PROJECT, + PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER, PERMISSIONS.USER_VIEW, PERMISSIONS.USER_CREATE, PERMISSIONS.USER_UPDATE, @@ -183,6 +184,28 @@ describe('canAssignRole', () => { expect(canAssignRole(superAdmin, ROLES.ORG_MANAGER, ORG, null)).toBe(true); }); + it('org-scoped Org Manager can assign Org Manager in their org (#337)', () => { + // Org Manager now holds role:assign:org_manager org-scoped (seeds/rbac.ts), + // which authorizes them to promote/demote Org Managers in their own org. + const orgManager = { + id: 5, + grants: [grant(ORG, null, [PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER])], + }; + expect(canAssignRole(orgManager, ROLES.ORG_MANAGER, ORG, null)).toBe(true); + // ...but the permission still cannot mint a SuperAdmin + expect(canAssignRole(orgManager, ROLES.SUPER_ADMIN, ORG, null)).toBe(false); + // ...or grant Org Manager in a different org + expect(canAssignRole(orgManager, ROLES.ORG_MANAGER, 2, null)).toBe(false); + }); + + it('project-pinned role:assign:org_manager grant cannot assign Org Manager at org scope', () => { + const pinned = { + id: 6, + grants: [grant(ORG, PROJ, [PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER])], + }; + expect(canAssignRole(pinned, ROLES.ORG_MANAGER, ORG, null)).toBe(false); + }); + it('org Manager with USER_CREATE can invite Org Member (create anchor row)', () => { const orgManager = { id: 5, diff --git a/src/middlewares/role-auth.test.ts b/src/middlewares/role-auth.test.ts new file mode 100644 index 00000000..4778e05a --- /dev/null +++ b/src/middlewares/role-auth.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { PERMISSIONS } from '@/lib/permissions'; + +import { requireSuperAdmin } from './role-auth'; + +const grant = (orgId: number | null, projectId: number | null, perms: string[]) => ({ + orgId, + projectId, + permissions: new Set(perms), +}); + +const ctx = (user: unknown) => ({ + get: (key: string) => (key === 'user' ? user : undefined), +}); + +describe('requireSuperAdmin', () => { + it('passes a global SuperAdmin grant holder', async () => { + const user = { + id: 1, + grants: [grant(null, null, [PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER])], + }; + const next = vi.fn(); + + await requireSuperAdmin(ctx(user), next); + + expect(next).toHaveBeenCalledOnce(); + }); + + it('rejects an Org Manager holding org-scoped role:assign:org_manager (#337)', async () => { + // Since #337, Org Manager holds role:assign:org_manager — but org-scoped, + // never global. requireSuperAdmin must still reject them. + const orgManager = { + id: 2, + grants: [grant(1, null, [PERMISSIONS.ROLE_ASSIGN_ORG_MANAGER])], + }; + const next = vi.fn(); + + await expect(requireSuperAdmin(ctx(orgManager), next)).rejects.toMatchObject({ + status: 403, + }); + expect(next).not.toHaveBeenCalled(); + }); + + it('rejects an unauthenticated request', async () => { + const next = vi.fn(); + + await expect(requireSuperAdmin(ctx(undefined), next)).rejects.toMatchObject({ + status: 401, + }); + expect(next).not.toHaveBeenCalled(); + }); +}); diff --git a/src/middlewares/role-auth.ts b/src/middlewares/role-auth.ts index c9d2f9eb..de1a06d9 100644 --- a/src/middlewares/role-auth.ts +++ b/src/middlewares/role-auth.ts @@ -119,7 +119,10 @@ export async function requireSuperAdmin(c: any, next: any) { } // A SuperAdmin must have a global grant (orgId=null, projectId=null) - // AND hold a SuperAdmin-exclusive permission (role:assign:org_manager). + // AND hold role:assign:org_manager. The permission is no longer + // SuperAdmin-exclusive (Org Manager holds it org-scoped since #337), but the + // global-grant requirement keeps this check airtight: an org-scoped grant + // can never have orgId === null. // Checking scope alone would let any future global read-only role pass. const isSuperAdmin = user.grants.some( (g: any) => From 8772ff003e378049faeeb2e1a319f539cfe7b896 Mon Sep 17 00:00:00 2001 From: kaseywright Date: Thu, 17 Sep 2026 13:13:02 -0400 Subject: [PATCH 2/3] chore: remove unused eslint-disable directive --- src/domains/users/users.route.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domains/users/users.route.ts b/src/domains/users/users.route.ts index fd803520..0b8f21cf 100644 --- a/src/domains/users/users.route.ts +++ b/src/domains/users/users.route.ts @@ -554,7 +554,7 @@ server.openapi(updateActiveOrgRoute, async (c) => { if (!belongsToOrg) { return c.json( { message: 'User does not belong to this organization' }, - // eslint-disable-next-line max-lines + HttpStatusCodes.FORBIDDEN ); } From 9bee1e36662326c89c5320a014e9ffe161388b66 Mon Sep 17 00:00:00 2001 From: kaseywright Date: Thu, 17 Sep 2026 13:34:12 -0400 Subject: [PATCH 3/3] fix(organizations): lock member anchor during org-role update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit on #340: the service-level membership check raced with removeOrgUser — a removal could commit between the check and the write transaction, leaving an org role grant without the Org Member anchor. Select the anchor FOR UPDATE inside the transaction so the update either locks the anchor before removal or observes the removal and returns USER_NOT_IN_ORGANIZATION. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../users/org-users.repository.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/domains/organizations/users/org-users.repository.ts b/src/domains/organizations/users/org-users.repository.ts index 4f811aad..6de5bc1c 100644 --- a/src/domains/organizations/users/org-users.repository.ts +++ b/src/domains/organizations/users/org-users.repository.ts @@ -109,6 +109,27 @@ export async function updateOrgUserRole( const orgMemberRoleId = await getRoleId(ROLES.ORG_MEMBER); return await db.transaction(async (tx) => { + // Lock the Org Member anchor first — it is the membership record, and + // the row lock serializes this update against a concurrent + // removeOrgUser: either we hold the anchor before removal deletes it, + // or removal already committed and we bail out as not-a-member. + const anchor = await tx + .select({ id: user_roles.id }) + .from(user_roles) + .where( + and( + eq(user_roles.userId, userId), + eq(user_roles.orgId, orgId), + isNull(user_roles.projectId), + eq(user_roles.roleId, orgMemberRoleId) + ) + ) + .for('update'); + + if (anchor.length === 0) { + throw new UserNotInOrgException('User not in organization'); + } + const nonAnchorOrgScope = and( eq(user_roles.userId, userId), eq(user_roles.orgId, orgId), @@ -147,6 +168,9 @@ export async function updateOrgUserRole( return ok(undefined); }); } catch (error) { + if (error instanceof UserNotInOrgException) { + return err(ErrorCode.USER_NOT_IN_ORGANIZATION); + } logger.error({ cause: error, message: 'Failed to update org-level role for user',