refactor(admin-roles): migrate from server by shikanime · Pull Request #2371 · cloud-pi-native/console

shikanime · 2026-07-27T09:23:05Z

Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964

Issues liées

Issues numéro: #2204
Reopened Pull Request: #2205

Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

github-actions · 2026-07-27T09:59:15Z

🤖 Hey !

The security scan report for the current pull request is available here.

github-actions · 2026-07-27T10:09:01Z

🤖 Hey !

The security scan report for the current pull request is available here.

github-actions · 2026-07-27T10:25:47Z

🤖 Hey !

The security scan report for the current pull request is available here.

shikanime · 2026-07-27T13:45:12Z

+      return createAdminRole(tx, body.name)
+    })
+
+    await this.eventEmitter.emitAsync('adminRole.upsert', {


Review: admin-roles migration (PR #2371)

Verified against head b2642e0 — typecheck (tsc --noEmit) clean, admin-role.service.spec.ts passes (1/1), eslint clean on all touched files. The module structure, DI wiring, telemetry, and Zod validation follow the established NestJS patterns correctly.

Blocker — emitted adminRole.* events have no consumer (silent feature regression): The service emits adminRole.upsert (service.ts:45, :124) and adminRole.delete (:169) via EventEmitter2. In the legacy server, deleteRole/createRole/patchRoles call hook.adminRole.upsert(...) / hook.adminRole.delete(...) (apps/server/src/resources/admin-role/business.ts:42,66,94), which is what drives the Keycloak OIDC-group sync (plugins/keycloak/src/functions.ts:246,292) and GitLab admin/auditor group sync (plugins/gitlab/src/functions.ts:238,277). In server-nestjs, every equivalent event is consumed by an @OnEvent handler that bridges into the plugin hook system (e.g. keycloak.service.ts:28 project.upsert, gitlab.service.ts:63). There is no @OnEvent('adminRole.upsert')/('adminRole.delete') handler anywhere in server-nestjs, and @cpn-console/hooks is not imported by the admin-role module. Net effect: once the legacy router is cut over, creating / patching / deleting an admin role will no longer sync the role's members into Keycloak/GitLab groups. The event is emitted and then dropped. Either add the @OnEvent bridge handlers (mirroring the project bridge) or document why the role hooks are intentionally not yet wired. This must be resolved before the legacy router is removed.

Warning — dead patch position-integrity guard: service.ts:92 throws only when positionsAvailable.length && positionsAvailable.length !== dbRoles.length. Because positionsAvailable is only populated for roles present in the DB (matchingRole found), and the legacy code uses the identical expression, the check is effectively unreachable for the normal partial-patch path (you only ever push positions that correspond to existing roles, so the array length always equals the number of matched DB roles). Flagging for parity-awareness; it reproduces the legacy behavior but is dead in practice.

Warning — delete side-effects before DB delete (transaction ordering): service.ts:151 loads the role, then :169 emits adminRole.delete inside the transaction before the role row is deleted (:185). Minor: if a future listener runs side-effects against a role that has just been (or is about to be) removed, ordering matters. Legacy does the hook before the delete too, so this is parity-preserving — just confirm it is intended.

Minor — unused testing-utils exports: admin-role-testing.utils.ts exports AdminRoleContract (:5), AdminRoleResponse (:6), and makeAdminRoleMember (:27) that nothing imports (the spec only uses makeAdminRole/makeCreateAdminRoleBody). Either wire them into the (currently single) spec or drop them — they add surface with no consumer. Low priority.

Note — list permission: controller.ts:20 intentionally leaves ListRoles unguarded (legacy-client behavior), matching the legacy router. Fine, but the TODO should record which legacy client path depends on it so it can be removed later.

Verdict: REQUEST CHANGES (on the emitted-event bridge). The migration is otherwise clean and ready to land once the adminRole.* → plugin-hook bridge is added or explicitly deferred with a tracked follow-up.

shikanime · 2026-07-27T13:45:35Z

+        })
+      }
+
+      if (positionsAvailable.length && positionsAvailable.length !== dbRoles.length) {


Dead position-integrity guard (parity note): positionsAvailable is only populated for roles found in dbRoles, so its length always equals the number of matched DB roles. The !== dbRoles.length branch is effectively unreachable on the normal partial-patch path. Reproduces legacy behavior (business.ts:39) — harmless, but worth a comment or removal so it is not mistaken for an active invariant.

Ben, surtout, il est où le test unitaire qui valide/invalide ce if ?

shikanime · 2026-07-27T13:45:36Z

+        select: { id: true, adminRoleIds: true },
+      })
+
+      await this.eventEmitter.emitAsync('adminRole.delete', {


adminRole.delete is emitted inside the transaction (before the row is deleted at :185). Legacy does the same (business.ts:94), so this is parity-preserving — but if a future @OnEvent(adminRole.delete) consumer reads the role row, ordering matters. Confirm intended.

D'ailleurs faudrait clarifier si on veut adminRole.delete (action de supprimer) ou adminRole.deleted (résultat de la suppression).

En termes d'intention et de conséquences, ça peut effectivement être problématique.

On a fait une carto des flux d'évènement d'ailleurs ? On se fait plutôt ça dans le ticket de carto des plugins ( #2181 )

shikanime · 2026-07-27T13:45:37Z

+import type { AdminRoleService } from './admin-role.service'
+import type { CreateAdminRoleBody, PatchAdminRolesBody } from './admin-role.utils'
+
+export type AdminRoleContract = Parameters<AdminRoleService['patch']>[0][number]


Unused testing-utils exports: AdminRoleContract, AdminRoleResponse, and makeAdminRoleMember (lines 5, 6, 27) have no importers — the spec only uses makeAdminRole / makeCreateAdminRoleBody. Drop them or wire them into the spec to avoid dead surface.

shikanime · 2026-07-27T13:45:38Z

+  @UseGuards(UserGuard)
+  // TODO: ListRoles is intentionally not protected by admin permission because of
+  // certain behaviours of the legacy client
+  // @RequireAdminPermission('ListRoles')


ListRoles intentionally unguarded (legacy-client behavior) — matches the legacy router. Low priority: note which legacy client path depends on this so the TODO can be closed later.

Ça demande un ticket, à mon avis, car c'est suffisament atomique, comme considération 🙂

shikanime

Verdict: REQUEST CHANGES

Migration of admin-roles from the legacy Fastify server into server-nestjs is well-structured (typecheck clean, admin-role.service.spec.ts passes, eslint clean). One blocker must be resolved before the legacy router is cut over.

Blocker — emitted adminRole.* events are never consumed (silent Keycloak/GitLab sync regression).
The service emits adminRole.upsert / adminRole.delete (service.ts:45, :124, :169) via EventEmitter2, but no @OnEvent('adminRole.upsert'|'adminRole.delete') handler exists in server-nestjs and @cpn-console/hooks is not imported by the module. In the legacy server, those same flows call hook.adminRole.upsert / hook.adminRole.delete (business.ts:42,66,94), which drive the Keycloak OIDC-group sync and GitLab admin/auditor group sync. Every other migrated entity bridges its event into the plugin hook system via an @OnEvent handler (e.g. keycloak.service.ts:28, gitlab.service.ts:63). Without that bridge, once the legacy route is removed, admin-role CRUD will silently stop syncing member groups. Add the @OnEvent bridge handlers (mirroring the project bridge) or explicitly defer with a tracked follow-up before cutover.

Warnings (parity-preserving, confirm): dead patch position-integrity guard (service.ts:92) — only reachable in the legacy-incompatible all-roles path; adminRole.delete emitted before the row delete inside the transaction (service.ts:169 vs :185) — legacy does the same.

Minor: unused testing-utils exports (AdminRoleContract, AdminRoleResponse, makeAdminRoleMember in admin-role-testing.utils.ts:5,6,27).

Full detail in the inline comments.

github-actions · 2026-07-27T14:42:46Z

🤖 Hey !

The security scan report for the current pull request is available here.

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964 Signed-off-by: Shikanime Deva <william.phetsinorath@shikanime.studio>

cloud-pi-native-sonarqube · 2026-07-28T13:33:12Z

Quality Gate passed

Issues

Measures

Project ID: cloud-pi-native_console_AYsa46O31ebDtzs-pYyZ

View in SonarQube

github-actions · 2026-07-28T13:35:39Z

🤖 Hey !

The security scan report for the current pull request is available here.

StephaneTrebel

Rien qui me choque, mais les questions évoquées me semblent pertinentes

StephaneTrebel · 2026-07-30T08:18:23Z

+  @UseGuards(UserGuard)
+  // TODO: ListRoles is intentionally not protected by admin permission because of
+  // certain behaviours of the legacy client
+  // @RequireAdminPermission('ListRoles')


Ça demande un ticket, à mon avis, car c'est suffisament atomique, comme considération 🙂

StephaneTrebel · 2026-07-30T08:20:43Z

+        })
+      }
+
+      if (positionsAvailable.length && positionsAvailable.length !== dbRoles.length) {


Ben, surtout, il est où le test unitaire qui valide/invalide ce if ?

StephaneTrebel · 2026-07-30T08:23:32Z

+        select: { id: true, adminRoleIds: true },
+      })
+
+      await this.eventEmitter.emitAsync('adminRole.delete', {


D'ailleurs faudrait clarifier si on veut adminRole.delete (action de supprimer) ou adminRole.deleted (résultat de la suppression).

En termes d'intention et de conséquences, ça peut effectivement être problématique.

On a fait une carto des flux d'évènement d'ailleurs ? On se fait plutôt ça dans le ticket de carto des plugins ( #2181 )

shikanime added this to the 9.24.0 milestone Jul 27, 2026

shikanime added the enhancement New feature or request label Jul 27, 2026

shikanime self-assigned this Jul 27, 2026

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from e25c2f2 to 3218292 Compare July 27, 2026 09:54

github-actions Bot added the built label Jul 27, 2026

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 3218292 to 3044f04 Compare July 27, 2026 10:04

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 3044f04 to b2642e0 Compare July 27, 2026 10:10

shikanime requested review from KepoParis and StephaneTrebel July 27, 2026 10:11

shikanime commented Jul 27, 2026

View reviewed changes

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from b2642e0 to b5725e4 Compare July 27, 2026 14:36

refactor(admin-roles): migrate from server

de5e877

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964 Signed-off-by: Shikanime Deva <william.phetsinorath@shikanime.studio>

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from b5725e4 to de5e877 Compare July 28, 2026 13:27

StephaneTrebel reviewed Jul 30, 2026

View reviewed changes

Uh oh!

Conversation

shikanime commented Jul 27, 2026

Issues liées

Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Review: admin-roles migration (PR #2371)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shikanime left a comment

Choose a reason for hiding this comment

Verdict: REQUEST CHANGES

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

cloud-pi-native-sonarqube Bot commented Jul 28, 2026

Quality Gate passed

Issues

Measures

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

StephaneTrebel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants