From e89c5cb3c8305b47a218ef58e5e21f289f274b19 Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Tue, 16 Aug 2022 16:57:02 +0200 Subject: [PATCH 2/2] authenticators: allow splitting configuration form (#67875) --- .../authenticators/journal_event_types.py | 5 +- src/authentic2/apps/authenticators/models.py | 13 +++-- .../authenticator_edit_form.html | 43 +++++++++++++--- src/authentic2/apps/authenticators/views.py | 50 ++++++++++++++++--- src/authentic2_auth_saml/forms.py | 32 +++++++++++- src/authentic2_auth_saml/models.py | 9 ++-- tests/test_manager_authenticators.py | 33 ++++++++++-- tests/test_manager_journal.py | 2 +- 8 files changed, 157 insertions(+), 30 deletions(-) diff --git a/src/authentic2/apps/authenticators/journal_event_types.py b/src/authentic2/apps/authenticators/journal_event_types.py index ef1feb79b..a2d9ea34d 100644 --- a/src/authentic2/apps/authenticators/journal_event_types.py +++ b/src/authentic2/apps/authenticators/journal_event_types.py @@ -49,8 +49,9 @@ class AuthenticatorEdit(AuthenticatorEvents): label = _('authenticator edit') @classmethod - def record(cls, *, user, session, form): - super().record(user=user, session=session, authenticator=form.instance, data=form_to_old_new(form)) + def record(cls, *, user, session, forms): + data = {k: v for form in forms for k, v in form_to_old_new(form).items()} + super().record(user=user, session=session, authenticator=forms[0].instance, data=data) @classmethod def get_message(cls, event, context): diff --git a/src/authentic2/apps/authenticators/models.py b/src/authentic2/apps/authenticators/models.py index 4c23c97d3..61c77f9e8 100644 --- a/src/authentic2/apps/authenticators/models.py +++ b/src/authentic2/apps/authenticators/models.py @@ -89,6 +89,10 @@ class BaseAuthenticator(models.Model): return '%s - %s' % (self._meta.verbose_name, self.name) return str(self._meta.verbose_name) + @property + def manager_form_classes(self): + return [(_('General'), self.manager_form_class)] + def get_identifier(self): return self.type if self.unique else '%s_%s' % (self.type, self.slug) @@ -128,10 +132,11 @@ class BaseAuthenticator(models.Model): return False def has_valid_configuration(self): - try: - self.full_clean(exclude=getattr(self.manager_form_class._meta, 'exclude', None)) - except ValidationError: - return False + for _, form_class in self.manager_form_classes: + try: + self.full_clean(exclude=getattr(form_class._meta, 'exclude', None)) + except ValidationError: + return False return True diff --git a/src/authentic2/apps/authenticators/templates/authentic2/authenticators/authenticator_edit_form.html b/src/authentic2/apps/authenticators/templates/authentic2/authenticators/authenticator_edit_form.html index b81d3dbce..79095e763 100644 --- a/src/authentic2/apps/authenticators/templates/authentic2/authenticators/authenticator_edit_form.html +++ b/src/authentic2/apps/authenticators/templates/authentic2/authenticators/authenticator_edit_form.html @@ -8,12 +8,39 @@ {% endblock %} {% block content %} -
- {% csrf_token %} - {{ form|with_template }} -
- - {% trans 'Cancel' %} -
-
+
+ {% if forms|length > 1 %} +
+ {% for form in forms %} + + {% endfor %} +
+ {% endif %} + + +
1 %}class="pk-tabs--container"{% endif %} enctype="multipart/form-data"> + {% csrf_token %} + {% if forms|length > 1 %} + {% for form in forms %} +
+ {{ form|with_template }} +
+ {% endfor %} + {% else %} + {{ forms.0|with_template }} + {% endif %} +
+ + {% trans 'Cancel' %} +
+
+
{% endblock %} diff --git a/src/authentic2/apps/authenticators/views.py b/src/authentic2/apps/authenticators/views.py index bb50fa564..d54a3d294 100644 --- a/src/authentic2/apps/authenticators/views.py +++ b/src/authentic2/apps/authenticators/views.py @@ -20,6 +20,7 @@ from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404 from django.urls import reverse, reverse_lazy from django.utils.functional import cached_property +from django.utils.text import slugify from django.utils.translation import ugettext as _ from django.views.generic import CreateView, DeleteView, DetailView, FormView, UpdateView from django.views.generic.list import ListView @@ -75,16 +76,51 @@ class AuthenticatorDetailView(AuthenticatorsMixin, DetailView): detail = AuthenticatorDetailView.as_view() -class AuthenticatorEditView(AuthenticatorsMixin, UpdateView): +def build_tab_is_not_default(form): + for field_name, field in form.fields.items(): + if field.initial is not None: + initial_value = field.initial() if callable(field.initial) else field.initial + if initial_value != form.initial.get(field_name): + return True + else: + if bool(form.initial.get(field_name)): + return True + return False + + +class MultipleFormsUpdateView(UpdateView): + def get_context_data(self, **kwargs): + kwargs['object'] = self.object + kwargs['forms'] = kwargs.get('forms') or self.get_forms() + return kwargs + + +class AuthenticatorEditView(AuthenticatorsMixin, MultipleFormsUpdateView): template_name = 'authentic2/authenticators/authenticator_edit_form.html' title = _('Edit authenticator') - def get_form_class(self): - return self.object.manager_form_class - - def form_valid(self, form): - self.request.journal.record('authenticator.edit', form=form) - return super().form_valid(form) + def get_forms(self): + forms = [] + for label, form_class in self.object.manager_form_classes: + form = form_class(**self.get_form_kwargs()) + form.tab_name = label + form.tab_slug = slugify(label) + form.is_not_default = build_tab_is_not_default(form) + forms.append(form) + return forms + + def post(self, request, *args, **kwargs): + self.object = self.get_object() + forms = self.get_forms() + + all_valid = all(form.is_valid() for form in forms) + if all_valid: + for form in forms: + form.save() + self.request.journal.record('authenticator.edit', forms=forms) + return HttpResponseRedirect(self.get_success_url()) + + return self.render_to_response(self.get_context_data(forms=forms)) edit = AuthenticatorEditView.as_view() diff --git a/src/authentic2_auth_saml/forms.py b/src/authentic2_auth_saml/forms.py index 0840189f6..c0af5890a 100644 --- a/src/authentic2_auth_saml/forms.py +++ b/src/authentic2_auth_saml/forms.py @@ -25,7 +25,37 @@ from .models import SAMLAuthenticator class SAMLAuthenticatorForm(forms.ModelForm): class Meta: model = SAMLAuthenticator - exclude = ('ou',) + exclude = ( + 'ou', + 'metadata_cache_time', + 'metadata_http_timeout', + 'verify_ssl_certificate', + 'transient_federation_attribute', + 'realm', + 'username_template', + 'name_id_policy_format', + 'name_id_policy_allow_create', + 'force_authn', + 'add_authnrequest_next_url_extension', + 'group_attribute', + 'create_group', + 'error_url', + 'error_redirect_after_timeout', + 'authn_classref', + 'attribute_mapping', + ) + + +class SAMLAuthenticatorAdvancedForm(forms.ModelForm): + class Meta: + model = SAMLAuthenticator + fields = SAMLAuthenticatorForm.Meta.exclude + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.instance.metadata_url: + del self.fields['metadata_cache_time'] + del self.fields['metadata_http_timeout'] class RoleChoiceField(forms.ModelChoiceField): diff --git a/src/authentic2_auth_saml/models.py b/src/authentic2_auth_saml/models.py index 1c581ef37..d291a492a 100644 --- a/src/authentic2_auth_saml/models.py +++ b/src/authentic2_auth_saml/models.py @@ -161,10 +161,13 @@ class SAMLAuthenticator(BaseAuthenticator): return settings @property - def manager_form_class(self): - from .forms import SAMLAuthenticatorForm + def manager_form_classes(self): + from .forms import SAMLAuthenticatorAdvancedForm, SAMLAuthenticatorForm - return SAMLAuthenticatorForm + return [ + (_('General'), SAMLAuthenticatorForm), + (_('Advanced'), SAMLAuthenticatorAdvancedForm), + ] def clean(self): if not (self.metadata or self.metadata_path or self.metadata_url): diff --git a/tests/test_manager_authenticators.py b/tests/test_manager_authenticators.py index 8c4d530bc..429952760 100644 --- a/tests/test_manager_authenticators.py +++ b/tests/test_manager_authenticators.py @@ -291,22 +291,47 @@ def test_authenticators_saml(app, superuser, ou1, ou2): assert 'configuration is not complete' in resp.text resp = resp.click('Edit') + assert resp.pyquery('button#tab-general').attr('class') == 'pk-tabs--button-marker' + assert not resp.pyquery('button#tab-advanced').attr('class') + 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['attribute_mapping'] = '[{"attribute": "email", "saml_attribute": "mail", "mandatory": false}]' resp = resp.form.submit().follow() - assert 'Metadata file path: /var/lib/authentic2/metadata.xml' in resp.text + resp = resp.click('Enable').follow() + assert 'Authenticator has been enabled.' in resp.text + + resp = resp.click('Edit') + resp.form['attribute_mapping'] = '[{"attribute": "email", "saml_attribute": "mail", "mandatory": false}]' + resp = resp.form.submit().follow() + authenticator.refresh_from_db() assert authenticator.attribute_mapping == [ {"attribute": "email", "saml_attribute": "mail", "mandatory": False} ] - resp = resp.click('Enable').follow() - assert 'Authenticator has been enabled.' in resp.text + resp = resp.click('Edit') + assert resp.pyquery('button#tab-advanced').attr('class') == 'pk-tabs--button-marker' + + +def test_authenticators_saml_hide_metadata_url_advanced_fields(app, superuser, ou1, ou2): + authenticator = SAMLAuthenticator.objects.create(slug='idp1') + + resp = login(app, superuser) + resp = app.get('/manage/authenticators/%s/edit/' % authenticator.pk) + 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() + + resp = resp.click('Edit') + assert 'Metadata cache time' in resp.text + assert 'Metadata HTTP timeout' in resp.text def test_authenticators_saml_attribute_lookup(app, superuser): diff --git a/tests/test_manager_journal.py b/tests/test_manager_journal.py index 302c24739..b454e7a49 100644 --- a/tests/test_manager_journal.py +++ b/tests/test_manager_journal.py @@ -303,7 +303,7 @@ def events(db, freezer): authenticator_edit_form.initial = {'name': 'old'} authenticator_edit_form.changed_data = ['name'] authenticator_edit_form.cleaned_data = {'name': 'new'} - make('authenticator.edit', user=agent, session=session2, form=authenticator_edit_form) + make('authenticator.edit', user=agent, session=session2, forms=[authenticator_edit_form]) make('authenticator.enable', user=agent, session=session2, authenticator=authenticator) make('authenticator.disable', user=agent, session=session2, authenticator=authenticator) make('authenticator.deletion', user=agent, session=session2, authenticator=authenticator) -- 2.30.2