src/authentic2/apps/authenticators/views.py

@@ -33,6 +33,7 @@ from django.views.generic.list import ListView
 from authentic2.apps.journal.views import JournalViewWithContext
 from authentic2.manager.journal_views import BaseJournalView
 from authentic2.manager.views import MediaMixin, PermissionMixin, TitleMixin
+from authentic2.utils.misc import build_tab_is_not_default
 
 from . import forms
 from .models import AuthenticatorImportError, BaseAuthenticator
@@ -83,18 +84,6 @@ class AuthenticatorDetailView(AuthenticatorsMixin, DetailView):
 detail = AuthenticatorDetailView.as_view()
 
 
-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

src/authentic2/manager/apiclient_views.py

@@ -16,14 +16,17 @@
 
 import uuid
 
+from django.http import HttpResponseRedirect
 from django.urls import reverse, reverse_lazy
+from django.utils.text import slugify
 from django.utils.translation import gettext_lazy as _
 from django.views.generic import CreateView, DeleteView, DetailView, ListView, UpdateView
 
 from authentic2.a2_rbac.models import OrganizationalUnit, Role
-from authentic2.manager import forms
+from authentic2.manager.forms import APIClientForm
 from authentic2.manager.views import MediaMixin, PermissionMixin, TitleMixin
 from authentic2.models import APIClient
+from authentic2.utils.misc import build_tab_is_not_default
 
 
 class APIClientsMixin(PermissionMixin, MediaMixin, TitleMixin):
@@ -92,7 +95,7 @@ detail = APIClientDetailView.as_view()
 class APIClientAddView(APIClientsFormViewMixin, CreateView):
     template_name = 'authentic2/manager/api_client_form.html'
     title = _('New API client')
-    form_class = forms.APIClientForm
+    form_class = APIClientForm
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
@@ -111,13 +114,58 @@ class APIClientAddView(APIClientsFormViewMixin, CreateView):
 add = APIClientAddView.as_view()
 
 
-class APIClientEditView(APIClientsFormViewMixin, UpdateView):
-    template_name = 'authentic2/manager/api_client_form.html'
+class APIClientEditView(APIClientsMixin, UpdateView):
+    template_name = 'authentic2/manager/api_client_edit_form.html'
     title = _('Edit API client')
-    form_class = forms.APIClientForm
+
+    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)
+            if form_class == APIClientForm:
+                if not self.request.user.has_perm('authentic2.admin_apiclient'):
+                    allowed_ous = []
+                    for ou in OrganizationalUnit.objects.all():
+                        if self.request.user.has_ou_perm('authentic2.admin_apiclient', ou):
+                            allowed_ous.append(ou.id)
+                    form.fields['ou'].queryset = OrganizationalUnit.objects.filter(id__in=allowed_ous)
+                    form.fields['ou'].required = True
+                    form.fields['ou'].empty_label = None
+                api_client = self.object
+                if api_client and api_client.ou is not None:
+                    form.fields['apiclient_roles'].queryset = Role.objects.filter(ou=api_client.ou).exclude(
+                        slug__startswith='_'
+                    )
+            forms.append(form)
+        forms[0].selected = True
+        return forms
+
+    def post(self, request, *args, **kwargs):
+        self.object = self.get_object()
+        forms = self.get_forms()
+
+        invalid_forms = [form for form in forms if not form.is_valid()]
+        if not invalid_forms:
+            for form in forms:
+                form.save()
+            return HttpResponseRedirect(self.get_success_url())
+        else:
+            forms[0].selected = False
+            invalid_forms[0].selected = True
+
+        return self.render_to_response(self.get_context_data(forms=forms))
 
     def get_context_data(self, **kwargs):
+        # generic django update view's context retrieval needs a single form
+        # as a keyword argument, so be it:
+        if 'forms' not in kwargs:
+            kwargs['forms'] = self.get_forms()
+        kwargs['form'] = kwargs['forms'][0]
         context = super().get_context_data(**kwargs)
+        context['object'] = self.object
         context['cancel_url'] = reverse('a2-manager-api-client-detail', kwargs={'pk': self.object.pk})
         return context
 

src/authentic2/manager/forms.py

@@ -943,24 +943,12 @@ class APIClientForm(forms.ModelForm):
         'password',
         'ou',
         'apiclient_roles',
-        # more specific config options, would deserve appearing in a separate tab
-        'restrict_to_anonymised_data',
-        'allowed_user_attributes',
     )
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        core_attributes = ['first_name', 'last_name', 'email', 'username']
-        # restrict to AttributeManager(disabled=False) and discard core attributes
-        self.fields['allowed_user_attributes'].queryset = Attribute.objects.exclude(name__in=core_attributes)
-        self.fields['allowed_user_attributes'].help_text = _(
-            "Select one or multiple attributes if you want to restrict the client's access to these attributes."
-        )
-
     def clean(self):
         ou = self.cleaned_data['ou']
         if ou:
