diff --git a/packages/@n8n/api-types/src/dto/roles/__tests__/create-role-mapping-rule.dto.test.ts b/packages/@n8n/api-types/src/dto/roles/__tests__/create-role-mapping-rule.dto.test.ts new file mode 100644 index 0000000000000..2c21539281cda --- /dev/null +++ b/packages/@n8n/api-types/src/dto/roles/__tests__/create-role-mapping-rule.dto.test.ts @@ -0,0 +1,39 @@ +import { CreateRoleMappingRuleDto } from '../create-role-mapping-rule.dto'; + +const VALID_BODY = { + expression: 'claims.admin === true', + role: 'global:member', + type: 'instance' as const, +}; + +describe('CreateRoleMappingRuleDto', () => { + describe('Valid requests', () => { + test.each([ + { name: 'order omitted', request: { ...VALID_BODY } }, + { name: 'order zero', request: { ...VALID_BODY, order: 0 } }, + { name: 'positive integer order', request: { ...VALID_BODY, order: 5 } }, + { + name: 'project rule with projectIds', + request: { + ...VALID_BODY, + type: 'project', + role: 'project:editor', + projectIds: ['p1'], + }, + }, + ])('accepts $name', ({ request }) => { + expect(CreateRoleMappingRuleDto.safeParse(request).success).toBe(true); + }); + }); + + describe('Invalid requests', () => { + test.each([ + { name: 'negative order', request: { ...VALID_BODY, order: -1 } }, + { name: 'non-integer order', request: { ...VALID_BODY, order: 1.5 } }, + { name: 'empty expression', request: { ...VALID_BODY, expression: '' } }, + { name: 'unknown type', request: { ...VALID_BODY, type: 'unknown' } }, + ])('rejects $name', ({ request }) => { + expect(CreateRoleMappingRuleDto.safeParse(request).success).toBe(false); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/roles/create-role-mapping-rule.dto.ts b/packages/@n8n/api-types/src/dto/roles/create-role-mapping-rule.dto.ts index 9916601e09f25..0005f92190e5e 100644 --- a/packages/@n8n/api-types/src/dto/roles/create-role-mapping-rule.dto.ts +++ b/packages/@n8n/api-types/src/dto/roles/create-role-mapping-rule.dto.ts @@ -6,6 +6,6 @@ export class CreateRoleMappingRuleDto extends Z.class({ expression: z.string().min(1), role: z.string().min(1).max(128), type: z.enum(['instance', 'project']), - order: z.number().int(), + order: z.number().int().min(0).optional(), projectIds: z.array(z.string()).optional(), }) {} diff --git a/packages/cli/src/modules/provisioning.ee/__tests__/role-mapping-rule.service.ee.test.ts b/packages/cli/src/modules/provisioning.ee/__tests__/role-mapping-rule.service.ee.test.ts index 0f1732c437141..6a711d27b0fe6 100644 --- a/packages/cli/src/modules/provisioning.ee/__tests__/role-mapping-rule.service.ee.test.ts +++ b/packages/cli/src/modules/provisioning.ee/__tests__/role-mapping-rule.service.ee.test.ts @@ -52,12 +52,26 @@ const projectRole: Role = { }; describe('RoleMappingRuleService', () => { + const defaultUpdateSpy = jest.fn().mockResolvedValue(undefined); + const defaultTransactionSpy = jest + .fn() + .mockImplementation(async (cb: (tx: { update: typeof defaultUpdateSpy }) => Promise) => { + await cb({ update: defaultUpdateSpy }); + }); + beforeEach(() => { jest.clearAllMocks(); roleMappingRuleRepository.findOne.mockResolvedValue(null); // normalizeOrderForType calls find after every mutation; default to empty // so existing tests hit the early-exit path and require no transaction mock. roleMappingRuleRepository.find.mockResolvedValue([]); + // create() always calls applyOrder (which uses a transaction) to splice the + // newly-saved rule into the existing order. Inner describes may override. + (roleMappingRuleRepository as unknown as Record).manager = { + transaction: defaultTransactionSpy, + }; + defaultUpdateSpy.mockClear(); + defaultTransactionSpy.mockClear(); }); describe('create', () => { @@ -134,22 +148,6 @@ describe('RoleMappingRuleService', () => { ).rejects.toThrow(BadRequestError); }); - it('should reject when another rule already uses the same type and order', async () => { - roleRepository.findOne.mockResolvedValue(globalRole); - roleMappingRuleRepository.findOne.mockResolvedValue({ - id: 'f47ac10b-58cc-4372-a567-0e02b2c3d479', - } as RoleMappingRule); - - await expect( - service.create({ - expression: 'true', - role: globalRole.slug, - type: 'instance', - order: 2, - }), - ).rejects.toThrow(ConflictError); - }); - it('should reject when some project ids do not exist', async () => { roleRepository.findOne.mockResolvedValue(projectRole); projectRepository.findBy.mockResolvedValue([{ id: 'p1' } as Project]); @@ -285,6 +283,146 @@ describe('RoleMappingRuleService', () => { expect(result.projectIds).toEqual([projA.id]); expect(projectRepository.findBy).toHaveBeenCalledTimes(1); }); + + it('should append when order is omitted', async () => { + roleRepository.findOne.mockResolvedValue(globalRole); + roleMappingRuleRepository.find.mockResolvedValue([ + { id: 'rule-a', order: 0 } as RoleMappingRule, + { id: 'rule-b', order: 1 } as RoleMappingRule, + ]); + + const savedRule = { + id: 'rule-c', + expression: 'claims.c', + role: globalRole, + type: 'instance', + order: 2, + projects: [], + createdAt: new Date('2025-01-01T00:00:00.000Z'), + updatedAt: new Date('2025-01-01T00:00:00.000Z'), + } as unknown as RoleMappingRule; + + roleMappingRuleRepository.save.mockImplementation(async (r) => ({ + ...(r as RoleMappingRule), + id: 'rule-c', + })); + roleMappingRuleRepository.findOneOrFail.mockResolvedValue(savedRule); + + await service.create({ + expression: 'claims.c', + role: globalRole.slug, + type: 'instance', + }); + + // applyOrder should renumber [rule-a, rule-b, rule-c] to [0, 1, 2] + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-a' }, + { order: 0 }, + ); + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-b' }, + { order: 1 }, + ); + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-c' }, + { order: 2 }, + ); + }); + + it('should insert at index 0 and shift existing rules down', async () => { + roleRepository.findOne.mockResolvedValue(globalRole); + roleMappingRuleRepository.find.mockResolvedValue([ + { id: 'rule-a', order: 0 } as RoleMappingRule, + { id: 'rule-b', order: 1 } as RoleMappingRule, + ]); + + const savedRule = { + id: 'rule-new', + expression: 'claims.new', + role: globalRole, + type: 'instance', + order: 0, + projects: [], + createdAt: new Date('2025-01-01T00:00:00.000Z'), + updatedAt: new Date('2025-01-01T00:00:00.000Z'), + } as unknown as RoleMappingRule; + + roleMappingRuleRepository.save.mockImplementation(async (r) => ({ + ...(r as RoleMappingRule), + id: 'rule-new', + })); + roleMappingRuleRepository.findOneOrFail.mockResolvedValue(savedRule); + + await service.create({ + expression: 'claims.new', + role: globalRole.slug, + type: 'instance', + order: 0, + }); + + // applyOrder should renumber [rule-new, rule-a, rule-b] to [0, 1, 2] + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-new' }, + { order: 0 }, + ); + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-a' }, + { order: 1 }, + ); + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-b' }, + { order: 2 }, + ); + }); + + it('should clamp supplied order beyond existing list length to the end', async () => { + roleRepository.findOne.mockResolvedValue(globalRole); + roleMappingRuleRepository.find.mockResolvedValue([ + { id: 'rule-a', order: 0 } as RoleMappingRule, + ]); + + const savedRule = { + id: 'rule-new', + expression: 'claims.new', + role: globalRole, + type: 'instance', + order: 1, + projects: [], + createdAt: new Date('2025-01-01T00:00:00.000Z'), + updatedAt: new Date('2025-01-01T00:00:00.000Z'), + } as unknown as RoleMappingRule; + + roleMappingRuleRepository.save.mockImplementation(async (r) => ({ + ...(r as RoleMappingRule), + id: 'rule-new', + })); + roleMappingRuleRepository.findOneOrFail.mockResolvedValue(savedRule); + + await service.create({ + expression: 'claims.new', + role: globalRole.slug, + type: 'instance', + order: 999, + }); + + // Clamped to end: [rule-a, rule-new] → orders [0, 1] + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-a' }, + { order: 0 }, + ); + expect(defaultUpdateSpy).toHaveBeenCalledWith( + expect.anything(), + { id: 'rule-new' }, + { order: 1 }, + ); + }); }); describe('list', () => { @@ -645,35 +783,6 @@ describe('RoleMappingRuleService', () => { expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'c' }, { order: 2 }); }); - it('should compact a large gap after create', async () => { - // Simulates [0, 1, 100] after creating a rule at order 100 - roleMappingRuleRepository.find.mockResolvedValue([ - makeRule('a', 0), - makeRule('b', 1), - makeRule('c', 100), - ]); - - roleRepository.findOne.mockResolvedValue(globalRole); - roleMappingRuleRepository.save.mockResolvedValue(makeRule('c', 100)); - roleMappingRuleRepository.findOneOrFail.mockResolvedValue({ - ...makeRule('c', 2), - role: globalRole, - projects: [], - createdAt: new Date('2025-01-01T00:00:00.000Z'), - updatedAt: new Date('2025-01-01T00:00:00.000Z'), - } as unknown as RoleMappingRule); - - await service.create({ - expression: 'true', - role: globalRole.slug, - type: 'instance', - order: 100, - }); - - expect(transactionSpy).toHaveBeenCalledTimes(1); - expect(updateSpy).toHaveBeenCalledWith(expect.anything(), { id: 'c' }, { order: 2 }); - }); - it('should normalize both types when type changes during patch', async () => { const existingRule = { id: 'rule-1', diff --git a/packages/cli/src/modules/provisioning.ee/role-mapping-rule.service.ee.ts b/packages/cli/src/modules/provisioning.ee/role-mapping-rule.service.ee.ts index 992fb67df1275..223b7870c49b8 100644 --- a/packages/cli/src/modules/provisioning.ee/role-mapping-rule.service.ee.ts +++ b/packages/cli/src/modules/provisioning.ee/role-mapping-rule.service.ee.ts @@ -90,8 +90,6 @@ export class RoleMappingRuleService { assertRoleCompatibleWithMappingType(role, dto.type); - await this.assertOrderAvailable(dto.type, dto.order); - const projects = uniqueProjectIds.length > 0 ? await this.projectRepository.findBy({ id: In(uniqueProjectIds) }) @@ -101,16 +99,37 @@ export class RoleMappingRuleService { throw new BadRequestError('One or more projects were not found'); } + const existingRules = await this.roleMappingRuleRepository.find({ + where: { type: dto.type }, + select: ['id', 'order'], + order: { order: 'ASC' }, + }); + + // Clamp the requested index into the valid range. Omitted order appends. + const requestedOrder = dto.order ?? existingRules.length; + const targetIndex = Math.min(Math.max(requestedOrder, 0), existingRules.length); + + // Save the new rule at a temporary order beyond any currently-used slot, + // so the unique (type, order) constraint cannot fire on the initial insert. + const maxOrder = existingRules.length > 0 ? existingRules[existingRules.length - 1].order : -1; + const tempOrder = Math.max(maxOrder, existingRules.length - 1) + 1; + const rule = new RoleMappingRule(); rule.expression = dto.expression; rule.role = role; rule.type = dto.type; - rule.order = dto.order; + rule.order = tempOrder; rule.projects = projects; const saved = await this.roleMappingRuleRepository.save(rule); - await this.normalizeOrderForType(dto.type); + // Build the final ordering: existing rules in their current order, with the + // newly saved rule spliced in at targetIndex. applyOrder atomically renumbers + // everything to [0..n-1] using its two-phase transaction. + const reorderedIds = existingRules.map((r) => r.id); + reorderedIds.splice(targetIndex, 0, saved.id); + + await this.applyOrder(reorderedIds); const loaded = await this.roleMappingRuleRepository.findOneOrFail({ where: { id: saved.id }, diff --git a/packages/cli/test/integration/role-mapping-rule.api.test.ts b/packages/cli/test/integration/role-mapping-rule.api.test.ts index 50ccda1774aba..c69b15aae419c 100644 --- a/packages/cli/test/integration/role-mapping-rule.api.test.ts +++ b/packages/cli/test/integration/role-mapping-rule.api.test.ts @@ -124,17 +124,134 @@ describe('POST /role-mapping-rule', () => { expect(stored!.projects).toHaveLength(0); }); - it('should return 409 when type and order match an existing rule', async () => { + it('should shift existing rules down when a new rule is created at an occupied order', async () => { + const first = await ownerAgent + .post('/role-mapping-rule') + .send(validInstancePayload) + .expect(200); + + const second = await ownerAgent + .post('/role-mapping-rule') + .send({ + ...validInstancePayload, + expression: 'claims.other === true', + order: 0, + }) + .expect(200); + + expect(second.body.data.order).toBe(0); + + const repo = Container.get(RoleMappingRuleRepository); + const all = await repo.find({ where: { type: 'instance' }, order: { order: 'ASC' } }); + expect(all.map((r) => r.id)).toEqual([second.body.data.id, first.body.data.id]); + expect(all.map((r) => r.order)).toEqual([0, 1]); + }); + + it('should insert at position 0 and shift all existing rules down', async () => { + const first = await ownerAgent + .post('/role-mapping-rule') + .send(validInstancePayload) + .expect(200); + const second = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.b', order: 1 }) + .expect(200); + + const inserted = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.new-head', order: 0 }) + .expect(200); + + expect(inserted.body.data.order).toBe(0); + + const list = await ownerAgent.get('/role-mapping-rule').expect(200); + expect(list.body.data.items.map((r: { id: string }) => r.id)).toEqual([ + inserted.body.data.id, + first.body.data.id, + second.body.data.id, + ]); + expect(list.body.data.items.map((r: { order: number }) => r.order)).toEqual([0, 1, 2]); + }); + + it('should append when order is omitted', async () => { await ownerAgent.post('/role-mapping-rule').send(validInstancePayload).expect(200); + await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.b', order: 1 }) + .expect(200); - const response = await ownerAgent.post('/role-mapping-rule').send({ - ...validInstancePayload, - expression: 'claims.other === true', - }); + const { role, type } = validInstancePayload; + const appended = await ownerAgent + .post('/role-mapping-rule') + .send({ expression: 'claims.appended', role, type }) + .expect(200); - expect(response.status).toBe(409); - expect(response.body.message).toContain('already exists'); - expect(response.body.message).toContain('order'); + expect(appended.body.data.order).toBe(2); + }); + + it('should clamp order to list length when supplied value is too high', async () => { + await ownerAgent.post('/role-mapping-rule').send(validInstancePayload).expect(200); + + const tooHigh = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.b', order: 999 }) + .expect(200); + + expect(tooHigh.body.data.order).toBe(1); + }); + + it('should insert at a middle position and shift subsequent rules down', async () => { + const first = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.a', order: 0 }) + .expect(200); + const second = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.b', order: 1 }) + .expect(200); + const third = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.c', order: 2 }) + .expect(200); + + const inserted = await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, expression: 'claims.middle', order: 1 }) + .expect(200); + + expect(inserted.body.data.order).toBe(1); + + const list = await ownerAgent.get('/role-mapping-rule').expect(200); + expect(list.body.data.items.map((r: { id: string }) => r.id)).toEqual([ + first.body.data.id, + inserted.body.data.id, + second.body.data.id, + third.body.data.id, + ]); + expect(list.body.data.items.map((r: { order: number }) => r.order)).toEqual([0, 1, 2, 3]); + }); + + it('should allow instance and project rules to share the same order value', async () => { + const teamProject = await createTeamProject(undefined, owner); + + await ownerAgent + .post('/role-mapping-rule') + .send({ ...validInstancePayload, order: 0 }) + .expect(200); + + const projectRule = await ownerAgent + .post('/role-mapping-rule') + .send({ + expression: 'claims.project', + role: 'project:editor', + type: 'project', + order: 0, + projectIds: [teamProject.id], + }) + .expect(200); + + expect(projectRule.body.data.order).toBe(0); + expect(projectRule.body.data.type).toBe('project'); }); it('should normalize order when created with an abnormally high order value', async () => { diff --git a/packages/frontend/@n8n/rest-api-client/src/api/roleMappingRule.ts b/packages/frontend/@n8n/rest-api-client/src/api/roleMappingRule.ts index 8a0894a496659..c8566a2a38a92 100644 --- a/packages/frontend/@n8n/rest-api-client/src/api/roleMappingRule.ts +++ b/packages/frontend/@n8n/rest-api-client/src/api/roleMappingRule.ts @@ -19,7 +19,7 @@ export type CreateRoleMappingRuleInput = { expression: string; role: string; type: RoleMappingRuleType; - order: number; + order?: number; projectIds?: string[]; }; diff --git a/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.test.ts b/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.test.ts index a4acb8d23d355..d3322367586cb 100644 --- a/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.test.ts @@ -204,4 +204,74 @@ describe('useRoleMappingRules', () => { expect(composable.projectRules.value.every((r) => r.type === 'project')).toBe(true); }); }); + + describe('save', () => { + it('should send the local order on creates so user-intended position is preserved', async () => { + vi.mocked(roleMappingRuleApi.listRoleMappingRules).mockResolvedValue([ + makeRule({ id: 'persisted-1', type: 'instance', order: 0 }), + ]); + vi.mocked(roleMappingRuleApi.createRoleMappingRule).mockResolvedValue(makeRule()); + vi.mocked(roleMappingRuleApi.updateRoleMappingRule).mockResolvedValue(makeRule()); + + await composable.loadRules(); + composable.addRule('instance'); + const localId = composable.instanceRules.value[1].id; + composable.updateRule(localId, { + expression: '$claims.admin', + role: 'global:member', + }); + // Drag the new local rule above the persisted one. + await composable.reorder('instance', 1, 0); + + await composable.save(); + + expect(roleMappingRuleApi.createRoleMappingRule).toHaveBeenCalledTimes(1); + const [, payload] = vi.mocked(roleMappingRuleApi.createRoleMappingRule).mock.calls[0]; + expect(payload).toMatchObject({ + expression: '$claims.admin', + role: 'global:member', + type: 'instance', + order: 0, + }); + }); + + it('should serialize create calls to avoid concurrent temp-order collisions', async () => { + const callOrder: string[] = []; + vi.mocked(roleMappingRuleApi.createRoleMappingRule).mockImplementation( + async (_ctx, input) => { + callOrder.push(input.expression); + await new Promise((r) => setTimeout(r, 0)); + return makeRule({ expression: input.expression }); + }, + ); + + composable.addRule('instance'); + composable.addRule('instance'); + composable.updateRule(composable.instanceRules.value[0].id, { expression: 'first' }); + composable.updateRule(composable.instanceRules.value[1].id, { expression: 'second' }); + + await composable.save(); + + expect(callOrder).toEqual(['first', 'second']); + }); + + it('should include order on updates to existing persisted rules', async () => { + vi.mocked(roleMappingRuleApi.listRoleMappingRules).mockResolvedValue([ + makeRule({ id: 'persisted-1', type: 'instance', order: 0, expression: 'orig' }), + ]); + vi.mocked(roleMappingRuleApi.updateRoleMappingRule).mockResolvedValue( + makeRule({ id: 'persisted-1' }), + ); + + await composable.loadRules(); + composable.updateRule('persisted-1', { expression: 'edited' }); + await composable.save(); + + expect(roleMappingRuleApi.updateRoleMappingRule).toHaveBeenCalledWith( + expect.anything(), + 'persisted-1', + expect.objectContaining({ order: 0 }), + ); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.ts b/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.ts index 05a3837c891bf..fdcb964672fc1 100644 --- a/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.ts +++ b/packages/frontend/editor-ui/src/features/settings/sso/provisioning/composables/useRoleMappingRules.ts @@ -122,6 +122,7 @@ export function useRoleMappingRules() { try { const allLocalRules = [...instanceRules.value, ...projectRules.value]; const localRuleIds = new Set(allLocalRules.map((r) => r.id)); + const rulePayload = (r: RoleMappingRuleResponse) => ({ expression: r.expression, role: r.role, @@ -130,16 +131,24 @@ export function useRoleMappingRules() { projectIds: r.projectIds, }); + const deleteIds = [...serverRuleIds].filter((id) => !localRuleIds.has(id)); + const updateRules = allLocalRules.filter( + (r) => !r.id.startsWith('local-') && serverRuleIds.has(r.id), + ); + const createRules = allLocalRules.filter((r) => r.id.startsWith('local-')); + + // Deletes and updates can run concurrently. Creates must be sequential + // because the backend reshuffles orders on each create, and race + // conditions between concurrent creates can collide on temp orders. await Promise.all([ - ...[...serverRuleIds] - .filter((id) => !localRuleIds.has(id)) - .map(async (id) => await api.deleteRule(id)), - ...allLocalRules.map(async (rule): Promise => { - if (rule.id.startsWith('local-')) await api.createRule(rulePayload(rule)); - else if (serverRuleIds.has(rule.id)) await api.updateRule(rule.id, rulePayload(rule)); - }), + ...deleteIds.map(async (id) => await api.deleteRule(id)), + ...updateRules.map(async (r) => await api.updateRule(r.id, rulePayload(r))), ]); + for (const rule of createRules) { + await api.createRule(rulePayload(rule)); + } + await loadRules(); } finally { isLoading.value = false;