Project

General

Profile

Development #26909

clean-unused-account doit dépendre d'une configuration lié à l'OU

Added by Benjamin Dauvergne 7 months ago. Updated 19 days ago.

Status:
En cours
Priority:
Normal
Category:
-
Target version:
-
Start date:
02 Oct 2018
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Par défaut on ne supprime rien.

Les OUs doivent contenir un champ délai avant suppression sur inactivité, D, donné en jours et une délai idem en jours pour envoyer un mail de rappel signalant une future dé-activation, R.

clean-unused-account doit envoyer deux mails: D-R jours après le dernier login un mail indiquant que le compte va être supprimé, ce mail doit être adaptable en fonction de l'ou (account-deletion-on-inactivity-reminder-<ou.slug>.html) puis D jours après le dernier login un mail signalant la suppression doit être envoyé.

0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch View (7.19 KB) Paul Marillonnet, 26 Mar 2019 05:39 PM

0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch View (10.3 KB) Paul Marillonnet, 27 Mar 2019 04:49 PM

0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch View (15 KB) Paul Marillonnet, 29 Mar 2019 04:16 PM

0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch View (16.7 KB) Paul Marillonnet, 29 Mar 2019 06:29 PM

0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch View (17.2 KB) Paul Marillonnet, 29 Mar 2019 07:30 PM


Related issues

Related to Publik - Development #26907: Cycle de vie des comptes Nouveau 02 Oct 2018

History

#2 Updated by Benjamin Dauvergne 7 months ago

#3 Updated by Paul Marillonnet 6 months ago

  • Assignee set to Paul Marillonnet

#4 Updated by Paul Marillonnet 5 months ago

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

#5 Updated by Benjamin Dauvergne 5 months ago

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

#6 Updated by Paul Marillonnet 5 months ago

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 ?

#7 Updated by Benjamin Dauvergne 5 months ago

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).

#8 Updated by Benjamin Dauvergne about 1 month ago

  • Tracker changed from Support to Development

#9 Updated by Paul Marillonnet 25 days ago

Benjamin Dauvergne a écrit :

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).

Voilà rapidement (en pseudo-code) ce que j'ai en tête.
Le code, simplifié (peut-être à outrance ?), ne supporte plus des périodes à respecter entre mails d'avertissement.
Au contraire il assume que :
- la commande est exécutée une fois par jour au moins (par un cron quotidien, par exemple), car si ce n'est pas le cas certains usagers ne vont pas recevoir le mail d'avertissement.
- en particulier, si la commande est exécutée alors qu'un usager ne s'est pas loggué depuis au moins R+1 jours (cf description du ticket), et que cette commande ne s'était pas exécutée lorsque l'usager ne s'était pas loggué depuis R jours exactement, alors cet usager ne recevra pas le mail d'avertissement.

Si ce n'est pas ce qu'on veut, alors il faut revoir le code pour qu'entre deux runs on conserve l'information nécessaire pour que ne survienne pas le cas où un usager voit son compte supprimé alors qu'il n'a pas reçu de mail d'avertissement.

Qu'en dis-tu ?

#10 Updated by Paul Marillonnet 25 days ago

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 ?

#11 Updated by Benjamin Dauvergne 25 days ago

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.

#12 Updated by Benjamin Dauvergne 25 days ago

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.

#13 Updated by Paul Marillonnet 24 days ago

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.

#14 Updated by Paul Marillonnet 24 days ago

#15 Updated by Paul Marillonnet 22 days ago

#16 Updated by Paul Marillonnet 22 days ago

Deux ajustements nécessaires dans tests/test_api.py

#17 Updated by Paul Marillonnet 22 days ago

Des outputs de sérialization JSON écrits en dur dans tests/test_all.py

#18 Updated by Benjamin Dauvergne 19 days ago

  • Status changed from Solution proposée to En cours
  • clean_unused_accounts_from_email est inutile, on ne va pas varier l'email pour chaque type de message.
  • clean_unused_accounts_filter aussi, on restreint déjà par OU, si besoin de plus on verra une autre fois.
  • 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 je serait pour un champ unique dérivé de CharField contenant une liste de nombres séparés par des virgules, ex.: 150, 300, 1 ça veut die une alerte au bout de 150 jours puis une autre à 300 puis suppression après 1j, il suffit toujours de conserver la date de la dernière alerte. C'est assez simple1.
  • 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 (logger juste en warning les erreurs SMTP, temporiser puis continuer).

1 https://stackoverflow.com/questions/45266996/django-field-that-stores-a-list-as-a-comma-separated-string il faut prévoir le FormField aussi.

Also available in: Atom PDF