Produits Entr'ouvert » Authentic 2

Est-ce que je peux me permettre de patcher directement le modèle de l'OU ?

Oui oui, tu peux même patcher le modèle User pour conserver la date du dernier rappel.

Benjamin, dans la description du ticket, tu commences pas "Par défaut on ne supprime rien".
Est-ce que ça veut dire qu'on vire l'option clean-threshold que cette commande peut prendre en paramètre ?

Qu'en est-il des options alert_threshold et period ? Est-ce qu'on :
1) Supprime ces options ?
2) Superpose les mails générés par ces deux options, en tant que politique globale de l'IdP, aux mails générés par la configuration lié à l'OU ?

Paul Marillonnet a écrit :

Benjamin, dans la description du ticket, tu commences pas "Par défaut on ne supprime rien".
Est-ce que ça veut dire qu'on vire l'option clean-threshold que cette commande peut prendre en paramètre ?

Yep, on base tout sur la configuration posée sur l'OU.

Qu'en est-il des options alert_threshold et period ? Est-ce qu'on :
1) Supprime ces options ?
2) Superpose les mails générés par ces deux options, en tant que politique globale de l'IdP, aux mails générés par la configuration lié à l'OU ?

Non non on vire tout et on repart proprement d'une configuration par OU, par défaut pour une OU, on ne supprime (ces champs sont vides).

Ah oui, autre chose encore : j'ai quand même gardé le filtre sur les usagers, qui me paraît utile indépendamment de toute appartenance à une OU.
En revanche, je réalise après coup qu'on aurait peut-être intérêt à rattacher à l'OU l'adresse courriel expéditrice des avertissements et notifications de suppression. Non ?

Je disais :

Oui oui, tu peux même patcher le modèle User pour conserver la date du dernier rappel.

Donc oui je m'attendais à ce qu'on ait de nouveaux champs sur l'objet user pour ne pas avoir de contraintes sur le délai entre lancement de la commande, c'est ingérable. Donc on garde trace de la date du dernier rappel, comme ça on peut aussi prévoir d'en faire plusieurs (si last_login > x jours et dernier_rappel is null ou dernier_rappel > y jours) alors nouveau rappel etc...

Quand je disais :

Par défaut on ne supprime rien.

Je parlais des utilisateurs que par défaut on ne supprimera plus, donc oui il faut supprimer les options de la commande, elle sera uniquement pilotée via le paramétrage sur les OUs, on peut potentiellement la virer même et s'appuyer sur la commande cleanupauthentic qui est déjà appelée tous les jours et appelle automatiquement les méthodes Model.objects.cleanup() quand elles existent.

Tout ceci étant dit je vais relire le patch mais je pense que ça ne colle pas.

Ne pas avoir peur d'ajouter des champs aux modèles en général, si c'est nécessaire, et bien c'est nécessaire.

Benjamin Dauvergne a écrit :

Donc oui je m'attendais à ce qu'on ait de nouveaux champs sur l'objet user pour ne pas avoir de contraintes sur le délai entre lancement de la commande, c'est ingérable. Donc on garde trace de la date du dernier rappel, comme ça on peut aussi prévoir d'en faire plusieurs (si last_login > x jours et dernier_rappel is null ou dernier_rappel > y jours) alors nouveau rappel etc...

Oui ok je comprends.

Je parlais des utilisateurs que par défaut on ne supprimera plus, donc oui il faut supprimer les options de la commande, elle sera uniquement pilotée via le paramétrage sur les OUs, on peut potentiellement la virer même et s'appuyer sur la commande cleanupauthentic qui est déjà appelée tous les jours et appelle automatiquement les méthodes Model.objects.cleanup() quand elles existent.

Tout ceci étant dit je vais relire le patch mais je pense que ça ne colle pas.

Je n'ai pas (encore) bougé le code dans la commande cleanupauthentic, je le ferais dans un second temps (une fois qu'on se sera mis d'accord sur ce que fait le code).

Je pose un patch WIP ici, je vais écrire des tests.

Valentin Deniaud a écrit :

Voilà, il refaut une relecture complète j'ai changé pas mal de trucs.

Notamment, je me suis retrouvé à faire des schémas pour essayer de comprendre les longs if dans le patch, sans grand succès, mais de tout façon rien que de devoir faire ça c'est une bonne raison pour les enlever. Vérifier donc que je ne suis pas passé à côté d'un comportement attendu.

