Development #66416
Ajout du support de ppolicy lors de la modification du mot de passe
0%
Description
Nous avions par le passé ajouté le support de ppolicy lors de la connexion et autre client nous demande aujourd'hui de l'ajouter lors du changement de mot de passe (changement & réinitialisation). Aujourd'hui, en cas de mot de passe refusé par l'annuaire LDAP (du fait de ppolicy ou non d'ailleurs), Authentic renvoi une erreur 500 sans précision. L'idée serait ici d'ajouter ici le contrôle PasswordPolicyControl lors de la modification du mot de passe d'un utilisateur (LDAPUser.set_password) comme cela avait été fait lors de la connexion (LDAPBackend.authenticate_block) de manière paramétrable via le paramètre use_controls existant.
Files
Related issues
Associated revisions
password_policy_control_messages: fix handling passwordExpired (#66416)
Licence: MIT
ldap: fix encoding password on modify_password (#66416)
Licence: MIT
ldap: rename process_controls method to process_bind_controls (#66416)
Licence: MIT
ldap: handle ppolicy controls at password-reset time (#66416)
Licence: MIT
ppolicy: handle reset redirect after a changeAfterReset error (#66416)
Licence: MIT
ldap: set sharper ppolicy_control error messages when relevant (#66416)
License: MIT
ldap: improve password expiration date formatting (#66416)
License: MIT
password_change: stay on form page when ldap ppolicy errors happen (#66416)
License: MIT
password_reset_confirm: handle ldap ppolicy errors (#66416)
License: MIT
tests/ldap: fix conflicting access rights with slapd>2.4 (#66416)
License: MIT
ldap: use a separate backend config flag for ppolicy controls (#66416)
translation update (#66416)
History
Updated by Benjamin Renard over 1 year ago
J'ai commencé à regarder le sujet et cela ne semble pas très compliqué. La seule problématique qui me pose problème c'est de savoir remonter l'erreur correctement à l'utilisateur. En effet, la méthode set_password étant appelée par la méthode save des formulaires Django et il ne suffit donc pas de déclencher simplement une exception ValidationError. J'ai fait quelques recherches sur le sujet et je n'ai pas trouvé de solution simple et propre documentée pour cela. Avez-vous une idée de comment faire ?
PS : dans l'idéal, on aurait pu tenter d'utiliser la fonctionnalité LDAPv3 Grouping of Related Operations pour implémenter une validation via form.clean avant form.save, mais cela n'est pas encore supporté par python ldap3 (et j'ai pas trouvé si c'était le cas par OpenLDAP).
Updated by Benjamin Renard over 1 year ago
Benjamin Renard a écrit :
J'ai commencé à regarder le sujet et cela ne semble pas très compliqué. La seule problématique qui me pose problème c'est de savoir remonter l'erreur correctement à l'utilisateur. En effet, la méthode set_password étant appelée par la méthode save des formulaires Django et il ne suffit donc pas de déclencher simplement une exception ValidationError. J'ai fait quelques recherches sur le sujet et je n'ai pas trouvé de solution simple et propre documentée pour cela. Avez-vous une idée de comment faire ?
Je me permets une petite relance sur le sujet : une idée pour ma problématique exposée ci-dessus ?
Updated by Benjamin Renard about 1 year ago
- File 0005-ldap-handle-ppolicy-control-changing-reseting-passwo.patch 0005-ldap-handle-ppolicy-control-changing-reseting-passwo.patch added
Le patch ci-joint implémente la gestion de PasswordPolicyControl en cas de modification ou de réinitialisation du mot de passe. Comme proposé, cela reste conditionné à l'activation du paramètre de configuration use_controls (au niveau du block), comme c'est le cas pour le support de l'authentification.
Updated by Benjamin Dauvergne about 1 year ago
- j'ai l'impression qu'il y a la correction d'un bug dans le tas :
modlist = [(ldap.MOD_REPLACE, key, [new_password.encode('utf-8')])]
si cet encodage est nécessaire il faut le séparer dans un autre patch - il faudrait découper ça en 3 patchs:
- renommage process_controls -> process_bind_controls
- à relire je n'ai pas l'impression qu'il y ait de différence entre les deux méthode à part le message de log, je me trompe ? Dans ce cas autant rendre le message variable et ne garder qu'une méthode,
- correction citée plus haut
- enfin l'ajout des contrôles, il faudrait factoriser ces lignes entre les différentes méthodes de modification
- renommage process_controls -> process_bind_controls
Il faudrait des tests.
Updated by Benjamin Renard about 1 year ago
- File 0006-Add-tests-on-LDAP-password-change-reset-with-ppolicy.patch 0006-Add-tests-on-LDAP-password-change-reset-with-ppolicy.patch added
- File 0005-ldap-handle-ppolicy-control-changing-reseting-passwo.patch 0005-ldap-handle-ppolicy-control-changing-reseting-passwo.patch added
- File 0004-ldap-rename-process_controls-method-to-process_bind_.patch 0004-ldap-rename-process_controls-method-to-process_bind_.patch added
- File 0003-ldap-fix-encoding-password-on-modify_password.patch 0003-ldap-fix-encoding-password-on-modify_password.patch added
- File 0002-password_policy_control_messages-fix-handling-passwo.patch 0002-password_policy_control_messages-fix-handling-passwo.patch added
- File 0001-test_ldap-use-USERNAME-PASS-instead-of-duplicated-ha.patch 0001-test_ldap-use-USERNAME-PASS-instead-of-duplicated-ha.patch added
Benjamin Dauvergne a écrit :
- j'ai l'impression qu'il y a la correction d'un bug dans le tas :
[...]
si cet encodage est nécessaire il faut le séparer dans un autre patch
Effectivement, contrairement au cas avec un AD, le mot de passe n'était pas encodé, python-ldap requiert un bytes-string. J'ai fait un patch dédié pour ça.
- il faudrait découper ça en 3 patchs:
- renommage process_controls -> process_bind_controls
- à relire je n'ai pas l'impression qu'il y ait de différence entre les deux méthodes à part le message de log, je me trompe ? Dans ce cas autant rendre le message variable et ne garder qu'une méthode,
Pas exactement, mais c'est pas loin effectivement : dans le cas du login, on gère la request et on affiche le message d'erreur via messages.add_message() en plus d'activer request.needs_password_change dans certains cas. Dans le cas d'une modification de mot de passe, on affiche l'erreur simplement en levant une exception PasswordChangeError.
J'ai mis le renommage dans un commit dédié.
- correction citée plus haut
- enfin l'ajout des contrôles, il faudrait factoriser ces lignes entre les différentes méthodes de modification
C'est fait également et dans un commit dédié.
Il faudrait des tests.
J'ai ajouté des tests via un commit dédié pour chacun des messages d'erreurs gérés sauf changeAfterReset que je ne vois pas trop comment provoquer.
Au passage, j'ai également ajouté des patchs pour :
- utiliser les variables USERNAME & PASS un peu partout dans les tests LDAP au lieu de dupliquer des valeurs hard-codées
- corriger la gestion des erreurs passwordExpired (son code vaut zéro et il fallait un donc faire un test à coup de is not None)
Note : je précise que les tests sont prévus pour fonctionner une fois l'ensemble des autres patchs intégrés, y compris ceux de #69468, #69466 et #69464.
Updated by Benjamin Renard about 1 year ago
- File 0007-ppolicy-handle-reset-password-redirection-after-a-ch.patch 0007-ppolicy-handle-reset-password-redirection-after-a-ch.patch added
Benjamin Renard a écrit :
J'ai ajouté des tests via un commit dédié pour chacun des messages d'erreurs gérés sauf changeAfterReset que je ne vois pas trop comment provoquer.
Après réflexion, j'ai trouvé un moyen de tester cette erreur : il faut bloquer le compte à force de connexion avec un mauvais mot de passe, débloquer le compte comme un admin le ferait en spécifiant pwdReset à TRUE et retenter une connexion avec le bon mot de passe cette fois-ci.
En implémentant ce test, je me suis par ailleurs rendu compte que dans cette situation, on devrait provoquer une redirection vers le formulaire de réinitialisation du mot de passe et non laisser l'utilisateur sur le formulaire de connexion. J'ai donc implémenté cela et ajuster mon test en conséquence.
Note : À ce sujet, je suis pas sûre que j'utilise la bonne manière pour détecter la next URL lors de la redirection vers le formulaire de réinitialisation du mot de passe.
Updated by Benjamin Dauvergne about 1 year ago
- Status changed from Nouveau to Solution proposée
- Assignee set to Benjamin Renard
Updated by Benjamin Renard 12 months ago
Je me permets une petite relance sur le sujet. Notre client nous relance sur le sujet.
Updated by Robot Gitea 7 months ago
- Status changed from Solution proposée to En cours
- Assignee changed from Benjamin Renard to Paul Marillonnet
Paul Marillonnet (pmarillonnet) a ouvert une pull request sur Gitea concernant cette demande :
- URL : https://git.entrouvert.org/entrouvert/authentic/pulls/56
- Titre : WIP: wip/66416-ldap-add-ppolicy-support
- Modifications : https://git.entrouvert.org/entrouvert/authentic/pulls/56/files
Updated by Paul Marillonnet 7 months ago
- Assignee changed from Paul Marillonnet to Benjamin Renard
J’ai rebasé et poussé le tout dans une branche à jour, mais des tests explosent :
FAILED tests/test_ldap.py::test_user_change_password_too_short - assert 'The password is too short (minimun length: 15)' in '<!DOCTYPE html>\n<html>\n <head>\n <meta charset="utf-8"/>\n <title>Authentic2 - testserver - \n Password cha...\n Co... FAILED tests/test_ldap.py::test_user_change_password_too_soon - assert 'It is too soon to change the password.' in '<!DOCTYPE html>\n<html>\n <head>\n <meta charset="utf-8"/>\n <title>Authentic2 - testserver - \n Password cha...\n Copyright ... FAILED tests/test_ldap.py::test_reset_password_must_supply_old_password - AssertionError: assert 'The old password must be supplied.' in <302 Found text/html location: / no body> FAILED tests/test_ldap.py::test_login_ppolicy_must_change_password_after_locked - assert 'after 2 failures' in '<ul class="messages">\n \n <li class="warning">The account is locked since 202305... \n </ul>\n ... FAILED tests/test_ldap.py::test_user_change_password_not_allowed - assert 'It is not possible to modify the password.' in '<!DOCTYPE html>\n<html>\n <head>\n <meta charset="utf-8"/>\n <title>Authentic2 - testserver - \n Password cha...\n Copyri... FAILED tests/test_ldap.py::test_login_ppolicy_pwdMaxFailure - assert 'after 2 failures' in '<ul class="messages">\n \n <li class="warning">The account is locked since 202305... \n </ul>\n ... FAILED tests/test_ldap.py::test_login_ppolicy_password_expired - IndexError: list index out of range FAILED tests/test_ldap.py::test_user_change_password_in_history - assert 'This password has already been used and can no longer be used.' in '<!DOCTYPE html>\n<html>\n <head>\n <meta charset="utf-8"/>\n <title>Authentic2 - testserver - \n Passw...
Updated by Benjamin Renard 5 months ago
- File complete.patch complete.patch added
Paul Marillonnet (retour le 10/07) a écrit :
J’ai rebasé et poussé le tout dans une branche à jour, mais des tests explosent :
[...]
J'ai enfin trouvé le temps de regarder cela. Le problème vient principalement du fait que tu n’as pas repris mes patchs des tickets #69468, #69466 et #69464 (cf. la note en fin de #note-5). Je viens de m'occuper de rebaser tous les commits concernés sur votre branche main (b450d83546d60a620baebf09fd5e156714afe931 / v4.88) et je te joins un patch les incluant tous. Chez moi, tous les tests ldap passent bien à l'exception de deux, mais pour le coup, il ne passe pas non plus sans mes patchs, donc je penche pour un problème d’environnement. Et d'ailleurs j'en ai cinq autres du même genre lorsque je lance tous les tests.
Updated by Frédéric Péters 5 months ago
(je viens de pousser cette série dans la branche.)
et le build réussit.
Updated by Benjamin Renard 5 months ago
Frédéric Péters a écrit :
(je viens de pousser cette série dans la branche.)
et le build réussit.
Top, tu penses que ce serait intégrable upstream du coup ?
Updated by Benjamin Renard 5 months ago
Frédéric Péters a écrit :
Paul revient lundi de congés, il reprendra la main.
Ça marche, merci !
Updated by Paul Marillonnet 5 months ago
- Assignee changed from Benjamin Renard to Paul Marillonnet
Je commence à relire en détails.
Updated by Paul Marillonnet 5 months ago
Paul Marillonnet a écrit :
Je commence à relire en détails.
J’ai posé des remarques suite à ma première relecture, directement dans la PR : https://git.entrouvert.org/entrouvert/authentic/pulls/56
Je m’occupe des modifications induites, et poursuis ma relecture entamée hier.
Updated by Benjamin Renard 5 months ago
Paul Marillonnet a écrit :
Paul Marillonnet a écrit :
Je commence à relire en détails.
J’ai posé des remarques suite à ma première relecture, directement dans la PR : https://git.entrouvert.org/entrouvert/authentic/pulls/56
Bonne idée, mais n'ayant pas de compte sur votre gitea, je ne peux réagir à tes commentaires. Il y a un moyen d'avoir un compte sur votre gitea ou c'est uniquement à usage interne ?
Je m’occupe des modifications induites, et poursuis ma relecture entamée hier.
À lire certains de tes commentaires, il faudra que je m'occupe de certains points. N'hésite pas à me nommer si c'est le cas !
Updated by Frédéric Péters 5 months ago
Bonne idée, mais n'ayant pas de compte sur votre gitea, je ne peux réagir à tes commentaires. Il y a un moyen d'avoir un compte sur votre gitea ou c'est uniquement à usage interne ?
Je viens de te créer un compte (il y manquera encore ensuite peut-être des permissions, j'ajusterai).
Updated by Benjamin Renard 2 months ago
- File 0003-ppolicy-improve-timeBeforeExpiration-date-formating.patch 0003-ppolicy-improve-timeBeforeExpiration-date-formating.patch added
- File 0002-ppolicy-clean-computing-passwordTooShort-error-messa.patch 0002-ppolicy-clean-computing-passwordTooShort-error-messa.patch added
- File 0001-ppolicy-clean-computing-accountLocked-error-message.patch 0001-ppolicy-clean-computing-accountLocked-error-message.patch added
Comme vu avec Paul, quelques patchs qu'il m'a demandé dans gitea.
PS : j'avais développé la solution proposée à propos de formatage de la date d'expiration future du mot de passe, alors je met le patch ici, mais je vous laisse voir si vous préférez rester comme précédemment.
Updated by Robot Gitea about 2 months ago
- Status changed from Solution proposée to Résolu (à déployer)
Paul Marillonnet (pmarillonnet) a mergé une pull request sur Gitea concernant cette demande :
- URL : https://git.entrouvert.org/entrouvert/authentic/pulls/56
- Titre : Ajout du support de ppolicy lors de la modification du mot de passe (#66416)
- Modifications : https://git.entrouvert.org/entrouvert/authentic/pulls/56/files
Updated by Transition automatique about 2 months ago
- Status changed from Résolu (à déployer) to Solution déployée
Updated by Paul Marillonnet about 2 months ago
- Related to Development #82521: ppolicy : désactiver l’exécution des tests ppolicy sur un environnement bullseye (?) added
test_ldap: use USERNAME & PASS instead of hard-coded values (#66416)
Licence: MIT