From 53f4fe91bfe2266b403b74e030f665d751941305 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 | 27 +++++++++++++++++++--------
 src/authentic2/manager/forms.py  | 19 +++----------------
 tests/test_role_manager.py       | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/src/authentic2/a2_rbac/models.py b/src/authentic2/a2_rbac/models.py
index a379eaf0..8b435e94 100644
--- a/src/authentic2/a2_rbac/models.py
+++ b/src/authentic2/a2_rbac/models.py
@@ -27,6 +27,8 @@ from django_rbac.models import (RoleAbstractBase, PermissionAbstractBase,
                                 CHANGE_OP, Operation)
 from django_rbac import utils as rbac_utils
 
+from authentic2.decorators import errorcollector
+
 try:
     from django.contrib.contenttypes.fields import GenericForeignKey, \
         GenericRelation
@@ -213,14 +215,23 @@ class Role(RoleAbstractBase):
             self_administered=True)
         return admin_role
 
-    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.')})
+    def validate_unique(self, exclude=None):
+        errors = {}
+
+        with errorcollector(errors):
+            super(Role, self).validate_unique(exclude=exclude)
+
+        exclude = exclude or []
+
+        if 'name' not in exclude:
+            qs = self.__class__.objects.filter(name=self.name, ou=self.ou)
+            if self.pk:
+                qs = qs.exclude(pk=self.pk)
+            if qs.exists():
+                errors.setdefault('name', []).append(_('Name already used'))
+
+        if errors:
+            raise ValidationError(errors)
 
     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 bb80789f..ef65e14c 100644
--- a/src/authentic2/manager/forms.py
+++ b/src/authentic2/manager/forms.py
@@ -241,6 +241,7 @@ class UserEditForm(LimitQuerysetFormMixin, CssClass, BaseUserForm):
                 raise forms.ValidationError({
                     'email': _('Email already used.')
                 })
+        return super(UserEditForm, self).clean()
 
     class Meta:
         model = User
@@ -437,10 +438,10 @@ 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)
+        return super(HideOUFieldMixin, self).clean()
 
 
 class OUSearchForm(FormWithRequest):
@@ -650,20 +651,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/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