From 18d17d4d5272608c59d325e68d9148061a7c6f30 Mon Sep 17 00:00:00 2001
From: Evgenii Alekseev <i@arcanis.me>
Date: Thu, 16 Nov 2023 16:42:27 +0200
Subject: [PATCH] feat: forbid form data in html

It has been a while since all pages have moved to json instead of form
data, except for login page. This commit changes login to json data
instead of form one
---
 .../templates/build-status/alerts.jinja2      |  8 +--
 .../templates/build-status/login-modal.jinja2 | 41 ++++++++++--
 src/ahriman/web/views/base.py                 | 43 +------------
 src/ahriman/web/views/v1/service/add.py       |  2 +-
 src/ahriman/web/views/v1/service/pgp.py       |  3 +-
 src/ahriman/web/views/v1/service/rebuild.py   |  2 +-
 src/ahriman/web/views/v1/service/remove.py    |  2 +-
 src/ahriman/web/views/v1/service/request.py   |  2 +-
 src/ahriman/web/views/v1/service/update.py    |  2 +-
 src/ahriman/web/views/v1/status/logs.py       |  2 +-
 src/ahriman/web/views/v1/status/package.py    |  2 +-
 src/ahriman/web/views/v1/status/patches.py    |  2 +-
 src/ahriman/web/views/v1/status/status.py     |  2 +-
 src/ahriman/web/views/v1/user/login.py        | 13 ++--
 tests/ahriman/web/views/test_view_base.py     | 62 -------------------
 .../v1/service/test_view_v1_service_update.py |  1 -
 .../views/v1/user/test_view_v1_user_login.py  | 16 ++++-
 17 files changed, 72 insertions(+), 133 deletions(-)

diff --git a/package/share/ahriman/templates/build-status/alerts.jinja2 b/package/share/ahriman/templates/build-status/alerts.jinja2
index 5b68d8e7..927b116f 100644
--- a/package/share/ahriman/templates/build-status/alerts.jinja2
+++ b/package/share/ahriman/templates/build-status/alerts.jinja2
@@ -1,7 +1,7 @@
 <script>
     const alertPlaceholder = $("#alert-placeholder");
 
-    function createAlert(title, message, clz) {
+    function createAlert(title, message, clz, action) {
         const wrapper = document.createElement("div");
         wrapper.classList.add("toast", clz);
         wrapper.role = "alert";
@@ -23,7 +23,7 @@
         const toast = new bootstrap.Toast(wrapper);
         wrapper.addEventListener("hidden.bs.toast", () => {
             wrapper.remove();  // bootstrap doesn't remove elements
-            reload();
+            (action || reload)();
         });
         toast.show();
     }
@@ -38,8 +38,8 @@
         createAlert(title, description(details), "text-bg-danger");
     }
 
