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

@@ -23,7 +23,7 @@
         {% if form.errors %}
             <div class="alert alert-danger d-flex align-items-center mb-4" role="alert">
                 <i class="bi bi-exclamation-triangle-fill fs-5 me-2 flex-shrink-0"></i>
-                <div>{% bootstrap_form_errors form type='non_fields' %}</div>
+                <div>{% bootstrap_form_errors form %}</div>
             </div>
         {% endif %}
 

accounts/templates/profile.html

@@ -33,7 +33,8 @@
                 <form method="post" action="" role="form" aria-label="Edit user info form">
                     {% 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 %}
                         <a href="{% url 'accounts:change_password' %}" class="btn btn-primary">
                             {% bs_icon 'lock' %} {% trans "Change Password" %}
                         </a>

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)
 

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})

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)

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"))

accounts/urls.py

@@ -34,6 +34,7 @@ urlpatterns = [
     path("ssh_key/create/", views.ssh_key_create, name="ssh_key_create"),
     path("ssh_key/<int:pk>/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/<str:token_key>/delete/", views.api_token_delete, name="api_token_delete"),
     path("email_otp/", views.email_otp, name="email_otp"),
     path(

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]

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():

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",