From fd0487f3ab2a24d121ebf7e9cf0bdbade928a4aa Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Wed, 26 Oct 2022 12:09:19 +0200 Subject: [PATCH] auth_saml: remove metadata file path field (#70491) --- .../migrations/0013_metadata_file_to_db.py | 46 +++++++++++++++++++ ..._remove_samlauthenticator_metadata_path.py | 17 +++++++ src/authentic2_auth_saml/models.py | 12 ++--- tests/auth_saml/test_login.py | 17 ++----- tests/auth_saml/test_migrations.py | 26 +++++++++++ tests/auth_saml/test_misc.py | 6 +-- tests/test_manager_authenticators.py | 5 +- 7 files changed, 101 insertions(+), 28 deletions(-) create mode 100644 src/authentic2_auth_saml/migrations/0013_metadata_file_to_db.py create mode 100644 src/authentic2_auth_saml/migrations/0014_remove_samlauthenticator_metadata_path.py diff --git a/src/authentic2_auth_saml/migrations/0013_metadata_file_to_db.py b/src/authentic2_auth_saml/migrations/0013_metadata_file_to_db.py new file mode 100644 index 000000000..96380b8ab --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0013_metadata_file_to_db.py @@ -0,0 +1,46 @@ +# Generated by Django 2.2.26 on 2022-10-26 10:01 + +import logging +import os + +from django.db import migrations, transaction + +logger = logging.getLogger('authentic2.auth_saml') + + +def metadata_file_to_db(apps, schema_editor): + SAMLAuthenticator = apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + + def move_data(authenticator): + path = authenticator.metadata_path + + if not os.path.exists(path): + return + + try: + with open(path) as fd: + metadata = fd.read() + except OSError: + logger.warning('auth_saml: open()/read() call failed for %s on %s', authenticator, path) + return + + authenticator.metadata = metadata + authenticator.save() + + for authenticator in SAMLAuthenticator.objects.exclude(metadata_path=''): + try: + with transaction.atomic(): + move_data(authenticator) + except Exception: + logger.exception('auth_saml: could not move metadata file to db for %s', authenticator) + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_auth_saml', '0012_move_add_role_action'), + ] + + operations = [ + migrations.RunPython(metadata_file_to_db, reverse_code=migrations.RunPython.noop), + ] diff --git a/src/authentic2_auth_saml/migrations/0014_remove_samlauthenticator_metadata_path.py b/src/authentic2_auth_saml/migrations/0014_remove_samlauthenticator_metadata_path.py new file mode 100644 index 000000000..3d2708ff5 --- /dev/null +++ b/src/authentic2_auth_saml/migrations/0014_remove_samlauthenticator_metadata_path.py @@ -0,0 +1,17 @@ +# Generated by Django 2.2.26 on 2022-10-26 10:09 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('authentic2_auth_saml', '0013_metadata_file_to_db'), + ] + + operations = [ + migrations.RemoveField( + model_name='samlauthenticator', + name='metadata_path', + ), + ] diff --git a/src/authentic2_auth_saml/models.py b/src/authentic2_auth_saml/models.py index a5d22fb64..856d90b04 100644 --- a/src/authentic2_auth_saml/models.py +++ b/src/authentic2_auth_saml/models.py @@ -33,12 +33,6 @@ class SAMLAuthenticator(BaseAuthenticator): metadata_cache_time = models.PositiveSmallIntegerField(_('Metadata cache time'), default=3600) metadata_http_timeout = models.PositiveSmallIntegerField(_('Metadata HTTP timeout'), default=10) - metadata_path = models.CharField( - _('Metadata file path'), - max_length=300, - help_text=_('Absolute path to the IdP metadata file.'), - blank=True, - ) metadata = models.TextField(_('Metadata (XML)'), blank=True) provision = models.BooleanField(_('Create user if their username does not already exists'), default=True) @@ -143,7 +137,7 @@ class SAMLAuthenticator(BaseAuthenticator): type = 'saml' how = ['saml'] manager_view_template_name = 'authentic2_auth_saml/authenticator_detail.html' - description_fields = ['show_condition', 'metadata_url', 'metadata_path', 'metadata', 'provision'] + description_fields = ['show_condition', 'metadata_url', 'metadata', 'provision'] class Meta: verbose_name = _('SAML') @@ -154,7 +148,7 @@ class SAMLAuthenticator(BaseAuthenticator): settings['AUTHN_CLASSREF'] = [x.strip() for x in settings['AUTHN_CLASSREF'].split(',') if x.strip()] - for setting in ('METADATA', 'METADATA_PATH', 'METADATA_URL'): + for setting in ('METADATA', 'METADATA_URL'): if not settings[setting]: del settings[setting] @@ -187,7 +181,7 @@ class SAMLAuthenticator(BaseAuthenticator): } def clean(self): - if not (self.metadata or self.metadata_path or self.metadata_url): + if not (self.metadata or self.metadata_url): raise ValidationError(_('One of the metadata fields must be filled.')) def autorun(self, request, block_id): diff --git a/tests/auth_saml/test_login.py b/tests/auth_saml/test_login.py index adb03bd04..25ed26a91 100644 --- a/tests/auth_saml/test_login.py +++ b/tests/auth_saml/test_login.py @@ -14,7 +14,6 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -import os from unittest import mock import pytest @@ -57,9 +56,7 @@ def test_providers_on_login_page(db, app, settings): def test_login_with_conditionnal_authenticators(db, app, settings, caplog): - authenticator = SAMLAuthenticator.objects.create( - enabled=True, metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml'), slug='idp1' - ) + authenticator = SAMLAuthenticator.objects.create(enabled=True, metadata='xxx', slug='idp1') response = app.get('/login/') assert 'login-saml-idp1' in response @@ -69,9 +66,7 @@ def test_login_with_conditionnal_authenticators(db, app, settings, caplog): response = app.get('/login/') assert 'login-saml-idp1' not in response - authenticator2 = SAMLAuthenticator.objects.create( - enabled=True, metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml'), slug='idp2' - ) + authenticator2 = SAMLAuthenticator.objects.create(enabled=True, metadata='xxx', slug='idp2') response = app.get('/login/') assert 'login-saml-idp1' not in response assert 'login-saml-idp2' in response @@ -86,13 +81,13 @@ def test_login_with_conditionnal_authenticators(db, app, settings, caplog): def test_login_condition_dnsbl(db, app, settings, caplog): SAMLAuthenticator.objects.create( enabled=True, - metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml'), + metadata='xxx', slug='idp1', show_condition='remote_addr in dnsbl(\'dnswl.example.com\')', ) SAMLAuthenticator.objects.create( enabled=True, - metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml'), + metadata='xxx', slug='idp2', show_condition='remote_addr not in dnsbl(\'dnswl.example.com\')', ) @@ -105,9 +100,7 @@ def test_login_condition_dnsbl(db, app, settings, caplog): def test_login_autorun(db, app, settings, patched_adapter): response = app.get('/login/') - authenticator = SAMLAuthenticator.objects.create( - enabled=True, metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml'), slug='idp1' - ) + authenticator = SAMLAuthenticator.objects.create(enabled=True, metadata='xxx', slug='idp1') # hide password block LoginPasswordAuthenticator.objects.update_or_create( slug='password-authenticator', defaults={'enabled': False} diff --git a/tests/auth_saml/test_migrations.py b/tests/auth_saml/test_migrations.py index 2272ab67f..615e0b733 100644 --- a/tests/auth_saml/test_migrations.py +++ b/tests/auth_saml/test_migrations.py @@ -400,3 +400,29 @@ def test_saml_authenticator_data_migration_rename_attributes(migration, settings assert set_attribute1.user_field == 'first_name' assert set_attribute2.saml_attribute == 'title' assert set_attribute2.user_field == 'title' + + +def test_saml_authenticator_data_migration_metadata_file_to_db(migration, settings): + migrate_from = [('authentic2_auth_saml', '0012_move_add_role_action')] + migrate_to = [('authentic2_auth_saml', '0014_remove_samlauthenticator_metadata_path')] + + old_apps = migration.before(migrate_from) + SAMLAuthenticator = old_apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + + SAMLAuthenticator.objects.create( + slug='idp1', metadata_path=os.path.join(os.path.dirname(__file__), 'metadata.xml') + ) + + SAMLAuthenticator.objects.create(slug='idp2', metadata='xxx') + SAMLAuthenticator.objects.create(slug='idp3', metadata_url='https://example.com') + SAMLAuthenticator.objects.create(slug='idp4', metadata_path='/unknown/') + + new_apps = migration.apply(migrate_to) + SAMLAuthenticator = new_apps.get_model('authentic2_auth_saml', 'SAMLAuthenticator') + + authenticator = SAMLAuthenticator.objects.get(slug='idp1') + assert authenticator.metadata.startswith('') + assert authenticator.metadata.endswith('\n') + + assert SAMLAuthenticator.objects.filter(slug='idp2', metadata='xxx').count() == 1 + assert SAMLAuthenticator.objects.filter(slug='idp3', metadata_url='https://example.com').count() == 1 diff --git a/tests/auth_saml/test_misc.py b/tests/auth_saml/test_misc.py index 1594f0c7e..4ce242fcb 100644 --- a/tests/auth_saml/test_misc.py +++ b/tests/auth_saml/test_misc.py @@ -49,17 +49,15 @@ def test_saml_authenticator_settings(db): ) assert 'METADATA' in authenticator.settings - assert 'METADATA_PATH' not in authenticator.settings assert 'METADATA_URL' not in authenticator.settings assert authenticator.settings['AUTHN_CLASSREF'] == ['a', 'b'] authenticator.metadata = '' - authenticator.metadata_path = '/some/path/metadata.xml' + authenticator.metadata_url = 'https://example.com/metadata.xml' authenticator.save() - assert 'METADATA_PATH' in authenticator.settings + assert 'METADATA_URL' in authenticator.settings assert 'METADATA' not in authenticator.settings - assert 'METADATA_URL' not in authenticator.settings authenticator.authn_classref = '' authenticator.save() diff --git a/tests/test_manager_authenticators.py b/tests/test_manager_authenticators.py index 0c59e8da7..9379c9719 100644 --- a/tests/test_manager_authenticators.py +++ b/tests/test_manager_authenticators.py @@ -444,9 +444,9 @@ def test_authenticators_saml(app, superuser, ou1, ou2): resp = resp.form.submit() assert 'One of the metadata fields must be filled.' in resp.text - resp.form['metadata_path'] = '/var/lib/authentic2/metadata.xml' + resp.form['metadata_url'] = 'https://example.com/metadata.xml' resp = resp.form.submit().follow() - assert 'Metadata file path: /var/lib/authentic2/metadata.xml' in resp.text + assert 'Metadata URL: https://example.com/metadata.xml' in resp.text resp = resp.click('Enable').follow() assert 'Authenticator has been enabled.' in resp.text @@ -472,7 +472,6 @@ def test_authenticators_saml_hide_metadata_url_advanced_fields(app, superuser, o assert 'Metadata cache time' not in resp.text assert 'Metadata HTTP timeout' not in resp.text - resp.form['metadata_path'] = '' resp.form['metadata_url'] = 'https://example.com/metadata.xml' resp = resp.form.submit().follow() -- 2.35.1