Conversation
Collaborator
|
New Issues (62)Checkmarx found the following issues in this Pull Request
Fixed Issues (82)Great job! The following issues were fixed in this Pull Request
Use @Checkmarx to interact with Checkmarx PR Assistant. |
Regzox
force-pushed
the
story_15455
branch
3 times, most recently
from
913c13d to
4f8e575
Compare
Regzox
force-pushed
the
story_15455
branch
5 times, most recently
from
c6ebb8d to
0f3f935
Compare
bbenaissa
reviewed
Feb 10, 2026
| return authUser.getProfileGroup() != null; | ||
| } | ||
|
|
||
| private void addAuthenticatedUserAttributes(AuthUserDto authUser, Map<String, List<Object>> attributes) { |
Collaborator
There was a problem hiding this comment.
Est ce normal que ce traitement est fait coté CAS et pas dans IAM?!
Contributor
Author
There was a problem hiding this comment.
Je ne sais pas. Cependant, ici on fait un login IAM pour obtenir les informations pour construire un Principal dans CAS.
lotfivitam
reviewed
Feb 12, 2026
Comment on lines
+340
to
+360
| # FIXME: Only for testing purpose. Should setup parametrization ? | ||
|
|
||
| cas.authn: | ||
| oauth.session-replication.cookie.crypto: | ||
| encryption.key: Fd-b-pjoN82hBdUti9HqzJXgvfs7pFtVYeaKIRhDdNE | ||
| signing.key: EsCugv7mIHZ3ecx-A5nf_75KsrmVGAWNMXwyEPXDV1jf0nmLrpu0Py6aO62yx4yd_W9_6nhMaGxhE9FQoeP7HQ | ||
| pac4j.core.session-replication.cookie: | ||
| name: DISSESSIONAD | ||
| crypto: | ||
| encryption.key: 9XnxhG7lfRQS9I5_86j3XQHZc19jZsamU97pDtut__o | ||
| signing.key: oFaBMTNLfKgCyqJPjxvZbdFhmxBKFgJP2_-rMDatBN91ZlU3eIpq2SYD_5ILHBSa616VEe5c0yUpACWne6TIlg | ||
| passwordless.tokens.crypto: | ||
| encryption.key: Bo_9H0xCT220Ogn4hvyrbH-x7j5hTAMcs3M8PdDiJ7w | ||
| signing.key: b_ZI3StT4vGnTrWIG4e7fdwNzbgQGN9IsqVo29HLqtstClu1ekzQ_d67K6wtQJLQPSU-RVCF31HuB2OfDkq0mA | ||
| pm.reset.crypto: | ||
| encryption.key: IG2rGK-5ctqoSWMv52XkZZ9BoMqRBcpjdzwLGJ6yiN0 | ||
| signing.key: xPVQOgj1-ywI-O_5fv5pTkg_bBS6CCXh7Yr9zMQoy2bmCjAJfLoVfycKYw-gERmBDozEGeT72HquVOT1dAZO5Q | ||
|
|
||
| cas.webflow.crypto.signing.key: =oebPIRe18A0cAeBdZCHkVlLPa_Kbthxo70iRpAhbk84dQGQj_8AOEMvEg3y7GAKYxtpF5nn6nx7vj5iU-eHStg | ||
| cas.webflow.crypto.encryption.key: 3FzNquczUjhmeJyqu251Ow | ||
| cas.webflow.crypto.encryption.key-size: 128 |
Contributor
Author
There was a problem hiding this comment.
J'ai ajouté la possibilité de configurer.
Contributor
There was a problem hiding this comment.
À corriger dans le cadre du bug 12418
lotfivitam
reviewed
Feb 14, 2026
Contributor
lotfivitam
left a comment
lotfivitam
left a comment
There was a problem hiding this comment.
Tout d'abord, bravo pour le taf colossal !
Quelques remarques cependant :
- Pas sûr de comprendre l'usage du "passwordless" vs loginform standard, et lequel est utilisé par lequel (côté WebFlow mais aussi écrans). J'ai essayé de générer un diagramme de séquence représentant le WebFlow de login, mais ça s'avère extrêmement complexe à analyser.
- Il reste un bon coup de cleaup général (cf commentaires de review)
- Prévoir une recette complète des cas avancés (reset de password suite à expiration, reset suite à perte, création de comptes, subrogation user générique, subrogation user nominatif, comptes bloqués, mfa...)
Regzox
force-pushed
the
story_15455
branch
10 times, most recently
from
a06ca5d to
1932f5a
Compare
bbenaissa
reviewed
Feb 17, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
bbenaissa
reviewed
Feb 20, 2026
Collaborator
bbenaissa
left a comment
bbenaissa
left a comment
There was a problem hiding this comment.
Il y a plein de modifications faites sur la PR, sur les workflows, ça nécessite une recette approfondie
Regzox
force-pushed
the
story_15455
branch
2 times, most recently
from
f4adc53 to
ecec3f7
Compare
GiooDev
requested changes
Mar 3, 2026
| # Crypto configurations | ||
|
|
||
| {% macro crypto_block(prefix, keys) %} | ||
| {% if keys is defined %} |
Contributor
There was a problem hiding this comment.
Revoir l'indentation des balises {% xxx %} pour faciliter la relecture ;)
| secret_token: changeit_cas_secret_token | ||
| crypto: | ||
| oauth: | ||
| encryption_key: "my_oauth_encryption_key" |
Contributor
There was a problem hiding this comment.
Comment on génère ces keys ?
Est-ce possible de ne pas les configurer ? Et si oui, est-ce que cas en génère par défaut (comme le cas actuel ?).
De plus attention, dans la même sous clé vitamui.cas_server tu mélanges un array et des hash.
Contributor
There was a problem hiding this comment.
Si ça démarre sans mettre ces configurations, je serai d'avis de le séparer dans un ticket dédié pour faire le lien avec le bug 12418.
Contributor
There was a problem hiding this comment.
Je retire ces modifs ça sera pris en compte dans un ticket associé en lien avec le Bug 12418.
GiooDev
requested changes
Mar 3, 2026
| port_admin: 7001 | ||
| cors: | ||
| enabled: true | ||
| crypto: |
Contributor
There was a problem hiding this comment.
Quels sont les cas où l'on devra modifier ces valeurs ?
Si aucun usage à les modifier, ne pas les renseigner ici mais directement dans le rôle associé au niveau de defaults pour éviter de surcharger ce fichier de variables d'éléments dont on ne veut pas qu'ils soient modifiés.
Le cas échéant, les documenter si besoin de les modifier plus tard.
Contributor
There was a problem hiding this comment.
Je sais pas dire si toutes les clés sont nécessaires, mais je ne suis pas fan de mettre des clés crypto par défaut.
Pour un env de dev, c'est pas choquant de laisser CAS les générés (si ça fonctionne sans ces clés dans le application.conf.j2).
Sinon, il faudra les garder paramétrables histoire d'éviter des prods avec des clés d'exemple.
| secret_token: changeit_cas_secret_token | ||
| crypto: | ||
| oauth: | ||
| encryption_key: "my_oauth_encryption_key" |
Contributor
There was a problem hiding this comment.
Si ça démarre sans mettre ces configurations, je serai d'avis de le séparer dans un ticket dédié pour faire le lien avec le bug 12418.
GiooDev
requested changes
Mar 3, 2026
bbenaissa
reviewed
Mar 6, 2026
| @RequiredArgsConstructor | ||
| public class IamApiDecorator { | ||
|
|
||
| private static final String DEFAULT_SERVICE_ACCOUNT = "admin@change-it.fr"; |
Collaborator
There was a problem hiding this comment.
je ne comprend pas l'utilité de ce décorateur
Contributor
Author
There was a problem hiding this comment.
Avec l’ancienne API, le contexte était passé lors de l’appel du client.
J’avais mis en place un RestTemplateCustomizer qui ne peuplait le contexte qu’au moment de l’exécution effective de la requête HTTP.
Le problème est que le client tentait d’utiliser ce contexte trop tôt, alors qu’il n’était pas encore initialisé, notamment pour construire les headers de la requête.
Le décorateur permet désormais d’initialiser le contexte avant l’appel du client, puis de le nettoyer automatiquement une fois l’appel terminé.
lotfivitam
reviewed
Mar 6, 2026
| cas_server: | ||
| # Secret internal token for CAS to vitam-ui communication | ||
| secret_token: changeit_cas_secret_token | ||
| crypto: |
Contributor
There was a problem hiding this comment.
Si on édite vault-vitamui.yml.example, il faut éditer vault-vitamui.yml au lieu de vitamui_vars.yml
| port_admin: 7001 | ||
| cors: | ||
| enabled: true | ||
| crypto: |
Contributor
There was a problem hiding this comment.
à déplacer dans vault-vitamui.yml
| action, | ||
| CasWebflowConstants.TRANSITION_ID_TICKET_GRANTING_TICKET_NOT_EXISTS, | ||
| CasWebflowConstants.STATE_ID_GATEWAY_REQUEST_CHECK | ||
| ACTION_STATE_CHECK_SUBROGATION |
Contributor
There was a problem hiding this comment.
Je comprend qu'on veuille ajouter un step pour traiter la subrogation séparément, mais le code de CheckSubrogationAction est au final assez redondant avec une bonne partie de CustomDelegatedClientAuthenticationAction. Est-ce normal?
Aussi, est-ce le bon état de départ pour ce check? (risque d'avoir un saut dans le workflow qui causerait ce traitement d'être skippé)
Contributor
Author
There was a problem hiding this comment.
Corrigé dans story #15813 refactor(cas): check subrogation action
On ne bypass plus les étapes, on proceed et laisse faire le flow actuel.
GiooDev
requested changes
Mar 6, 2026
| secret_token: changeit_cas_secret_token | ||
| crypto: | ||
| oauth: | ||
| encryption_key: "my_oauth_encryption_key" |
Contributor
There was a problem hiding this comment.
Je retire ces modifs ça sera pris en compte dans un ticket associé en lien avec le Bug 12418.
| "OIDC_CONFIG": { | ||
| "issuer": "{{ vitamui.cas_server.base_url | default(url_prefix+'/cas') }}/oidc", | ||
| "redirectUri": "?", | ||
| "redirectUri": "{{ vitamui.ui_portal.base_url | default(url_prefix+'/') }}", |
Contributor
There was a problem hiding this comment.
C'était un bug ça ? Qu'on doit backporter ?
C'est peut-être à cause de ça que la redirection en http ne fonctionne actuellement pas sur l'url oidc ?
| management.metrics.export.prometheus.enabled: true | ||
| management.prometheus.metrics.export.enabled: true | ||
|
|
||
| {% if sms.enabled|lower == "true" %} |
Contributor
There was a problem hiding this comment.
À vérifier si on peut maintenant réellement désactiver le sms car à l'époque il y avait un bug qui empêchait cas de démarrer en cas de désactivation.
Configuration sms renseignée dans deployment/environments/group_vars/all/infra.yml.
Contributor
|
Poursuite des travaux dans le cadre de la Story 15813. |
This was referenced Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.