🐛 Fix nitrate sso failure message (#11214)

backend/src/app/auth/oidc.clj

@@ -42,31 +42,52 @@
 ;; OIDC PROVIDER (GENERIC)
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(defn- raise-invalid-sso-config
+  "Raise a controlled validation error for OIDC provider configuration failures."
+  [& {:keys [hint cause] :as params}]
+  (throw (ex-info (or hint "invalid-sso-config")
+                  (-> params
+                      (dissoc :cause)
+                      (assoc :type :validation
+                             :code :invalid-sso-config))
+                  cause)))
+
 (defn- discover-oidc-config
   [cfg {:keys [base-uri skip-ssrf-check?] :as provider}]
-  (let [uri (u/join base-uri ".well-known/openid-configuration")
+  (let [uri (u/join base-uri ".well-known/openid-configuration")]
-        rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})]
+    (try
+      (let [rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})]
+        (if (= 200 (:status rsp))
+          (let [data       (-> rsp :body json/decode)
+                token-uri  (get data :token_endpoint)
+                auth-uri   (get data :authorization_endpoint)
+                user-uri   (get data :userinfo_endpoint)
+                jwks-uri   (get data :jwks_uri)
+                logout-uri (get data :end_session_endpoint)]
 
-    (if (= 200 (:status rsp))
+            (-> provider
-      (let [data       (-> rsp :body json/decode)
+                (assoc :token-uri token-uri)
-            token-uri  (get data :token_endpoint)
+                (assoc :auth-uri  auth-uri)
-            auth-uri   (get data :authorization_endpoint)
+                (assoc :user-uri  user-uri)
-            user-uri   (get data :userinfo_endpoint)
+                (assoc :jwks-uri jwks-uri)
-            jwks-uri   (get data :jwks_uri)
+                (assoc :logout-uri logout-uri)))
-            logout-uri (get data :end_session_endpoint)]
 
-        (-> provider
+          (raise-invalid-sso-config
-            (assoc :token-uri token-uri)
+           :hint "unable to discover OIDC configuration"
-            (assoc :auth-uri  auth-uri)
+           :discover-uri uri
-            (assoc :user-uri  user-uri)
+           :response-status-code (:status rsp))))
-            (assoc :jwks-uri jwks-uri)
+      (catch Throwable cause
-            (assoc :logout-uri logout-uri)))
+        ;; Controlled raises above are ExceptionInfo and would otherwise be
-
+        ;; re-wrapped by this catch, dropping fields like :response-status-code.
-      (ex/raise :type ::internal
+        (if (and (ex/error? cause)
-                :code :invalid-sso-config
+                 (= :invalid-sso-config (:code (ex-data cause))))
-                :hint "unable to discover OIDC configuration"
+          (throw cause)
-                :discover-uri uri
+          ;; Wrap SSRF blocks, DNS failures, TLS errors, etc. — from the caller's
-                :response-status-code (:status rsp)))))
+          ;; perspective these are all "bad/unreachable issuer URL".
+          (raise-invalid-sso-config
+           :hint "unable to discover OIDC configuration"
+           :discover-uri uri
+           :cause cause))))))
 
 (def ^:private default-oidc-scopes
   #{"openid" "profile" "email"})
@@ -107,16 +128,29 @@
 
 (defn- fetch-oidc-jwks
   [cfg jwks-uri {:keys [skip-ssrf-check?]}]
-  (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})]
+  (try
-    (if (= 200 status)
+    (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})]
-      (-> body json/decode :keys process-oidc-jwks)
+      (if (= 200 status)
-      (ex/raise :type ::internal
+        (-> body json/decode :keys process-oidc-jwks)
-                :code :unable-to-fetch-sso-jwks
+        (raise-invalid-sso-config
-                :hint "unable to retrieve JWKs (unexpected response status code)"
+         :hint "unable to retrieve JWKs (unexpected response status code)"
-                :response-status-code status))))
+         :jwks-uri jwks-uri
+         :response-status-code status)))
+    (catch Throwable cause
+      (if (and (ex/error? cause)
+               (= :invalid-sso-config (:code (ex-data cause))))
+        (throw cause)
+        (raise-invalid-sso-config
+         :hint "unable to retrieve JWKs"
+         :jwks-uri jwks-uri
+         :cause cause)))))
 
 (defn- populate-jwks
-  "Fetch and Add (if possible) JWK's to the OIDC provider"
+  "Fetch and add JWKs to the OIDC provider.
+
+  When `:strict-jwks?` is set (organization SSO), failures raise a controlled
+  validation error. Otherwise JWKS is best-effort: log and continue without keys
+  so global OIDC/GitLab providers can still initialize if JWKS is temporarily down."
   [cfg provider]
   (try
     (if-let [jwks (when-let [jwks-uri (:jwks-uri provider)]
@@ -124,20 +158,28 @@
       (assoc provider :jwks jwks)
       provider)
     (catch Throwable cause
-      (l/warn :hint "unable to fetch JWKs for the OIDC provider"
+      (if (:strict-jwks? provider)
-              :provider (str (:id provider))
+        (if (and (ex/error? cause)
-              :cause cause)
+                 (= :invalid-sso-config (:code (ex-data cause))))
-      provider)))
+          (throw cause)
+          (raise-invalid-sso-config
+           :hint "unable to retrieve JWKs"
+           :provider (:id provider)
+           :cause cause))
+        (do
+          (l/warn :hint "unable to fetch JWKs for the OIDC provider"
+                  :provider (str (:id provider))
+                  :cause cause)
+          provider)))))
 
 (defn- prepare-oidc-provider
   [cfg params]
   (when-not (and (string? (:base-uri params))
                  (string? (:client-id params))
                  (string? (:client-secret params)))
-    (ex/raise :type ::internal
+    (raise-invalid-sso-config
-              :code :invalid-sso-config
+     :hint "missing params for provider initialization"
-              :hint "missing params for provider initialization"
+     :provider (:id params)))
-              :provider (:id params)))
 
   (try
     (if (and (string? (:token-uri params))
@@ -150,11 +192,13 @@
         (with-meta provider {::discovered true})))
 
     (catch Throwable cause
-      (ex/raise :type ::internal
+      (if (and (ex/error? cause)
-                :type :invalid-sso-config
+               (= :invalid-sso-config (:code (ex-data cause))))
-                :hint "unexpected exception on configuring provider"
+        (throw cause)
-                :provider (:id params)
+        (raise-invalid-sso-config
-                :cause cause))))
+         :hint "unexpected exception on configuring provider"
+         :provider (:id params)
+         :cause cause)))))
 
 (defmethod ig/assert-key ::providers/generic
   [_ params]
@@ -322,10 +366,9 @@
   [cfg params]
   (when-not (and (string? (:client-id params))
                  (string? (:client-secret params)))
-    (ex/raise :type ::internal
+    (raise-invalid-sso-config
-              :code :invalid-sso-config
+     :hint "missing params for provider initialization"
-              :hint "missing params for provider initialization"
+     :provider (:id params)))
-              :provider (:id params)))
 
   (try
     (let [provider (populate-jwks cfg params)]
@@ -336,11 +379,13 @@
              :client-secret (d/obfuscate-string (:client-secret provider)))
       provider)
     (catch Throwable cause
-      (ex/raise :type ::internal
+      (if (and (ex/error? cause)
-                :type :invalid-sso-config
+               (= :invalid-sso-config (:code (ex-data cause))))
-                :hint "unexpected exception on configuring provider"
+        (throw cause)
-                :provider (:id params)
+        (raise-invalid-sso-config
-                :cause cause))))
+         :hint "unexpected exception on configuring provider"
+         :provider (:id params)
+         :cause cause)))))
 
 (defmethod ig/init-key ::providers/gitlab
   [_ cfg]
@@ -867,7 +912,10 @@
                           :base-uri         (some-> (non-blank-uri issuer)
                                                     (str/rtrim "/")
                                                     (str "/"))
-                          :scopes           default-oidc-scopes}))
+                          :scopes           default-oidc-scopes
+                          ;; Organization SSO is configured by customers; discovery
+                          ;; and JWKS failures must surface as controlled errors.
+                          :strict-jwks?     true}))
 
 (defn build-organization-sso-auth-redirect-uri
   "Build the OIDC authorization redirect URI for an organization SSO config.
@@ -877,16 +925,24 @@
         issuer          (organization-sso-discovery-uri sso)
         dest-url        (or dest-url (str (cf/get :public-uri)))]
     (when-not issuer
-      (ex/raise :type :validation
+      (raise-invalid-sso-config
-                :code :invalid-sso-config
+       :hint "missing issuer"
-                :hint "missing issuer"))
+       :organization-id organization-id))
-    (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso))
+    (try
-          state-token   (tokens/generate cfg {:iss             "oidc"
+      (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso))
-                                              :dest-url        dest-url
+            state-token   (tokens/generate cfg {:iss             "oidc"
-                                              :organization-id organization-id
+                                                :dest-url        dest-url
-                                              :issuer          issuer
+                                                :organization-id organization-id
-                                              :exp             (ct/in-future "4h")})]
+                                                :issuer          issuer
-      (build-auth-redirect-uri oidc-provider state-token))))
+                                                :exp             (ct/in-future "4h")})]
+        (build-auth-redirect-uri oidc-provider state-token))
+      (catch Throwable cause
+        (if (and (ex/error? cause)
+                 (= :invalid-sso-config (:code (ex-data cause))))
+          (throw (ex-info (ex-message cause)
+                          (assoc (ex-data cause) :organization-id organization-id)
+                          (ex-cause cause)))
+          (throw cause))))))
 
 (def ^:private probe-auth-code "penpot-sso-config-probe")
 

