feat(backend): refactor and document

This commit significantly cleans up the routes and middleware stack, and
adds documentation to the same.

The refactor resolves several problems with routes where format
transformation, authentication, and others were not happening in the
right order. It also removes several redundant or unnecessary
middlewares.

The result is a working Transit API + server-side rendered HTML pages.

Author: goshatch

src/main/parts/api/account.clj

@@ -16,7 +16,7 @@
   "Update own account info"
   [request]
   (let [user-id (get-in request [:identity :sub])
-        body (:body request)
+        body (:body-params request)
         updated-user (user/update! user-id body)]
     (mulog/log ::update-account-success :user-id user-id)
     (-> (response/response updated-user)
@@ -25,7 +25,7 @@
 (defn register-account
   "Creates a record for a new user account"
   [request]
-  (let [account (user/create! (:body request))]
+  (let [account (user/create! (:body-params request))]
     (mulog/log ::register :email (:email account) :username (:username account) :status :success)
     (-> (response/response account)
         (response/status 201))))

src/main/parts/api/auth.clj

@@ -5,8 +5,10 @@
    [ring.util.response :as response]))
 
 (defn login
+  "Handler for the POST /api/auth/login endpoint.
+  Generate new access and refresh tokens by authenticating via email/password"
   [request]
-  (let [{:keys [email password]} (:body request)]
+  (let [{:keys [email password]} (:body-params request)]
     (if-let [tokens (auth/authenticate {:email email :password password})]
       (do
         (mulog/log ::login :email email :status :success)
@@ -18,9 +20,10 @@
             (response/status 401))))))
 
 (defn refresh
-  "Generate new access and refresh tokens using a valid refresh token"
+  "Handler for the POST /api/auth/refresh endpoint.
+  Generate new access and refresh tokens using a valid refresh token"
   [request]
-  (let [refresh-token (get-in request [:body :refresh_token])]
+  (let [refresh-token (get-in request [:body-params :refresh_token])]
     (if-let [new-tokens (auth/refresh-auth-tokens refresh-token)]
       (do
         (mulog/log ::refresh :status :success)
@@ -32,9 +35,10 @@
             (response/status 401))))))
 
 (defn logout
-  "Invalidate the refresh token to prevent its future use"
+  "Handler for the POST /api/auth/logout endpoint.
+  Invalidate the refresh token to prevent its future use"
   [request]
-  (let [refresh-token (get-in request [:body :refresh_token])]
+  (let [refresh-token (get-in request [:body-params :refresh_token])]
     (if refresh-token
       (do
         (auth/invalidate-refresh-token refresh-token)

src/main/parts/api/systems.clj

@@ -1,6 +1,5 @@
 (ns parts.api.systems
   (:require
-   [parts.auth :as auth]
    [parts.entity.system :as system]
    [ring.util.response :as response]))
 
@@ -14,9 +13,9 @@
 
 (defn create-system
   "Create a new system"
-  [{:keys [identity body] :as _request}]
+  [{:keys [identity body-params] :as _request}]
   (let [user-id (:sub identity)
-        system-data (assoc body :owner_id user-id)
+        system-data (assoc body-params :owner_id user-id)
         created (system/create-system! system-data)]
     (-> (response/response created)
         (response/status 201))))
@@ -35,13 +34,13 @@
 
 (defn update-system
   "Update an existing system"
-  [{:keys [identity parameters body] :as _request}]
+  [{:keys [identity parameters body-params] :as _request}]
   (let [user-id (:sub identity)
         system-id (get-in parameters [:path :id])
         existing (system/get-system system-id)]
     (if (= user-id (:owner_id existing))
       (let [updated (system/update-system! system-id
-                                           (assoc body :owner_id (:owner_id existing)))]
+                                           (assoc body-params :owner_id (:owner_id existing)))]
         (-> (response/response updated)
             (response/status 200)))
       (-> (response/response {:error "Not authorized"})

src/main/parts/middleware.clj

@@ -6,31 +6,38 @@
    [com.brunobonacci.mulog :as mulog]
    [parts.auth :as auth]
    [reitit.ring.middleware.exception :as exception]
-   [ring.middleware.content-type :refer [wrap-content-type]]
-   [ring.middleware.defaults :refer [site-defaults wrap-defaults]]
+   [ring.middleware.anti-forgery :refer [wrap-anti-forgery]]
+   [ring.middleware.defaults :refer [api-defaults wrap-defaults site-defaults]]
    [ring.middleware.resource :refer [wrap-resource]]
-   [ring.middleware.session :refer [wrap-session]]
-   [ring.middleware.session.cookie :refer [cookie-store]]
    [ring.util.response :as response])
   (:import
    (org.sqlite SQLiteException)))
 
-(defn exception-handler
-  "Generic exceptions handler"
+(defn- exception-handler
+  "Generic exceptions handler used by the `exception` middleware.
+
+  Sets the response status to the provided `status`, and sets the response
+  message to the error message retrieved from the exception, or, failing that,
+  to the `message` provided."
   [message status]
   (fn [^Exception e _request]
     (let [error-message (.getMessage e)]
       {:status status
        :body {:error (or error-message message)}})))
 
 (def sqlite-errors
+  "A map containing substrings of a SQLiteException error message, and the
+  corresponding user friendly error message."
   {"UNIQUE constraint failed" "A resource with this unique identifier already exists"
    "CHECK constraint failed" "The provided data does not meet the required constraints"
    "NOT NULL constraint failed" "A required field was missing"
    "FOREIGN KEY constraint failed" "The referenced resource does not exist"})
 
 (defn sqlite-constraint-violation-handler
-  "Handler for SQLite-specific exceptions"
+  "Handler for SQLite-specific exceptions.
+
+  If the error message includes a string that is a key in `sqlite-errors`, the
+  error message will be the corresponding value; otherwise a generic message."
   [^SQLiteException e _request]
   (let [error-message (.getMessage e)]
     (mulog/log ::sqlite-exception :error error-message)
@@ -41,7 +48,9 @@
                        "A database constraint was violated")}}))
 
 (def exception
-  "Middleware handling exceptions"
+  "Middleware handling exceptions. Combines the default exception handlers from
+  Reitit with cutom handlers. New custom handlers should be added to this
+  function."
   (exception/create-exception-middleware
    (merge
     exception/default-handlers
@@ -50,61 +59,158 @@
 
      :not-found (exception-handler "Resource not found" 404)
 
-       ;; SQLite exceptions
+     ;; SQLite exceptions
      SQLiteException sqlite-constraint-violation-handler
 
-       ;; Default
+     ;; Default
      ::exception/default
      (fn [^Exception e _request]
        (mulog/log ::unhandled-exception :error (.getMessage e))
        {:status 500
         :body {:error "Internal server error"}})})))
 
 (defn logging
-  "Middleware logging each incoming request"
+  "Middleware logging each incoming request with minimal information.
+
+  This middleware is called before Muuntaja converts the input into clojure
+  objects, so to `:body` value is a `java.io.InputStream`, which is not easily
+  inspectable.  Later in the lifecycle, Muuntaja will insert a parsed body under
+  the `:parsed-params` key."
   [handler]
   (fn [request]
     (let [user-id (get-in request [:identity :sub])
+          request-info {:uri (:uri request)
+                        :request-method (:request-method request)
+                        :query-params (:query-params request)
+                        :remote-addr (:remote-addr request)
+                        :user-agent (get-in request [:headers "user-agent"])}
           authenticated? (boolean user-id)]
-      (mulog/log ::request :request request, :authenticated? authenticated? :user-id user-id)
+      (mulog/log ::request :info request-info :authenticated? authenticated? :user-id user-id)
       (handler request))))
 
 (defn wrap-jwt-authentication
-  "Middleware adding JWT authentication to a route"
+  "Middleware adding JWT authentication to a route. A route with this middleware
+  applied will have an authentication status which can be validated against."
   [handler]
   (-> handler
       (wrap-authentication auth/backend)
       (wrap-authorization auth/backend)))
 
 (defn jwt-auth
-  "Middleware ensuring a route is only accessible to authenticated users"
+  "Middleware ensuring a route is only accessible to authenticated users. "
   [handler]
   (fn [request]
     (if (authenticated? request)
       (handler request)
       (-> (response/response {:error "Unauthorized"})
           (response/status 401)))))
 
-(defn wrap-default-middlewares
-  "Wrap in the middlewares defined by ring-defaults"
+(defn wrap-html-defaults
+  "Middleware that applies a customized set of Ring defaults for HTML routes.
+
+  This middleware configures a subset of Ring's `site-defaults` that's
+  appropriate for server-rendered HTML pages.
+
+  Applied configurations:
+  - Parameters: Parses standard, nested, and keyword parameters
+  - Static resources: Enabled
+  - Security headers:
+    - X-Frame-Options
+    - X-Content-Type-Options
+    - X-XSS-Protection
+    - X-Permitted-Cross-Domain-Policies
+    - X-Download-Options
+  - Cookie response attributes
+
+  Explicitly disabled:
+  - Session handling: Disabled completely as we use stateless JWT auth
+  - Global anti-forgery: Disabled here but applied selectively to form-handling
+    routes, see `anti-forgery`
+
+  Usage: Apply this middleware to routes that serve HTML content, and separately
+  apply `anti-forgery` middleware only to routes that process form submissions."
+  [handler]
+  (wrap-defaults
+   handler
+   (-> site-defaults
+       (assoc :session false)
+       (assoc :security
+              (-> (:security site-defaults)
+                  (assoc :anti-forgery false))))))
+
+;; TODO: Investigate whether Content Security Policy is needed:
+;;  - https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
+;;  - https://cheatsheetseries.owasp.org/cheatsheets/Content_Security_Policy_Cheat_Sheet.html
+(defn wrap-api-defaults
+  "Apply default middleware for API routes.
+
+  This uses standard `api-defaults` (not secure-api-defaults) as the base
+  configuration, making it suitable for applications behind a reverse proxy that
+  handles SSL termination.
+
+  Security enhancements:
+  - Sets X-Frame-Options to SAMEORIGIN to prevent clickjacking attacks
+  - Sets X-Content-Type-Options to nosniff to prevent MIME type sniffing
+  - Enables X-XSS-Protection with mode=block to activate browser XSS filters
+
+  Notable omissions:
+  - No HTTPS enforcement (handled by reverse proxy)
+  - No Strict-Transport-Security header (better set at proxy level)
+  - No Content-Security-Policy"
+  [handler]
+  (wrap-defaults
+   handler
+   (-> api-defaults
+       (assoc-in [:security :frame-options] :sameorigin)
+       (assoc-in [:security :content-type-options] :nosniff)
+       (assoc-in [:security :xss-protection] {:mode :block}))))
+
+(defn wrap-core-middlewares
+  "Apply essential Ring middleware for the entire application. Currently, this
+  is only `wrap-resources`, which allows to download static files from
+  resources/public."
   [handler]
   (-> handler
-      (wrap-defaults (-> site-defaults
-                         (assoc-in [:session :store] (cookie-store))))
-      (wrap-resource "public")
-      (wrap-session)
-      (wrap-content-type)))
-
-;; FIXME: This feels like it shouldn't need a custom middleware, right? Is there
-;; a middleware supplied by either httpkit or hiccup2 already?
+      (wrap-resource "public")))
+
 (defn wrap-html-response
-  "Set content type to text/html and convert response to string"
+  "Middleware for properly formatting HTML responses.
+
+  This middleware performs two essential transformations for HTML routes:
+
+  1. It ensures the response body is a string by calling `str` on it.  This
+  handles various body types like Hiccup structures or other Clojure data that
+  should be rendered as HTML.
+
+  2. It sets the Content-Type header to 'text/html; charset=utf-8' if no
+  Content-Type is already specified, ensuring browsers properly interpret the
+  response.
+
+  This middleware only processes responses that:
+  - Are proper Ring response maps
+  - Don't already have a Content-Type header set
+
+  Usage: Apply this middleware to routes that return HTML content.  It's an
+  alternative to using Muuntaja for HTML formatting, which is more appropriate
+  for data formats rather than presentation formats."
   [handler]
   (fn [request]
     (let [response (handler request)]
       (if (and (map? response)
                (not (get-in response [:headers "Content-Type"])))
         (-> response
             (update :body str)
-            (assoc-in [:headers "Content-Type"] "text/html"))
+            (assoc-in [:headers "Content-Type"] "text/html; charset=utf-8"))
         response))))
+
+(def anti-forgery
+  "Anti-forgery middleware for HTML forms, see docs on `wrap-anti-forgery`"
+  wrap-anti-forgery)
+
+;; File upload handling is commented out for now
+;; To enable file uploads, uncomment the following:
+;; (defn wrap-multipart-params
+;;   "Handle multipart form data including file uploads"
+;;   [handler]
+;;   (-> handler
+;;       (multipart-params/wrap-multipart-params)))