From f22610c61d2f95330529d6106a5b4eac7d84dc76 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Thu, 13 Jun 2019 22:30:00 +0200
Subject: [PATCH 1/2] manager: always check role's name uniqueness (#33944)

---
 src/authentic2/a2_rbac/models.py              | 11 +++----
 src/authentic2/manager/forms.py               | 17 +---------
 .../manager/locale/fr/LC_MESSAGES/django.po   |  4 ---
 tests/test_role_manager.py                    | 32 +++++++++++++++++++
 4 files changed, 38 insertions(+), 26 deletions(-)

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')})
 
     def save(self, *args, **kwargs):
         # Service roles can only be part of the same ou as the service
diff --git a/src/authentic2/manager/forms.py b/src/authentic2/manager/forms.py
index 3f851ce7..ed339f46 100644
--- a/src/authentic2/manager/forms.py
+++ b/src/authentic2/manager/forms.py
@@ -436,10 +436,9 @@ class HideOUFieldMixin(object):
         if utils.get_ou_count() < 2:
             del self.fields['ou']
 
-    def save(self, *args, **kwargs):
+    def clean(self):
         if 'ou' not in self.fields:
             self.instance.ou = get_default_ou()
-        return super(HideOUFieldMixin, self).save(*args, **kwargs)
 
 
 class OUSearchForm(FormWithRequest):
@@ -649,20 +648,6 @@ class RoleEditForm(SlugMixin, HideOUFieldMixin, LimitQuerysetFormMixin, CssClass
     ou = forms.ModelChoiceField(queryset=get_ou_model().objects,
                                 required=True, label=_('Organizational unit'))
 
-    def clean_name(self):
-        qs = get_role_model().objects.all()
-        if self.instance and self.instance.pk:
-            qs = qs.exclude(pk=self.instance.pk)
-        ou = self.cleaned_data.get('ou')
-        # Test unicity of name for an OU and globally if no OU is present
-        name = self.cleaned_data.get('name')
-        if name and ou:
-            query = Q(name=name) & (Q(ou__isnull=True) | Q(ou=ou))
-            if qs.filter(query).exists():
-                raise ValidationError(
-                    {'name': _('This name is not unique over this organizational unit.')})
-        return name
-
     class Meta:
         model = get_role_model()
         fields = ('name', 'ou', 'description')
diff --git a/src/authentic2/manager/locale/fr/LC_MESSAGES/django.po b/src/authentic2/manager/locale/fr/LC_MESSAGES/django.po
index 00c5c882..9b11ecc4 100644
--- a/src/authentic2/manager/locale/fr/LC_MESSAGES/django.po
+++ b/src/authentic2/manager/locale/fr/LC_MESSAGES/django.po
@@ -133,10 +133,6 @@ msgstr "Aucune"
 msgid "Free text"
 msgstr "Recherche libre"
 
-#: src/authentic2/manager/forms.py:646
-msgid "This name is not unique over this organizational unit."
-msgstr "Ce nom n'est pas unique pour cette collectivité"
-
 #: src/authentic2/manager/forms.py:657 src/authentic2/manager/tables.py:62
 #: src/authentic2/manager/tables.py:86 src/authentic2/manager/tables.py:114
 #: src/authentic2/manager/tables.py:132
diff --git a/tests/test_role_manager.py b/tests/test_role_manager.py
index 1fb8f6d2..6731f870 100644
--- a/tests/test_role_manager.py
+++ b/tests/test_role_manager.py
@@ -35,3 +35,35 @@ def test_manager_role_export(app, admin, ou1, role_ou1, ou2, role_ou2):
     assert export.keys() == ['roles']
     assert len(export['roles']) == 1
     assert export['roles'][0]['slug'] == 'role_ou1'
+
+
+def test_manager_role_name_uniqueness_single_ou(app, admin):
+    response = login(app, admin, 'a2-manager-roles')
+
+    response = response.click('Add')
+    response.form.set('name', 'Role1')
+    response = response.form.submit('Save').follow()
+    response = response.click('Roles')
+    assert response.pyquery('td.name').text() == 'Role1'
+
+    response = response.click('Add')
+    response.form.set('name', 'Role1')
+    response = response.form.submit('Save')
+    assert response.pyquery('.errorlist').eq(1).text() == 'Name already used'
+
+
+def test_manager_role_name_uniqueness_multiple_ou(app, admin, ou1):
+    response = login(app, admin, 'a2-manager-roles')
+
+    response = response.click('Add')
+    response.form.set('ou', str(ou1.id))
+    response.form.set('name', 'Role1')
+    response = response.form.submit('Save').follow()
+    response = response.click('Roles')
+    assert response.pyquery('td.name').text() == 'Role1'
+
+    response = response.click('Add')
+    response.form.set('ou', str(ou1.id))
+    response.form.set('name', 'Role1')
+    response = response.form.submit('Save')
+    assert response.pyquery('.errorlist').eq(1).text() == 'Name already used'
-- 
2.20.1