-    function showSuccess(title, description) {
-        createAlert(title, description, "text-bg-success");
+    function showSuccess(title, description, action) {
+        createAlert(title, description, "text-bg-success", action);
     }
 
 </script>
diff --git a/package/share/ahriman/templates/build-status/login-modal.jinja2 b/package/share/ahriman/templates/build-status/login-modal.jinja2
index 1b4a54e9..53b9068a 100644
--- a/package/share/ahriman/templates/build-status/login-modal.jinja2
+++ b/package/share/ahriman/templates/build-status/login-modal.jinja2
@@ -1,7 +1,7 @@
 <div id="login-modal" tabindex="-1" role="dialog" class="modal fade">
     <div class="modal-dialog" role="document">
         <div class="modal-content">
-            <form action="/api/v1/login" method="post">
+            <form id="login-form" onsubmit="return false">
                 <div class="modal-header">
                     <h4 class="modal-title">Login</h4>
                     <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="close"></button>
@@ -26,7 +26,7 @@
                     </div>
                 </div>
                 <div class="modal-footer">
-                    <button class="btn btn-primary"><i class="bi bi-person"></i> login</button>
+                    <button type="submit" class="btn btn-primary" onclick="login()"><i class="bi bi-person"></i> login</button>
                 </div>
             </form>
         </div>
@@ -34,16 +34,45 @@
 </div>
 
 <script>
-    const passwordInput = $("#login-password");
+    const loginModal = $("#login-modal");
+    const loginForm = $("#login-form");
+    loginModal.on("hidden.bs.modal", () => {
+        loginForm.trigger("reset");
+    });
+
+    const loginPasswordInput = $("#login-password");
+    const loginUsernameInput = $("#login-username");
     const showHidePasswordButton = $("#login-show-hide-password-button");
 
+    function login() {
+        const password = loginPasswordInput.val();
+        const username = loginUsernameInput.val();
+
+        if (username && password) {
+            $.ajax({
+                url: "/api/v1/login",
+                data: JSON.stringify({username: username, password: password}),
+                type: "POST",
+                contentType: "application/json",
+                success: _ => {
+                    loginModal.modal("hide");
+                    showSuccess("Logged in", `Successfully logged in as ${username}`, () => location.href = "/");
+                },
+                error: (jqXHR, _, errorThrown) => {
+                    const message = _ => `Could not login as ${username}`;
+                    showFailure("Login error", message, jqXHR, errorThrown);
+                },
+            });
+        }
+    }
+
     function showPassword() {
-        if (passwordInput.attr("type") === "password") {
-            passwordInput.attr("type", "text");
+        if (loginPasswordInput.attr("type") === "password") {
+            loginPasswordInput.attr("type", "text");
             showHidePasswordButton.removeClass("bi-eye");
             showHidePasswordButton.addClass("bi-eye-slash");
         } else {
-            passwordInput.attr("type", "password");
+            loginPasswordInput.attr("type", "password");
             showHidePasswordButton.removeClass("bi-eye-slash");
             showHidePasswordButton.addClass("bi-eye");
         }
diff --git a/src/ahriman/web/views/base.py b/src/ahriman/web/views/base.py
index ea67fc2a..53209ed3 100644
--- a/src/ahriman/web/views/base.py
+++ b/src/ahriman/web/views/base.py
@@ -20,7 +20,7 @@
 from aiohttp_cors import CorsViewMixin  # type: ignore[import-untyped]
 from aiohttp.web import HTTPBadRequest, HTTPNotFound, Request, StreamResponse, View
 from collections.abc import Awaitable, Callable
-from typing import Any, TypeVar
+from typing import TypeVar
 
 from ahriman.core.auth import Auth
 from ahriman.core.configuration import Configuration
@@ -138,47 +138,6 @@ class BaseView(View, CorsViewMixin):
             raise KeyError(f"Key {key} is missing or empty") from None
         return value
 
-    async def data_as_json(self, list_keys: list[str]) -> dict[str, Any]:
-        """
-        extract form data and convert it to json object
-
-        Args:
-            list_keys(list[str]): list of keys which must be forced to list from form data
-
-        Returns:
-            dict[str, Any]: form data converted to json. In case if a key is found multiple times
-                it will be returned as list
-        """
-        raw = await self.request.post()
-        json: dict[str, Any] = {}
-        for key, value in raw.items():
-            if key in json and isinstance(json[key], list):
-                json[key].append(value)
-            elif key in json:
-                json[key] = [json[key], value]
-            elif key in list_keys:
-                json[key] = [value]
-            else:
-                json[key] = value
-        return json
-
-    async def extract_data(self, list_keys: list[str] | None = None) -> dict[str, Any]:
-        """
-        extract json data from either json or form data
-
-        Args:
-            list_keys(list[str] | None, optional): optional list of keys which must be forced to list from form data
-                (Default value = None)
-
-        Returns:
-            dict[str, Any]: raw json object or form data converted to json
-        """
-        try:
-            json: dict[str, Any] = await self.request.json()
-            return json
-        except ValueError:
-            return await self.data_as_json(list_keys or [])
-
     # pylint: disable=not-callable,protected-access
     async def head(self) -> StreamResponse:  # type: ignore[return]
         """
diff --git a/src/ahriman/web/views/v1/service/add.py b/src/ahriman/web/views/v1/service/add.py
index 9260c71e..7957cfa3 100644
--- a/src/ahriman/web/views/v1/service/add.py
+++ b/src/ahriman/web/views/v1/service/add.py
@@ -66,7 +66,7 @@ class AddView(BaseView):
             HTTPBadRequest: if bad data is supplied
         """
         try:
-            data = await self.extract_data(["packages", "patches"])
+            data = await self.request.json()
             packages = self.get_non_empty(lambda key: [package for package in data[key] if package], "packages")
             patches = [PkgbuildPatch(patch["key"], patch.get("value", "")) for patch in data.get("patches", [])]
         except Exception as ex:
diff --git a/src/ahriman/web/views/v1/service/pgp.py b/src/ahriman/web/views/v1/service/pgp.py
index 8ab94c31..37a34094 100644
--- a/src/ahriman/web/views/v1/service/pgp.py
+++ b/src/ahriman/web/views/v1/service/pgp.py
@@ -104,9 +104,8 @@ class PGPView(BaseView):
         Raises:
             HTTPBadRequest: if bad data is supplied
         """
-        data = await self.extract_data()
-
         try:
+            data = await self.request.json()
             key = self.get_non_empty(data.get, "key")
         except Exception as ex:
             raise HTTPBadRequest(reason=str(ex))
diff --git a/src/ahriman/web/views/v1/service/rebuild.py b/src/ahriman/web/views/v1/service/rebuild.py
index cd41a974..5952101b 100644
--- a/src/ahriman/web/views/v1/service/rebuild.py
+++ b/src/ahriman/web/views/v1/service/rebuild.py
@@ -65,7 +65,7 @@ class RebuildView(BaseView):
             HTTPBadRequest: if bad data is supplied
         """
         try:
-            data = await self.extract_data(["packages"])
+            data = await self.request.json()
             packages = self.get_non_empty(lambda key: [package for package in data[key] if package], "packages")
             depends_on = next(iter(packages))
         except Exception as ex:
diff --git a/src/ahriman/web/views/v1/service/remove.py b/src/ahriman/web/views/v1/service/remove.py
index ccbee7ac..d23ce648 100644
--- a/src/ahriman/web/views/v1/service/remove.py
+++ b/src/ahriman/web/views/v1/service/remove.py
@@ -65,7 +65,7 @@ class RemoveView(BaseView):
             HTTPBadRequest: if bad data is supplied
         """
         try:
-            data = await self.extract_data(["packages"])
+            data = await self.request.json()
             packages = self.get_non_empty(lambda key: [package for package in data[key] if package], "packages")
         except Exception as ex:
             raise HTTPBadRequest(reason=str(ex))
diff --git a/src/ahriman/web/views/v1/service/request.py b/src/ahriman/web/views/v1/service/request.py
index 2919f88e..ce99420a 100644
--- a/src/ahriman/web/views/v1/service/request.py
+++ b/src/ahriman/web/views/v1/service/request.py
@@ -66,7 +66,7 @@ class RequestView(BaseView):
             HTTPBadRequest: if bad data is supplied
         """
         try:
-            data = await self.extract_data(["packages", "patches"])
+            data = await self.request.json()
             packages = self.get_non_empty(lambda key: [package for package in data[key] if package], "packages")
             patches = [PkgbuildPatch(patch["key"], patch.get("value", "")) for patch in data.get("patches", [])]
         except Exception as ex:
diff --git a/src/ahriman/web/views/v1/service/update.py b/src/ahriman/web/views/v1/service/update.py
index b7187eb8..f5587436 100644
--- a/src/ahriman/web/views/v1/service/update.py
+++ b/src/ahriman/web/views/v1/service/update.py
@@ -65,7 +65,7 @@ class UpdateView(BaseView):
             HTTPBadRequest: if bad data is supplied
         """
         try:
-            data = await self.extract_data()
+            data = await self.request.json()
         except Exception as ex:
             raise HTTPBadRequest(reason=str(ex))
 
diff --git a/src/ahriman/web/views/v1/status/logs.py b/src/ahriman/web/views/v1/status/logs.py
index c6b2bffa..73a0b690 100644
--- a/src/ahriman/web/views/v1/status/logs.py
+++ b/src/ahriman/web/views/v1/status/logs.py
@@ -139,9 +139,9 @@ class LogsView(BaseView):
             HTTPNoContent: in case of success response
         """
         package_base = self.request.match_info["package"]
-        data = await self.extract_data()
 
         try:
+            data = await self.request.json()
             created = data["created"]
             record = data["message"]
             version = data["version"]
diff --git a/src/ahriman/web/views/v1/status/package.py b/src/ahriman/web/views/v1/status/package.py
index b7084331..d82f582b 100644
--- a/src/ahriman/web/views/v1/status/package.py
+++ b/src/ahriman/web/views/v1/status/package.py
@@ -142,9 +142,9 @@ class PackageView(BaseView):
             HTTPNoContent: in case of success response
         """
         package_base = self.request.match_info["package"]
-        data = await self.extract_data()
 
         try:
+            data = await self.request.json()
             package = Package.from_json(data["package"]) if "package" in data else None
             status = BuildStatusEnum(data["status"])
         except Exception as ex:
diff --git a/src/ahriman/web/views/v1/status/patches.py b/src/ahriman/web/views/v1/status/patches.py
index 8b01ca65..4892daf4 100644
--- a/src/ahriman/web/views/v1/status/patches.py
+++ b/src/ahriman/web/views/v1/status/patches.py
@@ -95,9 +95,9 @@ class PatchesView(BaseView):
             HTTPNoContent: on success response
         """
         package_base = self.request.match_info["package"]
-        data = await self.extract_data()
 
         try:
+            data = await self.request.json()
             key = data["key"]
             value = data["value"]
         except Exception as ex:
diff --git a/src/ahriman/web/views/v1/status/status.py b/src/ahriman/web/views/v1/status/status.py
index d16ae49e..d61655f1 100644
--- a/src/ahriman/web/views/v1/status/status.py
+++ b/src/ahriman/web/views/v1/status/status.py
@@ -102,7 +102,7 @@ class StatusView(BaseView):
             HTTPNoContent: in case of success response
         """
         try:
-            data = await self.extract_data()
+            data = await self.request.json()
             status = BuildStatusEnum(data["status"])
         except Exception as ex:
             raise HTTPBadRequest(reason=str(ex))
diff --git a/src/ahriman/web/views/v1/user/login.py b/src/ahriman/web/views/v1/user/login.py
index 9f197af5..543e03dc 100644
--- a/src/ahriman/web/views/v1/user/login.py
+++ b/src/ahriman/web/views/v1/user/login.py
@@ -19,7 +19,7 @@
 #
 import aiohttp_apispec  # type: ignore[import-untyped]
 
-from aiohttp.web import HTTPFound, HTTPMethodNotAllowed, HTTPUnauthorized
+from aiohttp.web import HTTPBadRequest, HTTPFound, HTTPMethodNotAllowed, HTTPUnauthorized
 
 from ahriman.core.auth.helpers import remember
 from ahriman.models.user_access import UserAccess
@@ -93,6 +93,7 @@ class LoginView(BaseView):
         description="Login by using username and password",
         responses={
             302: {"description": "Success response"},
+            400: {"description": "Bad data is supplied", "schema": ErrorSchema},
             401: {"description": "Authorization required", "schema": ErrorSchema},
             500: {"description": "Internal server error", "schema": ErrorSchema},
         },
@@ -107,11 +108,15 @@ class LoginView(BaseView):
             HTTPFound: on success response
             HTTPUnauthorized: if case of authorization error
         """
-        data = await self.extract_data()
-        identity = data.get("username")
+        try:
+            data = await self.request.json()
+            identity = data["username"]
+            password = data["password"]
+        except Exception as ex:
+            raise HTTPBadRequest(reason=str(ex))
 
         response = HTTPFound("/")
-        if identity is not None and await self.validator.check_credentials(identity, data.get("password")):
+        if await self.validator.check_credentials(identity, password):
             await remember(self.request, response, identity)
             raise response
 
diff --git a/tests/ahriman/web/views/test_view_base.py b/tests/ahriman/web/views/test_view_base.py
index 3faff16f..9c4ef4fe 100644
--- a/tests/ahriman/web/views/test_view_base.py
+++ b/tests/ahriman/web/views/test_view_base.py
@@ -87,68 +87,6 @@ def test_get_non_empty() -> None:
         BaseView.get_non_empty(lambda k: [], "key")
 
 
-async def test_data_as_json(base: BaseView) -> None:
-    """
-    must parse multi value form payload
-    """
-    json = {"key1": "value1", "key2": ["value2", "value3"], "key3": ["value4", "value5", "value6"]}
-
-    async def get_data():
-        result = MultiDict()
-        for key, values in json.items():
-            if isinstance(values, list):
-                for value in values:
-                    result.add(key, value)
-            else:
-                result.add(key, values)
-        return result
-
-    base._request = pytest.helpers.request(base.request.app, "", "", data=get_data)
-    assert await base.data_as_json([]) == json
-
-
-async def test_data_as_json_with_list_keys(base: BaseView) -> None:
-    """
-    must parse multi value form payload with forced list
-    """
-    json = {"key1": "value1"}
-
-    async def get_data():
-        return json
-
-    base._request = pytest.helpers.request(base.request.app, "", "", data=get_data)
-    assert await base.data_as_json(["key1"]) == {"key1": ["value1"]}
-
-
-async def test_extract_data_json(base: BaseView) -> None:
-    """
-    must parse and return json
-    """
-    json = {"key1": "value1", "key2": "value2"}
-
-    async def get_json():
-        return json
-
-    base._request = pytest.helpers.request(base.request.app, "", "", json=get_json)
-    assert await base.extract_data() == json
-
-
-async def test_extract_data_post(base: BaseView) -> None:
-    """
-    must parse and return form data
-    """
-    json = {"key1": "value1", "key2": "value2"}
-
-    async def get_json():
-        raise ValueError()
-
-    async def get_data():
-        return json
-
-    base._request = pytest.helpers.request(base.request.app, "", "", json=get_json, data=get_data)
-    assert await base.extract_data() == json
-
-
 async def test_head(client: TestClient) -> None:
     """
     must implement head as get method
diff --git a/tests/ahriman/web/views/v1/service/test_view_v1_service_update.py b/tests/ahriman/web/views/v1/service/test_view_v1_service_update.py
index 913fa23c..2990599a 100644
--- a/tests/ahriman/web/views/v1/service/test_view_v1_service_update.py
+++ b/tests/ahriman/web/views/v1/service/test_view_v1_service_update.py
@@ -63,7 +63,6 @@ async def test_post_empty(client: TestClient, mocker: MockerFixture) -> None:
     """
     must call raise 400 on invalid request
     """
-    mocker.patch("ahriman.web.views.base.BaseView.extract_data", side_effect=Exception())
     update_mock = mocker.patch("ahriman.core.spawn.Spawn.packages_update")
     response_schema = pytest.helpers.schema_response(UpdateView.post, code=400)
 
diff --git a/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py b/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py
index 9a47e250..ac531a28 100644
--- a/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py
+++ b/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py
@@ -125,9 +125,6 @@ async def test_post(client_with_auth: TestClient, user: User, mocker: MockerFixt
     response = await client_with_auth.post("/api/v1/login", json=payload)
     assert response.ok
 
-    response = await client_with_auth.post("/api/v1/login", data=payload)
-    assert response.ok
-
     remember_mock.assert_called()
 
 
@@ -156,3 +153,16 @@ async def test_post_unauthorized(client_with_auth: TestClient, user: User, mocke
     assert response.status == 401
     assert not response_schema.validate(await response.json())
     remember_mock.assert_not_called()
+
+
+async def test_post_invalid_json(client_with_auth: TestClient, mocker: MockerFixture) -> None:
+    """
+    must return unauthorized on invalid auth
+    """
+    response_schema = pytest.helpers.schema_response(LoginView.post, code=400)
+    remember_mock = mocker.patch("aiohttp_security.remember")
+
+    response = await client_with_auth.post("/api/v1/login")
+    assert response.status == 400
+    assert not response_schema.validate(await response.json())
+    remember_mock.assert_not_called()