From 16ba28029dbb3003bea99727726338f299564f40 Mon Sep 17 00:00:00 2001 From: Paul Marillonnet Date: Thu, 22 Dec 2022 10:05:45 +0100 Subject: [PATCH 1/4] models: drop non-nullity constraint ou API clients' OU (#72703) --- .../migrations/0045_auto_20221222_1013.py | 28 +++++++++++++++++++ src/authentic2/models.py | 2 ++ 2 files changed, 30 insertions(+) create mode 100644 src/authentic2/migrations/0045_auto_20221222_1013.py diff --git a/src/authentic2/migrations/0045_auto_20221222_1013.py b/src/authentic2/migrations/0045_auto_20221222_1013.py new file mode 100644 index 000000000..fae46bea8 --- /dev/null +++ b/src/authentic2/migrations/0045_auto_20221222_1013.py @@ -0,0 +1,28 @@ +# Generated by Django 2.2.26 on 2022-12-22 09:13 + +import django.db.models.deletion +from django.db import migrations, models + +import authentic2.a2_rbac.utils + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2', '0044_apiclient_ou'), + ] + + operations = [ + migrations.AlterField( + model_name='apiclient', + name='ou', + field=models.ForeignKey( + blank=True, + default=authentic2.a2_rbac.utils.get_default_ou_pk, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to='a2_rbac.OrganizationalUnit', + verbose_name='organizational unit', + ), + ), + ] diff --git a/src/authentic2/models.py b/src/authentic2/models.py index ca8378acd..5a952201e 100644 --- a/src/authentic2/models.py +++ b/src/authentic2/models.py @@ -727,6 +727,8 @@ class APIClient(models.Model): swappable=False, on_delete=models.CASCADE, default=get_default_ou_pk, + null=True, + blank=True, ) class Meta: -- 2.39.2 From 88fe8e143a23e6af91456e71fa1b33ea94e48185 Mon Sep 17 00:00:00 2001 From: Paul Marillonnet Date: Thu, 22 Dec 2022 10:20:35 +0100 Subject: [PATCH 2/4] manager: force api client ou assignment for local admins (#72703) --- src/authentic2/manager/apiclient_views.py | 2 ++ tests/test_manager_apiclient.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/src/authentic2/manager/apiclient_views.py b/src/authentic2/manager/apiclient_views.py index 6db6d1f27..b5d468cc2 100644 --- a/src/authentic2/manager/apiclient_views.py +++ b/src/authentic2/manager/apiclient_views.py @@ -55,6 +55,8 @@ class APIClientsFormViewMixin(APIClientsMixin): if self.request.user.has_ou_perm('authentic2.admin_apiclient', ou): allowed_ous.append(ou.id) form.fields['ou'].queryset = OrganizationalUnit.objects.filter(id__in=allowed_ous) + form.fields['ou'].required = True + form.fields['ou'].empty_label = None return form diff --git a/tests/test_manager_apiclient.py b/tests/test_manager_apiclient.py index 0d7077b5b..869b53111 100644 --- a/tests/test_manager_apiclient.py +++ b/tests/test_manager_apiclient.py @@ -165,6 +165,7 @@ def test_add(superuser, app): form = resp.form # password is prefilled assert form.get('password').value + assert ('', False, '---------') in form['ou'].options form.set('name', 'api-client-name') form.set('description', 'api-client-description') form.set('identifier', 'api-client-identifier') @@ -182,12 +183,14 @@ def test_add_local_admin(admin_ou1, app, ou1, ou2): resp = login(app, admin_ou1, 'a2-manager-api-client-add') form = resp.form assert len(form['ou'].options) == 1 + assert ('', False, '---------') not in form['ou'].options assert form['ou'].options[0][2] == 'OU1' role = Role.objects.get(slug='_a2-manager-of-api-clients-%s' % ou2.slug) admin_ou1.roles.add(role) resp = app.get(reverse('a2-manager-api-client-add')) assert len(resp.form['ou'].options) == 2 + assert ('', False, '---------') not in form['ou'].options def test_add_description_non_mandatory(superuser, app): @@ -246,6 +249,7 @@ def test_edit(superuser, app): resp = login(app, superuser, 'a2-manager-api-client-edit', kwargs={'pk': api_client.pk}) form = resp.form assert form.get('password').value == 'foo-password' + assert ('', False, '---------') in form['ou'].options resp.form.set('password', 'easy') response = form.submit().follow() assert urlparse(response.request.url).path == api_client.get_absolute_url() @@ -273,6 +277,7 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2): form = resp.form assert form.get('password').value == 'foo-password' resp.form.set('password', 'easy') + assert ('', False, '---------') not in form['ou'].options response = form.submit().follow() assert urlparse(response.request.url).path == api_client_ou1.get_absolute_url() api_client = APIClient.objects.get(password='easy') @@ -282,6 +287,7 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2): admin_ou1.roles.add(role) resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client_ou2.pk})) assert resp.form.get('password').value == 'bar-password' + assert ('', False, '---------') not in form['ou'].options resp.form.set('ou', ou1.id) resp.form.submit().follow() assert APIClient.objects.filter(ou=ou1).count() == 2 -- 2.39.2 From ed292f6515cf98536ba8620c77e71f1237a94ae3 Mon Sep 17 00:00:00 2001 From: Paul Marillonnet Date: Thu, 22 Dec 2022 11:10:01 +0100 Subject: [PATCH 3/4] manager: filter api client's assignable roles depending on its OU (#72703) --- src/authentic2/manager/apiclient_views.py | 7 +++++- tests/test_manager_apiclient.py | 26 +++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/authentic2/manager/apiclient_views.py b/src/authentic2/manager/apiclient_views.py index b5d468cc2..7fd46048c 100644 --- a/src/authentic2/manager/apiclient_views.py +++ b/src/authentic2/manager/apiclient_views.py @@ -20,7 +20,7 @@ from django.urls import reverse, reverse_lazy from django.utils.translation import gettext_lazy as _ from django.views.generic import CreateView, DeleteView, DetailView, ListView, UpdateView -from authentic2.a2_rbac.models import OrganizationalUnit +from authentic2.a2_rbac.models import OrganizationalUnit, Role from authentic2.manager import forms from authentic2.manager.views import MediaMixin, PermissionMixin, TitleMixin from authentic2.models import APIClient @@ -57,6 +57,11 @@ class APIClientsFormViewMixin(APIClientsMixin): form.fields['ou'].queryset = OrganizationalUnit.objects.filter(id__in=allowed_ous) form.fields['ou'].required = True form.fields['ou'].empty_label = None + api_client = self.object + if api_client and api_client.ou is not None: + form.fields['apiclient_roles'].queryset = Role.objects.filter(ou=api_client.ou).exclude( + slug__startswith='_' + ) return form diff --git a/tests/test_manager_apiclient.py b/tests/test_manager_apiclient.py index 869b53111..acf9abff8 100644 --- a/tests/test_manager_apiclient.py +++ b/tests/test_manager_apiclient.py @@ -241,9 +241,16 @@ def test_detail(superuser, app): assert delete_button.text() == 'Delete' -def test_edit(superuser, app): +def test_edit(superuser, app, ou1, ou2): + role_1 = Role.objects.create(name='role-1', ou=ou1) + role_2 = Role.objects.create(name='role-2', ou=ou2) + role_3 = Role.objects.create(name='role-3', ou=ou1) api_client = APIClient.objects.create( - name='foo', description='foo-description', identifier='foo-identifier', password='foo-password' + name='foo', + description='foo-description', + identifier='foo-identifier', + password='foo-password', + ou=ou1, ) assert APIClient.objects.count() == 1 resp = login(app, superuser, 'a2-manager-api-client-edit', kwargs={'pk': api_client.pk}) @@ -251,6 +258,12 @@ def test_edit(superuser, app): assert form.get('password').value == 'foo-password' assert ('', False, '---------') in form['ou'].options resp.form.set('password', 'easy') + with pytest.raises(KeyError): + # forcing values not presented by the Select2ModelMultipleChoiceField, + # should not happen in UI + form['apiclient_roles'].force_value([role_1.id, role_2.id]) + form.submit() + form['apiclient_roles'].force_value([role_1.id, role_3.id]) response = form.submit().follow() assert urlparse(response.request.url).path == api_client.get_absolute_url() assert APIClient.objects.count() == 1 @@ -259,6 +272,9 @@ def test_edit(superuser, app): def test_edit_local_admin(admin_ou1, app, ou1, ou2): + role_1 = Role.objects.create(name='role-1', ou=ou1) + role_2 = Role.objects.create(name='role-2', ou=ou2) + role_3 = Role.objects.create(name='role-3', ou=ou1) api_client_ou1 = APIClient.objects.create( name='foo', description='foo-description', @@ -278,6 +294,12 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2): assert form.get('password').value == 'foo-password' resp.form.set('password', 'easy') assert ('', False, '---------') not in form['ou'].options + with pytest.raises(KeyError): + # forcing values not presented by the Select2ModelMultipleChoiceField, + # should not happen in UI + form['apiclient_roles'].force_value([role_1.id, role_2.id]) + form.submit() + form['apiclient_roles'].force_value([role_1.id, role_3.id]) response = form.submit().follow() assert urlparse(response.request.url).path == api_client_ou1.get_absolute_url() api_client = APIClient.objects.get(password='easy') -- 2.39.2 From 1d668a16bbad2c8169f64620106a301c1870518b Mon Sep 17 00:00:00 2001 From: Paul Marillonnet Date: Thu, 22 Dec 2022 11:46:46 +0100 Subject: [PATCH 4/4] OU consistency check between api client and roles at validation (#72703) --- src/authentic2/manager/forms.py | 14 ++++++++++++++ tests/test_manager_apiclient.py | 30 ++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/authentic2/manager/forms.py b/src/authentic2/manager/forms.py index 0fc3ba3a3..f46293543 100644 --- a/src/authentic2/manager/forms.py +++ b/src/authentic2/manager/forms.py @@ -939,6 +939,20 @@ class APIClientForm(forms.ModelForm): 'apiclient_roles', ) + def clean(self): + ou = self.cleaned_data['ou'] + if ou: + unauthorized_roles = self.cleaned_data['apiclient_roles'].exclude(ou=ou) + if unauthorized_roles: + unauthorized_roles = ', '.join(unauthorized_roles.values_list('name', flat=True)) + self.add_error( + 'apiclient_roles', + _( + f'The following roles do not belong to organizational unit {ou.name}: {unauthorized_roles}.' + ), + ) + return super().clean() + class Meta: model = APIClient fields = ( diff --git a/tests/test_manager_apiclient.py b/tests/test_manager_apiclient.py index acf9abff8..ac6de0f09 100644 --- a/tests/test_manager_apiclient.py +++ b/tests/test_manager_apiclient.py @@ -20,6 +20,7 @@ import pytest from django.urls import reverse from authentic2.a2_rbac.models import Role +from authentic2.a2_rbac.utils import get_default_ou from authentic2.models import APIClient from .utils import login @@ -159,8 +160,8 @@ def test_list_show_objects_local_admin(admin_ou1, app, ou1, ou2): def test_add(superuser, app): assert APIClient.objects.count() == 0 - role_1 = Role.objects.create(name='role-1') - role_2 = Role.objects.create(name='role-2') + role_1 = Role.objects.create(name='role-1', ou=get_default_ou()) + role_2 = Role.objects.create(name='role-2', ou=get_default_ou()) resp = login(app, superuser, 'a2-manager-api-client-add') form = resp.form # password is prefilled @@ -195,8 +196,8 @@ def test_add_local_admin(admin_ou1, app, ou1, ou2): def test_add_description_non_mandatory(superuser, app): assert APIClient.objects.count() == 0 - role_1 = Role.objects.create(name='role-1') - role_2 = Role.objects.create(name='role-2') + role_1 = Role.objects.create(name='role-1', ou=get_default_ou()) + role_2 = Role.objects.create(name='role-2', ou=get_default_ou()) resp = login(app, superuser, 'a2-manager-api-client-add') form = resp.form form.set('name', 'api-client-name') @@ -270,6 +271,27 @@ def test_edit(superuser, app, ou1, ou2): api_client = APIClient.objects.get(password='easy') assert api_client.identifier == 'foo-identifier' + resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client.pk})) + form = resp.form + form.set('ou', ou2.id) + response = form.submit() + errmsg = response.pyquery('div.error')[0].text + assert "do not belong to organizational unit OU2: role-1, role-3." in errmsg + response.form.set('ou', ou2.id) + response.form['apiclient_roles'].force_value([]) + response.form.submit().follow() + api_client = APIClient.objects.get() + assert set(api_client.apiclient_roles.all()) == set() + assert api_client.ou == ou2 + + resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client.pk})) + form = resp.form + form['apiclient_roles'].force_value([role_2.id]) + response = form.submit().follow() + api_client = APIClient.objects.get() + assert api_client.ou == ou2 + assert set(api_client.apiclient_roles.all()) == {role_2} + def test_edit_local_admin(admin_ou1, app, ou1, ou2): role_1 = Role.objects.create(name='role-1', ou=ou1) -- 2.39.2