pix icon indicating copy to clipboard operation
pix copied to clipboard

[TECH] Utiliser le logger du service monitoring pour les erreurs d'authentification OIDC

Open Anne-Gaelle-S opened this issue 2 years ago • 1 comments

:unicorn: Problème

Aujourd'hui il est difficile de comprendre la source des erreurs liés à l'authentification des utilisateurs pole emploi car il y a peu d'information dans nos logger.

:robot: Solution

Utiliser le logErrorWithCorrelationIds du service monitoring-tools.js plutôt que le logger classique, afin d'avoir un peu plus de contexte lorsque ces erreurs apparaîssent (ex: l'userId, la requestId).

:rainbow: Remarques

Proposition : ne pas tester les loggers. Ce n'est pas une fonctionnalité de l'application et on est surement amené à les modifier pour de nouveaux besoins (comme ici). Les tests n'apportent ici pas vraiment de plus value je trouve 🤔

:100: Pour tester

Vérifier la CI passe

Anne-Gaelle-S avatar Sep 20 '22 08:09 Anne-Gaelle-S

I'm deploying this PR to these urls:

  • App (.fr): https://app-pr4947.review.pix.fr
  • App (.org): https://app-pr4947.review.pix.org
  • Orga: https://orga-pr4947.review.pix.fr
  • Certif: https://certif-pr4947.review.pix.fr
  • Admin: https://admin-pr4947.review.pix.fr
  • API: https://api-pr4947.review.pix.fr/api/

Please check it out!

pix-service avatar Sep 20 '22 08:09 pix-service

Je ne trouve pas ça vraiment plus simple 🤔 Dans le code actuel on a toute la manière de récupérer les info de l'utilisateur dans le même fichier. A mon sens ça évite de jongler avec 2 fichiers différents pour comprendre ce qu'il se passe. Le service oidc-authentication-service c'est un peu la brique technique qui détaille comment notre flow oidc marche et le usecase authenticate-oidc-user.js c'est plus le listing "métier" pour authentifier un utilisateur externe. Le usecase dit "va me chercher les infos de cet utilisateur ci", il n'a pas besoin de savoir le détail de comment techniquement on va chercher ces infos là, c'est plus la responsabilité de la couche technique (donc du service). Bon après ça sort un peu de la PR , mais perso je trouve que le code tel qu'il est répartie bien les responsabilités :)

Anne-Gaelle-S avatar Sep 22 '22 09:09 Anne-Gaelle-S