Repository navigation
feat(server-nestjs): consume adminRole events in keycloak and gitlab modules - #2749
Conversation
|
🤖 Hey ! A preview of the application is available at : https://console-pr-2749.dso.cpin-hp.numerique-interieur.fr Please be patient, deployment may take a few minutes. |
server-nestjs already excludes OIDC-backed role ids from member counts, while the frozen legacy handler computes NaN for them, fails the response contract and turns GET /api/v1/admin/roles/member-counts into a 500. The route flip depends on the adminRole event bridge (#2749) landing first. Refs #2781 Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I7d36cd930692f7b476d37909077bb7166a6a6964
server-nestjs already excludes OIDC-backed role ids from member counts, while the frozen legacy handler computes NaN for them, fails the response contract and turns GET /api/v1/admin/roles/member-counts into a 500. The route flip depends on the adminRole event bridge (#2749) landing first. Refs #2781 Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I7d36cd930692f7b476d37909077bb7166a6a6964
shikanime
left a comment
There was a problem hiding this comment.
Verdict : Changements demandés
Le pont GitLab est fidèle au legacy et bien testé, mais le pont Keycloak pour adminRole.delete relit la base à l'intérieur de la transaction émettrice et ne révoque donc jamais les membres — bloquant, il faut traiter le payload de delete ou émettre après commit — et le module se base sur un main pré-#2792 dont il duplique le split d'auditor paths. Bel effort otherwise : trois suites de tests dont la chaîne event→consommateurs, exactement le garde-fou qui manquait pour débloquer #2738.
5a1f4a9 to
0e5b588
Compare
server-nestjs already excludes OIDC-backed role ids from member counts, while the frozen legacy handler computes NaN for them, fails the response contract and turns GET /api/v1/admin/roles/member-counts into a 500. The route flip depends on the adminRole event bridge (#2749) landing first. Refs #2781 Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I7d36cd930692f7b476d37909077bb7166a6a6964
StephaneTrebel
left a comment
There was a problem hiding this comment.
Les consommateurs adminRole.* présents dans le diff relient bien les deux ponts demandés et le payload de suppression est maintenant consommé sans relecture du rôle supprimé. La branche reste toutefois à 3 commits de main et deux fils de revue pertinents restent ouverts : rebaser, intégrer #2792 puis réconcilier les fils et les contrôles avant nouvelle revue.
server-nestjs already excludes OIDC-backed role ids from member counts, while the frozen legacy handler computes NaN for them, fails the response contract and turns GET /api/v1/admin/roles/member-counts into a 500. The route flip depends on the adminRole event bridge (#2749) landing first. Refs #2781 Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I7d36cd930692f7b476d37909077bb7166a6a6964
shikanime
left a comment
There was a problem hiding this comment.
Comment s'articule la revue
L'implémentation suit bien les patterns existants : la réconciliation Keycloak repasse par ensureAdminRoleGroup (idempotent, avec relecture en base) et la révocation interroge le groupe via getGroupByPath sans le créer, avec tolérance 404. Le pont GitLab reflète fidèlement les fonctions plugin legacy, mais le chemin delete s'appuie sur upsertUser qui crée les comptes GitLab absents — invariant déjà corrigé sur la PR #2807. Par ailleurs, la comparaison du chemin admin en égalité stricte diverge du reste du même fichier qui passe par parseGroupPaths, ce qui casse silencieusement les configurations multi-chemins. Le reste (payload partagé AdminRoleEventPayload, specs ajoutées plutôt que supprimées, imports minimaux, pas de cast as) est conforme ; le flag draft est cohérent avec le statut du chantier.
StephaneTrebel
left a comment
There was a problem hiding this comment.
Verdict : changements demandés. Le rebase et la révocation Keycloak depuis le payload ont bien corrigé les points précédents, et les specs de chaîne sont utiles. Deux régressions restent toutefois dans le pont GitLab : une suppression peut provisionner un utilisateur GitLab absent, et une configuration admin multi-chemins n'est pas reconnue ; détails inline ci-dessous.
e081d11 to
c808c97
Compare
Traitement de la revue (c808c97)
Gates locaux : vitest gitlab.service.spec (30/30), |
StephaneTrebel
left a comment
There was a problem hiding this comment.
Verdict : changements demandés. Les deux constats fonctionnels précédents sont corrigés : les chemins admin multiples passent par parseGroupPaths, et une révocation n’essaie plus de provisionner les comptes GitLab absents ; les tests et les checks du head sont verts. La branche reste toutefois à 4 commits derrière main : rebase et relance des checks requis avant validation. Ledger #2723 : specs et parité sont couvertes par le diff ; le choix de conception n’est pas encore documenté dans l’issue comme le demande son premier critère.
c808c97 to
f34ad1f
Compare
433d9be to
d89b285
Compare
…modules Design: bridge adminRole.upsert/delete events emitted by admin-role.service and user migration to keycloak (group membership reconcile via ensureAdminRoleGroup, delete revokes from payload members) and gitlab (admin/auditor flags from the role oidc group, mirrors legacy upsertAdminRole/deleteAdminRole plugin functions). Shared payload type AdminRoleEventPayload in app-events.service.ts. Rebased onto current main; drops the stray packages/ts-config artifact flagged in review. Refs #2723 Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I5e020c8ea72c6ba782e73c62e29e9aad6a6a6964
…Lab accounts on revoke Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6b5e959fc804f5ea7d4cd9e06bfebabc6a6a6964
Co-authored-by: Automata <automata@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I3ac3c5df9f778e623f92e8383eb4ec906a6a6964
d89b285 to
9f4be27
Compare
|
StephaneTrebel
left a comment
There was a problem hiding this comment.
Verdict : approuvée. Les correctifs GitLab sont conformes à la décision documentée dans #2723 : les chemins multiples sont pris en charge et la console reste autoritaire sur le provisioning OIDC lors d’une révocation. Le pont couvre les événements upsert/delete dans Keycloak et GitLab, avec des specs de chaîne/service ; la branche est à jour sur main et tous les checks, dont Sonar, sont verts.

0 New Issues
0 Fixed Issues
0 Accepted Issues
Issues liées
Blocks the cut-over of #2738, #2495, #2496, #2740.
Quel est le comportement actuel ?
admin-role.service.tsalready emits these events on main, but no@OnEventconsumer exists — at cutover the Keycloak/GitLab group sync would be silently lost. Also unblocks #2738 (user migration emitsadminRole.upsertper impacted role).Quel est le nouveau comportement ?
Bridge
emitAsync('adminRole.upsert' | 'adminRole.delete')to actual consumers in server-nestjs:ensureAdminRoleGroup)upsertAdminRole/deleteAdminRoleplugin functions)Shared payload type
AdminRoleEventPayloadinapp-events.service.ts.Signed-off-by: Shikanime Deva 22115108+shikanime@users.noreply.github.com