refactor(user-tokens): migrate from server by shikanime · Pull Request #2279 · cloud-pi-native/console

shikanime · 2026-06-30T09:42:04Z

Issues liées

Issues numéro: #1889

Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

github-actions · 2026-07-01T13:39:37Z

🤖 Hey !

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

github-actions · 2026-07-16T16:10:00Z

🤖 Hey !

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

github-actions · 2026-07-21T12:27:05Z

🤖 Hey !

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

github-actions · 2026-07-23T10:17:43Z

🤖 Hey !

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

github-actions · 2026-07-23T10:18:09Z

🤖 Hey !

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

github-actions · 2026-07-23T11:46:00Z

🤖 Hey !

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

github-actions · 2026-07-23T12:36:15Z

🤖 Hey !

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

github-actions · 2026-07-23T12:46:57Z

🤖 Hey !

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

shikanime · 2026-07-27T08:53:03Z

Review: REQUEST CHANGES (posted as comment — GitHub does not allow the author to submit a formal review on their own PR).

Contract regressions in the token migration:

BLOCKER B1 — user-tokens.controller.ts:37 and admin-token.controller.ts:31: @Delete(':tokenId') uses bare @Param('tokenId') with no ParseUUIDPipe/zod pipe. Legacy contract enforced z.string().uuid() (400 on bad UUID); now a non-UUID id reaches Prisma → P2023 → unhandled 500. Fix: @Param('tokenId', ParseUUIDPipe) tokenId: string.

BLOCKER B2 — date parsing path (user-tokens.service.ts / admin-token.service.ts): z.coerce.date().parse() throws a raw ZodError (500) on bad input; legacy returned 400. Map ZodError → BadRequestException or route through ZodValidationPipe.

WARNINGS

Use the existing @RequireUserType('human') decorator instead of the hand-rolled userType !== 'human' checks (3×).
withRevoked === 'true' silently coerces invalid query values to false (legacy rejected them).

NITS

unused verifyTokenHash export; needless async on generateTokenPair; commit scope says user-tokens but half the diff is admin-token; confirm the strangler routing.conf is flipped to the NestJS route (or the new code is dead).

Good: token hash never selected/exposed, authz parity preserved, tests are meaningful (25/25).

github-actions · 2026-07-27T09:01:18Z

🤖 Hey !

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

shikanime

Review: #2279 — refactor(user-tokens): migrate from server

Verdict: REQUEST CHANGES (dead crypto export; strong authz parity + tests)

Findings

[blocker] apps/server-nestjs/src/utils/crypto.ts:46 verifyTokenHash is exported but never imported anywhere (verified: zero usages outside its own def + spec). Dead code in a security-sensitive file. Either wire it into the token-login validation path or drop it from this PR.