-            unauthorized_roles = self.cleaned_data['apiclient_roles'].exclude(ou=ou)
+            unauthorized_roles = self.cleaned_data.get('apiclient_roles', Role.objects.none()).exclude(ou=ou)
             if unauthorized_roles:
                 unauthorized_roles = ', '.join(unauthorized_roles.values_list('name', flat=True))
                 self.add_error(
@@ -979,13 +967,34 @@ class APIClientForm(forms.ModelForm):
             'identifier',
             'password',
             'ou',
-            'restrict_to_anonymised_data',
             'apiclient_roles',
-            'allowed_user_attributes',
         )
         field_classes = {'apiclient_roles': ChooseRolesField}
 
 
+class APIClientAdvancedForm(forms.ModelForm):
+    field_order = (
+        'restrict_to_anonymised_data',
+        'allowed_user_attributes',
+    )
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        core_attributes = ['first_name', 'last_name', 'email', 'username']
+        # restrict to AttributeManager(disabled=False) and discard core attributes
+        self.fields['allowed_user_attributes'].queryset = Attribute.objects.exclude(name__in=core_attributes)
+        self.fields['allowed_user_attributes'].help_text = _(
+            "Select one or multiple attributes if you want to restrict the client's access to these attributes."
+        )
+
+    class Meta:
+        model = APIClient
+        fields = (
+            'restrict_to_anonymised_data',
+            'allowed_user_attributes',
+        )
+
+
 class ServiceForm(forms.ModelForm):
     def __init__(self, *args, **kwargs):
         if 'user' in kwargs:

src/authentic2/manager/templates/authentic2/manager/api_client_edit_form.html

@@ -0,0 +1,46 @@
+{% extends "authentic2/manager/api_client_common.html" %}
+{% load i18n gadjo %}
+
+{% block breadcrumb %}
+  {{ block.super }}
+  <a href="{% url 'a2-manager-api-client-detail' pk=object.pk %}">{{ object }}</a>
+  <a href="#"></a>
+{% endblock %}
+
+{% block content %}
+  {% if forms|length > 1 %}
+    <div class="pk-tabs">
+      <div class="pk-tabs--tab-list" role="tablist" aria-label="{% trans "Configuration tabs" %}">
+        {% for form in forms %}
+          <button role="tab"
+                  aria-selected="{{ form.selected|yesno:"true,false" }}"
+                  aria-controls="panel-{{ form.tab_slug }}"
+                  id="tab-{{ form.tab_slug }}"
+                  tabindex="{{ forloop.first|yesno:"0,-1" }}"
+                  {% if form.is_not_default %}class="pk-tabs--button-marker"{% endif %}>{{ form.tab_name }}</button>
+        {% endfor %}
+      </div>
+  {% endif %}
+
+
+  <form method="post" {% if forms|length > 1 %}class="pk-tabs--container"{% endif %} enctype="multipart/form-data">
+    {% csrf_token %}
+    {% if forms|length > 1 %}
+      {% for form in forms %}
+        <div id="panel-{{ form.tab_slug }}"
+             role="tabpanel" tabindex="0" {{ form.selected|yesno:",hidden" }}
+             data-tab-slug="{{ form.tab_slug }}"
+             aria-labelledby="tab-{{ form.tab_slug }}">
+          {{ form|with_template }}
+        </div>
+      {% endfor %}
+    {% else %}
+      {{ forms.0|with_template }}
+    {% endif %}
+    <div class="buttons">
+      <button>{% trans "Save" %}</button>
+      <a class="cancel" href="{{ cancel_url }}">{% trans 'Cancel' %}</a>
+    </div>
+  </form>
+  {% if forms|length > 1 %}</div>{% endif %}
+{% endblock %}

src/authentic2/models.py

@@ -757,6 +757,15 @@ class APIClient(models.Model):
     def is_superuser(self):
         return False
 
+    @property
+    def manager_form_classes(self):
+        from .manager.forms import APIClientAdvancedForm, APIClientForm
+
+        return [
+            (_('General'), APIClientForm),
+            (_('Advanced'), APIClientAdvancedForm),
+        ]
+
     def has_perm(self, perm, obj=None):
         if self.is_active and self.is_superuser:
             return True

src/authentic2/utils/misc.py

@@ -1387,3 +1387,15 @@ RUNTIME_SETTINGS = {
         'type': 'url',
     },
 }
+
+
+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

tests/test_manager_apiclient.py

@@ -158,24 +158,11 @@ def test_list_show_objects_local_admin(admin_ou1, app, ou1, ou2):
     assert anchor.text() == 'bar (bar-description) - OU2'
 
 
