Project

General

Profile

Development #33944

Deux rôles peuvent avoir le même libellé

Added by Brice Mallet 2 months ago. Updated about 1 month ago.

Status:
Solution déployée
Priority:
Normal
Category:
-
Target version:
-
Start date:
13 Jun 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Sur une instance mono-collectivité (en multicollectivité c'est impossible, on a une erreur "Ce nom n'est pas unique pour cette collectivité") il est possible de créer un rôle qui a le même libellé qu'un rôle déjà existant.

Exemple sur https://connexion-atolcd.test.entrouvert.org/manage/roles/
où deux rôles "Agent" ont pu être créés :
https://connexion-atolcd.test.entrouvert.org/manage/roles/17/
https://connexion-atolcd.test.entrouvert.org/manage/roles/31/

0002-a2_rbac-add-partial-unique-index-on-Role-s-name-3394.patch View (1.28 KB) Benjamin Dauvergne, 13 Jun 2019 11:30 PM

0001-manager-always-check-role-s-name-uniqueness-33944.patch View (5.27 KB) Benjamin Dauvergne, 13 Jun 2019 11:30 PM

0002-a2_rbac-add-partial-unique-index-on-Role-s-name-3394.patch View (1.28 KB) Benjamin Dauvergne, 03 Jul 2019 10:09 AM

0001-manager-always-check-role-s-name-uniqueness-33944.patch View (5.59 KB) Benjamin Dauvergne, 03 Jul 2019 10:09 AM

Associated revisions

Revision df6cebea (diff)
Added by Benjamin Dauvergne about 1 month ago

manager: always check role's name uniqueness (#33944)

Revision 1c6fd888 (diff)
Added by Benjamin Dauvergne about 1 month ago

a2_rbac: add partial unique index on Role's name (#33944)

History

#1 Updated by Benjamin Dauvergne 2 months ago

  • Assignee set to Benjamin Dauvergne

#2 Updated by Benjamin Dauvergne 2 months ago

Corrigé au niveau du modèle, j'ai viré le code ad-hoc sur le formulaire et j'ai corrigé HideOUColumnMixin qui doit poser l'ou sur le modèle dans clean() et pas save() si on veut que les clean() des modèles fonctionnent (et dans l'esprit Django c'est bien dans clean() qu'on doit finir d'initialiser une instance, pas ModelForm.save()).

J'ai regardé du coté de la prod si on avait des doublons de ce genre, j'en ai trouvé deux qu'on devrait pouvoir corriger d'ici que ça passe en prod :

bdauvergne@authentic:/tmp$ cat check_role_s_name_uniqueness.py 
from django.db import connection
from django.db.models import Count

from authentic2.a2_rbac.models import Role

roles = Role.objects.filter(admin_scope_ct__isnull=True).values('ou', 'name').annotate(total=Count('id')).filter(total__gt=1).values_list('ou__name', 'name')

if roles:
    print connection.tenant
    for ou, name in roles:
        print '-', ou, name

bdauvergne@authentic:/tmp$ authentic2-multitenant-manage tenant_command runscript --all-tenants ./check_role_s_name_uniqueness.py 
connexion_mesdemarches_cca_bzh
- Collectivité par défaut Agent traitant DSI
connexion_nancy_fr
- Nancy Finances TH

idem en recette :

bdauvergne@authentic:/tmp$ authentic2-multitenant-manage tenant_command runscript --all-tenants ./check_role_s_name_uniqueness.py 
connexion_atolcd_test_entrouvert_org
- Collectivité par défaut Agent
connexion_nancy_test_entrouvert_org
- Collectivité par défaut Parking Abonnement Beaupré
- Collectivité par défaut Secrétariat Elu 38
connexion_strasbourg_test_entrouvert_org
- Strasbourg Formation – Gestionnaire déchets

Je peux aussi très bien pousser le premier commit et attendre un moment proprice pour le second.

#3 Updated by Paul Marillonnet about 2 months ago

diff --git a/src/authentic2/a2_rbac/models.py b/src/authentic2/a2_rbac/models.py
index a379eaf0..390d2b24 100644
--- a/src/authentic2/a2_rbac/models.py
+++ b/src/authentic2/a2_rbac/models.py
@@ -215,12 +215,11 @@ class Role(RoleAbstractBase):

     def clean(self):
         super(Role, self).clean()
-        if not self.service and not self.admin_scope_ct_id:
-            if not self.id and self.__class__.objects.filter(
-                    name=self.name, ou=self.ou):
-                raise ValidationError(
-                    {'name': _('This name is not unique over this '
-                               'organizational unit.')})
+        qs = self.__class__.objects.filter(name=self.name, ou=self.ou)
+        if self.pk:
+            qs = qs.exclude(pk=self.pk)
+        if qs.exists():
+            raise ValidationError({'name': _('Name already used')})

Puisque c'est d'une contrainte implicite d'unicité sur plusieurs champs du modèle dont il est question ici, je pense qu'on peut aussi renommer cette méthode validate_unique pour coller davantage au processus de validation de modèle de Django.

#4 Updated by Benjamin Dauvergne about 2 months ago

Yep.

#5 Updated by Benjamin Dauvergne about 2 months ago

Au passage je corrige une absence d'appel à super.clean() (quand on surcharge clean on doit toujours finir par un super.clean()).

#6 Updated by Paul Marillonnet about 2 months ago

  • Status changed from Solution proposée to Solution validée

D'ac.

#7 Updated by Benjamin Dauvergne about 1 month ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 1c6fd8886a5bc295581401aaf77afffffaf1f867
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Jun 13 23:23:02 2019 +0200

    a2_rbac: add partial unique index on Role's name (#33944)

commit df6cebea4575e677d5515f53377e0dea44a8f4bf
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Jun 13 22:30:00 2019 +0200

    manager: always check role's name uniqueness (#33944)

#8 Updated by Frédéric Péters about 1 month ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF