Project

General

Profile

Bug #20513

Ajouter une permission explicite pour gérer les membres d'un rôle

Added by Benjamin Dauvergne about 2 years ago. Updated about 2 months ago.

Status:
Solution proposée
Priority:
Normal
Category:
-
Target version:
-
Start date:
08 Dec 2017
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Les rôles d'administration des rôles ne devraient pas avoir comme permission admin sur le rôle administré, mais une permission plus fine nommée manager-members.

En plus d'ajouter cette permission et de modifier le code de création des rôles d'administration, il faudra une migration lancée par le signal post_migrate pour corriger tous les rôles d'administration des rôles existants.

0001-manager-add-manage-members-permission-for-role-admin.patch View (5.82 KB) Valentin Deniaud, 07 Oct 2019 11:11 AM

0002-a2_rbac-update-role-admins-using-post_migrate-signal.patch View (4.8 KB) Valentin Deniaud, 07 Oct 2019 03:53 PM

0001-manager-add-manage-members-permission-for-role-admin.patch View (5.81 KB) Valentin Deniaud, 07 Oct 2019 03:53 PM

0002-a2_rbac-update-role-admins-using-post_migrate-signal.patch View (4.67 KB) Valentin Deniaud, 15 Oct 2019 11:40 AM

0001-manager-add-manage-members-permission-for-role-admin.patch View (5.87 KB) Valentin Deniaud, 15 Oct 2019 11:40 AM

0004-manager-use-new-manage_members-permission-20513.patch View (9.94 KB) Valentin Deniaud, 22 Oct 2019 06:04 PM

0003-a2_rbac-update-role-admins-using-post_migrate-signal.patch View (4.64 KB) Valentin Deniaud, 22 Oct 2019 06:04 PM

0002-a2_rbac-add-manage-members-permission-for-role-admin.patch View (3.73 KB) Valentin Deniaud, 22 Oct 2019 06:04 PM

0001-Revert-manager-do-not-use-has_any_perm-to-get-add-pe.patch View (1.02 KB) Valentin Deniaud, 22 Oct 2019 06:04 PM

0004-manager-use-new-manage_members-permission-20513.patch View (11.8 KB) Valentin Deniaud, 29 Oct 2019 11:12 AM

0003-a2_rbac-update-role-admins-using-post_migrate-signal.patch View (4.8 KB) Valentin Deniaud, 30 Oct 2019 05:13 PM


Related issues

Related to Authentic 2 - Bug #20512: BO: le lien de création des rôles s'affiche aux utilisateurs qui ne peuvent administrer qu'un rôle et certainement pas en ajouter Fermé 08 Dec 2017
Blocks Authentic 2 - Development #37187: manager, affichage/lecture seule pour les rôles pilotés depuis un annuaire LDAP Solution proposée 24 Oct 2019

History

#1 Updated by Benjamin Dauvergne about 2 years ago

  • Related to Bug #20512: BO: le lien de création des rôles s'affiche aux utilisateurs qui ne peuvent administrer qu'un rôle et certainement pas en ajouter added

#2 Updated by Valentin Deniaud 2 months ago

  • Assignee changed from Benjamin Dauvergne to Valentin Deniaud

Je vais essayer de regarder, ça m'avait gêné sur l'auth multi-facteurs et le fix temporaire n'est pas terrible (il se contente de masquer un bouton ie l'admin d'un rôle peut toujours s'amuser à en ajouter en allant direct sur /manage/roles/add).

#3 Updated by Valentin Deniaud 2 months ago

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 ?

#4 Updated by Benjamin Dauvergne 2 months ago

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.

#5 Updated by Valentin Deniaud 2 months ago

Modif de settings.py dans le patch 1 pour enlever la permission de supprimer, et ajout du patch 2 avec la migration.

#6 Updated by Lauréline Guerin 2 months ago

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

#7 Updated by Benjamin Dauvergne 2 months ago

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.

#8 Updated by Valentin Deniaud 2 months ago

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.

#9 Updated by Valentin Deniaud 2 months ago

En attendant j'ai fixé mon test, ça devrait être bon (merci Lauréline c'était bien le flush le coupable).

#10 Updated by Benjamin Dauvergne 2 months ago

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.

#11 Updated by Benjamin Dauvergne about 2 months ago

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.

Aussi il faudrait vérifier dans le manager :
  • 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

#12 Updated by Valentin Deniaud about 2 months ago

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.

#13 Updated by Benjamin Dauvergne about 2 months ago

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.

#14 Updated by Valentin Deniaud about 2 months ago

  • 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 ?

#15 Updated by Benjamin Dauvergne about 2 months ago

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.

Mais donc juste pour savoir que c'est couvert pour plus tard il y aurait à tester :
  • 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.

#16 Updated by Valentin Deniaud about 2 months ago

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.

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.

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

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

#17 Updated by Benjamin Dauvergne about 2 months ago

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.

#18 Updated by Valentin Deniaud about 2 months ago

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.

#20 Updated by Benjamin Dauvergne about 2 months ago

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.

#21 Updated by Valentin Deniaud about 2 months ago

Ah oui, gros bug. get_admin_role récupère ou crée le rôle à partir de la permission (admin_role = get_mirror_role(perm, ...)) donc nouvelle permission ==> nouveau rôle. Et moi je me contente d'affecter la nouvelle permission à l'ancien rôle, donc ça marche sauf que les administrateurs passés et futurs n'auront pas le même rôle (mais les mêmes permissions, ça aurait pas été drôle à débugger).

Le fix, pas compliqué :

         admin_role = administered_role.get_admin_role()
         new_perm = admin_role.permissions.get(operation__slug=MANAGE_MEMBERS_OP.slug)
+        admin_role.delete()
+        role.admin_scope_id = new_perm.pk
+        role.save()
         role.permissions.remove(old_perm)
         role.permissions.add(new_perm)
+        assert role.pk == administered_role.get_admin_role().pk

#22 Updated by Valentin Deniaud 27 days ago

  • Blocks Development #37187: manager, affichage/lecture seule pour les rôles pilotés depuis un annuaire LDAP added

Also available in: Atom PDF