From 93d3cc7238d7ff5ce1427ad9b588422c94b8c100 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 26 Oct 2026 15:46:34 +0300 Subject: [PATCH] fix(accounts): SSH keys with spaced comments, one token per create, OTP errors - An SSH public key whose comment holds spaces (ssh-keygen -C "John Doe laptop") is accepted; only the type and the key are checked. - The created API token is shown by a GET after a redirect, so reloading the page no longer creates a second token the user never sees. - The OTP login page shows field errors instead of an empty error box. - A user without a local password (an LDAP user) is not offered a password change that could never succeed. --- accounts/templates/accounts/otp_login.html | 2 +- accounts/templates/profile.html | 3 ++- accounts/tests/test_api_tokens.py | 13 ++++++++++--- accounts/tests/test_change_password_permission.py | 6 ++++++ accounts/tests/test_otp_login.py | 11 +++++++++++ accounts/tests/test_utils.py | 4 +++- accounts/urls.py | 1 + accounts/utils.py | 6 +++--- accounts/views.py | 15 ++++++++++++++- webvirtcloud/tests/test_authorization_matrix.py | 1 + 10 files changed, 52 insertions(+), 10 deletions(-) diff --git a/accounts/templates/accounts/otp_login.html b/accounts/templates/accounts/otp_login.html index a11fd4e0..1dc2be83 100644 --- a/accounts/templates/accounts/otp_login.html +++ b/accounts/templates/accounts/otp_login.html @@ -23,7 +23,7 @@ {% if form.errors %} {% endif %} diff --git a/accounts/templates/profile.html b/accounts/templates/profile.html index 93b28ed5..c22a7d6e 100644 --- a/accounts/templates/profile.html +++ b/accounts/templates/profile.html @@ -33,7 +33,8 @@
{% csrf_token %} {% bootstrap_form profile_form layout='horizontal' %} - {% if perms.accounts.change_password %} + {# an LDAP user's password is changed in LDAP #} + {% if perms.accounts.change_password and user.has_usable_password %} {% bs_icon 'lock' %} {% trans "Change Password" %} diff --git a/accounts/tests/test_api_tokens.py b/accounts/tests/test_api_tokens.py index 0c6140d8..ca7bdd19 100644 --- a/accounts/tests/test_api_tokens.py +++ b/accounts/tests/test_api_tokens.py @@ -23,7 +23,7 @@ class ApiTokenTests(TestCase): def make(self, name="ansible", days="90"): """The token the creation page shows once.""" self.client.force_login(self.user) - response = self.client.post(reverse("accounts:api_token_create"), {"name": name, "days": days}) + response = self.client.post(reverse("accounts:api_token_create"), {"name": name, "days": days}, follow=True) self.assertEqual(response.status_code, 200) return re.search(r"wvc_[0-9a-f]{64}", response.content.decode()).group(0) @@ -48,9 +48,16 @@ class ApiTokenTests(TestCase): def test_the_page_showing_the_token_is_not_cached(self): self.client.force_login(self.user) - response = self.client.post(reverse("accounts:api_token_create"), {"name": "ci", "days": "30"}) + response = self.client.post(reverse("accounts:api_token_create"), {"name": "ci", "days": "30"}, follow=True) self.assertIn("no-store", response["Cache-Control"]) + def test_reloading_the_page_creates_no_second_token(self): + token = self.make(name="ci") + response = self.client.get(reverse("accounts:api_token_created")) + self.assertRedirects(response, reverse("accounts:profile") + "#apitokens", fetch_redirect_response=False) + self.assertNotIn(token, response.content.decode()) + self.assertEqual(ApiToken.objects.filter(user=self.user).count(), 1) + def test_a_token_lasts_the_days_chosen_or_until_deleted(self): self.make(name="month", days="30") self.make(name="always", days="") @@ -147,7 +154,7 @@ class AdminApiTokenTests(TestCase): self.user = get_user_model().objects.create_user("tok-user", password="pw") self.admin = get_user_model().objects.create_superuser("tok-admin", "admin@example.com", "pw") self.client.force_login(self.user) - response = self.client.post(reverse("accounts:api_token_create"), {"name": "ansible", "days": "90"}) + response = self.client.post(reverse("accounts:api_token_create"), {"name": "ansible", "days": "90"}, follow=True) self.token = re.search(r"wvc_[0-9a-f]{64}", response.content.decode()).group(0) self.stored = ApiToken.objects.get(user=self.user) diff --git a/accounts/tests/test_change_password_permission.py b/accounts/tests/test_change_password_permission.py index 163a06b4..cc50e014 100644 --- a/accounts/tests/test_change_password_permission.py +++ b/accounts/tests/test_change_password_permission.py @@ -34,6 +34,12 @@ class ChangePasswordPermissionTestCase(TestCase): self.client.force_login(user) self.assertEqual(self.client.get(reverse("accounts:change_password")).status_code, 403) + def test_a_user_without_a_local_password_cannot_change_it(self): + user = get_user_model().objects.create_user("ldap-user") # as django-auth-ldap makes them + self.client.force_login(user) + self.assertEqual(self.client.get(reverse("accounts:change_password")).status_code, 403) + self.assertNotContains(self.client.get(reverse("accounts:profile")), reverse("accounts:change_password")) + def test_create_form_ticks_it_and_the_admin_choice_wins(self): self.assertIn(permission(), UserCreateForm().fields["user_permissions"].initial) form = UserCreateForm(data={"username": "unticked", "password": "pw", "is_active": True}) diff --git a/accounts/tests/test_otp_login.py b/accounts/tests/test_otp_login.py index 540c5f59..10e397b8 100644 --- a/accounts/tests/test_otp_login.py +++ b/accounts/tests/test_otp_login.py @@ -178,3 +178,14 @@ class OTPLoginHTTPTestCase(TestCase): self.client.force_login(self.user) response = self.client.get(f"{reverse('accounts:login')}?next={reverse('computes')}") self.assertRedirects(response, reverse("computes"), fetch_redirect_response=False) + + +class OtpLoginPageTestCase(TestCase): + def test_a_field_error_is_shown_in_the_error_box(self): + from django.template.loader import render_to_string + + request = RequestFactory().post("/accounts/login/") + form = SingleDeviceOTPForm(request, data={"username": "u" * 200, "password": "pw"}) + self.assertFalse(form.is_valid()) + page = render_to_string("accounts/otp_login.html", {"form": form}, request=request) + self.assertIn(str(form.errors["username"][0]), page) diff --git a/accounts/tests/test_utils.py b/accounts/tests/test_utils.py index 78f9870f..ec85580d 100644 --- a/accounts/tests/test_utils.py +++ b/accounts/tests/test_utils.py @@ -34,11 +34,13 @@ class AccountsUtilsTestCase(TestCase): b64 = base64.b64encode(blob).decode() valid_key = f"ssh-rsa {b64} user@example.com" self.assertTrue(validate_ssh_key(valid_key)) + # ssh-keygen -C "John Doe laptop": the comment holds spaces + self.assertTrue(validate_ssh_key(f"ssh-rsa {b64} John Doe laptop")) + self.assertTrue(validate_ssh_key(f"ssh-rsa {b64}")) def test_validate_ssh_key_invalid_parts(self): self.assertFalse(validate_ssh_key("only_two parts")) self.assertFalse(validate_ssh_key("one_part")) - self.assertFalse(validate_ssh_key("four parts key here now")) def test_validate_ssh_key_invalid_base64(self): self.assertFalse(validate_ssh_key("ssh-rsa not-valid-b64! user@example.com")) diff --git a/accounts/urls.py b/accounts/urls.py index a794af7c..703dc60e 100644 --- a/accounts/urls.py +++ b/accounts/urls.py @@ -34,6 +34,7 @@ urlpatterns = [ path("ssh_key/create/", views.ssh_key_create, name="ssh_key_create"), path("ssh_key//delete/", views.ssh_key_delete, name="ssh_key_delete"), path("api_token/create/", views.api_token_create, name="api_token_create"), + path("api_token/created/", views.api_token_created, name="api_token_created"), path("api_token//delete/", views.api_token_delete, name="api_token_delete"), path("email_otp/", views.email_otp, name="email_otp"), path( diff --git a/accounts/utils.py b/accounts/utils.py index 290c1111..a2c1605c 100644 --- a/accounts/utils.py +++ b/accounts/utils.py @@ -33,9 +33,9 @@ def get_user_totp_device(user): def validate_ssh_key(key): - array = key.strip().encode().split() - # An OpenSSH public key has 2 parts (type, key) or 3 parts (type, key, comment). - if len(array) not in (2, 3): + # An OpenSSH public key is a type, a key and an optional comment, which may hold spaces + array = key.strip().encode().split(maxsplit=2) + if len(array) < 2: return False typeofkey = array[0] string = array[1] diff --git a/accounts/views.py b/accounts/views.py index de0b6c50..f8faa260 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -73,7 +73,18 @@ def api_token_create(request): name, days = form.cleaned_data["name"], form.cleaned_data["days"] with audit(request, None, f"API token {name}", "add"): _token, key = ApiToken.objects.create(request.user, expiry=timedelta(days=days) if days else None, name=name) - return render(request, "api_token_created.html", {"name": name, "key": key}) + # shown by a GET, so reloading the page does not create another token + request.session["api_token_created"] = {"name": name, "key": key} + return redirect("accounts:api_token_created") + + +@never_cache +def api_token_created(request): + """The token just created, shown once.""" + created = request.session.pop("api_token_created", None) + if created is None: + return redirect(reverse("accounts:profile") + "#apitokens") + return render(request, "api_token_created.html", created) def api_token_delete(request, token_key, user_id=None): @@ -180,6 +191,8 @@ def change_password(request): ).exists() if not must_change and not request.user.has_perm("accounts.change_password"): raise PermissionDenied + if not request.user.has_usable_password(): # an LDAP user: the password is changed in LDAP + raise PermissionDenied form = PasswordChangeForm(request.user, request.POST or None) if form.is_valid(): diff --git a/webvirtcloud/tests/test_authorization_matrix.py b/webvirtcloud/tests/test_authorization_matrix.py index f986ba1e..2de0a2e4 100644 --- a/webvirtcloud/tests/test_authorization_matrix.py +++ b/webvirtcloud/tests/test_authorization_matrix.py @@ -39,6 +39,7 @@ ALLOWED_FOR_ANY_USER = { "accounts:change_password": "the user's own password; new users get the permission by default", "accounts:ssh_key_create": "the user's own SSH keys", "accounts:api_token_create": "the user's own API tokens", + "accounts:api_token_created": "the token the user just created, from their own session", "set_language": "UI language switch", "console": "renders an error without a valid token; access is checked per VM", "ds_openstack_index": "cloud-init datasource, fetched by VMs",