Skip to content

Commit 351ed34

Browse files
fix(organizations): close self-join via shadowed Organization policy subject (#3868)
* fix(organizations): close self-join via shadowed Organization policy subject * fix(organizations): mirror /members carve-out on the admin path-subject * test(organizations): assert exact 403 on self-add + dedup org-route guard helper * docs(organizations): clarify addMember authorization JSDoc post-fix
1 parent 9f10307 commit 351ed34

5 files changed

Lines changed: 358 additions & 10 deletions

File tree

modules/organizations/controllers/organizations.membership.controller.js

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,18 @@ const findUserByEmail = async (req, res) => {
6767
* @description Endpoint for an org owner/admin to add a user as a member. Creates a
6868
* PENDING owner_add membership the INVITED USER must accept (consent — invariant #1).
6969
* Role gate mirrors updateRole: only OWNERS may grant owner/admin; admins may only
70-
* add plain members. CASL (`create Membership`) on the /members POST route already
71-
* restricts this to org owners/admins (+ global admins).
70+
* add plain members.
71+
*
72+
* Authorization is enforced at two layers:
73+
* (1) The `/members` POST route resolves to the `Membership` path-subject (owner/admin
74+
* gate via `create Membership`) because both the Organization document-subject and
75+
* the admin Organization path-subject explicitly exclude `/members` paths — preventing
76+
* the broader `create Organization` grant (available to any authenticated user) from
77+
* shadowing the Membership subject and allowing unauthenticated membership injection.
78+
* (2) The explicit actor-role check at the top of this handler (isGlobalAdmin || OWNER
79+
* || ADMIN) fails closed for non-members and plain members, because the type-level
80+
* CASL `create Membership` grant does not carry an org-scope condition on its own
81+
* and would otherwise pass for any user who holds that grant in the same org.
7282
* @param {Object} req - Express request object
7383
* @param {Object} res - Express response object
7484
* @returns {void}
@@ -78,9 +88,22 @@ const addMember = async (req, res) => {
7888
const { userId, role } = req.body;
7989
const requestedRole = role || MEMBERSHIP_ROLES.MEMBER;
8090

81-
// Elevated-role guard: only owners (or global admins) may invite an owner/admin.
91+
// Actor-role gate (mirrors updateRole/remove): only an org owner/admin (or a global
92+
// admin) may add a member. The type-level CASL `create Membership` check does not
93+
// carry the `{organizationId}` org-scope condition, so a non-member / plain member
94+
// must be rejected here. Without this, the Organization-subject shadowing bug aside,
95+
// a plain member could still inject a membership into their own org.
8296
const isPlatformAdmin = isGlobalAdmin(req.user);
83-
const actorIsOwner = req.membership?.role === MEMBERSHIP_ROLES.OWNER;
97+
const actorRole = req.membership?.role;
98+
const canAdd = isPlatformAdmin
99+
|| actorRole === MEMBERSHIP_ROLES.OWNER
100+
|| actorRole === MEMBERSHIP_ROLES.ADMIN;
101+
if (!canAdd) {
102+
return responses.error(res, 403, 'Forbidden', 'Insufficient organization role')();
103+
}
104+
105+
// Elevated-role guard: only owners (or global admins) may invite an owner/admin.
106+
const actorIsOwner = actorRole === MEMBERSHIP_ROLES.OWNER;
84107
if (requestedRole !== MEMBERSHIP_ROLES.MEMBER && !isPlatformAdmin && !actorIsOwner) {
85108
return responses.error(res, 403, 'Forbidden', 'Only owners can add a member with an elevated role')();
86109
}

modules/organizations/policies/organizations.policy.js

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@
33
*/
44
import { MEMBERSHIP_ROLES } from '../lib/constants.js';
55

6+
/**
7+
* Whether a route path targets an organization resource (regular or platform-admin form).
8+
* Both surfaces share the same /members and /requests sub-resources, so their dedicated
9+
* Membership path-subjects must match either prefix.
10+
* @param {string} p - Express route path
11+
* @returns {boolean} true for both /api/organizations and /api/admin/organizations
12+
*/
13+
const isOrganizationRoute = (p) => p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations');
14+
615
/**
716
* Register organization-related subjects for document-level and path-level resolution.
817
* The organization document subject uses a guard to exclude billing routes (which
@@ -16,16 +25,29 @@ export function organizationSubjectRegistration({ registerDocumentSubject, regis
1625
registerDocumentSubject('membershipDoc', 'Membership');
1726
// Guard: only resolve req.organization as an Organization subject on actual organization routes.
1827
// Other modules (billing, tasks, etc.) also set req.organization but authorize via their own subjects.
28+
//
29+
// The /members routes are deliberately EXCLUDED: req.organization is loaded there too,
30+
// but membership management must authorize via the dedicated Membership path-subject
31+
// (owner/admin gate), NOT the unconditional `create Organization` grant — otherwise the
32+
// Organization document-subject is resolved first (resolveSubject is first-match-wins) and
33+
// shadows the Membership subject, letting any authenticated user inject a membership.
34+
// Do NOT exclude /requests — that is the any-user JOIN-REQUEST flow, which legitimately
35+
// relies on `create Organization` (excluding it would 403 legitimate join requests).
1936
registerDocumentSubject('organization', 'Organization', (req) => {
2037
if (!req.route?.path) {
2138
return false;
2239
}
2340
const p = req.route.path;
24-
return p.startsWith('/api/organizations') || p.startsWith('/api/admin/organizations');
41+
return isOrganizationRoute(p) && !p.includes('/members');
2542
});
26-
registerPathSubject((p) => p.startsWith('/api/admin/organizations'), 'Organization');
27-
registerPathSubject((p) => p.startsWith('/api/organizations') && p.includes('/requests'), 'Membership');
28-
registerPathSubject((p) => p.startsWith('/api/organizations') && p.includes('/members'), 'Membership');
43+
// Admin organization routes resolve to the Organization subject, EXCEPT /members:
44+
// membership management must authorize via the dedicated Membership path-subject below,
45+
// mirroring the organization document-subject guard. Without this carve-out the broad
46+
// admin Organization match would shadow the Membership entry (resolution is first-match-wins),
47+
// so an admin /members path would resolve to Organization instead of Membership.
48+
registerPathSubject((p) => p.startsWith('/api/admin/organizations') && !p.includes('/members'), 'Organization');
49+
registerPathSubject((p) => isOrganizationRoute(p) && p.includes('/requests'), 'Membership');
50+
registerPathSubject((p) => isOrganizationRoute(p) && p.includes('/members'), 'Membership');
2951
registerPathSubject((p) => p.startsWith('/api/organizations'), 'Organization');
3052
}
3153

modules/organizations/tests/organizations.membership.controller.unit.tests.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,26 @@ describe('Membership controller unit tests:', () => {
253253
expect(res.status).not.toHaveBeenCalledWith(403);
254254
});
255255

256+
test('a plain MEMBER cannot add anyone → 403, service not called', async () => {
257+
const req = mockReq({ membership: { role: MEMBERSHIP_ROLES.MEMBER }, body: { userId: 'u9' } });
258+
const res = mockRes();
259+
260+
await membershipController.addMember(req, res);
261+
262+
expect(res.status).toHaveBeenCalledWith(403);
263+
expect(mockAddMember).not.toHaveBeenCalled();
264+
});
265+
266+
test('a non-member (no req.membership) cannot add anyone → 403, fail closed', async () => {
267+
const req = mockReq({ user: { _id: 'u1', roles: ['user'] }, membership: undefined, body: { userId: 'u9' } });
268+
const res = mockRes();
269+
270+
await membershipController.addMember(req, res);
271+
272+
expect(res.status).toHaveBeenCalledWith(403);
273+
expect(mockAddMember).not.toHaveBeenCalled();
274+
});
275+
256276
test('admin CANNOT add an elevated role (owner/admin) → 403, service not called', async () => {
257277
const req = mockReq({ membership: { role: MEMBERSHIP_ROLES.ADMIN }, body: { userId: 'u9', role: MEMBERSHIP_ROLES.ADMIN } });
258278
const res = mockRes();

modules/organizations/tests/organizations.policy.unit.tests.js

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,19 +7,37 @@ import { organizationSubjectRegistration } from '../policies/organizations.polic
77

88
/**
99
* Build a mock subject-registration registry that captures registered resolvers.
10-
* @returns {{ registerDocumentSubject: jest.Mock, registerPathSubject: jest.Mock, _documentResolvers: Map }}
10+
* @returns {{ registerDocumentSubject: jest.Mock, registerPathSubject: jest.Mock, _documentResolvers: Map, _pathResolvers: Array }}
1111
*/
1212
function mockRegistry() {
1313
const documentResolvers = new Map();
14+
const pathResolvers = [];
1415
return {
1516
registerDocumentSubject: jest.fn((prop, type, guard) => {
1617
documentResolvers.set(prop, { type, guard });
1718
}),
18-
registerPathSubject: jest.fn(),
19+
registerPathSubject: jest.fn((routeMatch, subjectType) => {
20+
pathResolvers.push({ routeMatch, subjectType });
21+
}),
1922
_documentResolvers: documentResolvers,
23+
_pathResolvers: pathResolvers,
2024
};
2125
}
2226

27+
/**
28+
* Replicate the production first-match-wins path-subject resolution
29+
* (see lib/middlewares/policy.js → deriveSubjectType) over the captured resolvers.
30+
* @param {Array} pathResolvers - Captured { routeMatch, subjectType } entries, in registration order
31+
* @param {string} routePath - Express route path to resolve
32+
* @returns {string|null} CASL subject type or null if not mappable
33+
*/
34+
function deriveSubjectType(pathResolvers, routePath) {
35+
for (const { routeMatch, subjectType } of pathResolvers) {
36+
if (routeMatch(routePath)) return subjectType;
37+
}
38+
return null;
39+
}
40+
2341
describe('organizationSubjectRegistration policy unit tests:', () => {
2442
test('should register membershipDoc, organization, and path subjects', () => {
2543
const registry = mockRegistry();
@@ -61,9 +79,60 @@ describe('organizationSubjectRegistration policy unit tests:', () => {
6179
expect(guard({ route: { path: '/api/admin/organizations/:id' } })).toBe(true);
6280
});
6381

82+
test('should still return true for the join-request flow (/:id/requests)', () => {
83+
// The any-user join-request flow legitimately relies on the Organization
84+
// subject's unconditional `create` grant — it must NOT be carved out.
85+
expect(guard({ route: { path: '/api/organizations/:organizationId/requests' } })).toBe(true);
86+
});
87+
88+
test('should return false for the /members management routes', () => {
89+
// /members must authorize via the dedicated Membership path-subject (owner/admin
90+
// gate), not the unconditional `create Organization` grant — excluding it here
91+
// prevents the Organization subject from shadowing the Membership subject.
92+
expect(guard({ route: { path: '/api/organizations/:organizationId/members' } })).toBe(false);
93+
expect(guard({ route: { path: '/api/organizations/:organizationId/members/:memberId' } })).toBe(false);
94+
expect(guard({ route: { path: '/api/admin/organizations/:id/members' } })).toBe(false);
95+
});
96+
6497
test('should return false for unrelated paths (e.g. billing routes)', () => {
6598
expect(guard({ route: { path: '/api/billing/plans' } })).toBe(false);
6699
expect(guard({ route: { path: '/api/tasks' } })).toBe(false);
67100
});
68101
});
102+
103+
describe('path subject resolution (first-match-wins):', () => {
104+
let pathResolvers;
105+
106+
beforeEach(() => {
107+
const registry = mockRegistry();
108+
organizationSubjectRegistration(registry);
109+
pathResolvers = registry._pathResolvers;
110+
});
111+
112+
test('should resolve regular /:organizationId/members to the Membership path-subject', () => {
113+
expect(deriveSubjectType(pathResolvers, '/api/organizations/:organizationId/members')).toBe('Membership');
114+
});
115+
116+
test('should resolve admin /:id/members to the Membership path-subject (mirrors the /members carve-out)', () => {
117+
// The admin path-subject excludes /members so it falls through to the
118+
// dedicated Membership entry — consistent with the regular /members route
119+
// and the organization document-subject guard. Without the carve-out the
120+
// broad admin Organization match would shadow it (first-match-wins).
121+
expect(deriveSubjectType(pathResolvers, '/api/admin/organizations/:id/members')).toBe('Membership');
122+
});
123+
124+
test('should resolve admin organization routes (non-members) to the Organization path-subject', () => {
125+
expect(deriveSubjectType(pathResolvers, '/api/admin/organizations')).toBe('Organization');
126+
expect(deriveSubjectType(pathResolvers, '/api/admin/organizations/:id')).toBe('Organization');
127+
});
128+
129+
test('should resolve /:organizationId/requests to the Membership path-subject', () => {
130+
expect(deriveSubjectType(pathResolvers, '/api/organizations/:organizationId/requests')).toBe('Membership');
131+
});
132+
133+
test('should resolve plain organization routes to the Organization path-subject', () => {
134+
expect(deriveSubjectType(pathResolvers, '/api/organizations')).toBe('Organization');
135+
expect(deriveSubjectType(pathResolvers, '/api/organizations/:id')).toBe('Organization');
136+
});
137+
});
69138
});

0 commit comments

Comments
 (0)