Produits Entr'ouvert » Authentic 2

Merci d'ignorer le patch précédente, voici celui que j'avais l'intention d'envoyer.

Nouvelle version qui corrige deux problèmes. D'abord que c'est pas très utile d'avoir une référence à un objet python dans les logs et c'est mieux d'avoir les attributs de l'objet et leur valeur. Et ensuite que ça marchait pas du tout parce que {1}mdb était utilisé dans le test alors que c'est {2}mdb qui a la ppolicy active. Un assert a été ajouté pour vérifier que la réponse est loggée au lieu de rien.

Nouvelle version qui ajoute des tests et produit des messages lisibles par un être humain, et i18n. Au passage je rencontre un mystérieux problème en testant pwdGraceAuthnLimit . La valeur décroît comme elle est supposé le faire. Mais quand elle arrive à 1 elle ne bouge plus et l'utilisateur peut toujours se loggé, ce qui n'est pas vraiment l'objectif. Le test qui montre le problème est test_authenticate_ppolicy_pwdGraceAuthnLimit.

L'objectif n'étant pas de vérifier le comportement du serveur LDAP mais d'assurer la couverture du code fourni, j'ai enlevé l'assert qui échoue (voir https://dev.entrouvert.org/issues/50959#note-3).

Cette nouvelle version du patch inclus

• un test pour pwdExpireWarning
• une version human readable pour tout les messages d'erreur
• une traduction française pour tous les messages

Cette version du patch est prête pour un review et les tests qu'il contient passent. Je ne doute pas qu'il faille changer des choses mais comme je trouve que l'ensemble est cohérent et que je pense qu'il fait ce qu'on attend, je vais m'abstenir d'avancer plus pour l'instant. L'affichage des messages via MessageMiddleware étant trivial, je l'ai ajouté ici au lieu d'en faire un patch séparé (merci Benjamin :-) ).

On a encore des machines en python 3.5, donc pas de f-string dans le code pour l'instant (f'The password will expire in {ctrl.timeBeforeExpiration} seconds.').

Les messages de log sont un peu succincts : log.info('%s: %s', authz_id, message) un suffixe (je sais il n'y en a pas ailleurs) style 'ldap: bind error with authz_id "%s" -> "%s"' serait plus lisible.

Expliciter le nom des variables retour plutôt que de créer un tuple ici :

                            results = conn.simple_bind_s(authz_id, password, serverctrls=[
                                ppolicy.PasswordPolicyControl()
                            ])
                            self.process_controls(request, authz_id, results[3])

On a des logs en base lisibles depuis le back-office, ex.:

src/authentic2/authenticators.py:139:                    request.journal.record('user.login.failure', username=username)

Pour ça m'a l'air bon j'ai juste une petite inquiétude sur le comportement des serveurs LDAP ne supportant pas cette extension. Par défaut les protocoles utilisant ASN/1 comme LDAP ou TLS utilisent un flag "criticalité" sur les extensions, qui la plupart du temps est à False, signifiant "si tu ne comprends pas cette extension, ce n'est pas grave ignore", c'est le cas sur PasswordPolicyControl par défaut avec python3-ldap :

$ grep criticality  /usr/lib/python3/dist-packages/ldap/controls/ppolicy.py
  def __init__(self,criticality=False):
    self.criticality = criticality

Merci pour la review détaillée. Je suis aussi très soulagé de ne pas m'être engagé dans une voie de garage, pfuiiii :-) Dans la série de patch tu trouveras le patch original histoire de tout avoir ensemble et un commit par modification pour que ce soit plus facile a relire.

• Les traductions ont été enlevées
• Les commits sont squash en un seul
• open a été remplacé par with open
• Des modifications mineures de lisibilité ont été faites sur les messages
• L'exception ldap.STRONG_AUTH_REQUIRED est ignorée et loggée. Elle est levée lorsque pwdAllowUserChange: FALSE raison pour laquelle elle se trouve dans ce commit mais je peux la mettre dans un commit séparé si tu le souhaites.

Merci pour les reviews, ça fait plaisir de voir ça avancer :-)

commit 814e0192f33ce13b7cf6630b21c19a3dcddc8c86
Author: Loïc Dachary <ldachary@easter-eggs.com>
Date:   Tue Feb 9 08:46:22 2021 +0100

    ldap: optionally collects messages from ppolicy

    Enable PasswordPolicyControl[0] in authenticate() and log the
    information it returns, on success or error. In the context of a
    request, this information is also set as a message[1] to be displayed
    to the user.

    [0] https://github.com/python-ldap/python-ldap/blob/python-ldap-3.3.1/Lib/ldap/controls/ppolicy.py
    [1] https://docs.djangoproject.com/en/3.1/ref/contrib/messages/

    Fixes: #50959

    License: MIT

Appliqué par commit authentic2|814e0192f33ce13b7cf6630b21c19a3dcddc8c86.

Loïc Dachary a écrit :

Je m'attendais à voir un résultat de jenkins apparaitre ici, il y a une raison pour laquelle ce n'est pas le cas ? En tout cas content et pas peu fier de ce premier commit non trivial :-) Merci pour le suivi rapide et surtout pour les reviews tres détaillées et claires.

L'intégration jenkins ne concerne que les branches wip/* sur notre dépôt, elle est faite de manière ad-hoc en javascript (appel ajax de redmine vers jenkins en cherchant une branche wip/<numéro-du-ticket>-.*.

(pas de ref ticket dans le commit)