-def test_add(superuser, app):
-    preferred_color = Attribute.objects.create(
-        name='preferred_color',
-        label='Preferred color',
-        kind='string',
-        disabled=False,
-        multiple=False,
-    )
-    phone2 = Attribute.objects.create(
-        name='phone2',
-        label='Second phone number',
-        kind='phone_number',
-        disabled=False,
-        multiple=False,
-    )
+def test_add(superuser, app, ou2):
     assert APIClient.objects.count() == 0
     role_1 = Role.objects.create(name='role-1', ou=get_default_ou())
     role_2 = Role.objects.create(name='role-2', ou=get_default_ou())
+    role_3 = Role.objects.create(name='role-3', ou=ou2)
     resp = login(app, superuser, 'a2-manager-api-client-add')
     form = resp.form
     # password is prefilled
@@ -185,13 +172,18 @@ def test_add(superuser, app):
     form.set('description', 'api-client-description')
     form.set('identifier', 'api-client-identifier')
     form.set('password', 'api-client-password')
+    form['apiclient_roles'].force_value([role_1.id, role_2.id, role_3.id])
+    resp = form.submit()
+    assert (
+        resp.pyquery('.error').text()
+        == 'The following roles do not belong to organizational unit Default organizational unit: role-3.'
+    )
+    form = resp.form
     form['apiclient_roles'].force_value([role_1.id, role_2.id])
-    form.set('allowed_user_attributes', [preferred_color.id, phone2.id])
     response = form.submit().follow()
     assert APIClient.objects.count() == 1
     api_client = APIClient.objects.get(name='api-client-name')
     assert set(api_client.apiclient_roles.all()) == {role_1, role_2}
-    assert set(api_client.allowed_user_attributes.all()) == {preferred_color, phone2}
     assert urlparse(response.request.url).path == api_client.get_absolute_url()
 
 
@@ -296,11 +288,12 @@ def test_edit(superuser, app, ou1, ou2):
     assert set(form.get('allowed_user_attributes').value) == {str(preferred_color.id), str(phone2.id)}
     assert ('', False, '---------') in form['ou'].options
     resp.form.set('password', 'easy')
-    with pytest.raises(KeyError):
-        # forcing values not presented by the Select2ModelMultipleChoiceField,
-        # should not happen in UI
-        form['apiclient_roles'].force_value([role_1.id, role_2.id])
-        form.submit()
+    # forcing values not presented by the Select2ModelMultipleChoiceField,
+    # should not happen in UI and is therefore ignored
+    form['apiclient_roles'].force_value([role_1.id, role_2.id])
+    form.submit()
+    assert not APIClient.objects.get(name='foo').apiclient_roles.count()
+
     form['apiclient_roles'].force_value([role_1.id, role_3.id])
     form['allowed_user_attributes'].force_value([phone2.id])
     response = form.submit().follow()
@@ -337,6 +330,21 @@ def test_edit(superuser, app, ou1, ou2):
     assert set(api_client.apiclient_roles.all()) == {role_2}
     assert set(api_client.allowed_user_attributes.all()) == {preferred_color}
 
+    # only the first tab is selected and displayed by default
+    resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client.pk}))
+    assert resp.pyquery('#tab-general')[0].get('aria-selected') == 'true'
+    assert resp.pyquery('#tab-advanced')[0].get('aria-selected') == 'false'
+    assert 'hidden' not in resp.pyquery('#panel-general')[0].keys()
+    assert 'hidden' in resp.pyquery('#panel-advanced')[0].keys()
+
+    # only the first tab is selected and displayed by default
+    resp.form['allowed_user_attributes'].force_value([555, 666, 777])  # surely not valid
+    resp = resp.form.submit()
+    assert resp.pyquery('#tab-general')[0].get('aria-selected') == 'false'
+    assert resp.pyquery('#tab-advanced')[0].get('aria-selected') == 'true'
+    assert 'hidden' in resp.pyquery('#panel-general')[0].keys()
+    assert 'hidden' not in resp.pyquery('#panel-advanced')[0].keys()
+
 
 def test_edit_local_admin(admin_ou1, app, ou1, ou2):
     role_1 = Role.objects.create(name='role-1', ou=ou1)
@@ -361,11 +369,12 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2):
     assert form.get('password').value == 'foo-password'
     resp.form.set('password', 'easy')
     assert ('', False, '---------') not in form['ou'].options
-    with pytest.raises(KeyError):
-        # forcing values not presented by the Select2ModelMultipleChoiceField,
-        # should not happen in UI
-        form['apiclient_roles'].force_value([role_1.id, role_2.id])
-        form.submit()
+    # forcing values not presented by the Select2ModelMultipleChoiceField,
+    # should not happen in UI and is therefore ignored
+    form['apiclient_roles'].force_value([role_1.id, role_2.id])
+    form.submit()
+    assert not APIClient.objects.get(name='foo').apiclient_roles.count()
+
     form['apiclient_roles'].force_value([role_1.id, role_3.id])
     response = form.submit().follow()
     assert urlparse(response.request.url).path == api_client_ou1.get_absolute_url()