Bug #91967
openOIDC : alimenter un claim à partir d'un attribut LDAP
0%
Description
L'alimentation d'un claim OIDC à partir d'un attribut LDAP ne fonctionne pas et retourne systématiquement une valeur nulle. Sur le même principe que dans l'implémentation de l'IDP CAS , je propose de corriger le problème en utilisant authentic2.utils.misc.get_user_from_session_key.
Note : il est peut-être nécessaire d'utiliser un utilisateur fournis par get_user_from_session_key_lors des autres utilisations de utils.create_user_info, mais je ne suis pas en mesure de l'affirmer en l'état et je n'ai pas d'environnement de tests pour m'en assurer.
Files
Updated by Robot Gitea almost 2 years ago
- Status changed from Nouveau to En cours
- Assignee set to Paul Marillonnet
Paul Marillonnet (pmarillonnet) a ouvert une pull request sur Gitea concernant cette demande :
- URL : https://git.entrouvert.org/entrouvert/authentic/pulls/365
- Titre : WIP: Fix providing OIDC claims & user info from LDAP attributes (#91967)
- Modifications : https://git.entrouvert.org/entrouvert/authentic/pulls/365/files
Updated by Robot Gitea about 1 year ago
Paul Marillonnet (pmarillonnet) a fermé une pull request sur Gitea concernant cette demande.
Updated by Benjamin Renard 11 months ago
Robot Gitea a écrit :
Paul Marillonnet (pmarillonnet) a fermé une pull request sur Gitea concernant cette demande.
Ce bug est toujours présent en version 5.98-1~eob120+1 et mon patch corrige toujours le problème à priori. Pourriez-vous me dire ce qui bloque son inclusion ?
Updated by Paul Marillonnet 10 months ago
- Status changed from En cours to Information nécessaire
- Assignee changed from Paul Marillonnet to Benjamin Renard
Bonjour Benjamin,
Dans une discussion jabber du 15 juillet 2024, je te signalais qu’une pull-request créée contenant ton patch cassait l’intégration continue, ce à quoi tu m’avais répondu que tu regardais pour fournir une nouvelle version du patch satisfaisant l’intégration continue. À ma connaissance une telle v2 n’a pas été proposée (?)
Updated by Benjamin Renard 10 months ago
Salut Paul,
Paul Marillonnet a écrit :
Bonjour Benjamin,
Dans une discussion jabber du 15 juillet 2024, je te signalais qu’une pull-request créée contenant ton patch cassait l’intégration continue, ce à quoi tu m’avais répondu que tu regardais pour fournir une nouvelle version du patch satisfaisant l’intégration continue. À ma connaissance une telle v2 n’a pas été proposée (?)
Ah désolé, j'avais complètement zappé ! Je regardes à jouer ces tests en local et je vois si je reproduis. Pour le moment, j'ai du mal à voir comment ce que j'ai changer as put poser ces échecs, surtout au sujet des test_authenticators_saml.
Updated by Benjamin Renard 10 months ago
Je reproduis les échecs des tests sur idp_oidc, mais aucun des autres tests. Pour les tests test_manager_authenticators, je constate que la fixture saml_metadata_url qui fait planter les tests est bien présente dans le fichier tests/conftest.py dans ta branche WIP, mais contrairement à la version upstream, il y a un décorateur @responses.activate en plus. Lorsque je l'ajoute chez moi, ça fait bien planter les tests avec la même erreur (fixture 'saml_metadata_url' not found). Tu pourrais rebaser ta branche pour voir si ça corrige bien ces problèmes déjà ?
Updated by Benjamin Renard 10 months ago
- File 0001-Fix-providing-OIDC-claims-user-info-from-LDAP-attrib.patch 0001-Fix-providing-OIDC-claims-user-info-from-LDAP-attrib.patch added
Concernant les trois tests restant, je ne gérais pas le cas des appels non-authentifiés. C'est corrigé (nouveau patch joint).
Updated by Paul Marillonnet 10 months ago
- Status changed from Information nécessaire to En cours
- Assignee changed from Benjamin Renard to Paul Marillonnet
Bonjour Benjamin,
Merci pour ces éléments, je regarde ça maintenant, je vais rebaser la branche distante sur gitea avec ce nouveau patch.
Updated by Paul Marillonnet 10 months ago
Effectivement la CI est bien plus contente avec cette nouvelle version du patch, que je vais de suite relire :)
Updated by Benjamin Renard 10 months ago
- File 0001-Fix-providing-OIDC-claims-user-info-from-LDAP-attrib.patch 0001-Fix-providing-OIDC-claims-user-info-from-LDAP-attrib.patch added
Salut, je viens de faire une mise à jour en 6.10 et je me rends compte que mon patch ne fonctionne plus en l'état (les attributs LDAP ne sont à nouveau plus correctement alimentés). Après quelques tests et ajustements, je te joins un nouveau patch (plus de test sur is_authenticated dans tokens_from_authz_code(), on passe toujours par get_user_from_session_key()). Dans l'idéal, il faudrait ajouter un test pour ce genre de cas de figures, mais je vois pas bien comment faire ça.
Updated by Benjamin Renard 8 months ago
Salut, je viens de me rendre compte que le test is_authenticated pose également problème dans user_info et qu'il faut également toujours passer par get_user_from_session_key() à cet endroit. Je n'ai pas encore tester si les tests unitaires passent correctement avec cette modification : je vais voir à m'en occuper et je te tiens au courant.
Updated by Paul Marillonnet 7 months ago
- Assignee changed from Paul Marillonnet to Benjamin Renard
Salut Benjamin, ça marche, je te laisse le ticket attribué le temps que tu reviennes vers moi à ce sujet.
Updated by Benjamin Renard 2 months ago
Paul Marillonnet a écrit (#note-14):
🤖 Pull request fermée.
Hum, c'est mergé au final au pas, pas sûre de comprendre ? Je maintiens ce patch chez nous depuis un moment et c'est en prod chez au moins 3 clients depuis plus d'un an.
Updated by Thomas Noël 2 months ago
- Status changed from En cours to Information nécessaire
- Assignee changed from Benjamin Renard to Paul Marillonnet
Updated by Paul Marillonnet 2 months ago
- Status changed from Information nécessaire to En cours
- Assignee changed from Paul Marillonnet to Benjamin Renard
Benjamin Renard a écrit (#note-15):
Paul Marillonnet a écrit (#note-14):
🤖 Pull request fermée.
Hum, c'est mergé au final au pas, pas sûre de comprendre ? Je maintiens ce patch chez nous depuis un moment et c'est en prod chez au moins 3 clients depuis plus d'un an.
Salut Benjamin,
Non, pas mergé, juste je fais le nettoyage dans mes pull-requests (celle-ci ne passait pas les tests et continuait de m’envoyer des messages intempestifs de CI en erreur), j’en rouvrirai une, une fois que, comme convenu, tu seras revenu vers moi :
Salut, je viens de me rendre compte que le test is_authenticated pose également problème dans user_info et qu'il faut également toujours passer par get_user_from_session_key() à cet endroit. Je n'ai pas encore tester si les tests unitaires passent correctement avec cette modification : je vais voir à m'en occuper et je te tiens au courant.