Produits Entr'ouvert » Authentic 2

Valentin Deniaud a écrit :

Déjà la partie qui ajoute la permission, reste la migration.
Le ticket sous-entend un peu qu'elle doit permettre seulement de toucher à la liste des membres, or actuellement être admin d'un rôle permet aussi de le supprimer. C'est désormais trivial de faire autrement, mais je suppose qu'on laisse comme c'est ?

Non justement on voudrait que les admins de rôles n'aient plus que le droit de modifier la liste des membres, il faudrait faire en sorte que ça évolue tout seul (je n'ai pas encore lu le patch c'est peut-être déjà le cas) via une migration.

Pourquoi faire une migration "à la main" sur un post_migrate, au lieu d'une data migration ?

Lauréline Guerin a écrit :

Pourquoi faire une migration "à la main" sur un post_migrate, au lieu d'une data migration ?

J'ai oublié, faisons une data migration.

Benjamin Dauvergne a écrit :

J'ai oublié

Pourrait-ce être parce que les rôles d'administration sont créés par des méthodes custom qui totalisent au final plus d'une centaine de lignes, avec en bonus des branchements conditionnels sur des settings, méthodes auxquelles on a pas accès avec les modèles historiques dans une data migration ? Donc data migration => réécrire plein de code bizarre pour un résultat pas garanti, post_migrate => 20 fois moins de code, 20 fois moins de risque d'erreur.

Valentin Deniaud a écrit :

Benjamin Dauvergne a écrit :

J'ai oublié

Pourrait-ce être parce que les rôles d'administration sont créés par des méthodes custom qui totalisent au final plus d'une centaine de lignes, avec en bonus des branchements conditionnels sur des settings, méthodes auxquelles on a pas accès avec les modèles historiques dans une data migration ? Donc data migration => réécrire plein de code bizarre pour un résultat pas garanti, post_migrate => 20 fois moins de code, 20 fois moins de risque d'erreur.

Ah oui effectivement sans les Manager customisé ça doit être un peu chiant.

Un souci dans l'héritage des permissions, quelqu'un qui a manage_members ne devrait pas avoir change; c'est justement l'objectif ici, ne pouvoir changer que les membres et pas le nom du rôle.

• que les boutons permissions et éditer sur la page d'un rôle ne sont plus disponible quand on est juste administrateur d'un rôle
• qu'on peut toujours :
  • faire hériter des membres d'un autre rôle X un rôle Y quand on est administrateur du rôle Y
  • faire hériter des permissions d'un rôle X un rôle Y quand on est administrateur du rôle X

Je fais ça. Comme tu ne les as pas incluses dans ta liste je suppose que les deux autres options sous « paramètres avancés », c'est à dire ajouter un administrateur au rôle et ajouter un rôle administrateur, doivent elles aussi être inaccessibles, dis moi si j'ai pas bon.

Valentin Deniaud a écrit :

Je fais ça. Comme tu ne les as pas incluses dans ta liste je suppose que les deux autres options sous « paramètres avancés », c'est à dire ajouter un administrateur au rôle et ajouter un rôle administrateur, doivent elles aussi être inaccessibles, dis moi si j'ai pas bon.

Yep, ça aussi c'est réservé à la permission change.

• qu'on peut toujours :
  • faire hériter des membres d'un autre rôle X un rôle Y quand on est administrateur du rôle Y
  • faire hériter des permissions d'un rôle X un rôle Y quand on est administrateur du rôle X

Plot twist, ça n'a jamais marché, en tout cas dans l'interface (la permission admin d'un rôle telle qu'elle existe actuellement ne donnant pas le droit de lister les rôles).

Donc ajouter un view_role_perm de manière analogue à view_user_perm dans a2_rbac.models.Role.get_admin_role ?

Valentin Deniaud a écrit :

• qu'on peut toujours :
  • faire hériter des membres d'un autre rôle X un rôle Y quand on est administrateur du rôle Y
  • faire hériter des permissions d'un rôle X un rôle Y quand on est administrateur du rôle X

Plot twist, ça n'a jamais marché, en tout cas dans l'interface (la permission admin d'un rôle telle qu'elle existe actuellement ne donnant pas le droit de lister les rôles).
Donc ajouter un view_role_perm de manière analogue à view_user_perm dans a2_rbac.models.Role.get_admin_role ?

Ce n'est pas gênant pour ce ticket, dans les faits soit on est administrateur de tous les rôles d'un OU ou d'un a2 complet, soit on est juste administrateurs des membres d'un rôle. Le cas administrateur de tout sur un seul rôle n'a pas de sens.

• j'ai la permission role_admin sur tout une OU U1
  • je peux tout faire
• j'ai la permission role_manage_members sur un rôle R1 de l'OU U1:
  • je vois bien ce rôle dans le listing des rôles
  • je vois les utilisateurs de U1 et je peux les affecter/retirer de R1
  • si j'ai la visibilité sur un rôle R2 (donnée fortuitement sur le test):
    • je peux hériter de ses membres sur la page d'administration de R1
    • je peux hériter de ses permissions sur la page d'administration de R2

On pourra réfléchir à view_role_perm sur un autre ticket je pense.

Valentin Deniaud a écrit :

OK je comprends pourquoi ça marche comme ça. Voici donc un nouveau patch avec le boulot dans le manager et les tests. Pas de changement autre part si ce n'est faire hériter les permissions change et admin de manage_members. J'ai aussi réorganisé les commits parce que je commençais à m'y perdre.

Ok je vais relire ça.

En passant, je suis pas fan de la sécurité « par l'affichage » (en l'état ça permet pas exemple d'hériter des membres d'un rôle x qu'on a pas le droit de voir si on en connaît l'id, ou plus grave d'hériter de ses permissions), mais je ne vais pas y toucher dans ce ticket sauf indication contraire.

Normalement non mais je veux bien que tu montres que c'est possible, il y a revalidation de l'id soumi par rapport au queryset qui le restreint.

• j'ai la permission role_admin sur tout une OU U1
  • je peux tout faire

Pas compris ce que ça avait à voir là dedans ?

Rien, juste si le test n'existe pas encore c'était l'occasion de l'avoir parce que c'est dans le même style.

Pendant que j'y pense, le dernier patch qui modifie le manager est incomplet, je n'ai pas fait le tour des vues qui permettent de modifier les membres d'un rôle (typiquement celles dans /manage/users/). Les ajouts seront mineurs, je laisse en Solution Proposée.

Il y a un truc que je ne comprends pas dans ce code :

        old_perm = role.permissions.get(operation__slug=ADMIN_OP.slug)
        administered_role = old_perm.target
        admin_role = administered_role.get_admin_role()
        new_perm = admin_role.permissions.get(operation__slug=MANAGE_MEMBERS_OP.slug)
        role.permissions.remove(old_perm)
        role.permissions.add(new_perm)

en tout logique on devrait avoir admin_role == role, sinon .get_admin_role() a un souci non ?
Je trouverai utile d'avoir des assert ici pour en être sûr.

Ok pour le reste.

(rebasé)

Yep, branche à jour.