🐛 Fix creating a demo share after opening a normal share

Why: - If an anonymous user opens a shared link, and after that goes to the demo and tries to share a link, it will crash because (some? (::opened-shares session)) is true but (:user/id auth/*user*) is nil. - Also changed generate-qr-codes to always call auth/with-user-from-session, because it had the same fragile code pattern as share-territory-link. - Fixes the exception: java.lang.AssertionError: Assert failed: id at territory_bro.api$current_user_id.invokeStatic(api.clj:171) at territory_bro.api$current_user_id.invoke(api.clj:169) at territory_bro.api$enrich_state_for_request.invokeStatic(api.clj:179) at territory_bro.api$enrich_state_for_request.invoke(api.clj:174) at territory_bro.api$state_for_request.invokeStatic(api.clj:183) at territory_bro.api$state_for_request.invoke(api.clj:181) at territory_bro.api$share_territory_link.invokeStatic(api.clj:411) at territory_bro.api$share_territory_link.invoke(api.clj:407) at territory_bro.api$fn__35021.invokeStatic(api.clj:528) at territory_bro.api$fn__35021.invoke(api.clj:528) at compojure.core$wrap_response$fn__13386.invoke(core.clj:158) at compojure.core$pre_init$fn__13487.invoke(core.clj:333) at ring.middleware.http_response$wrap_http_response$fn__13550.invoke(http_response.clj:19) at compojure.core$pre_init$fn__13489$fn__13492.invoke(core.clj:340) at compojure.core$pre_init$fn__13487.invoke(core.clj:333) at ring.middleware.format_params$wrap_format_params$fn__18913.invoke(format_params.clj:111) at ring.middleware.format_params$wrap_format_params$fn__18913.invoke(format_params.clj:113) at ring.middleware.format_params$wrap_format_params$fn__18913.invoke(format_params.clj:113) at ring.middleware.format_response$wrap_format_response$fn__19254.invoke(format_response.clj:175) at compojure.core$pre_init$fn__13489$fn__13492.invoke(core.clj:340) at compojure.core$wrap_route_middleware$fn__13370.invoke(core.clj:127) at compojure.core$wrap_route_info$fn__13375.invoke(core.clj:137) at compojure.core$wrap_route_matches$fn__13379.invoke(core.clj:146) at compojure.core$routing$fn__13394.invoke(core.clj:185) at clojure.core$some.invokeStatic(core.clj:2718) at clojure.core$some.invoke(core.clj:2709) at compojure.core$routing.invokeStatic(core.clj:185) at compojure.core$routing.doInvoke(core.clj:182) at clojure.lang.RestFn.applyTo(RestFn.java:139) at clojure.core$apply.invokeStatic(core.clj:669) at clojure.core$apply.invoke(core.clj:662) at compojure.core$routes$fn__13398.invoke(core.clj:192) at clojure.lang.Var.invoke(Var.java:384) at compojure.core$routing$fn__13394.invoke(core.clj:185) at clojure.core$some.invokeStatic(core.clj:2718) at clojure.core$some.invoke(core.clj:2709) at compojure.core$routing.invokeStatic(core.clj:185) at compojure.core$routing.doInvoke(core.clj:182) at clojure.lang.RestFn.applyTo(RestFn.java:139) at clojure.core$apply.invokeStatic(core.clj:669) at clojure.core$apply.invoke(core.clj:662) at compojure.core$routes$fn__13398.invoke(core.clj:192) at compojure.core$wrap_routes$fn__13499.invoke(core.clj:353) at compojure.core$wrap_routes$fn__13499.invoke(core.clj:353) at territory_bro.infra.middleware$wrap_auto_refresh_projections$fn__39042.invoke(middleware.clj:73) at ring.middleware.reload$wrap_reload$fn__19509.invoke(reload.clj:39) at territory_bro.infra.middleware$wrap_sqlexception_chain$fn__39028.invoke(middleware.clj:37) at ring.middleware.http_response$wrap_http_response$fn__13550.invoke(http_response.clj:19) at territory_bro.infra.middleware$wrap_default_content_type$fn__39032.invoke(middleware.clj:50) at territory_bro.ui.error_page$wrap_error_pages$fn__39007.invoke(error_page.clj:38) at ring.logger$wrap_log_response$fn__16906.invoke(logger.clj:159) at ring.logger$wrap_log_request_params$fn__16887.invoke(logger.clj:64) at ring.logger$wrap_log_request_start$fn__16893.invoke(logger.clj:99) at ring.middleware.session$wrap_session$fn__17229.invoke(session.clj:112) at ring.middleware.keyword_params$wrap_keyword_params$fn__17350.invoke(keyword_params.clj:53) at ring.middleware.nested_params$wrap_nested_params$fn__17408.invoke(nested_params.clj:89) at ring.middleware.multipart_params$handle_request_and_errors$fn__17711$fn__17712.invoke(multipart_params.clj:136) at ring.middleware.multipart_params$handle_request_and_errors.invokeStatic(multipart_params.clj:134) at ring.middleware.multipart_params$handle_request_and_errors.invoke(multipart_params.clj:133) at ring.middleware.multipart_params$wrap_multipart_params$fn__17717.invoke(multipart_params.clj:188) at ring.middleware.params$wrap_params$fn__17754.invoke(params.clj:75) at ring.middleware.cookies$wrap_cookies$fn__17179.invoke(cookies.clj:192) at ring.middleware.resource$wrap_resource_prefer_resources$fn__17770.invoke(resource.clj:25) at ring.middleware.content_type$wrap_content_type$fn__17855.invoke(content_type.clj:34) at ring.middleware.default_charset$wrap_default_charset$fn__17879.invoke(default_charset.clj:31) at ring.middleware.not_modified$wrap_not_modified$fn__17836.invoke(not_modified.clj:61) at ring.middleware.x_headers$wrap_x_header$fn__16942.invoke(x_headers.clj:22) at ring.middleware.x_headers$wrap_x_header$fn__16942.invoke(x_headers.clj:22) at ring.middleware.ssl$wrap_forwarded_scheme$fn__17925.invoke(ssl.clj:35) at ring.middleware.proxy_headers$wrap_forwarded_remote_addr$fn__17963.invoke(proxy_headers.clj:20) at territory_bro.infra.middleware$wrap_internal_error$fn__39023.invoke(middleware.clj:24) at clojure.lang.Var.invoke(Var.java:384) at ring.adapter.jetty$proxy_handler$fn__1977.invoke(jetty.clj:107) at ring.adapter.jetty.proxy$org.eclipse.jetty.servlet.ServletHandler$ff19274a.doHandle(Unknown Source) at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:221) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1381) at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:176) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:484) at ring.adapter.jetty.proxy$org.eclipse.jetty.servlet.ServletHandler$ff19274a.doScope(Unknown Source) at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:174) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1303) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:129) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:122) at org.eclipse.jetty.server.Server.handle(Server.java:563) at org.eclipse.jetty.server.HttpChannel$RequestDispatchable.dispatch(HttpChannel.java:1598) at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:753) at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:501) at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:287) at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:314) at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100) at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:421) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:390) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:277) at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.run(AdaptiveExecutionStrategy.java:199) at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:411) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:969) at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1194) at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1149) at java.base/java.lang.Thread.run(Thread.java:840)
luontola · Jul 2, 2024 · ed9e3a1 · ed9e3a1
1 parent c83c13d
commit ed9e3a1
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 52 deletions.
diff --git a/src/territory_bro/api.clj b/src/territory_bro/api.clj
@@ -405,31 +405,31 @@
         (ok {})))))
 
 (defn share-territory-link [request]
-  (if (= "demo" (get-in request [:params :congregation]))
-    (let [cong-id (:demo-congregation config/env)
-          territory-id (UUID/fromString (get-in request [:params :territory]))
-          state (state-for-request request)
-          share-key (share/demo-share-key territory-id)
-          territory (facade/get-demo-territory state cong-id territory-id)]
-      (ok {:url (share/build-share-url share-key (:territory/number territory))
-           :key share-key}))
-
-    (auth/with-user-from-session request
-      (require-logged-in!)
-      (let [cong-id (UUID/fromString (get-in request [:params :congregation]))
+  (auth/with-user-from-session request
+    (if (= "demo" (get-in request [:params :congregation]))
+      (let [cong-id (:demo-congregation config/env)
             territory-id (UUID/fromString (get-in request [:params :territory]))
             state (state-for-request request)
-            share-key (share/generate-share-key)
+            share-key (share/demo-share-key territory-id)
             territory (facade/get-demo-territory state cong-id territory-id)]
-        (db/with-db [conn {}]
-          (dispatch! conn state {:command/type :share.command/create-share
-                                 :share/id (UUID/randomUUID)
-                                 :share/key share-key
-                                 :share/type :link
-                                 :congregation/id cong-id
-                                 :territory/id territory-id})
-          (ok {:url (share/build-share-url share-key (:territory/number territory))
-               :key share-key}))))))
+        (ok {:url (share/build-share-url share-key (:territory/number territory))
+             :key share-key}))
+      (do
+        (require-logged-in!)
+        (let [cong-id (UUID/fromString (get-in request [:params :congregation]))
+              territory-id (UUID/fromString (get-in request [:params :territory]))
+              state (state-for-request request)
+              share-key (share/generate-share-key)
+              territory (facade/get-demo-territory state cong-id territory-id)]
+          (db/with-db [conn {}]
+            (dispatch! conn state {:command/type :share.command/create-share
+                                   :share/id (UUID/randomUUID)
+                                   :share/key share-key
+                                   :share/type :link
+                                   :congregation/id cong-id
+                                   :territory/id territory-id})
+            (ok {:url (share/build-share-url share-key (:territory/number territory))
+                 :key share-key})))))))
 
 (defn- generate-qr-code! [conn request cong-id territory-id]
   (let [share-key (share/generate-share-key)
@@ -443,36 +443,36 @@
     share-key))
 
 (defn generate-qr-codes [request]
-  (if (= "demo" (get-in request [:params :congregation]))
-    (let [territory-ids (->> (get-in request [:params :territories])
-                             (mapv #(UUID/fromString %)))
-          shares (into [] (for [territory-id territory-ids]
-                            (let [share-key (share/demo-share-key territory-id)]
-                              {:territory territory-id
-                               :key share-key
-                               :url (str (:qr-code-base-url config/env) "/" share-key)})))]
-      (ok {:qrCodes shares}))
-
-    (auth/with-user-from-session request
-      (require-logged-in!)
-      (let [cong-id (UUID/fromString (get-in request [:params :congregation]))
-            territory-ids (->> (get-in request [:params :territories])
-                               (mapv #(UUID/fromString %)))]
-        (db/with-db [conn {}]
-          (let [share-cache (or (-> request :session ::share-cache)
-                                {})
-                shares (into [] (for [territory-id territory-ids]
-                                  (let [share-key (or (get share-cache territory-id)
-                                                      (generate-qr-code! conn request cong-id territory-id))]
-                                    {:territory territory-id
-                                     :key share-key
-                                     :url (str (:qr-code-base-url config/env) "/" share-key)})))
-                share-cache (->> shares
-                                 (map (juxt :territory :key))
-                                 (into share-cache))]
-            (-> (ok {:qrCodes shares})
-                (assoc :session (-> (:session request)
-                                    (assoc ::share-cache share-cache))))))))))
+  (auth/with-user-from-session request
+    (if (= "demo" (get-in request [:params :congregation]))
+      (let [territory-ids (->> (get-in request [:params :territories])
+                               (mapv #(UUID/fromString %)))
+            shares (into [] (for [territory-id territory-ids]
+                              (let [share-key (share/demo-share-key territory-id)]
+                                {:territory territory-id
+                                 :key share-key
+                                 :url (str (:qr-code-base-url config/env) "/" share-key)})))]
+        (ok {:qrCodes shares}))
+      (do
+        (require-logged-in!)
+        (let [cong-id (UUID/fromString (get-in request [:params :congregation]))
+              territory-ids (->> (get-in request [:params :territories])
+                                 (mapv #(UUID/fromString %)))]
+          (db/with-db [conn {}]
+            (let [share-cache (or (-> request :session ::share-cache)
+                                  {})
+                  shares (into [] (for [territory-id territory-ids]
+                                    (let [share-key (or (get share-cache territory-id)
+                                                        (generate-qr-code! conn request cong-id territory-id))]
+                                      {:territory territory-id
+                                       :key share-key
+                                       :url (str (:qr-code-base-url config/env) "/" share-key)})))
+                  share-cache (->> shares
+                                   (map (juxt :territory :key))
+                                   (into share-cache))]
+              (-> (ok {:qrCodes shares})
+                  (assoc :session (-> (:session request)
+                                      (assoc ::share-cache share-cache)))))))))))
 
 (defn- refresh-projections! []
   (projections/refresh-async!)

diff --git a/test/territory_bro/api_test.clj b/test/territory_bro/api_test.clj
@@ -971,6 +971,36 @@
         (let [state-after (projections/cached-state)]
           (is (= state-before state-after)))))))
 
+(deftest share-demo-territory-link-bugfix-test
+  (let [*session (atom (login! app))
+        *session-anonymous (atom nil)
+        cong-id (create-congregation! @*session "Congregation")
+        territory-id (create-territory! cong-id)
+        share-key (-> (request :post (str "/api/congregation/" cong-id "/territory/" territory-id "/share"))
+                      (json-body {})
+                      (merge @*session)
+                      app
+                      :body
+                      :key)]
+    (assert (some? share-key))
+
+    (testing "given: anonymous user has opened a normal share"
+      (let [response (-> (request :get (str "/api/share/" share-key))
+                         app)]
+        (is (ok? response))
+        (reset! *session-anonymous (get-session response))))
+
+    (testing "when: anonymous user tries to create a demo share"
+      ;; This crashed because share-territory-link didn't call with-user-from-session for demo shares,
+      ;; and state-for-request depends on current-user-id which requires auth/*user* to be set.
+      ;; A more fundamental fix would be to always initialize auth/*user* in a middleware, after which
+      ;; this special case wouldn't exist anymore.
+      (let [response (-> (request :post (str "/api/congregation/demo/territory/" territory-id "/share"))
+                         (json-body {})
+                         (merge @*session-anonymous)
+                         app)]
+        (is (ok? response))))))
+
 (deftest create-qr-code-test
   (let [*session (atom (login! app))
         cong-id (create-congregation! @*session "Congregation")