From 403d3f5cb4528874f4a7e02d5fad573143a298ea Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 12:17:32 +0200 Subject: [PATCH 1/7] fix(gitlab): stop reconciles from demoting instance admins The project reconcile pipeline pushed admin: false into GitLab's user edit whenever the reconciled user did not hold the role mapped on the project's adminGroupPath. GitLab's edit API cannot un-set the admin flag: a falsy value demotes the instance admin. A platform admin owning a project lost is_admin on the first reprovision. Reconciles now transmit admin/auditor only when true. Promotion stays write-positive; revocation, if ever needed, belongs to a dedicated admin flow, not the reconcile path. Refs #2804 Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: Id7dbb0f40242a7d05178be8baff88ea06a6a6964 --- .../gitlab/gitlab-client.service.spec.ts | 19 +++++++++++++++++++ .../modules/gitlab/gitlab-client.service.ts | 8 +++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts index da791e8013..11ac94e6c5 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts @@ -538,6 +538,25 @@ describe('gitlab-client', () => { })) }) + it('should not demote an existing admin when the reconcile computes admin false', async () => { + const consoleUser = { id: 'u1', email: 'owner@example.com', firstName: 'Owner', lastName: 'User' } + const gitlabUser = { + email: consoleUser.email, + username: 'owner', + name: 'Owner User', + } + const gitlabUsersAllMock = gitlabApi.Users.all as MockedFunction + gitlabUsersAllMock.mockResolvedValue([ + makeExpandedUserSchema({ id: 1000, email: consoleUser.email, is_admin: true }), + ]) + + await service.upsertUser({ ...gitlabUser, admin: false }, { cpnUserId: consoleUser.id }) + + expect(gitlabApi.Users.edit).toHaveBeenCalledWith(1000, expect.not.objectContaining({ + admin: expect.anything(), + })) + }) + it('should not disable existing admin flag when enabling auditor flag', async () => { const consoleUser = { id: 'u1', email: 'admin-auditor@example.com', firstName: 'Admin', lastName: 'Auditor' } const gitlabUser = { diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index 9295d21181..a7b70e585c 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts @@ -461,8 +461,14 @@ export class GitlabClientService { ) { const existing = await this.getUserByEmail(user.email) + // GitLab's user-edit payload has no way to un-set `admin`/`auditor` — a falsy value + // DEMOTES the instance admin/auditor. Reconciles that run for a project's admin-group + // mapping (adminRoleFlag: role mapped on the project → true, otherwise false) must not + // degrade a platform admin whose DB row lacks that specific role id. + // ponytail: write-only positive promotion; explicit revocation (if ever needed) goes + // through a dedicated admin-flow API call, never through this reconcile path. const editOptions: EditUserOptions = { - ...user, + ...Object.fromEntries(Object.entries(user).filter(([, v]) => v !== false)), externUid: user.email, provider: 'openid_connect', } From 94a0897c2140df74fe3dbd4f80d656564fb01721 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 12:39:51 +0200 Subject: [PATCH 2/7] fix(gitlab): stop reconciles from demoting instance admins The project reconcile pipeline pushed admin: false into GitLab's user edit whenever the reconciled user did not hold the role mapped on the project's adminGroupPath. GitLab's edit API cannot un-set the admin flag: a falsy value demotes the instance admin. A platform admin owning a project lost is_admin on the first reprovision. Reconciles now transmit admin/auditor only when true. Promotion stays write-positive; revocation, if ever needed, belongs to a dedicated admin flow, not the reconcile path. Refs #2804 Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: Id7dbb0f40242a7d05178be8baff88ea06a6a6964 --- .../src/modules/gitlab/gitlab-client.service.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index a7b70e585c..5d821be538 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts @@ -461,12 +461,8 @@ export class GitlabClientService { ) { const existing = await this.getUserByEmail(user.email) - // GitLab's user-edit payload has no way to un-set `admin`/`auditor` — a falsy value - // DEMOTES the instance admin/auditor. Reconciles that run for a project's admin-group - // mapping (adminRoleFlag: role mapped on the project → true, otherwise false) must not - // degrade a platform admin whose DB row lacks that specific role id. - // ponytail: write-only positive promotion; explicit revocation (if ever needed) goes - // through a dedicated admin-flow API call, never through this reconcile path. + // Falsy `admin`/`auditor` demotes the instance flag (the edit API cannot un-set them): + // reconcile promotes only. Revocation, if ever needed, goes through a dedicated admin flow. const editOptions: EditUserOptions = { ...Object.fromEntries(Object.entries(user).filter(([, v]) => v !== false)), externUid: user.email, From 5f0deb488ec2a4b3b0470a518a94182d12fca778 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 12:51:47 +0200 Subject: [PATCH 3/7] fix(gitlab): make adminRoleFlag emit no claim instead of revocation adminRoleFlag returned false when a project maps an admin/auditor role the user does not hold, ordering upsertUser to demote that GitLab instance flag. Under cumulative permissions the producer must never emit a revocation: absence of membership is now `undefined` (no claim), `true` stays promotion. The upsertUser guard keeps enforcing the boundary for any future caller. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I501ee9e3c43068283763b2867950c92fada38ab3 --- .../server-nestjs/src/modules/gitlab/gitlab.service.spec.ts | 6 +++--- apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts | 4 ++-- apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts index 299b431fdc..73d9564069 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts @@ -464,15 +464,15 @@ describe('gitlabService', () => { await service.handleUpsert(project) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: false }), + expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: undefined }), expect.objectContaining({ cpnUserId: 'u1' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'auditor@example.com', admin: false, auditor: true }), + expect.objectContaining({ email: 'auditor@example.com', admin: undefined, auditor: true }), expect.objectContaining({ cpnUserId: 'u2' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: false }), + expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: undefined }), expect.objectContaining({ cpnUserId: 'o1' }), ) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts index 58055b670d..c0bc9adbe8 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts @@ -82,8 +82,8 @@ describe('adminRoleFlag', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() }) - it('should match membership against any resolved role id', () => { + it('should not claim membership the user lacks', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, ['y', 'x'])).toBe(true) - expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBe(false) + expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBeUndefined() }) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts index 63853a9123..5ca5703459 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts @@ -247,7 +247,7 @@ export function daysAgoFromNow(date: Date) { export function adminRoleFlag(user: Pick, adminRoleIds: string[]): boolean | undefined { if (adminRoleIds.length === 0) return undefined - return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) + return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) ? true : undefined } export function isGitbeakerNotFound(error: unknown): error is GitbeakerRequestError { From aa1382dedbe405e3303bb0395eb25e357cbe7830 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 13:05:18 +0200 Subject: [PATCH 4/7] test(gitlab): pin instance-admin survival in e2e suite Reproduces the reported incident against a live GitLab: a project member's instance admin flag is granted outside the console, the project is reprovisioned, and the flag must survive. Requires E2E=1 and integ credentials to execute; keeps the cumulative-permissions contract observable for the maintenance environment. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: If41eddf4a50094980dc961b3e4a83017ada6c37a --- apps/server-nestjs/test/gitlab.e2e-spec.ts | 72 ++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/apps/server-nestjs/test/gitlab.e2e-spec.ts b/apps/server-nestjs/test/gitlab.e2e-spec.ts index 2c1c2c501e..846c2ce2bb 100644 --- a/apps/server-nestjs/test/gitlab.e2e-spec.ts +++ b/apps/server-nestjs/test/gitlab.e2e-spec.ts @@ -401,6 +401,78 @@ describeWithGitLab('GitlabService (e2e)', () => { }, GITLAB_PURGE_SYNC_TIMEOUT) }) + describe('instance admin survival', () => { + let memberId: string + let memberGitlabId: number | undefined + + beforeAll(async () => { + memberId = faker.string.uuid() + + const member = await gitlabClient.Users.create({ + name: 'Test Admin', + password: faker.internet.password({ length: 24 }), + username: `test-admin-${memberId}`, + email: `test-admin-${memberId}@example.com`, + skipConfirmation: true, + }) + memberGitlabId = member.id + + await prisma.user.create({ + data: { + id: memberId, + email: member.email.toLowerCase(), + firstName: 'Test', + lastName: 'Admin', + type: 'human', + }, + }) + + await prisma.projectMembers.create({ + data: { + projectId: testProjectId, + userId: memberId, + roleIds: [], + }, + }) + }) + + afterAll(async () => { + if (memberGitlabId) { + await gitlabClient.Users.remove(memberGitlabId).catch(() => {}) + } + if (prisma && memberId) { + await prisma.projectMembers.deleteMany({ where: { userId: memberId } }).catch(() => {}) + await prisma.user.deleteMany({ where: { id: memberId } }).catch(() => {}) + } + }) + + it('reprovisioning does not strip a GitLab instance admin flag set outside the console', async () => { + if (!memberGitlabId) throw new Error('GitLab member was not created') + + const project = await prisma.project.findUniqueOrThrow({ + where: { id: testProjectId }, + select: projectSelect, + }) + await eventEmitter.emitAsync('project.upsert', project) + + const before = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) + expect(before.is_admin).toBe(false) + + // Reproduction of the reported incident: the flag is granted on the GitLab + // instance directly, then the project is reprovisioned from the console. + await gitlabClient.Users.edit(memberGitlabId, { admin: true }) + + const reprovisioned = await prisma.project.findUniqueOrThrow({ + where: { id: testProjectId }, + select: projectSelect, + }) + await expect(eventEmitter.emitAsync('project.upsert', reprovisioned)).resolves.not.toThrow() + + const after = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) + expect(after.is_admin).toBe(true) + }, GITLAB_SYNC_TIMEOUT) + }) + it('should remove project group from GitLab on delete', async () => { const project = await prisma.project.findUniqueOrThrow({ where: { id: testProjectId }, From f2e7e34a2d826952306062e66a279d1c82c98331 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 13:26:38 +0200 Subject: [PATCH 5/7] fix(gitlab): make role membership authoritative for instance flags Reverses the promote-only contract: a resolved admin/auditor role mapping is authoritative, so holding it transmits true (promote) and lacking it transmits false (demote). Removal of a role therefore removes the GitLab instance flag. An empty mapping stays unknowable and transmits no claim. Drops the boundary false-strip, which would now swallow demotions. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I5f3a9c2e7b1d4a8f0e6c3b9d2a7f1e4c8b0d5a3f --- .../src/modules/gitlab/gitlab-client.service.spec.ts | 8 ++++---- .../src/modules/gitlab/gitlab-client.service.ts | 12 +++++++----- .../src/modules/gitlab/gitlab.service.spec.ts | 6 +++--- .../src/modules/gitlab/gitlab.utils.spec.ts | 5 +++-- .../server-nestjs/src/modules/gitlab/gitlab.utils.ts | 2 +- apps/server-nestjs/test/gitlab.e2e-spec.ts | 12 +++--------- 6 files changed, 21 insertions(+), 24 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts index 11ac94e6c5..07a6f2f20b 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts @@ -538,7 +538,7 @@ describe('gitlab-client', () => { })) }) - it('should not demote an existing admin when the reconcile computes admin false', async () => { + it('should demote an existing admin when the reconcile computes admin false', async () => { const consoleUser = { id: 'u1', email: 'owner@example.com', firstName: 'Owner', lastName: 'User' } const gitlabUser = { email: consoleUser.email, @@ -552,12 +552,12 @@ describe('gitlab-client', () => { await service.upsertUser({ ...gitlabUser, admin: false }, { cpnUserId: consoleUser.id }) - expect(gitlabApi.Users.edit).toHaveBeenCalledWith(1000, expect.not.objectContaining({ - admin: expect.anything(), + expect(gitlabApi.Users.edit).toHaveBeenCalledWith(1000, expect.objectContaining({ + admin: false, })) }) - it('should not disable existing admin flag when enabling auditor flag', async () => { + it('should not transmit admin true when only the auditor flag changes', async () => { const consoleUser = { id: 'u1', email: 'admin-auditor@example.com', firstName: 'Admin', lastName: 'Auditor' } const gitlabUser = { email: consoleUser.email, diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index 5d821be538..d4d154d922 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts @@ -461,15 +461,17 @@ export class GitlabClientService { ) { const existing = await this.getUserByEmail(user.email) - // Falsy `admin`/`auditor` demotes the instance flag (the edit API cannot un-set them): - // reconcile promotes only. Revocation, if ever needed, goes through a dedicated admin flow. + const gitlabUser = existing ?? await this.createUser({ + ...user, + externUid: user.email, + provider: 'openid_connect', + }) + const editOptions: EditUserOptions = { - ...Object.fromEntries(Object.entries(user).filter(([, v]) => v !== false)), + ...user, externUid: user.email, provider: 'openid_connect', } - const gitlabUser = existing ?? await this.createUser(editOptions) - if (existing) { const hasDiff = Object.entries(editOptions).some(([key, value]) => { if (value === undefined) return false diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts index 73d9564069..299b431fdc 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts @@ -464,15 +464,15 @@ describe('gitlabService', () => { await service.handleUpsert(project) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: undefined }), + expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: false }), expect.objectContaining({ cpnUserId: 'u1' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'auditor@example.com', admin: undefined, auditor: true }), + expect.objectContaining({ email: 'auditor@example.com', admin: false, auditor: true }), expect.objectContaining({ cpnUserId: 'u2' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: undefined }), + expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: false }), expect.objectContaining({ cpnUserId: 'o1' }), ) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts index c0bc9adbe8..74b0e657bd 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts @@ -82,8 +82,9 @@ describe('adminRoleFlag', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() }) - it('should not claim membership the user lacks', () => { + it('transmits resolved-mapping authority: member true, non-member false', () => { + expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() expect(adminRoleFlag({ adminRoleIds: ['x'] }, ['y', 'x'])).toBe(true) - expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBeUndefined() + expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBe(false) }) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts index 5ca5703459..0571df7360 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts @@ -247,7 +247,7 @@ export function daysAgoFromNow(date: Date) { export function adminRoleFlag(user: Pick, adminRoleIds: string[]): boolean | undefined { if (adminRoleIds.length === 0) return undefined - return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) ? true : undefined + return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) ?? false } export function isGitbeakerNotFound(error: unknown): error is GitbeakerRequestError { diff --git a/apps/server-nestjs/test/gitlab.e2e-spec.ts b/apps/server-nestjs/test/gitlab.e2e-spec.ts index 846c2ce2bb..01ca7c599e 100644 --- a/apps/server-nestjs/test/gitlab.e2e-spec.ts +++ b/apps/server-nestjs/test/gitlab.e2e-spec.ts @@ -446,7 +446,7 @@ describeWithGitLab('GitlabService (e2e)', () => { } }) - it('reprovisioning does not strip a GitLab instance admin flag set outside the console', async () => { + it('reprovisioning applies revoked role membership as instance-flag demotion', async () => { if (!memberGitlabId) throw new Error('GitLab member was not created') const project = await prisma.project.findUniqueOrThrow({ @@ -458,18 +458,12 @@ describeWithGitLab('GitlabService (e2e)', () => { const before = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) expect(before.is_admin).toBe(false) - // Reproduction of the reported incident: the flag is granted on the GitLab - // instance directly, then the project is reprovisioned from the console. await gitlabClient.Users.edit(memberGitlabId, { admin: true }) - const reprovisioned = await prisma.project.findUniqueOrThrow({ - where: { id: testProjectId }, - select: projectSelect, - }) - await expect(eventEmitter.emitAsync('project.upsert', reprovisioned)).resolves.not.toThrow() + await expect(eventEmitter.emitAsync('project.upsert', project)).resolves.not.toThrow() const after = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) - expect(after.is_admin).toBe(true) + expect(after.is_admin).toBe(false) }, GITLAB_SYNC_TIMEOUT) }) From 42b8a2262641c9b759720e0aeca5d12e68476082 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Thu, 1 Oct 2026 13:58:36 +0200 Subject: [PATCH 6/7] revert(gitlab): drop /admin from the instance-flag mapping Platform /admin is the bootstrap group; the instance admin flag is governed by /console/admin membership. Union was a misdiagnosis built on an unverified hypothesis about the reported account. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I3b7e9d1f5a2c8e4b6d0f9a3c5e7b1d8f2a4c6e9b --- .../src/modules/gitlab/gitlab-client.service.ts | 8 ++------ .../src/modules/gitlab/gitlab.service.spec.ts | 1 + 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index d4d154d922..9295d21181 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts @@ -461,17 +461,13 @@ export class GitlabClientService { ) { const existing = await this.getUserByEmail(user.email) - const gitlabUser = existing ?? await this.createUser({ - ...user, - externUid: user.email, - provider: 'openid_connect', - }) - const editOptions: EditUserOptions = { ...user, externUid: user.email, provider: 'openid_connect', } + const gitlabUser = existing ?? await this.createUser(editOptions) + if (existing) { const hasDiff = Object.entries(editOptions).some(([key, value]) => { if (value === undefined) return false diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts index 299b431fdc..40555f17e2 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts @@ -444,6 +444,7 @@ describe('gitlabService', () => { }) datastore.getAdminRolesByOidcGroups.mockResolvedValue([ { id: 'admin-role-id', oidcGroup: '/console/admin' }, + { id: 'platform-admin-role-id', oidcGroup: '/admin' }, { id: 'auditor-role-id', oidcGroup: '/console/readonly' }, ]) From 36a8200d1dbfe7a224d4cced25d9da19a2c0f67e Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Fri, 2 Oct 2026 10:44:45 +0200 Subject: [PATCH 7/7] fix(gitlab): never demote instance flags when a mapped role is absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The head state reintroduced #2804: adminRoleFlag emitted false whenever a project mapped an admin/auditor role the user did not hold, and upsertUser forwarded it to Users.edit, demoting the GitLab instance admin on every reconcile. Role removal is expressed by group membership only — ensureGroupMemberAccessLevel / purgeOrphanMembers handle the group side — so the flag mapping is tri-state again: true promotes, absence of the claim (undefined) leaves the instance flag untouched, and the upsertUser boundary strips falsy admin/auditor for any future caller. Restores the e2e instance-admin-survival regression. Fixs #2804 Co-authored-by: Automata --- .../gitlab/gitlab-client.service.spec.ts | 6 +-- .../modules/gitlab/gitlab-client.service.ts | 6 ++- .../src/modules/gitlab/gitlab.service.spec.ts | 54 +++++++++++++++++-- .../src/modules/gitlab/gitlab.utils.spec.ts | 5 +- .../src/modules/gitlab/gitlab.utils.ts | 7 ++- apps/server-nestjs/test/gitlab.e2e-spec.ts | 12 +++-- 6 files changed, 76 insertions(+), 14 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts index 07a6f2f20b..113ec6c698 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts @@ -538,7 +538,7 @@ describe('gitlab-client', () => { })) }) - it('should demote an existing admin when the reconcile computes admin false', async () => { + it('should not demote an existing admin when the reconcile computes admin false', async () => { const consoleUser = { id: 'u1', email: 'owner@example.com', firstName: 'Owner', lastName: 'User' } const gitlabUser = { email: consoleUser.email, @@ -552,8 +552,8 @@ describe('gitlab-client', () => { await service.upsertUser({ ...gitlabUser, admin: false }, { cpnUserId: consoleUser.id }) - expect(gitlabApi.Users.edit).toHaveBeenCalledWith(1000, expect.objectContaining({ - admin: false, + expect(gitlabApi.Users.edit).toHaveBeenCalledWith(1000, expect.not.objectContaining({ + admin: expect.anything(), })) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index 9295d21181..5777c21258 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts @@ -461,8 +461,12 @@ export class GitlabClientService { ) { const existing = await this.getUserByEmail(user.email) + // Boundary: no reconcile path may ever demote an instance-level flag. GitLab's + // Users.edit cannot un-set admin/auditor — a falsy value strips them (#2804). + // Removal of a role is expressed by removing the user from the mapped group, + // never by transmitting the flag here. const editOptions: EditUserOptions = { - ...user, + ...Object.fromEntries(Object.entries(user).filter(([, v]) => v !== false)), externUid: user.email, provider: 'openid_connect', } diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts index 40555f17e2..d09fd2310a 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.service.spec.ts @@ -465,19 +465,67 @@ describe('gitlabService', () => { await service.handleUpsert(project) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: false }), + expect.objectContaining({ email: 'admin@example.com', admin: true, auditor: undefined }), expect.objectContaining({ cpnUserId: 'u1' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'auditor@example.com', admin: false, auditor: true }), + expect.objectContaining({ email: 'auditor@example.com', admin: undefined, auditor: true }), expect.objectContaining({ cpnUserId: 'u2' }), ) expect(gitlab.upsertUser).toHaveBeenCalledWith( - expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: false }), + expect.objectContaining({ email: 'owner@example.com', admin: true, auditor: undefined }), expect.objectContaining({ cpnUserId: 'o1' }), ) }) + it('should not demote the instance admin flag when the user loses the mapped admin role', async () => { + const project = makeProjectWithDetails({ + owner: { id: 'o1', email: 'owner@example.com', firstName: 'Owner', lastName: 'User', adminRoleIds: ['admin-role-id'] }, + members: [ + // held /console/admin before, now only carries an unrelated role id + { user: { id: 'u1', email: 'former-admin@example.com', firstName: 'Former', lastName: 'Admin', adminRoleIds: ['unrelated-role-id'] }, roleIds: [] }, + ], + }) + const group = makeGroupSchema({ id: 123, name: 'project-1', path: 'project-1', full_path: 'forge/console/project-1', full_name: 'forge/console/project-1', parent_id: 1 }) + + datastore.getAdminPluginConfig.mockImplementation(async (_pluginName: string, key: string) => { + if (key === 'adminGroupPath') return '/console/admin' + if (key === 'auditorGroupPath') return '/console/readonly' + return null + }) + datastore.getAdminRolesByOidcGroups.mockResolvedValue([ + { id: 'admin-role-id', oidcGroup: '/console/admin' }, + { id: 'auditor-role-id', oidcGroup: '/console/readonly' }, + ]) + + gitlab.getOrCreateProjectSubGroup.mockResolvedValue(group) + gitlab.getGroupMembers.mockResolvedValue([]) + gitlab.upsertUser.mockImplementation(async (user) => { + return makeExpandedUserSchema({ + id: faker.number.int(), + email: user.email, + username: user.email.split('@')[0], + name: user.name, + }) + }) + gitlab.getRepos.mockReturnValue((async function* () { })()) + gitlab.upsertProjectMirrorRepo.mockResolvedValue(makeProjectSchema({ id: 1, name: 'mirror', path: 'mirror', path_with_namespace: 'forge/console/project-1/mirror', empty_repo: false })) + gitlab.getOrCreateMirrorPipelineTriggerToken.mockResolvedValue(makePipelineTriggerToken()) + + await service.handleUpsert(project) + + // Losing the role must produce NO admin claim (undefined), never admin: false — + // the instance flag is untouched; losing access is expressed by group membership only. + expect(gitlab.upsertUser).toHaveBeenCalledWith( + expect.objectContaining({ email: 'former-admin@example.com', admin: undefined }), + expect.objectContaining({ cpnUserId: 'u1' }), + ) + expect(gitlab.upsertUser).not.toHaveBeenCalledWith( + expect.objectContaining({ email: 'former-admin@example.com', admin: false }), + expect.anything(), + ) + }) + it('should resolve auditor flags from the multi-path auditor defaults', async () => { const project = makeProjectWithDetails({ members: [ diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts index 74b0e657bd..14ba5d3495 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts @@ -82,9 +82,8 @@ describe('adminRoleFlag', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() }) - it('transmits resolved-mapping authority: member true, non-member false', () => { - expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() + it('never claims membership the user lacks: absence is no claim, not revocation', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, ['y', 'x'])).toBe(true) - expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBe(false) + expect(adminRoleFlag({ adminRoleIds: ['z'] }, ['y', 'x'])).toBeUndefined() }) }) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts index 0571df7360..5c628a160a 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts +++ b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts @@ -246,8 +246,13 @@ export function daysAgoFromNow(date: Date) { } export function adminRoleFlag(user: Pick, adminRoleIds: string[]): boolean | undefined { + // Never emit a revocation: a role mapped-but-not-held means the user must simply be + // removed from the corresponding group, not demoted at the instance level. GitLab's + // Users.edit cannot un-set admin/auditor, so a falsy value would strip the flag even + // when it was granted for reasons outside this project's mapping (#2804). + // Absence of membership is `undefined` (no claim), `true` stays promotion. if (adminRoleIds.length === 0) return undefined - return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) ?? false + return user.adminRoleIds?.some(id => adminRoleIds.includes(id)) ? true : undefined } export function isGitbeakerNotFound(error: unknown): error is GitbeakerRequestError { diff --git a/apps/server-nestjs/test/gitlab.e2e-spec.ts b/apps/server-nestjs/test/gitlab.e2e-spec.ts index 01ca7c599e..846c2ce2bb 100644 --- a/apps/server-nestjs/test/gitlab.e2e-spec.ts +++ b/apps/server-nestjs/test/gitlab.e2e-spec.ts @@ -446,7 +446,7 @@ describeWithGitLab('GitlabService (e2e)', () => { } }) - it('reprovisioning applies revoked role membership as instance-flag demotion', async () => { + it('reprovisioning does not strip a GitLab instance admin flag set outside the console', async () => { if (!memberGitlabId) throw new Error('GitLab member was not created') const project = await prisma.project.findUniqueOrThrow({ @@ -458,12 +458,18 @@ describeWithGitLab('GitlabService (e2e)', () => { const before = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) expect(before.is_admin).toBe(false) + // Reproduction of the reported incident: the flag is granted on the GitLab + // instance directly, then the project is reprovisioned from the console. await gitlabClient.Users.edit(memberGitlabId, { admin: true }) - await expect(eventEmitter.emitAsync('project.upsert', project)).resolves.not.toThrow() + const reprovisioned = await prisma.project.findUniqueOrThrow({ + where: { id: testProjectId }, + select: projectSelect, + }) + await expect(eventEmitter.emitAsync('project.upsert', reprovisioned)).resolves.not.toThrow() const after = z.object({ is_admin: z.boolean() }).parse(await gitlabClient.Users.show(memberGitlabId)) - expect(after.is_admin).toBe(false) + expect(after.is_admin).toBe(true) }, GITLAB_SYNC_TIMEOUT) })