backend/test/backend_tests/auth_oidc_test.clj

@@ -15,6 +15,7 @@
    [app.setup :as-alias setup]
    [app.tokens :as tokens]
    [clojure.test :as t]
+   [cuerdas.core :as str]
    [mockery.core :refer [with-mocks]]
    [yetti.response :as-alias yres]))
 
@@ -587,3 +588,138 @@
                                                    :issuer "https://idp.example.com"})
         (t/is (not (true? (:skip-ssrf-check? @captured-params)))
               "SSRF protection must be disabled for organization SSO")))))
+
+(defn- ssl-handshake-failure
+  []
+  (javax.net.ssl.SSLHandshakeException. "Remote host terminated the handshake"))
+
+(t/deftest prepare-organization-sso-provider-raises-on-discovery-network-failure
+  (t/testing "SSL/network failures during OIDC discovery become controlled validation errors"
+    (with-mocks [http-mock {:target 'app.http.client/req
+                            :side-effect (fn [& _] (throw (ssl-handshake-failure)))}]
+      (let [e (try
+                (#'oidc/prepare-organization-sso-provider
+                 {}
+                 {:client-id "test-client"
+                  :client-secret "test-secret"
+                  :issuer "https://wrong-idp.example.com"})
+                (catch Throwable t t))]
+        (t/is (ex/error? e))
+        (t/is (= :validation (:type (ex-data e))))
+        (t/is (= :invalid-sso-config (:code (ex-data e))))))))
+
+(t/deftest prepare-organization-sso-provider-raises-on-discovery-non-200
+  (t/testing "non-200 OIDC discovery responses become controlled validation errors"
+    (with-mocks [http-mock {:target 'app.http.client/req
+                            :return {:status 404 :body "not found"}}]
+      (let [e (try
+                (#'oidc/prepare-organization-sso-provider
+                 {}
+                 {:client-id "test-client"
+                  :client-secret "test-secret"
+                  :issuer "https://idp.example.com"})
+                (catch Throwable t t))
+            data (ex-data e)]
+        (t/is (ex/error? e))
+        (t/is (= :validation (:type data)))
+        (t/is (= :invalid-sso-config (:code data)))
+        (t/is (= 404 (:response-status-code data)))
+        (t/is (= "unable to discover OIDC configuration" (ex-message e)))
+        (t/is (str/includes? (str (:discover-uri data)) "openid-configuration"))))))
+
+(t/deftest prepare-organization-sso-provider-raises-on-ssrf-blocked-issuer
+  (t/testing "SSRF/DNS failures for the issuer URL become invalid-sso-config, not ssrf-blocked-target"
+    (with-mocks [http-mock {:target 'app.http.client/req
+                            :side-effect (fn [& _]
+                                           (ex/raise :type :validation
+                                                     :code :ssrf-blocked-target
+                                                     :hint "uri host could not be resolved"))}]
+      (let [e (try
+                (#'oidc/prepare-organization-sso-provider
+                 {}
+                 {:client-id "test-client"
+                  :client-secret "test-secret"
+                  :issuer "https://unresolvable.invalid"})
+                (catch Throwable t t))]
+        (t/is (ex/error? e))
+        (t/is (= :validation (:type (ex-data e))))
+        (t/is (= :invalid-sso-config (:code (ex-data e))))
+        (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e)))))))))
+
+(t/deftest prepare-organization-sso-provider-raises-on-jwks-network-failure
+  (t/testing "SSL/network failures while fetching JWKs become controlled validation errors"
+    (let [discovery-body (str "{\"authorization_endpoint\":\"https://idp.example.com/auth\","
+                              "\"token_endpoint\":\"https://idp.example.com/token\","
+                              "\"userinfo_endpoint\":\"https://idp.example.com/userinfo\","
+                              "\"jwks_uri\":\"https://idp.example.com/jwks\"}")]
+      (with-mocks [http-mock {:target 'app.http.client/req
+                              :side-effect (fn [_cfg request & _]
+                                             (if (str/includes? (str (:uri request)) "openid-configuration")
+                                               {:status 200 :body discovery-body}
+                                               (throw (ssl-handshake-failure))))}]
+        (let [e (try
+                  (#'oidc/prepare-organization-sso-provider
+                   {}
+                   {:client-id "test-client"
+                    :client-secret "test-secret"
+                    :issuer "https://idp.example.com"})
+                  (catch Throwable t t))]
+          (t/is (ex/error? e))
+          (t/is (= :validation (:type (ex-data e))))
+          (t/is (= :invalid-sso-config (:code (ex-data e)))))))))
+
+(t/deftest populate-jwks-strict-wraps-non-invalid-sso-config-errors
+  (t/testing "strict JWKS path wraps unrelated structured errors instead of rethrowing them"
+    (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks
+                             :side-effect (fn [& _]
+                                            (ex/raise :type :validation
+                                                      :code :ssrf-blocked-target
+                                                      :hint "uri host could not be resolved"))}]
+      (let [e (try
+                (#'oidc/populate-jwks
+                 {}
+                 {:id "oidc"
+                  :jwks-uri "https://idp.example.com/jwks"
+                  :strict-jwks? true})
+                (catch Throwable t t))]
+        (t/is (ex/error? e))
+        (t/is (= :validation (:type (ex-data e))))
+        (t/is (= :invalid-sso-config (:code (ex-data e))))
+        (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e)))))))))
+
+(t/deftest populate-jwks-strict-rethrows-invalid-sso-config
+  (t/testing "strict JWKS path rethrows an already-controlled invalid-sso-config"
+    (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks
+                             :side-effect (fn [& _]
+                                            (ex/raise :type :validation
+                                                      :code :invalid-sso-config
+                                                      :hint "unable to retrieve JWKs"
+                                                      :jwks-uri "https://idp.example.com/jwks"))}]
+      (let [e (try
+                (#'oidc/populate-jwks
+                 {}
+                 {:id "oidc"
+                  :jwks-uri "https://idp.example.com/jwks"
+                  :strict-jwks? true})
+                (catch Throwable t t))]
+        (t/is (ex/error? e))
+        (t/is (= :invalid-sso-config (:code (ex-data e))))
+        (t/is (= "unable to retrieve JWKs" (ex-message e)))
+        (t/is (= "https://idp.example.com/jwks" (:jwks-uri (ex-data e))))))))
+
+(t/deftest build-organization-sso-auth-redirect-uri-raises-on-unreachable-provider
+  (t/testing "check-nitrate-sso path surfaces a controlled error when the issuer is unreachable"
+    (with-mocks [http-mock {:target 'app.http.client/req
+                            :side-effect (fn [& _] (throw (ssl-handshake-failure)))}]
+      (let [e (try
+                (oidc/build-organization-sso-auth-redirect-uri
+                 {}
+                 {:client-id "test-client"
+                  :client-secret "test-secret"
+                  :issuer "https://wrong-idp.example.com"}
+                 :dest-url "https://localhost:3449/#/dashboard"
+                 :organization-id #uuid "00000000-0000-0000-0000-000000000001")
+                (catch Throwable t t))]
+        (t/is (ex/error? e))
+        (t/is (= :validation (:type (ex-data e))))
+        (t/is (= :invalid-sso-config (:code (ex-data e))))))))

frontend/src/app/main/errors.cljs

@@ -389,6 +389,12 @@
                   :level :error
                   :timeout 3000})))
 
