✅ Improve authorization testing of the UI pages

Why:
- The DMZ checks view permissions automatically, so the tests for
  individual pages don't need to check that.
- Added permission checks to viewing printouts and settings pages. Uses
  the same code which determines whether the links to those pages are
  visible in the navigation, so it should not be possible to access the
  pages with a hand-crafted URL.
- Made the error messages a more generic "Access denied" to avoid
  leaking information and to improve code reuse. The stack trace will
  anyway tell which permission check failed.

Author: luontola

src/territory_bro/domain/dmz.clj

@@ -61,16 +61,19 @@
   (when-not (auth/logged-in?)
     (http-response/unauthorized! "Not logged in")))
 
+(defn access-denied! []
+  (require-logged-in!) ; if the user is not logged in, first prompt them to log in - they might have access after logging in
+  (http-response/forbidden! "Access denied"))
+
 (defn- super-user? []
   (let [super-users (:super-users config/env)
         user auth/*user*]
     (or (contains? super-users (:user/id user))
         (contains? super-users (:sub user)))))
 
 (defn sudo [session]
-  (require-logged-in!)
   (when-not (super-user?)
-    (http-response/forbidden! "Not super user"))
+    (access-denied!))
   (log/info "Super user promotion")
   (assoc session ::sudo? true))
 
@@ -147,9 +150,7 @@
             (dissoc :congregation/loans-csv-url)
             (dissoc :congregation/schema-name))
         congregation))
-    (do
-      (require-logged-in!)
-      (http-response/forbidden! "No congregation access"))))
+    (access-denied!)))
 
 (defn list-congregations []
   (let [user-id (auth/current-user-id)]
@@ -195,9 +196,7 @@
       (if (= "demo" cong-id)
         (assoc territory :congregation/id "demo")
         (enrich-do-not-calls territory)))
-    (do
-      (require-logged-in!)
-      (http-response/forbidden! "No territory access"))))
+    (access-denied!)))
 
 (defn list-territories [cong-id {:keys [fetch-loans?]}]
   (cond

src/territory_bro/ui/printouts_page.clj

@@ -58,6 +58,8 @@
 
 (defn model! [request]
   (let [cong-id (get-in request [:path-params :congregation])
+        _ (when-not (dmz/view-printouts-page? cong-id)
+            (dmz/access-denied!))
         congregation (dmz/get-congregation cong-id)
         regions (->> (dmz/list-regions cong-id)
                      (sort-by (comp str :region/name)

src/territory_bro/ui/settings_page.clj

@@ -21,9 +21,9 @@
 
 (defn model! [request]
   (let [cong-id (get-in request [:path-params :congregation])
-        congregation (if (= "demo" cong-id) ; TODO: replace with permission check for editing settings
-                       (http-response/not-found! "Not available in demo")
-                       (dmz/get-congregation cong-id))
+        _ (when-not (dmz/view-settings-page? cong-id)
+            (dmz/access-denied!))
+        congregation (dmz/get-congregation cong-id)
         users (dmz/list-congregation-users cong-id)
         new-user (some-> (get-in request [:params :new-user])
                          (parse-uuid))]

test/territory_bro/browser_test.clj

@@ -231,7 +231,8 @@
         (is (= "/congregation/demo/settings" settings-path))
         (doto *driver*
           (b/go (str *base-url* settings-path))
-          (b/wait-has-text h1 "Page not found"))))))
+          (b/wait-has-text h1 "Welcome")) ; access denied, so redirects the anonymous user to Auth0 login screen
+        (is (str/includes? (b/get-url *driver*) ".auth0.com/"))))))
 
 
 (deftest registration-test

test/territory_bro/domain/dmz_test.clj

@@ -109,15 +109,10 @@
    :response {:status 401
               :body "Not logged in"
               :headers {}}})
-(def no-congregation-access
+(def access-denied
   {:type :ring.util.http-response/response
    :response {:status 403
-              :body "No congregation access"
-              :headers {}}})
-(def no-territory-access
-  {:type :ring.util.http-response/response
-   :response {:status 403
-              :body "No territory access"
+              :body "Access denied"
               :headers {}}})
 
 
@@ -140,7 +135,7 @@
 
       (testutil/with-user-id (UUID. 0 0x666)
         (testing "no permissions"
-          (is (thrown-match? ExceptionInfo no-congregation-access
+          (is (thrown-match? ExceptionInfo access-denied
                              (dmz/get-congregation cong-id)))))
 
       (testutil/with-anonymous-user
@@ -182,7 +177,7 @@
 
         (testutil/with-user-id (UUID. 0 0x666)
           (testing "no permissions"
-            (is (thrown-match? ExceptionInfo no-territory-access
+            (is (thrown-match? ExceptionInfo access-denied
                                (dmz/get-territory cong-id territory-id)))))
 
         (testutil/with-anonymous-user

test/territory_bro/ui/congregation_page_test.clj

@@ -10,8 +10,7 @@
             [territory-bro.test.testutil :as testutil]
             [territory-bro.ui.congregation-page :as congregation-page]
             [territory-bro.ui.html :as html])
-  (:import (clojure.lang ExceptionInfo)
-           (java.util UUID)))
+  (:import (java.util UUID)))
 
 (def model
   {:congregation {:congregation/name "Example Congregation"}
@@ -34,32 +33,12 @@
                                       (congregation/admin-permissions-granted cong-id user-id)])
         (testutil/with-user-id user-id
 
-          (testing "logged in"
+          (testing "default"
             (is (= model (congregation-page/model! request))))
 
-          (testing "logged in, no access"
-            (is (thrown-match? ExceptionInfo
-                               {:type :ring.util.http-response/response
-                                :response {:status 403
-                                           :body "No congregation access"
-                                           :headers {}}}
-                               (congregation-page/model! {:path-params {:congregation (UUID/randomUUID)}}))))
-
-          (testing "anonymous user"
-            (testutil/with-anonymous-user
-              (is (thrown-match? ExceptionInfo
-                                 {:type :ring.util.http-response/response
-                                  :response {:status 401
-                                             :body "Not logged in"
-                                             :headers {}}}
-                                 (congregation-page/model! request)))))
-
           (testing "demo congregation"
             (let [request {:path-params {:congregation "demo"}}]
-              (is (= demo-model
-                     (congregation-page/model! request)
-                     (testutil/with-anonymous-user
-                       (congregation-page/model! request)))))))))))
+              (is (= demo-model (congregation-page/model! request))))))))))
 
 (deftest view-test
   (testing "full permissions"

test/territory_bro/ui/home_page_test.clj

@@ -58,12 +58,12 @@
                        (replace-in [:congregations 1 :congregation/id] (UUID. 0 2) cong-id2))
                    (home-page/model! request)))))
 
-        (testing "anonymous user"
+        (testing "anonymous"
           (testutil/with-anonymous-user
             (is (= anonymous-model (home-page/model! request))))))
 
       (binding [config/env {:demo-congregation nil}]
-        (testing "anonymous user, no demo"
+        (testing "anonymous, no demo"
           (testutil/with-anonymous-user
             (is (= no-demo-model (home-page/model! request)))))))))
 
@@ -102,7 +102,7 @@
                                      (replace-in [:demo-available?] true false)))
                  html/visible-text))))
 
-    (testing "anonymous user"
+    (testing "anonymous"
       (is (= (html/normalize-whitespace
               (str introduction
                    "Login
@@ -112,7 +112,7 @@
              (-> (home-page/view anonymous-model)
                  html/visible-text))))
 
-    (testing "anonymous user, no demo"
+    (testing "anonymous, no demo"
       (is (= (html/normalize-whitespace
               (str introduction
                    "Login

test/territory_bro/ui/join_page_test.clj

@@ -5,6 +5,7 @@
 (ns territory-bro.ui.join-page-test
   (:require [clojure.test :refer :all]
             [matcher-combinators.test :refer :all]
+            [territory-bro.domain.dmz-test :as dmz-test]
             [territory-bro.test.fixtures :refer :all]
             [territory-bro.test.testutil :as testutil]
             [territory-bro.ui.html :as html]
@@ -22,13 +23,9 @@
       (testutil/with-user-id user-id
         (is (= model (join-page/model! request)))))
 
-    (testing "anonymous user"
+    (testing "anonymous"
       (testutil/with-anonymous-user
-        (is (thrown-match? ExceptionInfo
-                           {:type :ring.util.http-response/response
-                            :response {:status 401
-                                       :body "Not logged in"
-                                       :headers {}}}
+        (is (thrown-match? ExceptionInfo dmz-test/not-logged-in
                            (join-page/model! request)))))))
 
 (deftest view-test

test/territory_bro/ui/open_share_page_test.clj

@@ -8,6 +8,7 @@
             [reitit.core :as reitit]
             [territory-bro.domain.dmz :as dmz]
             [territory-bro.domain.share :as share]
+            [territory-bro.infra.authentication :as auth]
             [territory-bro.infra.config :as config]
             [territory-bro.infra.middleware :as middleware]
             [territory-bro.test.fixtures :refer :all]
@@ -23,7 +24,6 @@
         share-id (UUID. 0 3)
         share-key "abc123"
         demo-share-key (share/demo-share-key territory-id)
-        user-id (UUID. 0 0x10)
         request {:path-params {:share-key share-key}}]
     (testutil/with-events [{:event/type :share.event/share-created
                             :congregation/id cong-id
@@ -32,13 +32,13 @@
                             :share/key share-key
                             :share/type :link}]
       (binding [config/env {:now #(Instant/now)}]
+        (testutil/with-anonymous-user
 
-        (testing "open regular share"
-          (testutil/with-user-id user-id
+          (testing "open regular share"
             (with-fixtures [fake-dispatcher-fixture]
               (let [response (open-share-page/open-share! request)]
                 (is (= {:command/type :share.command/record-share-opened
-                        :command/user user-id
+                        :command/user auth/anonymous-user-id
                         :share/id share-id}
                        (dissoc @*last-command :command/time))
                     "records a history of opening the share")
@@ -48,10 +48,9 @@
                         ::middleware/mutative-operation? true
                         :body ""}
                        response)
-                    "stores in session which shares the user has opened")))))
+                    "stores in session which shares the user has opened"))))
 
-        (testing "keeps existing session state, supports opening multiple shares"
-          (testutil/with-anonymous-user
+          (testing "keeps existing session state, supports opening multiple shares"
             (with-fixtures [fake-dispatcher-fixture]
               (let [another-share-id (UUID/randomUUID)
                     request (assoc request :session {::dmz/opened-shares #{another-share-id}
@@ -60,11 +59,10 @@
                 (is (= {::dmz/opened-shares #{share-id
                                               another-share-id}
                         :other-session-state "stuff"}
-                       (:session response)))))))
+                       (:session response))))))
 
-        (testing "open demo share"
-          (let [request {:path-params {:share-key demo-share-key}}]
-            (testutil/with-anonymous-user
+          (testing "open demo share"
+            (let [request {:path-params {:share-key demo-share-key}}]
               (with-fixtures [fake-dispatcher-fixture]
                 (let [response (open-share-page/open-share! request)]
                   (is (= {:status 303
@@ -73,11 +71,10 @@
                          response)
                       "redirects to demo, without touching session state")
                   (is (nil? @*last-command)
-                      "does not record that a demo share was opened"))))))
+                      "does not record that a demo share was opened")))))
 
-        (testing "share not found"
-          (let [request {:path-params {:share-key "bad key"}}]
-            (testutil/with-anonymous-user
+          (testing "share not found"
+            (let [request {:path-params {:share-key "bad key"}}]
               (with-fixtures [fake-dispatcher-fixture]
                 (is (thrown-match? ExceptionInfo
                                    {:type :ring.util.http-response/response

test/territory_bro/ui/printouts_page_test.clj

@@ -5,8 +5,10 @@
 (ns territory-bro.ui.printouts-page-test
   (:require [clojure.string :as str]
             [clojure.test :refer :all]
+            [matcher-combinators.test :refer :all]
             [territory-bro.domain.congregation :as congregation]
             [territory-bro.domain.dmz :as dmz]
+            [territory-bro.domain.dmz-test :as dmz-test]
             [territory-bro.domain.do-not-calls :as do-not-calls]
             [territory-bro.domain.do-not-calls-test :as do-not-calls-test]
             [territory-bro.domain.share :as share]
@@ -20,7 +22,8 @@
             [territory-bro.ui.html :as html]
             [territory-bro.ui.map-interaction-help-test :as map-interaction-help-test]
             [territory-bro.ui.printouts-page :as printouts-page])
-  (:import (java.time Clock Duration Instant ZoneOffset ZonedDateTime)
+  (:import (clojure.lang ExceptionInfo)
+           (java.time Clock Duration Instant ZoneOffset ZonedDateTime)
            (java.util UUID)))
 
 (def cong-id (UUID. 0 1))
@@ -139,6 +142,14 @@
           (binding [dmz/*state* (permissions/revoke dmz/*state* user-id [:share-territory-link cong-id])]
             (is (= no-qr-codes-model (printouts-page/model! request)))))
 
+        (testing "has opened a share, cannot see all territories"
+          (let [user-id (UUID/randomUUID)
+                share-id (UUID/randomUUID)]
+            (testutil/with-user-id user-id
+              (binding [dmz/*state* (share/grant-opened-shares dmz/*state* [share-id] user-id)]
+                (is (thrown-match? ExceptionInfo dmz-test/access-denied
+                                   (printouts-page/model! request)))))))
+
         (testing "demo congregation"
           (binding [config/env {:demo-congregation cong-id}]
             (let [request {:path-params {:congregation "demo"}}]