diff --git a/config/templates/org-member-added.html b/config/templates/org-member-added.html new file mode 100644 index 000000000..12298e938 --- /dev/null +++ b/config/templates/org-member-added.html @@ -0,0 +1,8 @@ + +{{appName}} invitation + +

Hello {{displayName}},

+

You have been invited to join {{orgName}} on {{appName}}.

+

Review and accept the invitation here: {{url}}

+

The {{appName}} Team.

+ diff --git a/modules/organizations/controllers/organizations.membershipRequest.controller.js b/modules/organizations/controllers/organizations.membershipRequest.controller.js index d53a9b0db..bd5f35486 100644 --- a/modules/organizations/controllers/organizations.membershipRequest.controller.js +++ b/modules/organizations/controllers/organizations.membershipRequest.controller.js @@ -143,6 +143,32 @@ const accept = async (req, res) => { } }; +/** + * @function decline + * @description Endpoint for the INVITED USER to decline a pending owner_add + * membership — deletes the row (the user can be re-invited later). Same + * auth-only surface and consent gate as accept: the service returns null on any + * mismatch (wrong user, a join_request, an already-active or unknown + * membership) → 404, never leaking which condition failed. + * @param {Object} req - Express request object + * @param {Object} res - Express response object + * @returns {Promise} + */ +const decline = async (req, res) => { + try { + const membership = await MembershipService.declineMembership( + req.params.membershipId, + req.user._id || req.user.id, + ); + if (!membership) { + return responses.error(res, 404, 'Not Found', 'No pending invitation with that identifier has been found')(); + } + responses.success(res, 'membership invitation declined')(membership); + } catch (err) { + responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); + } +}; + /** * @function requestByID * @description Middleware to fetch a pending membership by its ID. @@ -180,5 +206,6 @@ export default { listMine, listMinePending, accept, + decline, requestByID, }; diff --git a/modules/organizations/repositories/organizations.membership.repository.js b/modules/organizations/repositories/organizations.membership.repository.js index fa6e0cdee..e9eefe16c 100644 --- a/modules/organizations/repositories/organizations.membership.repository.js +++ b/modules/organizations/repositories/organizations.membership.repository.js @@ -79,6 +79,18 @@ const update = (membership) => membership.save().then((doc) => doc.populate(defa */ const remove = (membership) => Membership.deleteOne({ _id: membership.id || membership._id }).exec(); +/** + * @function findOneAndDelete + * @description Atomically find a single membership matching a filter and delete it. + * Returns the deleted document (populated) or null if no document matched, allowing + * callers to implement consent-gated deletes in a single round-trip that is free + * of read-then-delete race conditions. + * @param {Object} filter - The filter to match the document to delete. + * @returns {Promise} The deleted membership (populated) or null. + */ +const findOneAndDelete = (filter) => + Membership.findOneAndDelete(filter).populate(defaultPopulate).exec(); + /** * @function count * @description Data access operation to count memberships matching a filter. @@ -124,6 +136,7 @@ export default { create, get, findOne, + findOneAndDelete, update, remove, count, diff --git a/modules/organizations/routes/organizations.membershipRequest.routes.js b/modules/organizations/routes/organizations.membershipRequest.routes.js index 07450eab0..908ef54f8 100644 --- a/modules/organizations/routes/organizations.membershipRequest.routes.js +++ b/modules/organizations/routes/organizations.membershipRequest.routes.js @@ -36,6 +36,17 @@ export default (app) => { .all(passport.authenticate('jwt', { session: false })) .put(membershipRequests.accept); + // The INVITED USER declines a pending owner_add membership (deletes the row so + // they can be re-invited). Auth-only — same consent gate as accept, enforced in + // the service. Registered AFTER /mine and /mine/pending so those literals are + // never captured as a :membershipId. NOTE: there is intentionally no app.param + // for :membershipId — the controller reads req.params.membershipId raw, exactly + // like accept does. + app + .route('/api/membership-requests/:membershipId') + .all(passport.authenticate('jwt', { session: false })) + .delete(membershipRequests.decline); + // Create a request to join an organization / list pending requests for an organization app .route('/api/organizations/:organizationId/requests') diff --git a/modules/organizations/services/organizations.membership.service.js b/modules/organizations/services/organizations.membership.service.js index 03d4f47b8..b920d66c4 100644 --- a/modules/organizations/services/organizations.membership.service.js +++ b/modules/organizations/services/organizations.membership.service.js @@ -55,7 +55,12 @@ const validateLastOwnerProtection = async (organizationId) => { /** * @function list - * @description Service to retrieve active memberships for an organization. + * @description Service to retrieve an organization's members surface: ACTIVE + * memberships PLUS pending owner_add invitations — so the inviting owner/admin + * can SEE (and cancel via DELETE /members/:memberId) an invite they created. + * Pending join_requests stay OUT: they have their own approval surface + * (listPending) and must never look like members. Rows carry status + source + * so callers can render the pending state. * @param {String} organizationId - The ID of the organization. * @param {String} [search] - Optional search string to filter by user name/email. * @param {Number} [page] - Optional page number for pagination. @@ -63,7 +68,13 @@ const validateLastOwnerProtection = async (organizationId) => { * @returns {Promise} A promise that resolves to the list of memberships. */ const list = async (organizationId, search, page, perPage) => { - const filter = { organizationId, status: MEMBERSHIP_STATUSES.ACTIVE }; + const filter = { + organizationId, + $or: [ + { status: MEMBERSHIP_STATUSES.ACTIVE }, + { status: MEMBERSHIP_STATUSES.PENDING, source: PENDING_SOURCES.OWNER_ADD }, + ], + }; if (search) { const matchingUsers = await UserService.searchByNameOrEmail(search); filter.userId = { $in: matchingUsers.map((u) => u._id) }; @@ -128,7 +139,10 @@ const updateRole = async (membership, role) => { * @returns {Promise} A promise resolving to a confirmation of the deletion. */ const remove = async (membership) => { - if (membership.role === MEMBERSHIP_ROLES.OWNER) { + // Last-owner protection applies ONLY to ACTIVE owner rows: cancelling a PENDING + // owner-role invite never reduces the active-owner count, so it must not be + // blocked when the org has a single active owner (the inviter) — #3831. + if (membership.role === MEMBERSHIP_ROLES.OWNER && membership.status === MEMBERSHIP_STATUSES.ACTIVE) { const orgId = membership.organizationId._id || membership.organizationId; await validateLastOwnerProtection(orgId); } @@ -352,7 +366,10 @@ const findUserByExactEmail = async (email) => { * @param {String} userId - The user being invited. * @param {String} [role] - The role to grant on acceptance (defaults to MEMBER). * @param {String} addedBy - The id of the owner/admin performing the add (provenance). - * @returns {Promise} The created PENDING owner_add membership. + * @returns {Promise} The created PENDING owner_add membership. When the + * mailer is configured, also notifies the invited user by email (invitation + * wording — the membership awaits THEIR acceptance) with a link to their + * organizations page. Fire-and-forget: a mail failure never fails the add. * @throws {Error} If userId is missing, the user does not exist, or a membership already exists. */ const addMember = async (organizationId, userId, role, addedBy) => { @@ -373,7 +390,7 @@ const addMember = async (organizationId, userId, role, addedBy) => { } // status EXPLICIT — never rely on the schema 'active' default (consent bypass). - return MembershipRepository.create({ + const membership = await MembershipRepository.create({ userId, organizationId, role: role || MEMBERSHIP_ROLES.MEMBER, @@ -381,6 +398,34 @@ const addMember = async (organizationId, userId, role, addedBy) => { source: PENDING_SOURCES.OWNER_ADD, addedBy, }); + + // Invitation notification (parity with the join-request emails). The membership + // is PENDING — only the invitee can activate it via acceptMembership (consent + // invariant #1) — so the wording is an invitation, never a "you were added" + // fait accompli. Fire-and-forget: any failure (DB read or mailer) must never + // fail the add — the entire block is in a try/catch for that guarantee. + if (mailer.isConfigured() && user?.email) { + try { + const org = await OrganizationRepository.get(organizationId); + if (org?.name) { + mailer.sendMail({ + to: user.email, + subject: `You have been invited to join ${org.name}`, + template: 'org-member-added', + params: { + displayName: [user.firstName, user.lastName].filter(Boolean).join(' '), + orgName: org.name, + appName: config.app.title, + url: `${getBaseUrl()}/users/organizations`, + }, + }).catch((err) => logger.warn('organizations.membership.addMember: invitation email failed', { message: err?.message, stack: err?.stack })); + } + } catch (err) { + logger.warn('organizations.membership.addMember: invitation email setup failed', { message: err?.message, stack: err?.stack }); + } + } + + return membership; }; /** @@ -430,6 +475,40 @@ const acceptMembership = async (membershipId, acceptingUserId) => { return result; }; +/** + * @function declineMembership + * @description The INVITED USER declines a pending owner_add membership, deleting + * the row. CONSENT-CRITICAL — the gate is IDENTICAL to acceptMembership: the + * membership must be PENDING + source 'owner_add' AND belong to the + * authenticated caller; any mismatch (wrong user, a join_request, an + * already-active or unknown membership) returns null so the controller can + * answer 404 without leaking which condition failed. Deleting (not flagging) + * mirrors rejectRequest — the user can be re-invited later. This makes the + * createJoinRequest copy ('Please accept or decline it') honest. + * @param {String} membershipId - The pending owner_add membership to decline. + * @param {String} decliningUserId - The authenticated user declining (must be the invitee). + * @returns {Promise} The deleted membership row, or null if not declinable by this user. + */ +const declineMembership = async (membershipId, decliningUserId) => { + // Self-defending consent gate, same rationale as acceptMembership: reject an + // absent caller up-front so a null/undefined id never accidentally matches a + // document (MongoDB $eq null matches null + missing fields). + if (!decliningUserId) return null; + + // Atomic consent-gated delete: the filter encodes ALL consent conditions so no + // separate read is needed. A concurrent accept that flipped status→ACTIVE will + // cause the filter to miss the document (status !== PENDING), safely returning + // null — the accept wins and the decline is silently a no-op, which is correct + // (the invitee accepted; there is nothing left to decline). + const deleted = await MembershipRepository.findOneAndDelete({ + _id: membershipId, + userId: decliningUserId, + status: MEMBERSHIP_STATUSES.PENDING, + source: PENDING_SOURCES.OWNER_ADD, + }); + return deleted || null; +}; + /** * @function leave * @description Leave an organization. Prevents the last owner from leaving. @@ -505,6 +584,7 @@ export default { findUserByExactEmail, addMember, acceptMembership, + declineMembership, count, aggregateCountByOrganizations, deleteMany, diff --git a/modules/organizations/tests/organizations.decline.e2e.tests.js b/modules/organizations/tests/organizations.decline.e2e.tests.js new file mode 100644 index 000000000..6cc223494 --- /dev/null +++ b/modules/organizations/tests/organizations.decline.e2e.tests.js @@ -0,0 +1,177 @@ +/** + * @desc E2E tests for the owner_add decline flow (#3831). + * Tests: signup owner → signup invitee → owner adds invitee (pending owner_add, + * visible in the members list) → the owner CANNOT decline on the invitee's + * behalf (404, consent) → the invitee declines (row deleted) → the invitee can + * then request to join — proving the createJoinRequest copy + * 'Please accept or decline it' is honest. + */ +import request from 'supertest'; +import path from 'path'; + +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; +import config from '../../../config/index.js'; + +describe('Organizations owner_add decline E2E tests:', () => { + let UserService; + let OrganizationsRepository; + let MembershipRepository; + let agent; + + // Store original config + const originalOrganizations = { ...config.organizations }; + + /** + * @description Reset organizations config to original state. + * @returns {void} + */ + const resetOrgConfig = () => { + config.organizations = { ...originalOrganizations }; + }; + + /** + * @description Clean up a user and their associated organizations/memberships. + * @param {Object} user - The user object to clean up. + * @returns {Promise} + */ + const cleanupUser = async (user) => { + if (!user) return; + try { + const memberships = await MembershipRepository.list({ userId: user.id || user._id }); + for (const m of memberships) { + const orgId = m.organizationId._id || m.organizationId; + await MembershipRepository.deleteMany({ organizationId: orgId }); + await OrganizationsRepository.deleteMany({ _id: orgId }); + } + await UserService.remove(user); + } catch (_) { /* cleanup — ignore errors */ } + }; + + beforeAll(async () => { + try { + const init = await bootstrap(); + UserService = (await import(path.resolve('./modules/users/services/users.service.js'))).default; + OrganizationsRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.repository.js'))).default; + MembershipRepository = (await import(path.resolve('./modules/organizations/repositories/organizations.membership.repository.js'))).default; + agent = request.agent(init.app); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + + describe('Full decline flow', () => { + let owner; + let invitee; + let org; + let agentOwner; + let agentInvitee; + + afterAll(async () => { + try { + if (org) { + await MembershipRepository.deleteMany({ organizationId: org._id }); + await OrganizationsRepository.deleteMany({ _id: org._id }); + } + } catch (_) { /* cleanup */ } + await cleanupUser(invitee); + await cleanupUser(owner); + }); + + test('owner adds → invite visible → owner cannot decline → invitee declines → invitee can request to join', async () => { + config.organizations = { enabled: true, autoCreate: true, domainMatching: false }; + agentOwner = request.agent(agent.app); + agentInvitee = request.agent(agent.app); + + // Step 1: signup the owner, auto-creates the org + const resultOwner = await agentOwner + .post('/api/auth/signup') + .send({ + firstName: 'DeclineOwner', + lastName: 'User', + email: 'decline-owner@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + owner = resultOwner.body.user; + org = resultOwner.body.organization; + expect(org).toBeDefined(); + expect(org).not.toBeNull(); + + // Step 2: signup the invitee + const resultInvitee = await agentInvitee + .post('/api/auth/signup') + .send({ + firstName: 'DeclineInvitee', + lastName: 'User', + email: 'decline-invitee@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + invitee = resultInvitee.body.user; + + // Step 3: owner adds the invitee → PENDING owner_add membership + const addResult = await agentOwner + .post(`/api/organizations/${org._id}/members`) + .send({ userId: invitee.id }) + .expect(200); + expect(addResult.body.message).toBe('membership invitation created'); + const invitationId = addResult.body.data._id; + expect(invitationId).toBeDefined(); + + // Step 4: the pending invite is now VISIBLE in the owner's members list, + // with status + source so the UI can render the pending state. + const membersResult = await agentOwner + .get(`/api/organizations/${org._id}/members`) + .expect(200); + const pendingRow = membersResult.body.data.find((m) => m._id === invitationId); + expect(pendingRow).toBeDefined(); + expect(pendingRow.status).toBe('pending'); + expect(pendingRow.source).toBe('owner_add'); + + // Step 5: the OWNER cannot decline on the invitee's behalf → 404 (consent + // gate, same opaque copy as accept) and the row survives. + await agentOwner + .delete(`/api/membership-requests/${invitationId}`) + .expect(404); + const stillThere = await MembershipRepository.findOne({ + userId: invitee.id, + organizationId: org._id, + status: 'pending', + }); + expect(stillThere).not.toBeNull(); + + // Step 6: the INVITEE declines → 200, row DELETED (not flagged) + const declineResult = await agentInvitee + .delete(`/api/membership-requests/${invitationId}`) + .expect(200); + expect(declineResult.body.message).toBe('membership invitation declined'); + const deleted = await MembershipRepository.findOne({ + userId: invitee.id, + organizationId: org._id, + }); + expect(deleted).toBeNull(); + + // Step 7: the 'Please accept or decline it' copy is honest — having + // declined, the invitee can now request to join (no pending row blocks it). + const joinResult = await agentInvitee + .post(`/api/organizations/${org._id}/requests`) + .expect(200); + expect(joinResult.body.message).toBe('membership request created'); + }); + }); + + // Mongoose disconnect + afterAll(async () => { + resetOrgConfig(); + try { + await mongooseService.disconnect(); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); +}); diff --git a/modules/organizations/tests/organizations.membership.addMember.email.unit.tests.js b/modules/organizations/tests/organizations.membership.addMember.email.unit.tests.js new file mode 100644 index 000000000..c0a5928b4 --- /dev/null +++ b/modules/organizations/tests/organizations.membership.addMember.email.unit.tests.js @@ -0,0 +1,120 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +import { MEMBERSHIP_ROLES } from '../lib/constants.js'; + +/** + * Unit tests — addMember invitation email (#3832). + * The owner-add creates a PENDING membership the invitee must ACCEPT (consent + * invariant #1), so the notification is an INVITATION — never a "you were + * added" fait accompli: + * - mailer configured → sendMail called once with the exact invite payload; + * - mailer unconfigured → sendMail never called, membership still created; + * - user without email → sendMail never called, membership still created. + */ + +const mockWarn = jest.fn(); +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: mockWarn, error: jest.fn(), info: jest.fn() }, +})); + +const mockGetBrut = jest.fn(); +jest.unstable_mockModule('../../users/services/users.service.js', () => ({ + default: { getBrut: mockGetBrut, updateById: jest.fn().mockResolvedValue({}) }, +})); + +const mockFindOne = jest.fn(); +const mockCreate = jest.fn(); +jest.unstable_mockModule('../repositories/organizations.membership.repository.js', () => ({ + default: { + findOne: mockFindOne, + create: mockCreate, + update: jest.fn(), + remove: jest.fn(), + list: jest.fn(), + count: jest.fn(), + get: jest.fn(), + }, +})); + +const mockOrgGet = jest.fn(); +jest.unstable_mockModule('../repositories/organizations.repository.js', () => ({ + default: { get: mockOrgGet }, +})); + +const mockIsConfigured = jest.fn(); +const mockSendMail = jest.fn(); +jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: mockIsConfigured, sendMail: mockSendMail }, +})); + +jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), +})); + +jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { app: { title: 'Test' } }, +})); + +jest.unstable_mockModule('../../../lib/helpers/emailVerification.js', () => ({ + assertEmailVerified: jest.fn(), +})); + +const { default: MembershipService } = await import('../services/organizations.membership.service.js'); + +describe('organizations.membership.service addMember invitation email:', () => { + const user = { _id: 'u1', email: 'invitee@x.com', firstName: 'Ada', lastName: 'Lovelace' }; + const org = { _id: 'org1', name: 'TestOrg' }; + + beforeEach(() => { + jest.clearAllMocks(); + mockGetBrut.mockResolvedValue({ ...user }); + mockFindOne.mockResolvedValue(null); + mockCreate.mockImplementation((data) => Promise.resolve({ _id: 'm-new', ...data })); + mockOrgGet.mockResolvedValue(org); + mockSendMail.mockResolvedValue({}); + }); + + test('mailer configured: sends the invitation email with the exact payload', async () => { + mockIsConfigured.mockReturnValue(true); + + const membership = await MembershipService.addMember('org1', 'u1', MEMBERSHIP_ROLES.MEMBER, 'owner1'); + + expect(membership._id).toBe('m-new'); + expect(mockSendMail).toHaveBeenCalledTimes(1); + expect(mockSendMail).toHaveBeenCalledWith({ + to: 'invitee@x.com', + subject: 'You have been invited to join TestOrg', + template: 'org-member-added', + params: { + displayName: 'Ada Lovelace', + orgName: 'TestOrg', + appName: 'Test', + url: 'http://localhost:3000/users/organizations', + }, + }); + }); + + test('mailer NOT configured: creates the membership and never calls sendMail', async () => { + mockIsConfigured.mockReturnValue(false); + + const membership = await MembershipService.addMember('org1', 'u1', MEMBERSHIP_ROLES.MEMBER, 'owner1'); + + expect(membership._id).toBe('m-new'); + expect(mockSendMail).not.toHaveBeenCalled(); + expect(mockOrgGet).not.toHaveBeenCalled(); + }); + + test('user without an email: creates the membership and never calls sendMail', async () => { + mockIsConfigured.mockReturnValue(true); + mockGetBrut.mockResolvedValue({ _id: 'u1', firstName: 'Ada', lastName: 'Lovelace' }); + + const membership = await MembershipService.addMember('org1', 'u1', MEMBERSHIP_ROLES.MEMBER, 'owner1'); + + expect(membership._id).toBe('m-new'); + expect(mockSendMail).not.toHaveBeenCalled(); + expect(mockOrgGet).not.toHaveBeenCalled(); + }); +}); diff --git a/modules/organizations/tests/organizations.membership.lifecycle.unit.tests.js b/modules/organizations/tests/organizations.membership.lifecycle.unit.tests.js new file mode 100644 index 000000000..59bf8b815 --- /dev/null +++ b/modules/organizations/tests/organizations.membership.lifecycle.unit.tests.js @@ -0,0 +1,230 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +import { MEMBERSHIP_STATUSES, MEMBERSHIP_ROLES, PENDING_SOURCES } from '../lib/constants.js'; + +/** + * Lifecycle unit tests for the pending owner_add membership (#3831). + * + * Covers the three service-level fixes: + * 1. declineMembership — the invitee-side delete path, consent gate IDENTICAL + * to acceptMembership (pending + source owner_add + caller is the invitee); + * 2. remove() last-owner scope — the protection only applies to ACTIVE owner + * rows, so cancelling a PENDING owner-role invite never spuriously throws; + * 3. list() pending visibility — active members + pending owner_add invites, + * pending join_requests stay on their own approval surface (listPending). + */ + +const mockFindOne = jest.fn(); +const mockCreate = jest.fn(); +const mockUpdate = jest.fn(); +const mockGet = jest.fn(); +const mockList = jest.fn(); +const mockCount = jest.fn(); +const mockRemove = jest.fn(); +const mockFindOneAndDelete = jest.fn(); +jest.unstable_mockModule('../repositories/organizations.membership.repository.js', () => ({ + default: { + findOne: mockFindOne, + create: mockCreate, + update: mockUpdate, + get: mockGet, + list: mockList, + count: mockCount, + remove: mockRemove, + findOneAndDelete: mockFindOneAndDelete, + }, +})); + +const mockOrgGet = jest.fn(); +jest.unstable_mockModule('../repositories/organizations.repository.js', () => ({ + default: { get: mockOrgGet }, +})); + +const mockGetBrut = jest.fn(); +const mockUpdateById = jest.fn().mockResolvedValue({}); +const mockSearchByNameOrEmail = jest.fn(); +jest.unstable_mockModule('../../users/services/users.service.js', () => ({ + default: { getBrut: mockGetBrut, updateById: mockUpdateById, searchByNameOrEmail: mockSearchByNameOrEmail }, +})); + +jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, +})); +jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), +})); +jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { app: { title: 'Test' } }, +})); +jest.unstable_mockModule('../../../lib/helpers/emailVerification.js', () => ({ + assertEmailVerified: jest.fn(), +})); +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, +})); + +const { default: MembershipService } = await import('../services/organizations.membership.service.js'); + +const ORG = 'org1'; +const USER = 'user1'; +const OWNER = 'owner1'; + +describe('Membership owner_add lifecycle unit tests:', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockGetBrut.mockResolvedValue({ _id: USER, currentOrganization: 'other-org' }); + mockRemove.mockResolvedValue({ deletedCount: 1 }); + }); + + describe('declineMembership — atomic consent-gated delete', () => { + /** + * @desc Build a pending owner_add membership owned by USER. + * @param {Object} overrides - field overrides + * @returns {Object} membership-like doc + */ + const ownerAddPending = (overrides = {}) => ({ + _id: 'm1', + userId: USER, + organizationId: ORG, + status: MEMBERSHIP_STATUSES.PENDING, + source: PENDING_SOURCES.OWNER_ADD, + ...overrides, + }); + + test('the INVITED USER can decline their own pending owner_add → deleted doc returned', async () => { + const membership = ownerAddPending(); + mockFindOneAndDelete.mockResolvedValue(membership); + + const result = await MembershipService.declineMembership('m1', USER); + + expect(result).toBe(membership); + expect(mockFindOneAndDelete).toHaveBeenCalledTimes(1); + expect(mockFindOneAndDelete).toHaveBeenCalledWith({ + _id: 'm1', + userId: USER, + status: MEMBERSHIP_STATUSES.PENDING, + source: PENDING_SOURCES.OWNER_ADD, + }); + expect(mockRemove).not.toHaveBeenCalled(); + }); + + test('a DIFFERENT user (e.g. the inviting owner) cannot decline → null (filter miss)', async () => { + // findOneAndDelete with userId=OWNER will find nothing (userId mismatch) + mockFindOneAndDelete.mockResolvedValue(null); + + const result = await MembershipService.declineMembership('m1', OWNER); + + expect(result).toBeNull(); + // called with OWNER as userId — the DB filter correctly rejects cross-user deletes + expect(mockFindOneAndDelete).toHaveBeenCalledWith({ + _id: 'm1', + userId: OWNER, + status: MEMBERSHIP_STATUSES.PENDING, + source: PENDING_SOURCES.OWNER_ADD, + }); + expect(mockRemove).not.toHaveBeenCalled(); + }); + + test('unknown membership id → null (findOneAndDelete returns null)', async () => { + mockFindOneAndDelete.mockResolvedValue(null); + const result = await MembershipService.declineMembership('missing', USER); + expect(result).toBeNull(); + }); + + test('SELF-DEFENDING GATE: an undefined decliningUserId → null, findOneAndDelete never called', async () => { + const result = await MembershipService.declineMembership('m1', undefined); + expect(result).toBeNull(); + expect(mockFindOneAndDelete).not.toHaveBeenCalled(); + }); + + test('concurrent accept wins: findOneAndDelete returns null (status no longer PENDING) → null', async () => { + // Simulates a race where acceptMembership flipped status→ACTIVE after our + // decliningUserId guard passed but before deletion; filter misses the doc. + mockFindOneAndDelete.mockResolvedValue(null); + + const result = await MembershipService.declineMembership('m1', USER); + + expect(result).toBeNull(); + expect(mockRemove).not.toHaveBeenCalled(); + }); + }); + + describe('remove() — last-owner protection scoped to ACTIVE owner rows', () => { + test('cancelling a PENDING owner-role invite in a 1-active-owner org succeeds (guard skipped)', async () => { + const invite = { + _id: 'm2', + userId: USER, + organizationId: ORG, + role: MEMBERSHIP_ROLES.OWNER, + status: MEMBERSHIP_STATUSES.PENDING, + source: PENDING_SOURCES.OWNER_ADD, + }; + mockCount.mockResolvedValue(1); // would make the guard throw if it ran + mockList.mockResolvedValue([]); + + await expect(MembershipService.remove(invite)).resolves.toEqual({ success: true }); + expect(mockCount).not.toHaveBeenCalled(); + expect(mockRemove).toHaveBeenCalledWith(invite); + }); + + test('removing the LAST ACTIVE owner still throws', async () => { + const lastOwner = { + _id: 'm3', + userId: USER, + organizationId: ORG, + role: MEMBERSHIP_ROLES.OWNER, + status: MEMBERSHIP_STATUSES.ACTIVE, + }; + mockCount.mockResolvedValue(1); + + await expect(MembershipService.remove(lastOwner)).rejects.toThrow('at least one active owner'); + expect(mockRemove).not.toHaveBeenCalled(); + }); + + test('removing an ACTIVE owner among several still succeeds (guard runs and passes)', async () => { + const owner = { + _id: 'm4', + userId: USER, + organizationId: ORG, + role: MEMBERSHIP_ROLES.OWNER, + status: MEMBERSHIP_STATUSES.ACTIVE, + }; + mockCount.mockResolvedValue(2); + mockList.mockResolvedValue([]); + + await expect(MembershipService.remove(owner)).resolves.toEqual({ success: true }); + expect(mockCount).toHaveBeenCalledTimes(1); + expect(mockRemove).toHaveBeenCalledWith(owner); + }); + }); + + describe('list() — pending owner_add visibility for the inviting owner/admin', () => { + test('filters to ACTIVE rows OR pending owner_add rows (pending join_requests stay out)', async () => { + mockList.mockResolvedValue([]); + + await MembershipService.list(ORG); + + const filter = mockList.mock.calls[0][0]; + expect(filter.organizationId).toBe(ORG); + expect(filter.status).toBeUndefined(); + expect(filter.$or).toEqual([ + { status: MEMBERSHIP_STATUSES.ACTIVE }, + { status: MEMBERSHIP_STATUSES.PENDING, source: PENDING_SOURCES.OWNER_ADD }, + ]); + }); + + test('search still narrows by matched userIds on top of the $or filter', async () => { + mockList.mockResolvedValue([]); + mockSearchByNameOrEmail.mockResolvedValue([{ _id: 'u7' }]); + + await MembershipService.list(ORG, 'ada'); + + const filter = mockList.mock.calls[0][0]; + expect(filter.userId).toEqual({ $in: ['u7'] }); + expect(filter.$or).toBeDefined(); + }); + }); +}); diff --git a/modules/organizations/tests/organizations.membership.silent.catch.unit.tests.js b/modules/organizations/tests/organizations.membership.silent.catch.unit.tests.js index f02190177..f391d2b74 100644 --- a/modules/organizations/tests/organizations.membership.silent.catch.unit.tests.js +++ b/modules/organizations/tests/organizations.membership.silent.catch.unit.tests.js @@ -5,7 +5,8 @@ import { jest, describe, test, expect } from '@jest/globals'; /** * Unit tests — verify that logger.warn is called when fire-and-forget emails - * fail in organizations.membership.service (createJoinRequest, approveRequest, rejectRequest). + * fail in organizations.membership.service (createJoinRequest, approveRequest, + * rejectRequest, addMember). */ const mockWarn = jest.fn(); @@ -131,4 +132,22 @@ describe('organizations.membership.service silent-catch error logging:', () => { { message: emailError.message, stack: emailError.stack }, ); }); + + test('addMember: should call logger.warn when invitation email fails', async () => { + mockWarn.mockClear(); + mockUserGetBrut.mockResolvedValueOnce({ _id: 'u1', email: 'user@x.com', firstName: 'A', lastName: 'B' }); + mockMembershipFindOne.mockResolvedValueOnce(null); + mockMembershipCreate.mockResolvedValueOnce({ _id: 'm-add' }); + mockOrgGet.mockResolvedValueOnce(org); + mockSendMail.mockRejectedValueOnce(emailError); + + await MembershipService.addMember('org1', 'u1', 'member', 'owner1'); + + await new Promise((r) => setTimeout(r, 20)); + + expect(mockWarn).toHaveBeenCalledWith( + 'organizations.membership.addMember: invitation email failed', + { message: emailError.message, stack: emailError.stack }, + ); + }); }); diff --git a/modules/organizations/tests/organizations.membershipRequest.controller.unit.tests.js b/modules/organizations/tests/organizations.membershipRequest.controller.unit.tests.js index cb26dcd6e..542494277 100644 --- a/modules/organizations/tests/organizations.membershipRequest.controller.unit.tests.js +++ b/modules/organizations/tests/organizations.membershipRequest.controller.unit.tests.js @@ -14,6 +14,7 @@ import { MEMBERSHIP_STATUSES, PENDING_SOURCES } from '../lib/constants.js'; const mockGet = jest.fn(); const mockAcceptMembership = jest.fn(); +const mockDeclineMembership = jest.fn(); const mockListPendingOwnerAddsByUser = jest.fn(); const mockListPendingByUser = jest.fn(); @@ -21,6 +22,7 @@ jest.unstable_mockModule('../services/organizations.membership.service.js', () = default: { get: mockGet, acceptMembership: mockAcceptMembership, + declineMembership: mockDeclineMembership, listPendingOwnerAddsByUser: mockListPendingOwnerAddsByUser, listPendingByUser: mockListPendingByUser, listPending: jest.fn(), @@ -134,4 +136,50 @@ describe('membershipRequest controller — consent surfaces:', () => { expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ data: [{ id: 'inv1' }] })); }); }); + + describe('decline (invited user declines their owner_add)', () => { + test('passes the membershipId + the AUTHENTICATED user to the service', async () => { + const deleted = { id: 'm1', status: 'pending', source: 'owner_add' }; + mockDeclineMembership.mockResolvedValue(deleted); + const req = { params: { membershipId: 'm1' }, user: { _id: 'invitee' } }; + const res = mockRes(); + + await controller.decline(req, res); + + expect(mockDeclineMembership).toHaveBeenCalledWith('m1', 'invitee'); + expect(res.status).not.toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ data: deleted })); + }); + + test('a null service result (consent mismatch / not found) → 404, never leaks which', async () => { + mockDeclineMembership.mockResolvedValue(null); + const req = { params: { membershipId: 'm1' }, user: { _id: 'someone-else' } }; + const res = mockRes(); + + await controller.decline(req, res); + + expect(res.status).toHaveBeenCalledWith(404); + }); + + test('falls back to req.user.id when _id is absent', async () => { + const deleted = { id: 'm2', status: 'pending', source: 'owner_add' }; + mockDeclineMembership.mockResolvedValue(deleted); + const req = { params: { membershipId: 'm2' }, user: { id: 'invitee2' } }; + const res = mockRes(); + + await controller.decline(req, res); + + expect(mockDeclineMembership).toHaveBeenCalledWith('m2', 'invitee2'); + }); + + test('service throws → 422 Unprocessable Entity', async () => { + mockDeclineMembership.mockRejectedValue(new Error('db error')); + const req = { params: { membershipId: 'm1' }, user: { _id: 'u1' } }; + const res = mockRes(); + + await controller.decline(req, res); + + expect(res.status).toHaveBeenCalledWith(422); + }); + }); }); diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index 50d758e2d..e388302cd 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -197,6 +197,12 @@ const remove = async (user) => { await MembershipService.deleteMany({ _id: membership._id }); } + // Sweep the user's PENDING rows (both join_request and owner_add). The cleanup + // loop above iterates listByUser, which is ACTIVE-only — without this sweep a + // deleted user's pending invitations / join requests would survive as orphans + // pointing at a dead userId (and keep occupying the (user, org) unique slot). + await MembershipRepository.deleteMany({ userId, status: MEMBERSHIP_STATUSES.PENDING }); + const result = await UserRepository.remove(user); return result; }; diff --git a/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js b/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js new file mode 100644 index 000000000..b78c1cb89 --- /dev/null +++ b/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js @@ -0,0 +1,122 @@ +/** + * Unit tests — users.service.remove must sweep the deleted user's PENDING + * membership rows (both join_request and owner_add): the cleanup loop iterates + * listByUser, which is ACTIVE-only, so without the sweep pending rows survive + * as orphans pointing at a dead userId. Issue #3831. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +const mockMembershipServiceListByUser = jest.fn(); +const mockMembershipServiceCount = jest.fn(); +const mockMembershipServiceDeleteMany = jest.fn(); +const mockMembershipRepositoryList = jest.fn(); +const mockMembershipRepositoryDeleteMany = jest.fn(); +const mockUserRepositoryFindWithFilter = jest.fn(); +const mockUserRepositoryUpdateById = jest.fn(); +const mockUserRepositoryRemove = jest.fn(); +const mockOrgRepositoryRemove = jest.fn(); + +jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() }, +})); + +jest.unstable_mockModule('../repositories/users.repository.js', () => ({ + default: { + list: jest.fn(), + create: jest.fn(), + get: jest.fn(), + update: jest.fn(), + remove: mockUserRepositoryRemove, + stats: jest.fn(), + updateById: mockUserRepositoryUpdateById, + findWithFilter: mockUserRepositoryFindWithFilter, + findByIdAndUpdatePopulated: jest.fn(), + searchByNameOrEmail: jest.fn(), + search: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../organizations/services/organizations.membership.service.js', () => ({ + default: { + listByUser: mockMembershipServiceListByUser, + count: mockMembershipServiceCount, + deleteMany: mockMembershipServiceDeleteMany, + }, +})); + +jest.unstable_mockModule('../../organizations/repositories/organizations.repository.js', () => ({ + default: { + remove: mockOrgRepositoryRemove, + findOne: jest.fn().mockResolvedValue(null), + list: jest.fn().mockResolvedValue([]), + create: jest.fn(), + update: jest.fn(), + get: jest.fn(), + exists: jest.fn().mockResolvedValue(false), + updateById: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../organizations/repositories/organizations.membership.repository.js', () => ({ + default: { + list: mockMembershipRepositoryList, + findOne: jest.fn(), + create: jest.fn(), + deleteMany: mockMembershipRepositoryDeleteMany, + update: jest.fn(), + remove: jest.fn(), + count: jest.fn(), + }, +})); + +jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({ + default: { signOut: jest.fn() }, +})); + +jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { organizations: { enabled: true } }, +})); + +jest.unstable_mockModule('../utils/sanitizeUser.js', () => ({ + removeSensitive: jest.fn((u) => u), +})); + +jest.unstable_mockModule('lodash', () => ({ + default: { pick: jest.fn((o) => o) }, +})); + +const { default: UsersService } = await import('../services/users.service.js'); + +describe('users.service.remove — pending membership sweep (#3831):', () => { + const user = { _id: 'uid1', id: 'uid1' }; + + beforeEach(() => { + jest.clearAllMocks(); + mockUserRepositoryRemove.mockResolvedValue({ deletedCount: 1 }); + mockMembershipServiceDeleteMany.mockResolvedValue({}); + mockMembershipRepositoryDeleteMany.mockResolvedValue({ deletedCount: 0 }); + }); + + test('sweeps the user PENDING rows (both sources) even with no active membership', async () => { + mockMembershipServiceListByUser.mockResolvedValue([]); + + await UsersService.remove(user); + + expect(mockMembershipRepositoryDeleteMany).toHaveBeenCalledWith({ userId: 'uid1', status: 'pending' }); + expect(mockUserRepositoryRemove).toHaveBeenCalledTimes(1); + }); + + test('sweeps PENDING rows after processing active memberships (non-owner path)', async () => { + mockMembershipServiceListByUser.mockResolvedValue([ + { _id: 'm1', role: 'member', organizationId: { _id: 'orgX' }, status: 'active' }, + ]); + + await UsersService.remove(user); + + // Active membership handled via the existing per-row delete... + expect(mockMembershipServiceDeleteMany).toHaveBeenCalledWith({ _id: 'm1' }); + // ...and the pending sweep ran exactly once with the userId + pending filter. + expect(mockMembershipRepositoryDeleteMany).toHaveBeenCalledTimes(1); + expect(mockMembershipRepositoryDeleteMany).toHaveBeenCalledWith({ userId: 'uid1', status: 'pending' }); + }); +});