From 8496f9b3ad0d8716332cd9efcba524e50c138071 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 --- .../0009_statically_rename_attributes.py | 33 ++++++++++++ .../0010_delete_renameattributeaction.py | 16 ++++++ src/authentic2_auth_saml/models.py | 12 ----- .../authenticator_detail.html | 5 -- src/authentic2_auth_saml/views.py | 10 +--- tests/test_auth_saml.py | 50 ++++++++++++++++--- tests/test_manager_authenticators.py | 11 ---- tests/test_manager_journal.py | 24 ++++----- 9 files changed, 106 insertions(+), 61 deletions(-) create mode 100644 src/authentic2_auth_saml/migrations/0009_statically_rename_attributes.py create mode 100644 src/authentic2_auth_saml/migrations/0010_delete_renameattributeaction.py diff --git a/src/authentic2_auth_saml/adapters.py b/src/authentic2_auth_saml/adapters.py index 9f6fd4487..b729861d9 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/0009_statically_rename_attributes.py b/src/authentic2_auth_saml/migrations/0009_statically_rename_attributes.py new file mode 100644 index 000000000..1ee4cb60b --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0009_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', '0008_auto_20220913_1105'), + ] + + operations = [ + migrations.RunPython(rename_attributes, reverse_code=migrations.RunPython.noop), + ] diff --git a/src/authentic2_auth_saml/migrations/0010_delete_renameattributeaction.py b/src/authentic2_auth_saml/migrations/0010_delete_renameattributeaction.py new file mode 100644 index 000000000..81a9ac325 --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0010_delete_renameattributeaction.py @@ -0,0 +1,16 @@ +# Generated by Django 2.2.26 on 2022-09-14 12:46 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_auth_saml', '0009_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 7c35d4920..c4a2df5af 100644 --- a/src/authentic2_auth_saml/models.py +++ b/src/authentic2_auth_saml/models.py @@ -214,18 +214,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/templates/authentic2_auth_saml/authenticator_detail.html b/src/authentic2_auth_saml/templates/authentic2_auth_saml/authenticator_detail.html index 41a08989c..9a03b60ba 100644 --- a/src/authentic2_auth_saml/templates/authentic2_auth_saml/authenticator_detail.html +++ b/src/authentic2_auth_saml/templates/authentic2_auth_saml/authenticator_detail.html @@ -13,7 +13,6 @@ {% block extra-tab-buttons %} - {% endblock %} @@ -23,10 +22,6 @@ {% include 'authentic2_auth_saml/related_object_list.html' with object_list=object.attribute_lookups.all model_name='samlattributelookup' %} - - diff --git a/src/authentic2_auth_saml/views.py b/src/authentic2_auth_saml/views.py index 67e2c5b05..95c390f16 100644 --- a/src/authentic2_auth_saml/views.py +++ b/src/authentic2_auth_saml/views.py @@ -12,13 +12,7 @@ from authentic2.manager.views import MediaMixin, TitleMixin from authentic2.utils.misc import redirect_to_login from .forms import RoleChoiceField -from .models import ( - AddRoleAction, - RenameAttributeAction, - SAMLAttributeLookup, - SAMLAuthenticator, - SetAttributeAction, -) +from .models import AddRoleAction, SAMLAttributeLookup, SAMLAuthenticator, SetAttributeAction def login(request, authenticator, *args, **kwargs): @@ -53,7 +47,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 5a87656cb..9215aa0d7 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, @@ -99,12 +98,7 @@ def idp(db): SetAttributeAction.objects.create( authenticator=authenticator, user_field='first_name', - saml_attribute='first_name', - ) - RenameAttributeAction.objects.create( - authenticator=authenticator, - from_name='http://nice/attribute/givenName', - to_name='first_name', + saml_attribute='http://nice/attribute/givenName', ) return authenticator.settings @@ -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', '0008_auto_20220913_1105')] + migrate_to = [('authentic2_auth_saml', '0009_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 cca74fc44..f5b4c81af 100644 --- a/tests/test_manager_authenticators.py +++ b/tests/test_manager_authenticators.py @@ -379,17 +379,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 b454e7a49..f844469d1 100644 --- a/tests/test_manager_journal.py +++ b/tests/test_manager_journal.py @@ -29,7 +29,7 @@ from authentic2.custom_user.models import Profile, ProfileType, User from authentic2.journal import journal from authentic2.models import Service from authentic2_auth_oidc.models import OIDCClaimMapping, OIDCProvider -from authentic2_auth_saml.models import RenameAttributeAction, SAMLAuthenticator +from authentic2_auth_saml.models import SAMLAuthenticator, SetAttributeAction from .utils import login, logout, text_content @@ -61,7 +61,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) + set_attribute_action = SetAttributeAction.objects.create(authenticator=saml_authenticator) oidc_provider = OIDCProvider.objects.create(slug='oidc') oidc_claim_mapping = OIDCClaimMapping.objects.create(provider=oidc_provider) @@ -311,10 +311,10 @@ def events(db, freezer): 'authenticator.saml.related_object.creation', user=agent, session=session2, - related_object=rename_attribute_action, + related_object=set_attribute_action, ) action_edit_form = mock.Mock(spec=['instance', 'initial', 'changed_data', 'cleaned_data']) - action_edit_form.instance = rename_attribute_action + action_edit_form.instance = set_attribute_action action_edit_form.initial = {'from_name': 'old'} action_edit_form.changed_data = ['from_name'] action_edit_form.cleaned_data = {'from_name': 'new'} @@ -323,7 +323,7 @@ def events(db, freezer): 'authenticator.saml.related_object.deletion', user=agent, session=session2, - related_object=rename_attribute_action, + related_object=set_attribute_action, ) make('authenticator.oidc.claim.creation', user=agent, session=session2, claim=oidc_claim_mapping) claim_edit_form = mock.Mock(spec=['instance', 'initial', 'changed_data', 'cleaned_data']) @@ -369,7 +369,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() + set_attribute_action = SetAttributeAction.objects.get() oidc_claim_mapping = OIDCClaimMapping.objects.get() # remove event about admin login @@ -727,22 +727,22 @@ def test_global_journal(app, superuser, events): 'user': 'agent', }, { - 'message': 'creation of RenameAttributeAction (%s) in authenticator "SAML"' - % rename_attribute_action.pk, + 'message': 'creation of SetAttributeAction (%s) in authenticator "SAML"' + % set_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)' - % rename_attribute_action.pk, + 'message': 'edit SetAttributeAction (%s) in authenticator "SAML" (from_name)' + % set_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"' - % rename_attribute_action.pk, + 'message': 'deletion of SetAttributeAction (%s) in authenticator "SAML"' + % set_attribute_action.pk, 'timestamp': 'Jan. 3, 2020, 10 a.m.', 'type': 'authenticator.saml.related_object.deletion', 'user': 'agent', -- 2.30.2