From 8feb5f607dc0d15694f4671d162d5d212b8baaaa Mon Sep 17 00:00:00 2001 From: Uwamari Goddey Date: Mon, 5 Oct 2026 12:50:48 -0400 Subject: [PATCH] fix(teams): make team names unique per organization A team name was unique across every organization (teams_name_key UNIQUE (name)), so one organization creating a team blocked every other organization from using that name. Uniqueness is now scoped to the organization: teams_organization_id_name_key UNIQUE (organization_id, name). The migration is administrative (teams is postgres-owned in production) and adds the new constraint before dropping the old one, inside the runner's per-migration transaction. The create handler answers 409 for either constraint name, so the code can be deployed before the migration is applied. Any other unique violation is still a 500, and no lookup is added: the database decides. Co-Authored-By: Claude Opus 5.5 --- backend/src/controllers/teams.controller.ts | 7 +- .../__tests__/teams-duplicate-name.test.ts | 265 +++++++++++++++--- ...240_teams_name_unique_per_organization.sql | 52 ++++ 3 files changed, 291 insertions(+), 33 deletions(-) create mode 100644 database/migrations-admin/202610051240_teams_name_unique_per_organization.sql diff --git a/backend/src/controllers/teams.controller.ts b/backend/src/controllers/teams.controller.ts index 84cffe20..214cf52e 100644 --- a/backend/src/controllers/teams.controller.ts +++ b/backend/src/controllers/teams.controller.ts @@ -6,12 +6,17 @@ const repository = new TeamsRepository(); const TEAM_NAME_CONFLICT_MESSAGE = 'A team with this name already exists.'; +// The team-name uniqueness constraint under either of its names: the +// per-organization one, and the global one it replaces, which is still what a +// database reports until the migration that swaps them has been applied. +const TEAM_NAME_CONSTRAINTS = new Set(['teams_organization_id_name_key', 'teams_name_key']); + // Only the unique violation on the team name. Every other database error, // including a unique violation on any other constraint, is left to the // caller's generic handling. function isTeamNameConflict(error: unknown): boolean { const { code, constraint } = (error ?? {}) as { code?: unknown; constraint?: unknown }; - return code === '23505' && constraint === 'teams_name_key'; + return code === '23505' && typeof constraint === 'string' && TEAM_NAME_CONSTRAINTS.has(constraint); } export class TeamsController { diff --git a/backend/src/routes/__tests__/teams-duplicate-name.test.ts b/backend/src/routes/__tests__/teams-duplicate-name.test.ts index 988b43dd..3e053b21 100644 --- a/backend/src/routes/__tests__/teams-duplicate-name.test.ts +++ b/backend/src/routes/__tests__/teams-duplicate-name.test.ts @@ -1,9 +1,18 @@ /** * POST /api/teams when the requested name is already taken. * + * A team name is unique within its organization + * (teams_organization_id_name_key), not across organizations. + * * Behaviour under test: * - The unique violation on the team name is answered with 409 and a fixed * message. The body carries nothing else: no identifiers, no existing row. + * - The same name in a different organization is not a conflict. + * - Both names of the team-name constraint are recognised: the + * per-organization one the schema has now, and the global teams_name_key + * a database still reports until the migration replacing it is applied. + * - Uniqueness is the database's: the create issues no lookup of its own. + * - Comparison is exact: names differing only in case are different names. * - A successful create is unchanged (201 with the created team). * - Every other database error is unchanged (500 with the generic message), * including a unique violation on any other constraint. @@ -12,11 +21,14 @@ * * Real routes over an in-process HTTP server against live Postgres. Only * authService.verifyToken (to choose the caller) is stubbed, plus the - * repository for the one error the schema cannot produce. + * repository for the errors the schema cannot produce. Statements are + * observed, not altered, at the pg Client, which every connection in this + * process goes through: the app pool hands requests their own client, so + * watching pool.query would miss them. */ import express from 'express'; import http from 'http'; -import { Pool } from 'pg'; +import { Client, Pool } from 'pg'; import teamsRoutes from '../teams.routes'; import { errorHandler } from '../../middleware/error-handler'; import { authService } from '../../services/auth.service'; @@ -37,6 +49,24 @@ const CONFLICT_MESSAGE = 'A team with this name already exists.'; const pool = new Pool(dbConfig()); +// Records what is sent while `recording` is set, then runs the real query. +// Installed before any connection exists, since the app pool keeps its own +// reference to a client's query the first time it checks that client out. +const sent: Array<{ text: string; values: unknown }> = []; +let recording = false; +const clientQuery = Client.prototype.query; +Client.prototype.query = function (this: Client, ...args: unknown[]) { + if (recording) { + const config = args[0] as string | { text?: unknown; values?: unknown } | null; + sent.push( + typeof config === 'string' + ? { text: config, values: Array.isArray(args[1]) ? args[1] : undefined } + : { text: String(config?.text ?? ''), values: config?.values } + ); + } + return (clientQuery as unknown as (...a: unknown[]) => unknown).apply(this, args); +} as unknown as typeof Client.prototype.query; + function uniqueSuffix(): string { return `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; } @@ -46,34 +76,72 @@ function teamName(): string { } async function teamsNamed(name: string) { - const { rows } = await pool.query('SELECT id, organization_id FROM teams WHERE name = $1', [name]); + const { rows } = await pool.query( + 'SELECT id, organization_id FROM teams WHERE name = $1 ORDER BY created_at, id', + [name] + ); return rows; } -let server: http.Server; -let baseUrl: string; -let orgId: string; -let userId: string; -let errorSpy: jest.SpyInstance; +interface Caller { + orgId: string; + userId: string; +} -beforeAll(async () => { +async function createCaller(label: string): Promise { const suffix = uniqueSuffix(); const org = await pool.query( `INSERT INTO organizations (name, slug, display_name, subscription_tier, subscription_status) VALUES ($1, $2, $1, 'enterprise', 'active') RETURNING id`, - [`Teams Duplicate Name ${suffix}`, `teams-duplicate-name-${suffix}`] + [`Teams Duplicate Name ${label} ${suffix}`, `teams-duplicate-name-${label}-${suffix}`] ); - orgId = org.rows[0].id; const user = await pool.query( `INSERT INTO users (email, password_hash, full_name) VALUES ($1, 'x', 'Teams Duplicate Name User') RETURNING id`, - [`teams-duplicate-name-${suffix}@example.com`] + [`teams-duplicate-name-${label}-${suffix}@example.com`] ); - userId = user.rows[0].id; await pool.query( `INSERT INTO organization_memberships (organization_id, user_id, role, joined_at, is_active) VALUES ($1, $2, 'owner', NOW(), true)`, - [orgId, userId] + [org.rows[0].id, user.rows[0].id] ); + return { orgId: org.rows[0].id, userId: user.rows[0].id }; +} + +async function removeCaller(caller: Caller) { + await pool.query('DELETE FROM teams WHERE organization_id = $1', [caller.orgId]); + await pool.query('DELETE FROM organization_memberships WHERE organization_id = $1', [caller.orgId]); + await pool.query('DELETE FROM organizations WHERE id = $1', [caller.orgId]); + await pool.query('DELETE FROM users WHERE id = $1', [caller.userId]); +} + +function actAs(caller: Partial) { + jest.spyOn(authService, 'verifyToken').mockReturnValue({ + userId: caller.userId, + email: 'teams-duplicate-name-caller@example.com', + organizationId: caller.orgId, + role: 'owner', + type: 'access', + } as unknown as ReturnType); +} + +function uniqueViolation(constraint: string) { + return Object.assign(new Error(`duplicate key value violates unique constraint "${constraint}"`), { + code: '23505', + constraint, + }); +} + +let server: http.Server; +let baseUrl: string; +let caller: Caller; +let otherCaller: Caller; +let orgId: string; +let errorSpy: jest.SpyInstance; + +beforeAll(async () => { + caller = await createCaller('a'); + otherCaller = await createCaller('b'); + orgId = caller.orgId; const app = express(); app.use(express.json()); @@ -88,27 +156,22 @@ beforeAll(async () => { beforeEach(() => { errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - jest.spyOn(authService, 'verifyToken').mockReturnValue({ - userId, - email: 'teams-duplicate-name-caller@example.com', - organizationId: orgId, - role: 'owner', - type: 'access', - } as unknown as ReturnType); + actAs(caller); }); afterEach(() => { + recording = false; + sent.length = 0; jest.restoreAllMocks(); }); afterAll(async () => { await new Promise((resolve) => server.close(() => resolve())); - await pool.query('DELETE FROM teams WHERE organization_id = $1', [orgId]); - await pool.query('DELETE FROM organization_memberships WHERE organization_id = $1', [orgId]); - await pool.query('DELETE FROM organizations WHERE id = $1', [orgId]); - await pool.query('DELETE FROM users WHERE id = $1', [userId]); + await removeCaller(caller); + await removeCaller(otherCaller); await pool.end(); await appPool.end(); + Client.prototype.query = clientQuery; }); function createTeam(body: Record) { @@ -138,15 +201,23 @@ describe('POST /api/teams', () => { expect(await teamsNamed(name)).toEqual([{ id: body.data.id, organization_id: orgId }]); }); - it('answers 409 with a fixed message when the name is already taken', async () => { + it('answers 409 with a fixed message when the name is already taken in the same organization', async () => { const name = teamName(); const first = await createTeam({ name, owner: 'owner@example.com' }); expect(first.status).toBe(201); const firstId = (await first.json()).data.id; + // Not a stub: the real create runs, and is only watched for what it rejects with. + const createSpy = jest.spyOn(TeamsRepository.prototype, 'create'); const res = await createTeam({ name, owner: 'second-owner@example.com', description: 'Second' }); expect(res.status).toBe(409); + // The conflict is the database's own, on the per-organization constraint. + expect(createSpy).toHaveBeenCalledTimes(1); + await expect(createSpy.mock.results[0].value).rejects.toMatchObject({ + code: '23505', + constraint: 'teams_organization_id_name_key', + }); // The whole body: nothing about the existing team or any organization. expect(await res.json()).toEqual({ success: false, @@ -158,6 +229,141 @@ describe('POST /api/teams', () => { expect(errorSpy).not.toHaveBeenCalled(); }); + it('creates the same name in two different organizations', async () => { + const name = teamName(); + + const first = await createTeam({ name, owner: 'owner@example.com' }); + actAs(otherCaller); + const second = await createTeam({ name, owner: 'owner@example.com' }); + + expect(first.status).toBe(201); + expect(second.status).toBe(201); + const firstBody = await first.json(); + const secondBody = await second.json(); + expect(firstBody.data).toMatchObject({ name, organization_id: caller.orgId }); + expect(secondBody.data).toMatchObject({ name, organization_id: otherCaller.orgId }); + expect(await teamsNamed(name)).toEqual([ + { id: firstBody.data.id, organization_id: caller.orgId }, + { id: secondBody.data.id, organization_id: otherCaller.orgId }, + ]); + + // Each organization still conflicts with itself. + const repeat = await createTeam({ name, owner: 'owner@example.com' }); + expect(repeat.status).toBe(409); + expect(await teamsNamed(name)).toHaveLength(2); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('treats names differing only in case as different names', async () => { + const base = teamName(); + const upper = `Platform-${base}`; + const lower = `platform-${base}`; + + const first = await createTeam({ name: upper, owner: 'owner@example.com' }); + const second = await createTeam({ name: lower, owner: 'owner@example.com' }); + + expect(first.status).toBe(201); + expect(second.status).toBe(201); + expect((await first.json()).data).toMatchObject({ name: upper, organization_id: orgId }); + expect((await second.json()).data).toMatchObject({ name: lower, organization_id: orgId }); + expect(await teamsNamed(upper)).toHaveLength(1); + expect(await teamsNamed(lower)).toHaveLength(1); + }); + + it('checks uniqueness with no lookup of its own, in any organization', async () => { + const name = teamName(); + actAs(otherCaller); + expect((await createTeam({ name, owner: 'owner@example.com' })).status).toBe(201); + actAs(caller); + const findAll = jest.spyOn(TeamsRepository.prototype, 'findAll'); + const findById = jest.spyOn(TeamsRepository.prototype, 'findById'); + + recording = true; + const created = await createTeam({ name, owner: 'owner@example.com' }); + const conflict = await createTeam({ name, owner: 'owner@example.com' }); + recording = false; + + expect(created.status).toBe(201); + expect(conflict.status).toBe(409); + expect(findAll).not.toHaveBeenCalled(); + expect(findById).not.toHaveBeenCalled(); + // Every statement the two requests sent, on any connection, that names + // the teams table or carries the requested name: one INSERT each, with + // the caller's own organization, and nothing else. + expect(sent.length).toBeGreaterThan(2); + const aboutTeams = sent.filter( + ({ text, values }) => /\bteams\b/i.test(text) || (Array.isArray(values) && values.includes(name)) + ); + expect(aboutTeams).toHaveLength(2); + for (const { text, values } of aboutTeams) { + expect(text.trim()).toMatch(/^INSERT INTO teams\b/); + expect(text).not.toMatch(/\bSELECT\b/i); + expect(values).toEqual([name, 'owner@example.com', undefined, caller.orgId]); + } + }); + + it('answers 409 for the global constraint a not-yet-migrated database reports', async () => { + // The schema under test no longer has teams_name_key, so the violation is supplied. + jest.spyOn(TeamsRepository.prototype, 'create').mockRejectedValueOnce(uniqueViolation('teams_name_key')); + + const res = await createTeam({ name: teamName(), owner: 'owner@example.com' }); + + expect(res.status).toBe(409); + expect(await res.json()).toEqual({ + success: false, + error: CONFLICT_MESSAGE, + message: CONFLICT_MESSAGE, + }); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('answers 409 for the per-organization constraint by name', async () => { + jest + .spyOn(TeamsRepository.prototype, 'create') + .mockRejectedValueOnce(uniqueViolation('teams_organization_id_name_key')); + + const res = await createTeam({ name: teamName(), owner: 'owner@example.com' }); + + expect(res.status).toBe(409); + expect(await res.json()).toEqual({ + success: false, + error: CONFLICT_MESSAGE, + message: CONFLICT_MESSAGE, + }); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('still requires an organization', async () => { + const name = teamName(); + const createSpy = jest.spyOn(TeamsRepository.prototype, 'create'); + // A token carrying no organization never reaches the controller. + actAs({ userId: caller.userId }); + + const res = await createTeam({ name, owner: 'owner@example.com' }); + + expect(res.status).toBe(401); + expect(createSpy).not.toHaveBeenCalled(); + // And the column itself refuses a team with none. + await expect( + pool.query('INSERT INTO teams (name, owner, organization_id) VALUES ($1, $2, NULL)', [name, 'owner@example.com']) + ).rejects.toMatchObject({ code: '23502', column: 'organization_id' }); + expect(await teamsNamed(name)).toEqual([]); + }); + + it('enforces the name with exactly one unique constraint, scoped to the organization', async () => { + const { rows } = await pool.query( + `SELECT conname, pg_get_constraintdef(oid) AS definition + FROM pg_constraint + WHERE conrelid = 'public.teams'::regclass AND contype IN ('p', 'u') + ORDER BY conname` + ); + + expect(rows).toEqual([ + { conname: 'teams_organization_id_name_key', definition: 'UNIQUE (organization_id, name)' }, + { conname: 'teams_pkey', definition: 'PRIMARY KEY (id)' }, + ]); + }); + it('leaves an unrelated database error as a 500', async () => { // Longer than the name column allows: a database error that is not a // unique violation. @@ -171,12 +377,7 @@ describe('POST /api/teams', () => { }); it('leaves a unique violation on any other constraint as a 500', async () => { - jest.spyOn(TeamsRepository.prototype, 'create').mockRejectedValueOnce( - Object.assign(new Error('duplicate key value violates unique constraint "some_other_key"'), { - code: '23505', - constraint: 'some_other_key', - }) - ); + jest.spyOn(TeamsRepository.prototype, 'create').mockRejectedValueOnce(uniqueViolation('some_other_key')); const res = await createTeam({ name: teamName(), owner: 'owner@example.com' }); diff --git a/database/migrations-admin/202610051240_teams_name_unique_per_organization.sql b/database/migrations-admin/202610051240_teams_name_unique_per_organization.sql new file mode 100644 index 00000000..5af4a1b4 --- /dev/null +++ b/database/migrations-admin/202610051240_teams_name_unique_per_organization.sql @@ -0,0 +1,52 @@ +-- Migration: 202610051240_teams_name_unique_per_organization.sql +-- Description: Scopes team-name uniqueness to the organization. A team name +-- was unique across every organization (teams_name_key UNIQUE (name), from +-- 001_create_platform_tables.sql's inline UNIQUE), so one organization +-- creating a team blocked every other organization from using that name. +-- After this migration a name is unique within its organization only: +-- teams_organization_id_name_key UNIQUE (organization_id, name). +-- Date: 2026-10-05 +-- +-- ADMINISTRATIVE MIGRATION -- see database/migrations-admin/README.md. +-- +-- PLACEMENT NOTE: classified into database/migrations-admin/ from the start, +-- not after a failed ordinary-path attempt. teams is confirmed postgres-owned +-- in production (it is one of the tables covered by the batch ownership audit +-- documented in database/migrations-admin/README.md, and was re-confirmed by +-- this change's own read-only preflight), so ALTER TABLE against it is known +-- in advance to fail under devcontrol's ordinary-path grants with 42501: must +-- be owner of table teams. +-- +-- The new constraint is added before the old one is removed, so there is no +-- point at which team names are unconstrained. Both statements run inside the +-- runner's single per-migration transaction: if either fails, neither takes +-- effect. Neither statement is guarded with IF EXISTS / IF NOT EXISTS -- a +-- database whose teams constraints are not what this migration assumes should +-- fail loudly and roll back rather than be recorded as applied. +-- +-- Depends on teams.organization_id being NOT NULL (005_migrate_existing_data.sql). +-- A UNIQUE constraint treats NULLs as distinct, so a nullable organization_id +-- would let rows with no organization share a name. +-- +-- Unchanged: every teams column, organization_id's nullability and foreign +-- key, RLS and its policies, and the non-unique indexes idx_teams_name, +-- idx_teams_owner and idx_teams_org. Comparison stays exact: names differing +-- only in case or surrounding whitespace remain distinct. +-- +-- Reverse: +-- ALTER TABLE teams ADD CONSTRAINT teams_name_key UNIQUE (name); +-- ALTER TABLE teams DROP CONSTRAINT teams_organization_id_name_key; +-- The reverse cannot succeed once two organizations hold teams with the same +-- name: UNIQUE (name) cannot be recreated while such rows exist. + +ALTER TABLE teams + ADD CONSTRAINT teams_organization_id_name_key UNIQUE (organization_id, name); + +ALTER TABLE teams + DROP CONSTRAINT teams_name_key; + +DO $$ +BEGIN + RAISE NOTICE 'Migration 202610051240 completed successfully!'; + RAISE NOTICE 'teams: name is now unique per organization (teams_organization_id_name_key), teams_name_key removed'; +END $$;