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..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,26 @@ describe('gitlab-client', () => { })) }) - it('should not disable existing admin flag when enabling auditor flag', 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, + 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 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 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 299b431fdc..d09fd2310a 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' }, ]) @@ -464,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 58055b670d..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,8 +82,8 @@ describe('adminRoleFlag', () => { expect(adminRoleFlag({ adminRoleIds: ['x'] }, [])).toBeUndefined() }) - it('should match membership against any resolved role id', () => { + 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 63853a9123..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)) + 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 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 },