From beb6fa19d0e759e42d50ef84f6041eb787e83caa Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Tue, 26 May 2020 17:53:35 +0200 Subject: [PATCH] manager: forbid changing role members when synced from ldap (#37187) --- .../0023_role_manage_members_allowed.py | 20 +++++++++ src/authentic2/a2_rbac/models.py | 4 ++ src/authentic2/backends/ldap_backend.py | 4 ++ src/authentic2/manager/role_views.py | 9 ++++ src/authentic2/manager/tables.py | 4 +- .../authentic2/manager/role_members.html | 5 +++ .../authentic2/manager/user_roles_table.html | 6 ++- tests/test_ldap.py | 41 +++++++++++++++++-- 8 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 src/authentic2/a2_rbac/migrations/0023_role_manage_members_allowed.py diff --git a/src/authentic2/a2_rbac/migrations/0023_role_manage_members_allowed.py b/src/authentic2/a2_rbac/migrations/0023_role_manage_members_allowed.py new file mode 100644 index 00000000..5e9175f8 --- /dev/null +++ b/src/authentic2/a2_rbac/migrations/0023_role_manage_members_allowed.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.18 on 2020-05-27 13:22 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('a2_rbac', '0022_auto_20200402_1101'), + ] + + operations = [ + migrations.AddField( + model_name='role', + name='manage_members_allowed', + field=models.BooleanField(default=True, verbose_name='Allow adding or deleting role members'), + ), + ] diff --git a/src/authentic2/a2_rbac/models.py b/src/authentic2/a2_rbac/models.py index b78a2963..bf6c20ed 100644 --- a/src/authentic2/a2_rbac/models.py +++ b/src/authentic2/a2_rbac/models.py @@ -227,6 +227,10 @@ class Role(RoleAbstractBase): content_type_field='target_ct', object_id_field='target_id') + manage_members_allowed = models.BooleanField( + default=True, + verbose_name=_('Allow adding or deleting role members')) + def get_admin_role(self, create=True): from . import utils diff --git a/src/authentic2/backends/ldap_backend.py b/src/authentic2/backends/ldap_backend.py index b3b27d63..8215a88c 100644 --- a/src/authentic2/backends/ldap_backend.py +++ b/src/authentic2/backends/ldap_backend.py @@ -783,6 +783,10 @@ class LDAPBackend(object): # Remove extra roles elif dn not in role_dns and role in roles: user.roles.remove(role) + if role.manage_members_allowed: + log.info('role %s is now only manageable through LDAP', role) + role.manage_members_allowed = False + role.save() def get_ldap_group_dns(self, user, dn, conn, block, attributes): '''Retrieve group DNs from the LDAP by attributes (memberOf) or by diff --git a/src/authentic2/manager/role_views.py b/src/authentic2/manager/role_views.py index 3d4a522f..8c75abe1 100644 --- a/src/authentic2/manager/role_views.py +++ b/src/authentic2/manager/role_views.py @@ -149,6 +149,14 @@ class RoleMembersView(views.HideOUColumnMixin, RoleViewMixin, views.BaseSubTable def title(self): return self.get_instance_name() + @property + def can_manage_members(self): + return self.object.manage_members_allowed and getattr(self, '_can_manage_members', False) + + @can_manage_members.setter + def can_manage_members(self, value): + self._can_manage_members = value + def get_table_queryset(self): return self.object.all_members() @@ -189,6 +197,7 @@ class RoleMembersView(views.HideOUColumnMixin, RoleViewMixin, views.BaseSubTable ctx['admin_roles'] = views.filter_view(self.request, self.object.get_admin_role().children(include_self=False, annotate=True)) + ctx['from_ldap'] = self._can_manage_members and not self.can_manage_members return ctx members = RoleMembersView.as_view() diff --git a/src/authentic2/manager/tables.py b/src/authentic2/manager/tables.py index 224725f1..8ef2e385 100644 --- a/src/authentic2/manager/tables.py +++ b/src/authentic2/manager/tables.py @@ -122,7 +122,9 @@ class OuUserRolesTable(tables.Table): 'indeterminate{%% endif %%}"' ' name="role-{{ record.pk }}" type="checkbox" {%% if record.member %%}checked{%% endif %%} ' '{%% if not record.has_perm %%}disabled ' - 'title="{%% trans "%s" %%}"{%% endif %%}/>' % ugettext_noop('You are not authorized to manage this role'), + 'title="{%% trans "%s" %%}"{%% endif %%} ' + '{%% if not record.manage_members_allowed %%}disabled ' + 'title="{%% trans "%s" %%}"{%% endif %%}/>' % (ugettext_noop('You are not authorized to manage this role'), ugettext_noop('This role is synchronised from LDAP, changing members is not allowed.')), verbose_name=_('Member'), order_by=('member', 'via', 'name')) diff --git a/src/authentic2/manager/templates/authentic2/manager/role_members.html b/src/authentic2/manager/templates/authentic2/manager/role_members.html index 340eb6c4..6ccfe7bf 100644 --- a/src/authentic2/manager/templates/authentic2/manager/role_members.html +++ b/src/authentic2/manager/templates/authentic2/manager/role_members.html @@ -46,6 +46,11 @@ {% endblock %} {% block main %} + {% if from_ldap %} +
+ {% trans "This role is synchronised from LDAP, changing members is not allowed." %} +
+ {% endif %} {% with row_link=1 url_name="a2-manager-user-detail" %} {% render_table table "authentic2/manager/role_members_table.html" %} {% endwith %} diff --git a/src/authentic2/manager/templates/authentic2/manager/user_roles_table.html b/src/authentic2/manager/templates/authentic2/manager/user_roles_table.html index d173a54f..da7dbc8a 100644 --- a/src/authentic2/manager/templates/authentic2/manager/user_roles_table.html +++ b/src/authentic2/manager/templates/authentic2/manager/user_roles_table.html @@ -7,6 +7,10 @@ {% endblock %} {% block table.tbody.last.column %} - {% if table.context.view.can_change and row.record.member %}{% endif %} + + {% if table.context.view.can_change and row.record.member %} + + {% endif %} + {% endblock %} {% endif %} diff --git a/tests/test_ldap.py b/tests/test_ldap.py index dc7ac488..9553d2b5 100644 --- a/tests/test_ldap.py +++ b/tests/test_ldap.py @@ -34,6 +34,7 @@ from django.utils import timezone from django.utils.six.moves.urllib import parse as urlparse from authentic2.models import Service +from authentic2.a2_rbac.models import Role from authentic2.a2_rbac.utils import get_default_ou from django_rbac.utils import get_ou_model from authentic2.backends import ldap_backend @@ -310,8 +311,6 @@ def test_posix_group_mapping(slapd, settings, client, db): def test_group_to_role_mapping(slapd, settings, client, db): - from authentic2.a2_rbac.models import Role - Role.objects.get_or_create(name='Role1') settings.LDAP_AUTH_SETTINGS = [{ 'url': [slapd.ldap_url], @@ -329,8 +328,6 @@ def test_group_to_role_mapping(slapd, settings, client, db): def test_posix_group_to_role_mapping(slapd, settings, client, db): - from authentic2.a2_rbac.models import Role - Role.objects.get_or_create(name='Role2') settings.LDAP_AUTH_SETTINGS = [{ 'url': [slapd.ldap_url], @@ -348,6 +345,42 @@ def test_posix_group_to_role_mapping(slapd, settings, client, db): assert response.context['user'].roles.count() == 1 +def test_group_to_role_mapping_modify_disabled(slapd, settings, db, app, admin, client): + role = Role.objects.create(name='Role3') + settings.LDAP_AUTH_SETTINGS = [{ + 'url': [slapd.ldap_url], + 'basedn': u'o=ôrga', + 'use_tls': False, + 'group_to_role_mapping': [ + ['cn=group1,o=ôrga', ['Role3']], + ], + }] + response = client.post('/login/', {'login-password-submit': '1', + 'username': USERNAME, + 'password': PASS}, follow=True) + user = response.context['user'] + assert user.roles.count() == 1 + + utils.login(app, admin, '/manage/') + + response = app.get('/manage/users/%s/roles/?search-ou=%s' % (user.pk, user.ou.pk)) + q = response.pyquery.remove_namespaces() + assert q('table tbody td.name').text() == 'Role3' + assert q('table tbody td.member input').attr('disabled') + + response = app.get('/manage/users/%s/roles/?search-ou=all' % user.pk) + q = response.pyquery.remove_namespaces() + assert q('table tbody td.name').text() == 'Role3' + assert q('table tbody td.member input').attr('disabled') + + response = app.get('/manage/roles/%s/' % (role.pk)) + assert 'synchronised from LDAP' in response.text + q = response.pyquery.remove_namespaces() + assert not q('form.manager-m2m-add-form') + assert q('div.role-inheritance .role-add.disabled') + assert not q('table tbody td a.icon-remove-sign js-remove-object') + + def test_group_su(slapd, settings, client, db): from django.contrib.auth.models import Group -- 2.20.1