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
History
Updated by Benjamin Renard 9 months 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 9 months 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 6 months 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 5 months 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 5 months 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 5 months 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 4 months ago
- Status changed from Nouveau to Solution proposée
- Assignee set to Benjamin Renard
Updated by Benjamin Renard 4 months ago
Je me permets une petite relance sur le sujet. Notre client nous relance sur le sujet.