+    (= code :invalid-sso-config)
+    ;; SSO error page needs :organization-id to retry
+    (if (:organization-id error)
+      (st/async-emit! (rt/assign-exception (assoc error :type :sso-error)))
+      (st/async-emit! (rt/assign-exception error)))
+
     :else
     (st/async-emit! (rt/assign-exception error))))
 

frontend/test/frontend_tests/main_errors_test.cljs

@@ -12,7 +12,8 @@
     - exception->error-data       – pure transformer
     - on-error re-entrancy guard  – prevents recursive invocations
     - flash schedules async emit  – ntf/show is not emitted synchronously
-    - organization SSO recovery   – expired SSO sessions go back to the provider"
+    - organization SSO recovery   – expired SSO sessions go back to the provider
+    - invalid-sso-config handler  – requires :organization-id to promote to :sso-error"
   (:require
    [app.main.errors :as errors]
    [app.main.repo :as rp]
@@ -351,3 +352,47 @@
              (t/is (nil? @assigned*))
              (done'))))
         done))))
+
+;; ---------------------------------------------------------------------------
+;; :validation / :invalid-sso-config
+;;
+;; The SSO error page needs an organization-id to retry meaningfully. Promote
+;; to :sso-error only when that id is present; otherwise keep :validation so
+;; we do not surface a broken SSO dialog for a future code path that omits it.
+;; ---------------------------------------------------------------------------
+
+(defn- capture-async-exception
+  "Invoke `ptk/handle-error` while capturing the error map passed to
+  `rt/assign-exception` via `st/async-emit!`.
+
+  `st/async-emit!` is variadic (`[& params]`); the mock must be too,
+  otherwise CLJS looks up `IFn$_invoke$arity$variadic` and throws."
+  [error]
+  (let [captured (atom nil)]
+    (with-redefs [st/async-emit!      (fn [& events]
+                                        (reset! captured (first events)))
+                  rt/assign-exception (fn [err] err)]
+      (ptk/handle-error error)
+      @captured)))
+
+(t/deftest invalid-sso-config-with-organization-id-promotes-to-sso-error
+  (t/testing "invalid-sso-config with :organization-id is shown as :sso-error"
+    (let [org-id #uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
+          assigned (capture-async-exception
+                    {:type :validation
+                     :code :invalid-sso-config
+                     :organization-id org-id
+                     :hint "missing issuer"})]
+      (t/is (= :sso-error (:type assigned)))
+      (t/is (= org-id (:organization-id assigned)))
+      (t/is (= :invalid-sso-config (:code assigned))))))
+
+(t/deftest invalid-sso-config-without-organization-id-keeps-validation
+  (t/testing "invalid-sso-config without :organization-id must not become :sso-error"
+    (let [assigned (capture-async-exception
+                    {:type :validation
+                     :code :invalid-sso-config
+                     :hint "missing issuer"})]
+      (t/is (= :validation (:type assigned)))
+      (t/is (nil? (:organization-id assigned)))
+      (t/is (= :invalid-sso-config (:code assigned))))))