Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ConfigType } from '@nestjs/config'
import type { DeepMockProxy } from 'vitest-mock-extended'
import { DISABLED } from '@cpn-console/shared'
import { DISABLED, PROJECT_PERMS } from '@cpn-console/shared'
import { Test } from '@nestjs/testing'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { mockDeep } from 'vitest-mock-extended'
Expand All @@ -13,6 +13,9 @@ import { makeProject } from './observability-testing.utils'
import { ENABLED_PLUGIN_KEY } from './observability.constants'
import { ObservabilityService } from './observability.service'

const PROD_STAGE = { name: 'prod' } as const
const HPROD_STAGE = { name: 'hprod' } as const

describe('observabilityService', () => {
let service: ObservabilityService
let datastore: DeepMockProxy<ObservabilityDatastoreService>
Expand Down Expand Up @@ -106,4 +109,44 @@ describe('observabilityService', () => {
expect(client.deleteProjectConfig).not.toHaveBeenCalled()
})
})

describe('syncKeycloakGroups', () => {
it('does not remove existing members from either pair when both stages exist (regression: grafana access loss)', async () => {
const project = makeProject({
ownerId: 'owner-1',
members: [{
roleIds: ['role-ro'],
user: { id: 'user-ro', email: 'ro@test.com' },
}],
roles: [{ id: 'role-ro', permissions: PROJECT_PERMS.LIST_ENVIRONMENTS, oidcGroup: '', type: 'managed' }],
environments: [
{ id: 'env-prod', name: 'prod', stage: PROD_STAGE },
{ id: 'env-hprod', name: 'dev', stage: HPROD_STAGE },
],
})

const pairs: Record<string, { id: string, name: string, path: string, members: { id: string }[] }> = {
'hprod-RW': { id: 'g-hprod-rw', name: 'hprod-RW', path: '/test-project/grafana/hprod-RW', members: [{ id: 'owner-1' }] },
'hprod-RO': { id: 'g-hprod-ro', name: 'hprod-RO', path: '/test-project/grafana/hprod-RO', members: [{ id: 'owner-1' }, { id: 'user-ro' }] },
'prod-RW': { id: 'g-prod-rw', name: 'prod-RW', path: '/test-project/grafana/prod-RW', members: [{ id: 'owner-1' }] },
'prod-RO': { id: 'g-prod-ro', name: 'prod-RO', path: '/test-project/grafana/prod-RO', members: [{ id: 'owner-1' }, { id: 'user-ro' }] },
}
keycloak.getSubGroups.mockImplementation((parentId: string) =>
(async function* () {
if (parentId === 'group-1') yield { id: 'g-grafana', name: 'grafana' }
})(),
)
keycloak.getOrCreateSubGroupByName.mockImplementation((_parentId: string, name: string) =>
Promise.resolve(pairs[name] ?? { id: `sub-${name}`, name, path: `/test-project/grafana/${name}` }),
)
keycloak.getGroupMembers.mockImplementation((groupId: string) =>
Promise.resolve(Object.values(pairs).find(g => g.id === groupId)?.members ?? []),
)

await service.handleUpsert(project)

expect(keycloak.removeUserFromGroup).not.toHaveBeenCalled()
expect(keycloak.addUserToGroup).not.toHaveBeenCalled()
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('getListPerms', () => {
expect(perms.prod.view).toContain('owner-1')
})

it('assigns to prod bucket when a prod environment exists', () => {
it('fills the prod bucket when only a prod environment exists', () => {
const project = makeProject({
ownerId: 'owner-1',
environments: [{ id: 'env-1', name: 'prod-env', stage: PROD_STAGE }],
Expand All @@ -36,7 +36,7 @@ describe('getListPerms', () => {
expect(perms['hors-prod'].edit).not.toContain('owner-1')
})

it('assigns to hors-prod bucket when no prod environment', () => {
it('fills the hors-prod bucket when only non-prod environments exist', () => {
const project = makeProject({
ownerId: 'owner-1',
environments: [{ id: 'env-1', name: 'dev-env', stage: HPROD_STAGE }],
Expand All @@ -46,6 +46,42 @@ describe('getListPerms', () => {
expect(perms.prod.edit).not.toContain('owner-1')
})

it('assigns to BOTH buckets when both stages exist (legacy parity)', () => {
const project = makeProject({
ownerId: 'owner-1',
environments: [
{ id: 'env-prod', name: 'prod', stage: PROD_STAGE },
{ id: 'env-hprod', name: 'hprod', stage: HPROD_STAGE },
],
})
const perms = getListPerms(project)
expect(perms.prod.edit).toContain('owner-1')
expect(perms['hors-prod'].edit).toContain('owner-1')
expect(perms.prod.view).toContain('owner-1')
expect(perms['hors-prod'].view).toContain('owner-1')
})

it('assigns a RO member to BOTH buckets when both stages exist (legacy parity)', () => {
const roRoleId = 'role-ro'
const project = makeProject({
ownerId: 'owner-1',
members: [{
roleIds: [roRoleId],
user: { id: 'user-ro', email: 'ro@test.com' },
}],
roles: [{ id: roRoleId, permissions: PROJECT_PERMS.LIST_ENVIRONMENTS, oidcGroup: '', type: 'managed' }],
environments: [
{ id: 'env-prod', name: 'prod', stage: PROD_STAGE },
{ id: 'env-hprod', name: 'hprod', stage: HPROD_STAGE },
],
})
const perms = getListPerms(project)
expect(perms.prod.view).toContain('user-ro')
expect(perms['hors-prod'].view).toContain('user-ro')
expect(perms.prod.edit).not.toContain('user-ro')
expect(perms['hors-prod'].edit).not.toContain('user-ro')
})

it('grants ro but not rw to a member with only LIST_ENVIRONMENTS', () => {
const roRoleId = 'role-ro'
const project = makeProject({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,31 @@ export function getListPerms(project: ProjectWithDetails): ListPerms {
prod: { edit: [], view: [] },
}

const hasProd = project.environments.some(e => isProdStage(e.stage))
const hasHprod = project.environments.some(e => e.stage?.name != null && !isProdStage(e.stage))
for (const userId of projectUserIds) {
const { ro, rw } = resolveUserPerms(project, rolesById, userId)
const hasProd = project.environments.some(e => isProdStage(e.stage))
const bucket = hasProd ? perms.prod : perms['hors-prod']
if (rw && !bucket.edit.includes(userId)) bucket.edit.push(userId)
if (ro && !bucket.view.includes(userId)) bucket.view.push(userId)
const userPerms = resolveUserPerms(project, rolesById, userId)
// Legacy parity (console-plugin-observability): the permission roster goes
// to EVERY stage bucket the project exposes — prod if a prod environment
// exists AND hors-prod if a non-prod one does. Reconcile removes anyone
// absent from the desired list, so a single-bucket fill would wipe the
// other pair's Keycloak memberships on the first sync.
if (hasProd) addUserToBucket(perms.prod, userId, userPerms)
Comment thread
shikanime marked this conversation as resolved.
if (hasHprod) addUserToBucket(perms['hors-prod'], userId, userPerms)
}

return perms
}

function addUserToBucket(
bucket: ListPerms['prod'],
userId: string,
{ ro, rw }: { ro: boolean, rw: boolean },
): void {
if (rw && !bucket.edit.includes(userId)) bucket.edit.push(userId)
if (ro && !bucket.view.includes(userId)) bucket.view.push(userId)
}

function resolveUserPerms(
project: ProjectWithDetails,
rolesById: Record<string, ProjectWithDetails['roles'][number]>,
Expand Down
Loading