Often SLO won't be setup on the IdP but we should still delete the se…

…ssion. (#40459)

* Often slo won't be setup, but we should still delete the session

* test that we delete the session even when "/handle_slo" is not called

* cleanup test

* take the cookie from the session, not as a parameter

enterprise/backend/src/metabase_enterprise/sso/api/sso.clj

@@ -10,10 +10,12 @@
    [metabase-enterprise.sso.integrations.saml]
    [metabase-enterprise.sso.integrations.sso-settings :as sso-settings]
    [metabase.api.common :as api]
+   [metabase.server.middleware.session :as mw.session]
    [metabase.util :as u]
    [metabase.util.i18n :refer [trs]]
    [metabase.util.log :as log]
    [metabase.util.malli :as mu]
+   [metabase.util.malli.schema :as ms]
    [metabase.util.urls :as urls]
    [saml20-clj.core :as saml]
    [saml20-clj.encode-decode :as encode-decode]
@@ -68,22 +70,27 @@
 ;; POST /auth/sso/logout
 (api/defendpoint POST "/logout"
   "Logout."
-  [:as {:keys [metabase-session-id]}]
-  (api/check-exists? :model/Session metabase-session-id)
-  (let [{:keys [email sso_source]}
-        (t2/query-one {:select [:u.email :u.sso_source]
-                       :from   [[:core_user :u]]
-                       :join   [[:core_session :session] [:= :u.id :session.user_id]]
-                       :where  [:= :session.id metabase-session-id]})]
-    {:saml-logout-url
-     (when (and (sso-settings/saml-enabled)
-                (= sso_source "saml"))
-       (saml/logout-redirect-location
-        :idp-url (sso-settings/saml-identity-provider-uri)
-        :issuer (sso-settings/saml-application-name)
-        :user-email email
-        :relay-state (encode-decode/str->base64
-                      (str (urls/site-url) metabase-slo-redirect-url))))}))
+  [:as {cookies :cookies}]
+  {cookies [:map [mw.session/metabase-session-cookie [:map [:value ms/NonBlankString]]]]}
+  (let [metabase-session-id (get-in cookies [mw.session/metabase-session-cookie :value])]
+    (api/check-exists? :model/Session metabase-session-id)
+    (let [{:keys [email sso_source]}
+          (t2/query-one {:select [:u.email :u.sso_source]
+                         :from   [[:core_user :u]]
+                         :join   [[:core_session :session] [:= :u.id :session.user_id]]
+                         :where  [:= :session.id metabase-session-id]})]
+      ;; If a user doesn't have SLO setup on their IdP,
+      ;; they will never hit "/handle_slo" so we must delete the session here:
+      (t2/delete! :model/Session :id metabase-session-id)
+      {:saml-logout-url
+       (when (and (sso-settings/saml-enabled)
+                  (= sso_source "saml"))
+         (saml/logout-redirect-location
+          :idp-url (sso-settings/saml-identity-provider-uri)
+          :issuer (sso-settings/saml-application-name)
+          :user-email email
+          :relay-state (encode-decode/str->base64
+                        (str (urls/site-url) metabase-slo-redirect-url))))})))
 
 ;; POST /auth/sso/handle_slo
 (api/defendpoint POST "/handle_slo"

enterprise/backend/test/metabase_enterprise/sso/integrations/saml_test.clj

@@ -686,3 +686,16 @@
                 "After a successful log-out, you don't have a session")
             (is (not (t2/exists? :model/Session :id session-id))
                 "After a successful log-out, the session is deleted")))))))
+
+(deftest logout-should-delete-session-when-idp-slo-conf-missing-test
+  (testing "Missing SAML SLO config logouts should still delete the user's session."
+    (let [session-id (str (random-uuid))]
+      (mt/with-temp [:model/User user {:email "saml_test@metabase.com" :sso_source "saml"}
+                     :model/Session _ {:user_id (:id user) :id session-id}]
+        (is (t2/exists? :model/Session :id session-id))
+        (let [req-options (-> (saml-post-request-options
+                               (saml-test-response)
+                               (saml/str->base64 default-redirect-uri))
+                              (assoc-in [:request-options :cookies mw.session/metabase-session-cookie :value] session-id))]
+          (client :post "/auth/sso/logout" req-options)
+          (is (not (t2/exists? :model/Session :id session-id))))))))