Admin: Mise à jour des permissions du groupe itou-admin

[tonial]

🤔 Pourquoi ?

Pour permettre de retirer les droits super utilisateur à un maximum d'utilisateurs.

J'ai regardé les LogEntry avec user__is_superuser=True sur les 6 derniers mois, et non créés par un dev.

Il y a quelques points qui nécessitent discussion:

• Les modèles siae_evalutations -> je pense qu'on peut les mettre en PERMS_READ dans itou-admin seulement (pas utile au pilotage et gps)
• Je ne comprends pas sous quelle condition on peut réussi à supprimer un PASS depuis l'admin (dans mes tests, soit j'avais un pass sans candidature acceptée et ça raise Cannot delete an approval without an accepted job application, soit il y a une candidature acceptée qui pointe sur le PASS en on_delete=restrict, et on ne peut pas supprimer le code)
• les CompanyToken sont-ils modifié par supportix ou le support ?

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

• Mettre à jour le CHANGELOG_breaking_changes.md ?
• Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

[xavfernandez]

nit: réordonner les modèles dans un premier commit et modifier les droits dans un second faciliterait la relecture 👀

[tonial]

J'ai ajouté un commit plus restrictif

[xavfernandez]

Je pense que pilotage-admin* peut également en avoir besoin (et pareil pour `SiaeFinancialAnnex).

[rsebille]

Sanctions je dirais presque sûr que non, et SiaeFinancialAnnex probablement pas besoin non plus.

[xavfernandez]

🙈

[tonial]

ça a été fait sur les 6 derniers mois, cc @rsebille ?

[rsebille]

Je pense qu'on pourrais juste avoir PERM_EDIT 🙈, là comme ça (mais j'ai pas non plus tout mes neurones qui se touchent xD) j'ai pas de cas où on devrais l'autoriser et ça semble assez peu utilisé au final :

SELECT date_trunc('month', action_time), COUNT(*) FROM django_admin_log WHERE content_type_id = 46 AND action_flag = 3 GROUP BY 1 ORDER BY 1 DESC;
       date_trunc       | count 
------------------------+-------
 2025-03-01 00:00:00+00 |     3
 2025-02-01 00:00:00+00 |     1
 2025-01-01 00:00:00+00 |     4
 2024-12-01 00:00:00+00 |     3
 2024-11-01 00:00:00+00 |     7
 2024-10-01 00:00:00+00 |    13
 2024-09-01 00:00:00+00 |    12
 2024-08-01 00:00:00+00 |     4
 2024-07-01 00:00:00+00 |     6
 2024-06-01 00:00:00+00 |     1
 2024-05-01 00:00:00+00 |     3
 2024-04-01 00:00:00+00 |     4
 2024-03-01 00:00:00+00 |     4
 2024-01-01 00:00:00+00 |     3
 2023-12-01 00:00:00+00 |     9
 2023-11-01 00:00:00+00 |     4
 2023-10-01 00:00:00+00 |     8
 2023-09-01 00:00:00+00 |     8
 2023-08-01 00:00:00+00 |    28
 2023-07-01 00:00:00+00 |    21
 2023-06-01 00:00:00+00 |    30
 2023-05-01 00:00:00+00 |     8
 2023-04-01 00:00:00+00 |    35
 2023-03-01 00:00:00+00 |     8
 2023-02-01 00:00:00+00 |     8
 2023-01-01 00:00:00+00 |    16
 2022-12-01 00:00:00+00 |    26
 2022-11-01 00:00:00+00 |    14
 2022-10-01 00:00:00+00 |    27
 2022-09-01 00:00:00+00 |     8
 2022-08-01 00:00:00+00 |    31
 2022-07-01 00:00:00+00 |    25
 2022-06-01 00:00:00+00 |    66
 2022-05-01 00:00:00+00 |    55
 2022-04-01 00:00:00+00 |    73
 2022-03-01 00:00:00+00 |    72
 2022-02-01 00:00:00+00 |    50
 2022-01-01 00:00:00+00 |    31
 2021-12-01 00:00:00+00 |    42
 2021-11-01 00:00:00+00 |   228
 2021-10-01 00:00:00+00 |   191

[tonial]

Je me rends compte que je ne comprends pas trop la construction des listes (cc @rsebille)

On a un always_read_only_models qui sert à lister des modèles avec la permission READ.
Elle est injectée dans les permissions itou-admin et itou-admin-readonly
Jusque là, c'est bon.

Par contre on injecte la permission READ des modèles de cette liste dans les 4 autres groupes, seulement si le modèle est également dans la liste du groupe en question... sauf que si c'est le cas, il a au moins une permission READ 🤔

Je pense qu'il faudrait y mettre les objets auxquels on pense que tout le monde doit avoir accès.
Et les autres vont directement dans les dossiers concernés.

[rsebille]

Je pense qu'on pourrais juste avoir PERM_EDIT 🙈, là comme ça (mais j'ai pas non plus tout mes neurones qui se touchent xD) j'ai pas de cas où on devrais l'autoriser et ça semble assez peu utilisé au final :

SELECT date_trunc('month', action_time), COUNT(*) FROM django_admin_log WHERE content_type_id = 46 AND action_flag = 3 GROUP BY 1 ORDER BY 1 DESC;
       date_trunc       | count 
------------------------+-------
 2025-03-01 00:00:00+00 |     3
 2025-02-01 00:00:00+00 |     1
 2025-01-01 00:00:00+00 |     4
 2024-12-01 00:00:00+00 |     3
 2024-11-01 00:00:00+00 |     7
 2024-10-01 00:00:00+00 |    13
 2024-09-01 00:00:00+00 |    12
 2024-08-01 00:00:00+00 |     4
 2024-07-01 00:00:00+00 |     6
 2024-06-01 00:00:00+00 |     1
 2024-05-01 00:00:00+00 |     3
 2024-04-01 00:00:00+00 |     4
 2024-03-01 00:00:00+00 |     4
 2024-01-01 00:00:00+00 |     3
 2023-12-01 00:00:00+00 |     9
 2023-11-01 00:00:00+00 |     4
 2023-10-01 00:00:00+00 |     8
 2023-09-01 00:00:00+00 |     8
 2023-08-01 00:00:00+00 |    28
 2023-07-01 00:00:00+00 |    21
 2023-06-01 00:00:00+00 |    30
 2023-05-01 00:00:00+00 |     8
 2023-04-01 00:00:00+00 |    35
 2023-03-01 00:00:00+00 |     8
 2023-02-01 00:00:00+00 |     8
 2023-01-01 00:00:00+00 |    16
 2022-12-01 00:00:00+00 |    26
 2022-11-01 00:00:00+00 |    14
 2022-10-01 00:00:00+00 |    27
 2022-09-01 00:00:00+00 |     8
 2022-08-01 00:00:00+00 |    31
 2022-07-01 00:00:00+00 |    25
 2022-06-01 00:00:00+00 |    66
 2022-05-01 00:00:00+00 |    55
 2022-04-01 00:00:00+00 |    73
 2022-03-01 00:00:00+00 |    72
 2022-02-01 00:00:00+00 |    50
 2022-01-01 00:00:00+00 |    31
 2021-12-01 00:00:00+00 |    42
 2021-11-01 00:00:00+00 |   228
 2021-10-01 00:00:00+00 |   191

[rsebille]

Il me manque peut-être (sûrement) du contexte mais le support à parfois besoin de créer des entreprises, par exemple les SIAE en attendant le flux IAE donc est-ce qu'on veux vraiment le forcer en lecture seule sauf pour les superadmin ?
Je suis également étonné de voir d'autre modèle important dans marqué comme always_read_only_models, l'idée initiale de cette variable et du code associé c'est que quelque soit le groupe ça sera RO.

[tonial]

en fait on avait des modèles dans always_read_only_models qui étaient aussi dans group_itou_admin_permissions.

Je pense virer always_read_only_models et être explicite sur les permissions du groupe itou-admin

[rsebille]

Sanctions je dirais presque sûr que non, et SiaeFinancialAnnex probablement pas besoin non plus.

[rsebille]

Je me rends compte que je ne comprends pas trop la construction des listes (cc @rsebille)

On a un always_read_only_models qui sert à lister des modèles avec la permission READ. Elle est injectée dans les permissions itou-admin et itou-admin-readonly Jusque là, c'est bon.

Je viens de voir ton commentaire :/.
L'dée derrière always_read_only_models c'est de lister, et faire en sorte, que les modèles qui ne sont sensé jamais être modifiés par quelqu'un quelque soit sont groupe, donc c'est majoritairement tout ce qui est référentiel mais aussi tout ce qui peux nous servir pour de l'audit.

Je pense que la confusion vient du fait que le code aurais être de la même forme que pour les autres groupes plutôt que de faire un raccourcis :

"itou-admin": {
            **group_itou_admin_permissions,
            **{model: PERMS_READ for model in always_read_only_models if model in group_itou_admin_permissions},
        },
        "itou-admin-readonly": {
            **{model: PERMS_READ for model in group_itou_admin_permissions},
            **{model: PERMS_READ for model in always_read_only_models if model in group_itou_admin_permissions},
        },

Par contre on injecte la permission READ des modèles de cette liste dans les 4 autres groupes, seulement si le modèle est également dans la liste du groupe en question... sauf que si c'est le cas, il a au moins une permission READ 🤔

Oui, c'est pour ne pas donner la lecture seule sur des modèles auxquelles le groupe n'est pas sensé avoir accès ;)

Je pense qu'il faudrait y mettre les objets auxquels on pense que tout le monde doit avoir accès. Et les autres vont directement dans les dossiers concernés.

On pourrais avoir une liste de ce style mais de mémoire c'est assez faible car après tu n'as pas forcément les mêmes permissions et ça me semble plus simple de dupliquer pour chaque groupe comme ça on voit d'un coup à quel modèle il a accès, peut-être pas avec la bonne permission mais ça reste "plat".

[tonial]

Yeah ! je comprends mieux.

Nouvelle version alors :
J'ai mis explicitement dans chaque liste les permissions auxquelles ils ont droit, et j'utilise cette liste always_read_only_models pour raiser une erreur si on met une permission autre que PERMS_READ sur un de ces modèles.
Je trouve ça plus clair à comprendre, et au moins on aura une erreur si on veut donner un accès en écriture à un modèle au lieu de ne pas le faire silencieusement.

Tu en penses quoi ?

[xavfernandez]

Pareil, je l'aurais bien vu dans un commit à part 👼

[rsebille]

Je l'aurais aussi bien vu dans un test ou un check, soit en remplacement soit en ceinture-bretelle au runtime.

[tonial]

Je séparerais les 2 commits quand on sera fixé sur les permissions à changer (les fixups sont un peu pénibles à faire)

Pour l'instant tests/users/test_sync_group_and_perms.py::test_command s'assure que la commande passe et donc qu'on n'a pas de permissions interdites, mais c'est pas super explicite.

Je vais bouger la liste dans un test séparé, mais j'ai un peu peur que ce ne soit pas vraiment tenu à jour 🤷

[rsebille]

Je suis toujours frileux sur le test implicite de comportement, avec le temps on a tendance à oublier et un jour c'est le drame :P. Mais dans le cas actuel j'imagine qu'on à encore 1-3 modifications de code avant de tomber dedans donc sans doute pas critique.

[xavfernandez]

On pourrait directement remettre **always_read_only_models dans group_itou_admin_permissions.

[tonial]

Je ne trouve pas très lisible le fait que les modèles alternent entre une liste et une autre.

[xavfernandez]

A priori toutes les entrées de always_read_only_models sont attendues dans group_itou_admin_permissions.
Donc un peu dommage de dupliquer ces 23 lignes, en les mélangeant avec les autres...

[tonial]

L'idée est aussi de vérifier qu'elles ne sont pas dans les permissions données à gps ou pilotage.
Je commence à me demander si on ne pourrait pas juste virer cette liste en se disant qu'on contrôle de tout manière très bien ce qu'il y a dedans, non ?

[rsebille]

Yeah ! je comprends mieux.

Nouvelle version alors : J'ai mis explicitement dans chaque liste les permissions auxquelles ils ont droit, et j'utilise cette liste always_read_only_models pour raiser une erreur si on met une permission autre que PERMS_READ sur un de ces modèles. Je trouve ça plus clair à comprendre, et au moins on aura une erreur si on veut donner un accès en écriture à un modèle au lieu de ne pas le faire silencieusement.

Tu en penses quoi ?

Que c'est bien mieux ! 😁

[rsebille]

Je l'aurais aussi bien vu dans un test ou un check, soit en remplacement soit en ceinture-bretelle au runtime.

[tonial]

@rsebille @xavfernandez c'est bon, j'ai sorti la vérification dans un test dédié.
Je ne sais pas si c'est vraiment mieux.

Je découperai en commits unitaires quand on aura tranché sur ce test (car les fixup sont trop pénibles à faire pour le moment)

[tonial]

@xavfernandez @rsebille j'y ai perdu un bras, mais normalement c'est clean

[xavfernandez]

Merci pour le joli découpage 🙏