diff --git a/canaille/app/forms.py b/canaille/app/forms.py index fcce59fd..45008f29 100644 --- a/canaille/app/forms.py +++ b/canaille/app/forms.py @@ -1,5 +1,6 @@ import datetime import math +import re import pytz import wtforms @@ -23,6 +24,17 @@ def is_uri(form, field): raise wtforms.ValidationError(_("This is not a valid URL")) +def unique_values(form, field): + values = set() + for subfield in field: + if subfield.data in values: + subfield.errors.append(_("This value is a duplicate")) + raise wtforms.ValidationError(_("This value is a duplicate")) + + if subfield.data: + values.add(subfield.data) + + meta = DefaultMeta() @@ -36,6 +48,26 @@ class I18NFormMixin: class HTMXFormMixin: + SEPARATOR = "-" + + def field_from_name(self, field_name): + """ + Returns a tuple containing a field and its rendering context + """ + if self.SEPARATOR not in field_name: + field = self[field_name] if field_name in self else None + return field, {} + + parts = field_name.split(self.SEPARATOR) + fieldlist_name = self.SEPARATOR.join(parts[:-1]) + try: + indice = int(parts[-1]) + except ValueError: + return None, {} + fieldlist, _ = self.field_from_name(fieldlist_name) + context = {"parent_list": fieldlist, "parent_indice": indice} + return fieldlist[indice], context + def validate(self, *args, **kwargs): """ If the request is a HTMX request, this will only render the field @@ -46,21 +78,68 @@ class HTMXFormMixin: return super().validate(*args, **kwargs) field_name = request.headers.get("HX-Trigger-Name") - if field_name in self: - self.validate_field(field_name, *args, **kwargs) - self.render_field(field_name) - abort(400) + field, context = self.field_from_name(field_name) + if field: + self.validate_field(field, *args, **kwargs) + self.render_field(field, **context) - def validate_field(self, field_name, *args, **kwargs): - self[field_name].widget.hide_value = False + abort(400, f"{field_name} is not a valid field for inline validation") + + def validate_field(self, field, *args, **kwargs): + field.widget.hide_value = False self.process(request.form) - super().validate(*args, **kwargs) + return field.validate(self, *args, **kwargs) - def render_field(self, field_name, *args, **kwargs): + def render_field(self, field, *args, **kwargs): form_macro = current_app.jinja_env.get_template("macro/form.html") - response = make_response(form_macro.module.render_field(self[field_name])) + response = make_response(form_macro.module.render_field(field, *args, **kwargs)) abort(response) + def form_control(self): + """ + Checks wether the current request is the result of the users + adding or removing a field from a FieldList. + """ + FIELDLIST_ADD_BUTTON = "fieldlist_add" + FIELDLIST_REMOVE_BUTTON = "fieldlist_remove" + + fieldlist_suffix = rf"{self.SEPARATOR}(\d+)$" + if field_name := request.form.get(FIELDLIST_ADD_BUTTON): + fieldlist_name = re.sub(fieldlist_suffix, "", field_name) + fieldlist, context = self.field_from_name(fieldlist_name) + + if not fieldlist or not isinstance(fieldlist, wtforms.FieldList): + abort(400) + + if request_is_htmx(): + self.validate_field(fieldlist) + + fieldlist.append_entry() + + if request_is_htmx(): + self.render_field(fieldlist, **context) + + return True + + if field_name := request.form.get(FIELDLIST_REMOVE_BUTTON): + fieldlist_name = re.sub(fieldlist_suffix, "", field_name) + fieldlist, context = self.field_from_name(fieldlist_name) + + if not fieldlist or not isinstance(fieldlist, wtforms.FieldList): + abort(400) + + if request_is_htmx(): + self.validate_field(fieldlist) + + fieldlist.pop_entry() + + if request_is_htmx(): + self.render_field(fieldlist, **context) + + return True + + return False + class HTMXForm(HTMXFormMixin, I18NFormMixin, FlaskForm): pass diff --git a/canaille/oidc/clients.py b/canaille/oidc/clients.py index 8abafb31..5213e7f2 100644 --- a/canaille/oidc/clients.py +++ b/canaille/oidc/clients.py @@ -3,7 +3,6 @@ import datetime from canaille.app import models from canaille.app.flask import permissions_needed from canaille.app.flask import render_htmx_template -from canaille.app.flask import request_is_htmx from canaille.app.forms import TableForm from canaille.oidc.forms import ClientAddForm from flask import abort @@ -37,7 +36,7 @@ def index(user): def add(user): form = ClientAddForm(request.form or None) - if not request.form: + if not request.form or form.form_control(): return render_template( "oidc/admin/client_add.html", form=form, menuitem="admin" ) @@ -57,11 +56,11 @@ def add(user): client_id=client_id, client_id_issued_at=client_id_issued_at, client_name=form["client_name"].data, - contacts=[form["contacts"].data], + contacts=form["contacts"].data, client_uri=form["client_uri"].data, grant_types=form["grant_types"].data, - redirect_uris=[form["redirect_uris"].data], - post_logout_redirect_uris=[form["post_logout_redirect_uris"].data], + redirect_uris=form["redirect_uris"].data, + post_logout_redirect_uris=form["post_logout_redirect_uris"].data, response_types=form["response_types"].data, scope=form["scope"].data.split(" "), token_endpoint_auth_method=form["token_endpoint_auth_method"].data, @@ -90,17 +89,9 @@ def add(user): @bp.route("/edit/", methods=["GET", "POST"]) @permissions_needed("manage_oidc") def edit(user, client_id): - if ( - request.method == "GET" - or request.form.get("action") == "edit" - or request_is_htmx() - ): - return client_edit(client_id) - - if request.form.get("action") == "delete": + if request.form and request.form.get("action") == "delete": return client_delete(client_id) - - abort(400) + return client_edit(client_id) def client_edit(client_id): @@ -111,17 +102,10 @@ def client_edit(client_id): data = {attribute: getattr(client, attribute) for attribute in client.attributes} data["scope"] = " ".join(data["scope"]) - data["redirect_uris"] = data["redirect_uris"][0] if data["redirect_uris"] else "" - data["contacts"] = data["contacts"][0] if data["contacts"] else "" - data["post_logout_redirect_uris"] = ( - data["post_logout_redirect_uris"][0] - if data["post_logout_redirect_uris"] - else "" - ) data["preconsent"] = client.preconsent form = ClientAddForm(request.form or None, data=data, client=client) - if not request.form: + if not request.form or form.form_control(): return render_template( "oidc/admin/client_edit.html", form=form, client=client, menuitem="admin" ) @@ -137,11 +121,11 @@ def client_edit(client_id): client.update( client_name=form["client_name"].data, - contacts=[form["contacts"].data], + contacts=form["contacts"].data, client_uri=form["client_uri"].data, grant_types=form["grant_types"].data, - redirect_uris=[form["redirect_uris"].data], - post_logout_redirect_uris=[form["post_logout_redirect_uris"].data], + redirect_uris=form["redirect_uris"].data, + post_logout_redirect_uris=form["post_logout_redirect_uris"].data, response_types=form["response_types"].data, scope=form["scope"].data.split(" "), token_endpoint_auth_method=form["token_endpoint_auth_method"].data, diff --git a/canaille/oidc/forms.py b/canaille/oidc/forms.py index 5af2c982..b67da4ff 100644 --- a/canaille/oidc/forms.py +++ b/canaille/oidc/forms.py @@ -2,6 +2,7 @@ import wtforms from canaille.app import models from canaille.app.forms import HTMXForm from canaille.app.forms import is_uri +from canaille.app.forms import unique_values from flask_babel import lazy_gettext as _ @@ -23,10 +24,14 @@ class ClientAddForm(HTMXForm): validators=[wtforms.validators.DataRequired()], render_kw={"placeholder": "Client Name"}, ) - contacts = wtforms.EmailField( - _("Contact"), - validators=[wtforms.validators.Optional(), wtforms.validators.Email()], - render_kw={"placeholder": "admin@mydomain.tld"}, + contacts = wtforms.FieldList( + wtforms.EmailField( + _("Contact"), + validators=[wtforms.validators.Optional(), wtforms.validators.Email()], + render_kw={"placeholder": "admin@mydomain.tld"}, + ), + min_entries=1, + validators=[unique_values], ) client_uri = wtforms.URLField( _("URI"), @@ -36,21 +41,31 @@ class ClientAddForm(HTMXForm): ], render_kw={"placeholder": "https://mydomain.tld"}, ) - redirect_uris = wtforms.URLField( - _("Redirect URIs"), - validators=[ - wtforms.validators.DataRequired(), - is_uri, - ], - render_kw={"placeholder": "https://mydomain.tld/callback"}, + redirect_uris = wtforms.FieldList( + wtforms.URLField( + _("Redirect URIs"), + validators=[ + wtforms.validators.DataRequired(), + is_uri, + ], + render_kw={"placeholder": "https://mydomain.tld/callback"}, + ), + min_entries=1, + validators=[unique_values], ) - post_logout_redirect_uris = wtforms.URLField( - _("Post logout redirect URIs"), - validators=[ - wtforms.validators.Optional(), - is_uri, - ], - render_kw={"placeholder": "https://mydomain.tld/you-have-been-disconnected"}, + post_logout_redirect_uris = wtforms.FieldList( + wtforms.URLField( + _("Post logout redirect URIs"), + validators=[ + wtforms.validators.Optional(), + is_uri, + ], + render_kw={ + "placeholder": "https://mydomain.tld/you-have-been-disconnected" + }, + ), + min_entries=1, + validators=[unique_values], ) grant_types = wtforms.SelectMultipleField( _("Grant types"), diff --git a/canaille/static/css/base.css b/canaille/static/css/base.css index 23135635..6b279c15 100644 --- a/canaille/static/css/base.css +++ b/canaille/static/css/base.css @@ -100,6 +100,19 @@ i.massive.massive.massive.portrait.icon, i.massive.massive.massive.portrait.icon background: rgba(0,0,0,.05) !important; } +/** + * Workaround for + * https://github.com/fomantic/Fomantic-UI/issues/2829 + */ +.ui.corner.labeled.action.input .ui.corner.label { + right:40px; + z-index: 5; +} + +.ui.corner.labeled.action.input .ui.button { + z-index: 99; +} + @media (prefers-color-scheme: dark) { .logo img { filter: invert(.8) !important; diff --git a/canaille/templates/macro/form.html b/canaille/templates/macro/form.html index 4a4b6304..3d547bb4 100644 --- a/canaille/templates/macro/form.html +++ b/canaille/templates/macro/form.html @@ -6,7 +6,9 @@ container=true, noindicator=false, indicator_icon=none, indicator_text=none, -display=true +display=true, +add_button=false, +del_button=false ) -%} {% set field_visible = field.type != 'HiddenField' and field.type !='CSRFTokenField' %} {% if container and field_visible %} @@ -33,6 +35,7 @@ display=true
{% endif %} @@ -66,7 +69,38 @@ display=true
{% endif %} {% endif %} + {% if field_visible %} + {% if del_button %} + + {% endif %} + {% if add_button %} + + {% endif %} {% endif %} @@ -90,14 +124,39 @@ display=true {% endfor %} {% endmacro %} -{% macro render_field(field) -%} - {% if field.type == "BooleanField" %} +{% macro render_field(field, parent_list=none, parent_indice=none) -%} + {% if parent_list %} + {% set last = parent_indice >= parent_list.entries|len -1 %} + {% set ignore_me = kwargs.update({ + "label_visible": false, + "add_button": (last and (not parent_list.max_entries or parent_indice < parent_list.max_entries)), + "del_button": (last and parent_list.min_entries and parent_indice >= parent_list.min_entries), + }) %} + {% endif %} + {% if field.type == "FieldList" %} + {{ render_list(field, **kwargs) }} + {% elif field.type == "BooleanField" %} {{ render_checkbox(field, **kwargs) }} {% else %} {{ render_input(field, **kwargs) }} {% endif %} {%- endmacro %} +{% macro render_list(field) -%} +
+ {# Strangely enough, translations are not rendered when using field.label() #} + {{ field[0].label() }} + {% for subfield in field %} + {{ render_field( + subfield, + parent_list=field, + parent_indice=loop.index0, + **kwargs + ) }} + {% endfor %} +
+{%- endmacro %} + {% macro render_checkbox(field, display=true) -%}
' ) + + +def test_fieldlist_add(testclient, logged_admin): + assert not models.Client.query() + + res = testclient.get("/admin/client/add") + assert "redirect_uris-1" not in res.form.fields + + data = { + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + } + for k, v in data.items(): + res.form[k].force_value(v) + + res = res.form.submit(status=200, name="fieldlist_add", value="redirect_uris-0") + assert not models.Client.query() + + data["redirect_uris-1"] = "https://foo.bar/callback2" + for k, v in data.items(): + res.form[k].force_value(v) + + res = res.form.submit(status=302, name="action", value="edit") + res = res.follow(status=200) + + client_id = res.forms["readonly"]["client_id"].value + client = models.Client.get(client_id=client_id) + + assert client.redirect_uris == [ + "https://foo.bar/callback", + "https://foo.bar/callback2", + ] + + client.delete() + + +def test_fieldlist_delete(testclient, logged_admin): + assert not models.Client.query() + res = testclient.get("/admin/client/add") + + data = { + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback1", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + } + for k, v in data.items(): + res.form[k].force_value(v) + res = res.form.submit(status=200, name="fieldlist_add", value="redirect_uris-0") + + res.form["redirect_uris-1"] = "https://foo.bar/callback2" + res = res.form.submit(status=200, name="fieldlist_remove", value="redirect_uris-1") + assert not models.Client.query() + assert "redirect_uris-1" not in res.form.fields + + res = res.form.submit(status=302, name="action", value="edit") + res = res.follow(status=200) + + client_id = res.forms["readonly"]["client_id"].value + client = models.Client.get(client_id=client_id) + + assert client.redirect_uris == [ + "https://foo.bar/callback1", + ] + + client.delete() + + +def test_fieldlist_add_invalid_field(testclient, logged_admin): + res = testclient.get("/admin/client/add") + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + "fieldlist_add": "invalid", + } + testclient.post("/admin/client/add", data, status=400) + + +def test_fieldlist_delete_invalid_field(testclient, logged_admin): + assert not models.Client.query() + res = testclient.get("/admin/client/add") + + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback1", + "redirect_uris-1": "https://foo.bar/callback2", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + "fieldlist_remove": "invalid", + } + testclient.post("/admin/client/add", data, status=400) + + +def test_fieldlist_duplicate_value(testclient, logged_admin, client): + res = testclient.get("/admin/client/add") + data = { + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/samecallback", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + } + for k, v in data.items(): + res.form[k].force_value(v) + res = res.form.submit(status=200, name="fieldlist_add", value="redirect_uris-0") + res.form["redirect_uris-1"] = "https://foo.bar/samecallback" + res = res.form.submit(status=200, name="action", value="edit") + res.mustcontain("This value is a duplicate") + + +def test_fieldlist_empty_value(testclient, logged_admin, client): + res = testclient.get("/admin/client/add") + data = { + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/samecallback", + "post_logout_redirect_uris-0": "https://foo.bar/callback1", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + } + for k, v in data.items(): + res.form[k].force_value(v) + res = res.form.submit( + status=200, name="fieldlist_add", value="post_logout_redirect_uris-0" + ) + res.form.submit(status=302, name="action", value="edit") + client = models.Client.get() + client.delete() + + +def test_fieldlist_add_field_htmx(testclient, logged_admin): + res = testclient.get("/admin/client/add") + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + "fieldlist_add": "redirect_uris-0", + } + response = testclient.post( + "/admin/client/add", + data, + headers={ + "HX-Request": "true", + "HX-Trigger-Name": "listfield_add", + }, + ) + assert 'name="redirect_uris-0' in response.text + assert 'name="redirect_uris-1' in response.text + + +def test_fieldlist_add_field_htmx_validation(testclient, logged_admin): + res = testclient.get("/admin/client/add") + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "not-a-valid-uri", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + "fieldlist_add": "redirect_uris-0", + } + response = testclient.post( + "/admin/client/add", + data, + headers={ + "HX-Request": "true", + "HX-Trigger-Name": "listfield_add", + }, + ) + assert 'name="redirect_uris-0' in response.text + assert 'name="redirect_uris-1' in response.text + assert "This is not a valid URL" in response.text + + +def test_fieldlist_remove_field_htmx(testclient, logged_admin): + res = testclient.get("/admin/client/add") + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "https://foo.bar/callback1", + "redirect_uris-1": "https://foo.bar/callback2", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + "fieldlist_remove": "redirect_uris-1", + } + response = testclient.post( + "/admin/client/add", + data, + headers={ + "HX-Request": "true", + "HX-Trigger-Name": "listfield_remove", + }, + ) + assert 'name="redirect_uris-0' in response.text + assert 'name="redirect_uris-1' not in response.text + + +def test_fieldlist_inline_validation(testclient, logged_admin): + res = testclient.get("/admin/client/add") + data = { + "csrf_token": res.form["csrf_token"].value, + "client_name": "foobar", + "client_uri": "https://foo.bar", + "redirect_uris-0": "invalid-url", + "redirect_uris-1": "https://foo.bar/callback2", + "grant_types": ["password", "authorization_code"], + "response_types": ["code", "token"], + "token_endpoint_auth_method": "none", + } + response = testclient.post( + "/admin/client/add", + data, + headers={ + "HX-Request": "true", + "HX-Trigger-Name": "redirect_uris-0", + }, + ) + assert 'name="redirect_uris-0' in response.text + assert 'name="redirect_uris-1' not in response.text + assert "This is not a valid URL" in response.text + + +def test_inline_validation_invalid_field(testclient, logged_admin, user): + res = testclient.get("/profile") + testclient.post( + "/profile", + { + "csrf_token": res.form["csrf_token"].value, + "email": "john@doe.com", + }, + headers={ + "HX-Request": "true", + "HX-Trigger-Name": "invalid-field", + }, + status=400, + ) diff --git a/tests/core/test_profile_creation.py b/tests/core/test_profile_creation.py index c74bf21c..5b1960dd 100644 --- a/tests/core/test_profile_creation.py +++ b/tests/core/test_profile_creation.py @@ -80,24 +80,6 @@ def test_profile_creation_dynamic_validation(testclient, logged_admin, user): res.mustcontain("The email 'john@doe.com' is already used") -def test_profile_creation_dynamic_validation_invalid_field( - testclient, logged_admin, user -): - res = testclient.get("/profile") - testclient.post( - "/profile", - { - "csrf_token": res.form["csrf_token"].value, - "email": "john@doe.com", - }, - headers={ - "HX-Request": "true", - "HX-Trigger-Name": "invalid-field", - }, - status=400, - ) - - def test_user_creation_without_password(testclient, logged_moderator): res = testclient.get("/profile", status=200) res.form["user_name"] = "george" diff --git a/tests/oidc/test_client_admin.py b/tests/oidc/test_client_admin.py index abba083b..89e864a5 100644 --- a/tests/oidc/test_client_admin.py +++ b/tests/oidc/test_client_admin.py @@ -87,9 +87,9 @@ def test_client_add(testclient, logged_admin): res = testclient.get("/admin/client/add") data = { "client_name": "foobar", - "contacts": "foo@bar.com", + "contacts-0": "foo@bar.com", "client_uri": "https://foo.bar", - "redirect_uris": ["https://foo.bar/callback"], + "redirect_uris-0": "https://foo.bar/callback", "grant_types": ["password", "authorization_code"], "scope": "openid profile", "response_types": ["code", "token"], @@ -103,12 +103,12 @@ def test_client_add(testclient, logged_admin): "jwks_uri": "https://foo.bar/jwks.json", "audience": [], "preconsent": False, - "post_logout_redirect_uris": ["https://foo.bar/disconnected"], + "post_logout_redirect_uris-0": "https://foo.bar/disconnected", } for k, v in data.items(): res.form[k].force_value(v) - res = res.form.submit(status=302, name="action", value="edit") + res = res.form.submit(status=302, name="action", value="add") res = res.follow(status=200) client_id = res.forms["readonly"]["client_id"].value @@ -149,9 +149,9 @@ def test_client_edit(testclient, client, logged_admin, other_client): res = testclient.get("/admin/client/edit/" + client.client_id) data = { "client_name": "foobar", - "contacts": "foo@bar.com", + "contacts-0": "foo@bar.com", "client_uri": "https://foo.bar", - "redirect_uris": ["https://foo.bar/callback"], + "redirect_uris-0": "https://foo.bar/callback", "grant_types": ["password", "authorization_code"], "scope": "openid profile", "response_types": ["code", "token"], @@ -165,7 +165,7 @@ def test_client_edit(testclient, client, logged_admin, other_client): "jwks_uri": "https://foo.bar/jwks.json", "audience": [client.id, other_client.id], "preconsent": True, - "post_logout_redirect_uris": ["https://foo.bar/disconnected"], + "post_logout_redirect_uris-0": "https://foo.bar/disconnected", } for k, v in data.items(): res.forms["clientaddform"][k].force_value(v) @@ -182,7 +182,10 @@ def test_client_edit(testclient, client, logged_admin, other_client): assert client.client_name == "foobar" assert client.contacts == ["foo@bar.com"] assert client.client_uri == "https://foo.bar" - assert client.redirect_uris == ["https://foo.bar/callback"] + assert client.redirect_uris == [ + "https://foo.bar/callback", + "https://mydomain.tld/redirect2", + ] assert client.grant_types == ["password", "authorization_code"] assert client.scope == ["openid", "profile"] assert client.response_types == ["code", "token"] @@ -247,11 +250,6 @@ def test_client_delete_invalid_client(testclient, logged_admin, client): ) -def test_invalid_request(testclient, logged_admin, client): - res = testclient.get("/admin/client/edit/" + client.client_id) - res = res.forms["clientaddform"].submit(name="action", value="invalid", status=400) - - def test_client_edit_preauth(testclient, client, logged_admin, other_client): assert not client.preconsent