From 6861a12414e00d60f3cc07feb6d2cca7b18740ab Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Wed, 24 Aug 2022 17:06:10 +0200 Subject: [PATCH] auth_saml: remove rename attribute action (#68383) --- src/authentic2_auth_saml/adapters.py | 6 --- .../0010_statically_rename_attributes.py | 33 ++++++++++++ .../0011_delete_renameattributeaction.py | 16 ++++++ src/authentic2_auth_saml/models.py | 12 ----- src/authentic2_auth_saml/views.py | 10 +--- tests/test_auth_saml.py | 54 +++++++++++++++---- tests/test_manager_authenticators.py | 11 ---- tests/test_manager_journal.py | 12 ++--- 8 files changed, 102 insertions(+), 52 deletions(-) create mode 100644 src/authentic2_auth_saml/migrations/0010_statically_rename_attributes.py create mode 100644 src/authentic2_auth_saml/migrations/0011_delete_renameattributeaction.py diff --git a/src/authentic2_auth_saml/adapters.py b/src/authentic2_auth_saml/adapters.py index 9f6fd448..b729861d 100644 --- a/src/authentic2_auth_saml/adapters.py +++ b/src/authentic2_auth_saml/adapters.py @@ -111,15 +111,9 @@ class AuthenticAdapter(DefaultAdapter): def provision_a2_attributes(self, user, idp, saml_attributes): saml_attributes = saml_attributes.copy() - self.rename_attributes(idp, saml_attributes) self.set_attributes(user, idp, saml_attributes) self.action_add_role(user, idp, saml_attributes) - def rename_attributes(self, idp, saml_attributes): - for action in idp['authenticator'].rename_attribute_actions.all(): - if action.from_name in saml_attributes: - saml_attributes[action.to_name] = saml_attributes[action.from_name] - def set_attributes(self, user, idp, saml_attributes): user_modified = False for action in idp['authenticator'].set_attribute_actions.all(): diff --git a/src/authentic2_auth_saml/migrations/0010_statically_rename_attributes.py b/src/authentic2_auth_saml/migrations/0010_statically_rename_attributes.py new file mode 100644 index 00000000..73f17eec --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0010_statically_rename_attributes.py @@ -0,0 +1,33 @@ +# Generated by Django 2.2.26 on 2022-08-24 14:55 + +import logging + +from django.db import migrations + +logger = logging.getLogger('authentic2.auth_saml') + + +def rename_attributes(apps, schema_editor): + SAMLAuthenticator = apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + + for authenticator in SAMLAuthenticator.objects.all(): + rename_map = {x.to_name: x.from_name for x in authenticator.rename_attribute_actions.all()} + + for action in authenticator.set_attribute_actions.all(): + action.saml_attribute = rename_map.get(action.saml_attribute, action.saml_attribute) + action.save() + + for lookup in authenticator.attribute_lookups.all(): + lookup.saml_attribute = rename_map.get(lookup.saml_attribute, lookup.saml_attribute) + lookup.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_auth_saml', '0009_rename_attribute_field'), + ] + + operations = [ + migrations.RunPython(rename_attributes, reverse_code=migrations.RunPython.noop), + ] diff --git a/src/authentic2_auth_saml/migrations/0011_delete_renameattributeaction.py b/src/authentic2_auth_saml/migrations/0011_delete_renameattributeaction.py new file mode 100644 index 00000000..e9202c92 --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0011_delete_renameattributeaction.py @@ -0,0 +1,16 @@ +# Generated by Django 2.2.26 on 2022-08-24 15:07 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_auth_saml', '0010_statically_rename_attributes'), + ] + + operations = [ + migrations.DeleteModel( + name='RenameAttributeAction', + ), + ] diff --git a/src/authentic2_auth_saml/models.py b/src/authentic2_auth_saml/models.py index 9b7cb1c3..7e20e0cf 100644 --- a/src/authentic2_auth_saml/models.py +++ b/src/authentic2_auth_saml/models.py @@ -221,18 +221,6 @@ class SAMLRelatedObjectBase(models.Model): return SelectAttributeWidget.get_options().get(self.user_field, self.user_field) -class RenameAttributeAction(SAMLRelatedObjectBase): - from_name = models.CharField(_('From'), max_length=1024) - to_name = models.CharField(_('To'), max_length=64) - - class Meta: - default_related_name = 'rename_attribute_actions' - verbose_name = _('Rename an attribute') - - def __str__(self): - return '%s → %s' % (self.from_name, self.to_name) - - class SAMLAttributeLookup(SAMLRelatedObjectBase): user_field = models.CharField(_('User field'), max_length=256) saml_attribute = models.CharField(_('SAML attribute'), max_length=1024) diff --git a/src/authentic2_auth_saml/views.py b/src/authentic2_auth_saml/views.py index 5d052cc0..269a3e98 100644 --- a/src/authentic2_auth_saml/views.py +++ b/src/authentic2_auth_saml/views.py @@ -11,13 +11,7 @@ from authentic2.manager.views import MediaMixin, TitleMixin from authentic2.utils.misc import redirect_to_login from .forms import RoleChoiceField, SelectAttributeWidget -from .models import ( - AddRoleAction, - RenameAttributeAction, - SAMLAttributeLookup, - SAMLAuthenticator, - SetAttributeAction, -) +from .models import AddRoleAction, SAMLAttributeLookup, SAMLAuthenticator, SetAttributeAction def login(request, authenticator, *args, **kwargs): @@ -52,7 +46,7 @@ def profile(request, *args, **kwargs): class SAMLAuthenticatorMixin(MediaMixin, TitleMixin): - allowed_models = (SAMLAttributeLookup, RenameAttributeAction, SetAttributeAction, AddRoleAction) + allowed_models = (SAMLAttributeLookup, SetAttributeAction, AddRoleAction) def dispatch(self, request, *args, **kwargs): self.authenticator = get_object_or_404(SAMLAuthenticator, pk=kwargs.get('authenticator_pk')) diff --git a/tests/test_auth_saml.py b/tests/test_auth_saml.py index 5a87656c..36e2b894 100644 --- a/tests/test_auth_saml.py +++ b/tests/test_auth_saml.py @@ -30,7 +30,6 @@ from authentic2.models import Attribute from authentic2_auth_saml.adapters import AuthenticAdapter, MappingError from authentic2_auth_saml.models import ( AddRoleAction, - RenameAttributeAction, SAMLAttributeLookup, SAMLAuthenticator, SetAttributeAction, @@ -101,11 +100,6 @@ def idp(db): user_field='first_name', saml_attribute='first_name', ) - RenameAttributeAction.objects.create( - authenticator=authenticator, - from_name='http://nice/attribute/givenName', - to_name='first_name', - ) return authenticator.settings @@ -122,7 +116,7 @@ def saml_attributes(): 'name_id_format': lasso.SAML2_NAME_IDENTIFIER_FORMAT_PERSISTENT, 'mail': ['john.doe@example.com'], 'title': ['Mr.'], - 'http://nice/attribute/givenName': ['John'], + 'first_name': ['John'], } @@ -155,7 +149,7 @@ def test_apply_attribute_mapping_missing_attribute_logged( caplog, adapter, idp, saml_attributes, title_attribute, user ): caplog.set_level('WARNING') - saml_attributes['http://nice/attribute/givenName'] = [] + saml_attributes['first_name'] = [] adapter.provision_a2_attributes(user, idp, saml_attributes) assert re.match('.*no value.*first_name', caplog.records[-1].message) @@ -163,7 +157,7 @@ def test_apply_attribute_mapping_missing_attribute_logged( def test_apply_attribute_mapping_missing_attribute_exception( adapter, idp, saml_attributes, title_attribute, user, rf ): - saml_attributes['http://nice/attribute/givenName'] = [] + saml_attributes['first_name'] = [] SetAttributeAction.objects.filter(user_field='first_name').update(mandatory=True) with pytest.raises(MappingError, match='no value'): adapter.provision_a2_attributes(user, idp, saml_attributes) @@ -679,3 +673,45 @@ def test_saml_authenticator_data_migration_json_fields_log_errors(migration, set 'lookup by attributes for SAMLAuthenticator object (%s): [{"user_field": "email", "saml_attribute": "email"}]' % authenticator.pk, ] + + +def test_saml_authenticator_data_migration_rename_attributes(migration, settings): + migrate_from = [('authentic2_auth_saml', '0009_rename_attribute_field')] + migrate_to = [('authentic2_auth_saml', '0010_statically_rename_attributes')] + + old_apps = migration.before(migrate_from) + SAMLAuthenticator = old_apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + RenameAttributeAction = old_apps.get_model('authentic2_auth_saml', 'RenameAttributeAction') + SetAttributeAction = old_apps.get_model('authentic2_auth_saml', 'SetAttributeAction') + SAMLAttributeLookup = old_apps.get_model('authentic2_auth_saml', 'SAMLAttributeLookup') + + authenticator = SAMLAuthenticator.objects.create(slug='idp1') + RenameAttributeAction.objects.create( + authenticator=authenticator, from_name='http://nice/attribute/givenName', to_name='first_name' + ) + SAMLAttributeLookup.objects.create( + authenticator=authenticator, user_field='first_name', saml_attribute='first_name' + ) + SAMLAttributeLookup.objects.create( + authenticator=authenticator, user_field='title', saml_attribute='title' + ) + SetAttributeAction.objects.create( + authenticator=authenticator, user_field='first_name', saml_attribute='first_name' + ) + SetAttributeAction.objects.create(authenticator=authenticator, user_field='title', saml_attribute='title') + + new_apps = migration.apply(migrate_to) + SAMLAuthenticator = new_apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + authenticator = SAMLAuthenticator.objects.get() + + attribute_lookup1, attribute_lookup2 = authenticator.attribute_lookups.all().order_by('pk') + assert attribute_lookup1.saml_attribute == 'http://nice/attribute/givenName' + assert attribute_lookup1.user_field == 'first_name' + assert attribute_lookup2.saml_attribute == 'title' + assert attribute_lookup2.user_field == 'title' + + set_attribute1, set_attribute2 = authenticator.set_attribute_actions.all().order_by('pk') + assert set_attribute1.saml_attribute == 'http://nice/attribute/givenName' + assert set_attribute1.user_field == 'first_name' + assert set_attribute2.saml_attribute == 'title' + assert set_attribute2.user_field == 'title' diff --git a/tests/test_manager_authenticators.py b/tests/test_manager_authenticators.py index dba28f09..3139f667 100644 --- a/tests/test_manager_authenticators.py +++ b/tests/test_manager_authenticators.py @@ -323,17 +323,6 @@ def test_authenticators_saml_attribute_lookup(app, superuser): assert_event('authenticator.saml.related_object.deletion', user=superuser, session=app.session) -def test_authenticators_saml_rename_attribute(app, superuser): - authenticator = SAMLAuthenticator.objects.create(metadata='meta1.xml', slug='idp1') - resp = login(app, superuser, path=authenticator.get_absolute_url()) - - resp = resp.click('Add', href='renameattributeaction') - resp.form['from_name'] = 'a' - resp.form['to_name'] = 'b' - resp = resp.form.submit().follow() - assert 'a → b' in resp.text - - def test_authenticators_saml_set_attribute(app, superuser): authenticator = SAMLAuthenticator.objects.create(metadata='meta1.xml', slug='idp1') resp = login(app, superuser, path=authenticator.get_absolute_url()) diff --git a/tests/test_manager_journal.py b/tests/test_manager_journal.py index 66990dc4..d43ff03e 100644 --- a/tests/test_manager_journal.py +++ b/tests/test_manager_journal.py @@ -28,7 +28,7 @@ from authentic2.apps.journal.models import Event, _registry from authentic2.custom_user.models import Profile, ProfileType, User from authentic2.journal import journal from authentic2.models import Service -from authentic2_auth_saml.models import RenameAttributeAction, SAMLAuthenticator +from authentic2_auth_saml.models import SAMLAuthenticator, SetAttributeAction from .utils import login, logout, text_content @@ -60,7 +60,7 @@ def events(db, freezer): service = Service.objects.create(name="service") authenticator = LoginPasswordAuthenticator.objects.create(slug='test') saml_authenticator = SAMLAuthenticator.objects.create(slug='saml') - rename_attribute_action = RenameAttributeAction.objects.create(authenticator=saml_authenticator) + rename_attribute_action = SetAttributeAction.objects.create(authenticator=saml_authenticator) class EventFactory: date = make_aware(datetime.datetime(2020, 1, 1)) @@ -358,7 +358,7 @@ def extract_journal(response): def test_global_journal(app, superuser, events): response = login(app, user=superuser, path="/manage/") - rename_attribute_action = RenameAttributeAction.objects.get() + rename_attribute_action = SetAttributeAction.objects.get() # remove event about admin login Event.objects.filter(user=superuser).delete() @@ -715,21 +715,21 @@ def test_global_journal(app, superuser, events): 'user': 'agent', }, { - 'message': 'creation of RenameAttributeAction (%s) in authenticator "SAML"' + 'message': 'creation of SetAttributeAction (%s) in authenticator "SAML"' % rename_attribute_action.pk, 'timestamp': 'Jan. 3, 2020, 8 a.m.', 'type': 'authenticator.saml.related_object.creation', 'user': 'agent', }, { - 'message': 'edit RenameAttributeAction (%s) in authenticator "SAML" (from_name)' + 'message': 'edit SetAttributeAction (%s) in authenticator "SAML" (from_name)' % rename_attribute_action.pk, 'timestamp': 'Jan. 3, 2020, 9 a.m.', 'type': 'authenticator.saml.related_object.edit', 'user': 'agent', }, { - 'message': 'deletion of RenameAttributeAction (%s) in authenticator "SAML"' + 'message': 'deletion of SetAttributeAction (%s) in authenticator "SAML"' % rename_attribute_action.pk, 'timestamp': 'Jan. 3, 2020, 10 a.m.', 'type': 'authenticator.saml.related_object.deletion', -- 2.30.2