Benjamin Dauvergne a écrit :

ce mail doit être adaptable en fonction de l'ou (account-deletion-on-inactivity-reminder-<ou.slug>.html)

Je vire cette partie là, elle est déjà dans le premier patch de #38966, à finir de relire.

Le numéro du ticket n'est pas le bon.

Il manque un clean pour vérifier que clean_unused_accounts_alert est inférieur à clean_unused_accounts_deletion, comme ça rend les choses compliqués

Compliqué, une condition de plus dans clean() ?

Compliqué la fonctionnement pas le code, je trouvais plus simple et souple d'avoir une liste de nombres sans contraintes (tu mets 100, 10, 1, ça fait 100j -> alerte, 110j -> alerte et 111j -> suppression). C'est facile à afficher sur la vue de l'OU, à traiter et à comprendre. Mais ça reste juste une suggestion, les champs actuels suffisent bien sûr.

il faut un atomic() autour de l'envoi de mails et de l'écriture de last_account_deletion_alert, comme ça en cas de problème SMTP on refera

Puisqu'avant de supprimer on regarde si on a envoyé l'alerte, ce comportement implique donc qu'un authentic qui n'arrive jamais à envoyer de mail ne supprimera jamais de comptes, moi ça me va c'était juste histoire de le noter explicitement.

Oui on ne doit rien faire sans avoir une demi-certitude qu'on a bien tenté de notifier les gens, c'est super important. S'ils ne reçoivent pas les mails pour d'autres raisons, ce n'est pas notre souci.

Quant à bouger le code pour le faire appeler par cleanupauthentic, je le ferai dans un nouveau ticket si toujours besoin.

Puisqu'on réécrit ce serait mieux de commencer à centraliser tous les appels vers tous les cron-like pour tuer les commandes spécifiques un peu comme dans passerelle.

Benjamin Dauvergne a écrit :

Je vire cette partie là, elle est déjà dans le premier patch de #38966, à finir de relire.

Le numéro du ticket n'est pas le bon.

Oups, il s'agit de #35774.

Quant à bouger le code pour le faire appeler par cleanupauthentic, je le ferai dans un nouveau ticket si toujours besoin.

Puisqu'on réécrit ce serait mieux de commencer à centraliser tous les appels vers tous les cron-like pour tuer les commandes spécifiques un peu comme dans passerelle.

Qu'est-ce que ça implique pour ce ticket ? (je ne vais pas bouger de code, ça ferait trop de taf pour rebaser la suite des patches)

Sinon un test ne passe pas sans que je m'explique pourquoi, en local ça marche, et surtout les tickets suivants basés sur ces patches sont verts... J'ai relancé, sans succès.

Voilà, poussé la même chose dans une nouvelle branche et c'est vert.

• Attention à la comparaison entre les deux champs nouvellement ajoutés à l'ou dans la méthode clean, qui est cassée en python3 :

Python 3.8.2 (default, Feb 25 2020, 13:04:52) 
[GCC 9.2.1 20200220] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from django_rbac.utils import get_ou_model
>>> ou = get_ou_model().objects.first()
>>> ou
<OrganizationalUnit 'default' 'Default organizational unit'>
>>> ou.clean_unused_accounts_alert
>>> ou.clean_unused_accounts_alert = 3
>>> ou.save()
>>> ou.clean_unused_accounts_alert >= ou.clean_unused_accounts_deletion
Traceback (most recent call last):
  File "<console>", line 1, in <module>
TypeError: '>=' not supported between instances of 'int' and 'NoneType'