[warning] admin-token/admin-token.service.ts:58 BigInt(data.permissions) — permissionLevelSchema is z.coerce.string(), so permissions is a string. BigInt("chocapics") throws a raw SyntaxError → 500, not a BadRequestException. Validate with z.coerce.bigint() or wrap in a typed parse. (Latent in the Fastify version too; you're rewriting, so fix it.)

[warning] admin-token.service.ts:39 / user-tokens.service.ts — z.coerce.date().parse("toto") returns Invalid Date (not a ZodError), and isAtLeastTomorrow then yields a confusing "trop courte" message. Add explicit Number.isNaN(date.getTime()) → BadRequestException('Date d\'expiration invalide'). (Aligns with StephaneTrebel's still-open "la date peut aussi être invalide".)

[resolved/non-issue] Items from StephaneTrebel's review:

RequireUserType('human') — the repo has no HUMAN_KIND constant; literal 'human' matches the Prisma enum and existing usage. No change needed.
withRevoked="chocapics" — ZodValidationPipe + CoerceBooleanSchema rejects non-boolean with 400. Already mitigated.

Compliance checklist

Conventional title
Authz preserved: admin routes use RequireAdminPermission; user routes UserGuard + RequireUserType('human') — mirrors Fastify AdminAuthorized / ProjectAuthorized
No secret leakage: hash omitted from all selects/responses
Tests present (service specs + crypto specs + gated e2e)
Remove dead verifyTokenHash export before merge

github-actions · 2026-07-28T12:22:47Z

🤖 Hey !

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

github-actions · 2026-07-28T12:24:49Z

🤖 Hey !

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

github-actions · 2026-07-28T12:26:01Z

🤖 Hey !

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

github-actions · 2026-07-28T12:39:56Z

🤖 Hey !

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

shikanime · 2026-08-04T14:01:13Z

Review: refactor(user-tokens): migrate from server — REQUEST CHANGES

Migrates admin + personal-access tokens from Fastify server to server-nestjs. Storage format kept compatible (unsalted sha256(token) hex), good test coverage (unit + guarded e2e), clean three-layer module structure. One runtime blocker on the admin list endpoint, plus a minor perf/no-test gap.

🔴 blockers

B1 — GET /api/v1/admin/tokens 400s when withRevoked is omitted (default path)
apps/server-nestjs/src/modules/admin-token/admin-token.controller.ts:14

@Query('withRevoked', new ZodValidationPipe(CoerceBooleanSchema)) withRevoked?: boolean

ZodValidationPipe throws BadRequestException on undefined (proven by zod-validation.pipe.spec.ts:94-98, and router-execution-context.js getParamValue runs the pipe with no undefined guard). A named @Query('x') yields undefined when the param is absent, so the plain GET /api/v1/admin/tokens (the documented default) returns 400 instead of the active-token list.
Fix (shortest, matches existing listProjects contract pattern): drop the per-param pipe and validate the whole query object, or give the pipe a .optional() passthrough. Simplest:

@Query(new ZodValidationPipe(z.object({ withRevoked: CoerceBooleanSchema.optional() }))) query: { withRevoked?: boolean }

then call this.service.list(query.withRevoked === true).

🟡 warnings

W1 — admin list does N+1 + unbatched reads, no controller/HTTP e2e
apps/server-nestjs/src/modules/admin-token/admin-token.service.ts:27-33 iterates listAdminTokens results (1 query) — fine, but there is no admin-token.controller.spec.ts and no HTTP e2e (apps/server-nestjs/test/admin-token.e2e-spec.ts only boots AdminTokenModule and calls the service, no supertest). The B1 bug would have been caught by a single HTTP e2e hitting GET /api/v1/admin/tokens without the param. Add one.

🟢 nits

N1 — coverage reporter changed from json to lcov
apps/server-nestjs/vitest.config.ts:18 (reporter: ['text','lcov']). Sonar uses json in the CI workflow ("Run code quality analysis"); dropping it may break Sonar ingestion. Revert json or confirm Sonar is now fed lcov.

N2 — revokeAdminToken uses updateMany by id
apps/server-nestjs/src/modules/admin-token/admin-token-queries.utils.ts:51 works and is owner-authed at the controller, but update would assert the row exists (clearer 404 path). Optional.

Verified OK

Token storage compatibility: generateTokenPair → createHash('sha256').update(password).digest('hex'), identical to apps/server/src/resources/admin-token/business.ts:36 and user/tokens/business.ts:22. Shared between old/new servers. ✅
AdminAuthorized.ListAdminToken / ManageAdminToken exist (packages/shared/src/utils/permissions.ts:107,115); RequireAdminPermission typed against them. ✅
User PAT delete correctly scopes { id, userId } (no cross-user deletion) — e2e proves it. ✅
Hash never returned to client (select omits it; e2e asserts). ✅
ExpirationDateSchema rejects past/today dates; e2e covers. ✅
main.module.ts wires KeycloakModule/ServiceChainModule/UserTokensModule — modules exist. ✅

CI is green, but CI's unit suite never exercises the param pipe on a missing query string, so B1 slipped through. Fix B1 (and ideally add the HTTP e2e from W1), then re-request review.

shikanime · 2026-08-06T12:21:25Z

Review: #2279 — refactor(user-tokens): migrate from server

Verdict: REQUEST CHANGES (reviewer agent)

Re-verified against head c86c0aa86f. The earlier blocker (dead verifyTokenHash export) is resolved — crypto.ts was relocated and the dead export is gone. Authz parity, secret-omission, and test coverage are good.

Two latent trust-boundary validation bugs remain from the Fastify→NestJS rewrite (fix before cutover):

[warning] admin-token.service.ts:53 BigInt(data.permissions) — the @Body is validated by ZodValidationPipe against CreateAdminTokenBodySchema where permissions is z.coerce.string(). A non-numeric value (e.g. "chocapics") passes Zod, then BigInt(...) throws a raw SyntaxError → HTTP 500 instead of BadRequestException. Use z.coerce.bigint() (or a typed parse).
[warning] admin-token.service.ts expirationDate — z.coerce.date() turns garbage like "toto" into Invalid Date (not a ZodError), so later validation yields a misleading message. Add an explicit Number.isNaN(date.getTime()) guard → BadRequestException('Date d\'expiration invalide').

Fix these two, then approve.

StephaneTrebel · 2026-08-07T07:57:30Z

+  constructor(@Inject(PrismaService) private readonly prisma: PrismaService) {}
+
+  @StartActiveSpan()
+  async list(withRevoked = false) {


thought: Hmm ok, pas déconnant que par défaut on ne ramène pas tout 🤔

StephaneTrebel · 2026-08-07T07:58:53Z

+  }
+
+  @StartActiveSpan()
+  async create(data: CreateAdminTokenBody) {


issue: arrêtez de me mettre du data partout 😅
Les choses ont un nom. ici on a un CreateAdminTokenBody

Sinon on renomme toutes les variables en data1, data2, foo, bar… 😁

StephaneTrebel · 2026-08-07T08:25:00Z

+    expect(found).toBeTruthy()
+    expect(found).not.toHaveProperty('hash')
+  })
+})


