diff --git a/modules/organizations/migrations/20260612120000-membership-user-org-unique-index.js b/modules/organizations/migrations/20260612120000-membership-user-org-unique-index.js new file mode 100644 index 000000000..3bae6aa53 --- /dev/null +++ b/modules/organizations/migrations/20260612120000-membership-user-org-unique-index.js @@ -0,0 +1,142 @@ +/** + * Module dependencies + */ +import mongoose from 'mongoose'; + +const INDEX_NAME = 'user_org_unique'; +const INDEX_KEY = { userId: 1, organizationId: 1 }; + +/** + * @desc Exact-key match helper: a two-field index keyed (userId:1, organizationId:1) + * in that order. Order-aware so the (organizationId, userId) prefix indexes are + * never touched. + * @param {Object} ix - an index document from listIndexes() + * @return {boolean} true when the index key is exactly { userId:1, organizationId:1 } + */ +const sameKey = (ix) => { + const keys = Object.keys(ix.key || {}); + return keys.length === 2 && keys[0] === 'userId' && keys[1] === 'organizationId' + && ix.key.userId === 1 && ix.key.organizationId === 1; +}; + +/** + * Migration: working (userId, organizationId) partial-unique membership index (#3841). + * + * The schema previously declared partialFilterExpression + * `{ userId: { $exists: true, $ne: null } }` — MongoDB does NOT support `$ne` in a + * partialFilterExpression, so server-side creation always failed. Mongoose autoIndex + * reports that failure on the model's 'index' event, where nothing listens, so the + * index NEVER existed on any deployed database and the duplicate-membership guard + * ran on application code alone (racy findOne-then-create in addMember / + * createJoinRequest). + * + * New spec (the schema declares the IDENTICAL twin): unique on + * { userId: 1, organizationId: 1 }, partialFilterExpression + * { userId: { $type: 'objectId' } }. `$type` is a supported partial-index operator + * and preserves the original intent: `userId: null` rows are BSON type null, not + * objectId, so they stay excluded and may repeat per organization. + * + * Safety / ordering: + * (a) Pre-check for existing duplicate (userId, organizationId) pairs that would + * violate the unique index. If any exist we ABORT (throw) WITHOUT touching + * indexes — picking which duplicate row wins is an operator decision, not a + * migration's call. The thrown error lists document ids only (PII-safe). + * (b) Drop any divergent index first: a same-key index under another name + * (phantom/legacy spec) or a namesake whose options drifted — either would + * conflict with or shadow the canonical index. Unlike the email-index + * migration there is no "window without a constraint" to protect: the + * constraint never materialized anywhere (that IS the bug), and dropping + * first avoids an IndexOptionsConflict on create. + * (c) Create `user_org_unique`. Idempotent: re-running after success is a no-op. + * + * autoIndex race: mongoose autoIndex:true (the default — db.options sets no + * override) builds the schema-declared twin on connect; identical specs make the + * race benign and syncIndexes() idempotent. This migration is the AUTHORITATIVE + * creator. + * + * @returns {Promise} + */ +export async function up() { + const memberships = mongoose.connection.db.collection('memberships'); + + // ── (a) Pre-check: refuse to run if duplicate (userId, organizationId) pairs exist ── + // Scope to rows the partial index will cover (userId is an ObjectId): null-userId + // rows are excluded by the partialFilterExpression and may legitimately repeat. + const duplicates = await memberships + .aggregate([ + { $match: { userId: { $type: 'objectId' } } }, + { $group: { _id: { userId: '$userId', organizationId: '$organizationId' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, + ]) + .toArray(); + + if (duplicates.length > 0) { + // Emit only membership document ids — never user/org identities — in + // operator-facing errors. + const sample = duplicates + .slice(0, 10) + .map((d) => `(${d.count} docs, ids: ${d.ids.slice(0, 3).join(',')}${d.ids.length > 3 ? ',…' : ''})`) + .join('; '); + throw new Error( + `[migration] membership-user-org-unique-index ABORTED: ${duplicates.length} duplicate membership (userId, organizationId) group(s) would violate the unique index. ` + + `Remediate (delete/merge the duplicate rows) before re-running — this migration will NOT pick winners. Sample (ids only): ${sample}`, + ); + } + + // Snapshot existing indexes once (listIndexes throws if the collection does not + // exist yet — tolerate that: a fresh DB has no memberships collection and + // autoIndex / syncIndexes will create the index from the schema declaration). + let existing = []; + try { + existing = await memberships.listIndexes().toArray(); + } catch (err) { + if (err?.codeName === 'NamespaceNotFound' || err?.code === 26) { + console.info('[migration] membership-user-org-unique-index: memberships collection does not exist yet — nothing to migrate'); + return; + } + throw err; + } + + // ── (b) Drop divergent indexes / detect the exact expected shape ── + let hasIndex = false; + for (const ix of existing) { + if (ix.name === '_id_') continue; + const keyMatches = sameKey(ix); + const exactShape = keyMatches + && ix.name === INDEX_NAME + && ix.unique === true + && ix.partialFilterExpression?.userId?.$type === 'objectId'; + if (exactShape) { + hasIndex = true; + } else if (keyMatches || ix.name === INDEX_NAME) { + await memberships.dropIndex(ix.name); + console.info(`[migration] membership-user-org-unique-index: dropped divergent index '${ix.name}'`); + } + } + + // ── (c) Create the partial-unique index (idempotent) ── + if (!hasIndex) { + await memberships.createIndex(INDEX_KEY, { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { userId: { $type: 'objectId' } }, + }); + console.info('[migration] membership-user-org-unique-index: created partial-unique index on (userId, organizationId)'); + } else { + console.info('[migration] membership-user-org-unique-index: partial-unique index already present — skipping create'); + } +} + +/** + * Down: no-op (warn). The pre-fix state was a unique guard that silently never + * existed — restoring "no index" would reintroduce the bug. Rollback = revert the + * schema declaration deliberately, then drop `user_org_unique` by hand if truly + * needed. + * + * @returns {void} + */ +export function down() { + console.warn( + '[migration] membership-user-org-unique-index DOWN: no-op; drop user_org_unique manually only alongside a deliberate schema revert', + ); +} diff --git a/modules/organizations/models/organizations.membership.model.mongoose.js b/modules/organizations/models/organizations.membership.model.mongoose.js index f6262bb74..323b8b32e 100644 --- a/modules/organizations/models/organizations.membership.model.mongoose.js +++ b/modules/organizations/models/organizations.membership.model.mongoose.js @@ -69,11 +69,25 @@ MembershipMongoose.pre('validate', function enforcePendingSource() { }); /** - * Compound unique index to prevent duplicate memberships + * Compound unique index preventing duplicate (userId, organizationId) memberships. + * + * The partialFilterExpression uses `$type: 'objectId'` — NOT `$ne: null` — because + * MongoDB does not support `$ne` (or `$not`) inside a partialFilterExpression. The + * previous spec (`{ userId: { $exists: true, $ne: null } }`) was rejected + * server-side on every build attempt; mongoose autoIndex reports that failure on + * the model's 'index' event, where nothing listens, so the index NEVER existed on + * any deployed database and duplicate protection silently fell back to the racy + * findOne-then-create guards in the membership service (#3841). `$type: 'objectId'` + * preserves the original intent: `userId: null` rows are BSON type null (not + * objectId), so they stay outside the index and may repeat per organization. + * + * The migration (migrations/20260612120000-membership-user-org-unique-index.js) is + * the AUTHORITATIVE creator; this declaration is its IDENTICAL twin (same key, + * options, AND explicit name) so autoIndex / syncIndexes stay idempotent. */ MembershipMongoose.index( { userId: 1, organizationId: 1 }, - { unique: true, partialFilterExpression: { userId: { $exists: true, $ne: null } } }, + { unique: true, name: 'user_org_unique', partialFilterExpression: { userId: { $type: 'objectId' } } }, ); /** diff --git a/modules/organizations/tests/organizations.membershipUserOrgUniqueIndex.migration.integration.tests.js b/modules/organizations/tests/organizations.membershipUserOrgUniqueIndex.migration.integration.tests.js new file mode 100644 index 000000000..c1bfb45aa --- /dev/null +++ b/modules/organizations/tests/organizations.membershipUserOrgUniqueIndex.migration.integration.tests.js @@ -0,0 +1,144 @@ +/** + * Module dependencies. + */ +import mongoose from 'mongoose'; + +import { beforeAll, afterAll, describe, test, expect } from '@jest/globals'; +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; +import { PENDING_SOURCES } from '../lib/constants.js'; + +import { up } from '../migrations/20260612120000-membership-user-org-unique-index.js'; + +const INDEX_NAME = 'user_org_unique'; + +/** + * Migration `20260612120000-membership-user-org-unique-index` (#3841). + * + * The old schema spec used `$ne` in its partialFilterExpression — unsupported by + * MongoDB — so the unique (userId, organizationId) guard NEVER materialized on + * any database (autoIndex failures land on the model's unlistened 'index' event). + * Verifies the fixed end state via the RAW collection driver: + * - up() creates `user_org_unique` with the EXACT spec (key, unique, $type filter); + * - idempotent (a second run leaves exactly one index on the key); + * - a same-key index living under another name is dropped and replaced; + * - pre-existing duplicate (userId, organizationId) pairs ABORT the migration + * before any index work. + */ +describe('Migration membership-user-org-unique-index:', () => { + let memberships; + const orgId = new mongoose.Types.ObjectId(); + const userId = new mongoose.Types.ObjectId(); + + beforeAll(async () => { + await bootstrap(); + memberships = mongoose.connection.db.collection('memberships'); + }); + + afterAll(async () => { + try { + await memberships.deleteMany({ organizationId: orgId }); + } catch (_) { /* cleanup */ } + try { + await mongooseService.disconnect(); + } catch (e) { + console.log(e); + expect(e).toBeFalsy(); + } + }); + + /** + * @desc Finds an index by name in the memberships collection. + * @param {string} name - the index name to look up. + * @returns {Promise} the index descriptor if found, undefined otherwise. + */ + const findIndex = async (name) => { + const indexes = await memberships.listIndexes().toArray(); + return indexes.find((ix) => ix.name === name); + }; + + test('up() creates user_org_unique with the exact spec', async () => { + await up(); + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ userId: 1, organizationId: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ userId: { $type: 'objectId' } }); + }); + + test('is idempotent — a second run leaves exactly one index on the key', async () => { + await up(); + await up(); + const indexes = await memberships.listIndexes().toArray(); + const sameKey = indexes.filter((ix) => ix.key && ix.key.userId === 1 && ix.key.organizationId === 1); + expect(sameKey.length).toBe(1); + expect(sameKey[0].name).toBe(INDEX_NAME); + }); + + test('drops a same-key index living under another name and installs the canonical one', async () => { + try { await memberships.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + await memberships.createIndex({ userId: 1, organizationId: 1 }, { name: 'userId_1_organizationId_1' }); + await up(); + expect(await findIndex('userId_1_organizationId_1')).toBeUndefined(); + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.unique).toBe(true); + }); + + test('ABORTS on pre-existing duplicate (userId, organizationId) pairs without touching indexes', async () => { + const dupA = new mongoose.Types.ObjectId(); + const dupB = new mongoose.Types.ObjectId(); + try { + try { await memberships.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + await memberships.insertOne({ + _id: dupA, userId, organizationId: orgId, role: 'member', status: 'pending', + source: PENDING_SOURCES.OWNER_ADD, createdAt: new Date(), updatedAt: new Date(), + }); + await memberships.insertOne({ + _id: dupB, userId, organizationId: orgId, role: 'member', status: 'pending', + source: PENDING_SOURCES.JOIN_REQUEST, createdAt: new Date(), updatedAt: new Date(), + }); + await expect(up()).rejects.toThrow(/duplicate membership/); + // Abort happened before any index work — the index is still absent. + expect(await findIndex(INDEX_NAME)).toBeUndefined(); + } finally { + await memberships.deleteMany({ _id: { $in: [dupA, dupB] } }); + await up(); // restore the migrated end state for the suites that follow + } + }); + + test('schema twin is IDENTICAL — syncIndexes() has nothing to drop or rebuild', async () => { + await up(); + const Membership = mongoose.model('Membership'); + const dropped = await Membership.syncIndexes(); + expect(dropped).toEqual([]); + expect(await findIndex(INDEX_NAME)).toBeDefined(); + }); + + test('DB backstop: a second pending row for the same (user, org) rejects with E11000', async () => { + await up(); + const Membership = mongoose.model('Membership'); + // Bypass the service guards (findOne-then-create) on purpose: the index itself + // must reject the duplicate, proving addMember/createJoinRequest now have a + // race-proof DB backstop instead of an application-level check alone. + await Membership.create({ + userId, organizationId: orgId, role: 'member', + status: 'pending', source: PENDING_SOURCES.OWNER_ADD, + }); + await expect( + Membership.create({ + userId, organizationId: orgId, role: 'member', + status: 'pending', source: PENDING_SOURCES.JOIN_REQUEST, + }), + ).rejects.toMatchObject({ code: 11000 }); + expect(await Membership.countDocuments({ userId, organizationId: orgId })).toBe(1); + }); + + test('null-userId rows stay OUTSIDE the partial index (may repeat per org)', async () => { + const Membership = mongoose.model('Membership'); + // userId defaults to null; status defaults to active (no source required). + await Membership.create({ organizationId: orgId }); + await Membership.create({ organizationId: orgId }); + expect(await Membership.countDocuments({ organizationId: orgId, userId: null })).toBe(2); + }); +});