• Le days_to_deletion dans send_alert est je crois erroné, il faut non pas indiquer user.ou.clean_unused_accounts_deletion - alert_delay mais le nombre de jours restants sachant le dernier login avant la suppression (puisque c'est ce test qui est effectué pour déterminer la liste des usagers à supprimer) —— nombre qui peut-être inférieur à ton user.ou.clean_unused_accounts_deletion - alert_delay.

• Et donc de la même façon, toujours dans send_alert, alert_delay ne devrait peut-être pas pas valoir user.ou.clean_unused_accounts_alert mais le nombre de jours depuis le dernier login, qui peut être supérieur à cette valeur user.ou.clean_unused_accounts_alert en fonction de la fréquence d'exécution de la commande.

• Sur cette partie du patch :
  

  +            for user in users.filter(last_account_deletion_alert__isnull=True):
  +                logger.info('%s last login %d days ago, sending alert', user,
  +                         ou.clean_unused_accounts_alert)
  +                self.send_alert(user)
  +
  +            to_delete = users.filter(
  +                last_login__lte=now-deletion_delay,
  +                # ensure respect of alert delay before deletion
  +                last_account_deletion_alert__lte=now - (deletion_delay - alert_delay)
  +            )
  

  
  peut-être mettre un commentaire sur le fait que les deux filtres sont mutuellement exclusifs, (càd que last_account_deletion_alert__isnull=True et last_account_deletion_alert__lte=now - (…) ne peuvent arriver en même temps, ce qui n'est pas une évidence à mon avis).

• Il faut sans doute aussi gérer les cas où la première alerte n'a été envoyée qu'alors que le nombre de jours avant suppression est bien inférieur à ou.clean_unused_accounts_deletion - ou.clean_unused_accounts_alert. Il faut sans doute accorder un délai supplémentaire (parce que ce n'est pas la faute de l'usager si (i) la commande n'est pas exécutée assez fréquemment, ou si (ii) la fréquence est élevée mais la date de première exécution de la commande est proche de user.last_login + timedelta(days=user.ou.clean_unused_accounts_deletion auquel cas l'usager va avoir très peu de temps pour tenir compte de l'alerte).

• Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Paul Marillonnet a écrit :

• Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Ouch non pardon, c'est géré par le to_delete = users.filter(…).

Paul Marillonnet a écrit :

Paul Marillonnet a écrit :

• Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Ouch non pardon, c'est géré par le to_delete = users.filter(…).

En fait non, pas forcément. Encore une fois ça dépend de la fréquence d'exécution de la commande.

Une remarque qui ne me vient à l'esprit que maintenant : je verrais bien des PositiveIntegerField pour les deux champs nouvellement ajoutés à l'OU (au lieu d'IntegerField) pour éviter les soucis d'arithmétique sur les durées et sur les dates dans le code.
Notamment, il n'y a pas d'erreur levée pour un timedelta(days=foo) avec foo entier négatif, même si j'avoue ne pas cerner ce que signifie une durée négative :)

Valentin Deniaud a écrit :

Le vrai problème derrière étant de permettre de mettre un délai d'alerte sans délai de suppression, ou inversement, c'est corrigé.

Ok. Une dernière question à ce sujet, est-ce qu'on n'essaierait pas d'éviter le scénario catastrophe où l'administrateur fonctionnel pense désactiver l'envoi d'alertes et la suppression en mentionnant 0 dans les deux durées dans le formulaire d'édition de l'OU?
Genre remplacer :

for ou in get_ou_model().objects.filter(clean_unused_accounts_alert__isnull=False):

for ou in get_ou_model().objects.filter(clean_unused_accounts_alert__gt=0):

Hum, il me semble que ces trois remarques sont caduques du fait des lignes
[...]
Et du test correspondant, test_clean_unused_account_always_alert.
Parce que le seul cas problématique, ce serait si on disait que le compte allait être supprimé dans 14j et qu'il l'était au bout de 10. Si c'est l'inverse qui se produit, ce n'est pas grave, et pour moi c'est plus important d'avoir un mail carré qui te dise quel est la politique de suppression des vieux comptes qu'un mail ultra précis. D'ailleurs si je pense aux mails du genre que j'ai déjà reçus, c'est toujours comme ça.
Je considère aussi que la commande va s'exécuter tous les jours, si le cron est buggé il faut faire en sorte qu'il ne se produise pas de catastrophe, ce qui normalement est le cas, mais les imprécisions qu'un échec ou deux peuvent induire doivent être négligées pour garder le code le plus lisible possible IMO.
Si au contraire je n'ai pas compris tes remarques le plus efficace est sûrement que tu écrives un test pour montrer ce qui ne marche pas.

Non non c'est très clair, c'est moi qui suis passé trop vite sur ce bout de code que tu cites.

Il faut faire confiance aux tests pour ça, je ne pense pas qu'il faille indiquer dans notre code comment fonctionne l'ORM django, et il y a un commentaire nettement plus important et utile à proximité, donc je passe mon tour là dessus.

Ok pas de souci.

Très bien vu, je corrige et ajoute un morceau de test.

Nickel.