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..6de5bc1c 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,98 @@ export async function removeOrgUser(orgId: number, userId: number): Promise> { + try { + 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), + 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) { + if (error instanceof UserNotInOrgException) { + return err(ErrorCode.USER_NOT_IN_ORGANIZATION); + } + 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..0b8f21cf 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) { @@ -569,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 ); } 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) =>