praise: Bon, c'est pas mal sur le reste 👍

StephaneTrebel · 2026-08-07T08:26:07Z

+  it('should create a personal access token with plaintext password', async () => {
+    const result = await service.create({
+      name: `e2e-pat-${faker.string.uuid()}`,
+      expirationDate: new Date(Date.now() + 86400000),


nitpick: Come on, un const ONE_DAY_IN_MILLISECONDS = 86400000, ça ne coûte pas bien cher 😅

StephaneTrebel · 2026-08-07T08:28:01Z

+      select: { hash: true },
+    })
+
+    const expectedHash = createHash('sha256').update(created.password).digest('hex')


issue: Là on n'est même plus "trop proches de l'implémentation", on copie-colle l'implémentation, avec tous les soucis que ça peut poser...à ce niveau-là appelle carrément la fonction de hashage.

StephaneTrebel · 2026-08-07T08:28:50Z

    coverage: {
      provider: 'v8',
-      reporter: ['text', 'json', 'html'],
+      reporter: ['text', 'lcov'],


nitpick: Limite on les mets tous. Et un commit dédié ça pourrait être pas mal (non aux Cavaliers Législatifs !)

StephaneTrebel · 2026-08-07T08:29:31Z

@@ -1,3 +1,4 @@
+export * from './_utils.js'


question: Ça vient d'où, ça ? 🤔

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I00a18a853330301a735cdfcf3fb6955a6a6a6964

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: Ib1b5cbbfcd8274912ed9ffb20d60cbed6a6a6964

cloud-pi-native-sonarqube · 2026-08-07T14:49:51Z

Quality Gate passed

Issues

Measures

Project ID: cloud-pi-native_console_AYsa46O31ebDtzs-pYyZ

View in SonarQube

github-advanced-security AI found potential problems Jun 30, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/modules/admin-token/admin-token.service.ts Fixed

Comment thread apps/server-nestjs/src/modules/user-tokens/user-tokens.service.ts Fixed

shikanime force-pushed the shikanime/push-uotmotuknkvy branch from a9a75b7 to 64a5234 Compare June 30, 2026 10:02

github-advanced-security AI found potential problems Jun 30, 2026

View reviewed changes

shikanime force-pushed the shikanime/push-uotmotuknkvy branch 10 times, most recently from b37e308 to 0331d48 Compare July 1, 2026 09:54

github-code-quality Bot found potential problems Jul 1, 2026

View reviewed changes

Comment thread ...odules/infrastructure/auth/keycloak-secret-provider/keycloak-secret-provider.service.spec.ts Fixed

Comment thread apps/server-nestjs/src/modules/registry/registry-client.service.spec.ts Fixed

shikanime force-pushed the shikanime/push-uotmotuknkvy branch from 0331d48 to c53cb23 Compare July 1, 2026 10:19

github-code-quality Bot found potential problems Jul 1, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts Fixed

Comment thread apps/server-nestjs/src/modules/gitlab/gitlab.service.ts Fixed

shikanime force-pushed the shikanime/push-uotmotuknkvy branch 2 times, most recently from 09f3402 to b667443 Compare July 1, 2026 11:29

github-code-quality Bot found potential problems Jul 1, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/modules/project-roles/project-roles.service.ts Fixed

shikanime force-pushed the shikanime/push-uotmotuknkvy branch 2 times, most recently from 1b31261 to f16a730 Compare July 1, 2026 12:00

shikanime changed the title ~~refactor(server-nestjs): migrate project hook~~ refactor(user-tokens): migrate from server Jul 1, 2026

shikanime force-pushed the shikanime/push-uotmotuknkvy branch 4 times, most recently from cecad1b to 10b4770 Compare July 1, 2026 13:35

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

shikanime force-pushed the shikanime/push-uotmotuknkvy branch 2 times, most recently from 0eb3e93 to 43d21dc Compare July 1, 2026 13:56

shikanime mentioned this pull request Jul 23, 2026

refactor: use HTTP code constant from NestJS instead of static values #2357

Merged

shikanime commented Jul 27, 2026

View reviewed changes

github-code-quality Bot found potential problems Jul 27, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/modules/admin-token/admin-token.service.spec.ts Fixed

Comment thread apps/server-nestjs/src/modules/user-tokens/user-tokens.service.spec.ts Fixed

github-advanced-security AI found potential problems Aug 3, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/utils/crypto.utils.ts Fixed

Comment thread apps/server-nestjs/src/utils/crypto.utils.ts Fixed

github-code-quality Bot found potential problems Aug 3, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/main.module.ts Fixed

github-advanced-security AI found potential problems Aug 3, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/utils/crypto.utils.ts Dismissed

StephaneTrebel requested changes Aug 7, 2026

View reviewed changes

shikanime added 2 commits August 7, 2026 16:41

refactor(shared): add token pair generation and expiration schema

cc12cc9

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I00a18a853330301a735cdfcf3fb6955a6a6a6964

refactor(user-tokens): migrate from server

541e2a2

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: Ib1b5cbbfcd8274912ed9ffb20d60cbed6a6a6964

Uh oh!

Conversation

shikanime commented Jun 30, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

shikanime commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

shikanime left a comment

Choose a reason for hiding this comment

Review: #2279 — refactor(user-tokens): migrate from server

Findings

Compliance checklist

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shikanime commented Aug 4, 2026

Review: refactor(user-tokens): migrate from server — REQUEST CHANGES

🔴 blockers

🟡 warnings

🟢 nits

Verified OK

Uh oh!

shikanime commented Aug 6, 2026

Review: #2279 — refactor(user-tokens): migrate from server

Uh oh!

StephaneTrebel Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

StephaneTrebel Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephaneTrebel Aug 7, 2026