🐛 Fix SSO failure logging user out instead of showing error page (#11129)

* 🐛 Fix SSO failure logging user out instead of showing error page

* 📎 Code review

backend/src/app/auth/oidc.clj

@@ -650,6 +650,13 @@
                     (assoc :query (u/map->query-string params)))]
      (redirect-response uri))))
 
+(defn- redirect-with-organization-sso-error
+  [{:keys [dest-url organization-id]}]
+  (-> (str (or dest-url (cf/get :public-uri)))
+      (u/append-query-param :sso-error true)
+      (u/append-query-param :organization-id organization-id)
+      (redirect-response)))
+
 (defn- redirect-to-register
   [cfg info provider]
   (let [info   (assoc info
@@ -887,6 +894,39 @@
     {::yres/status 200
      ::yres/body {:redirect-uri uri}}))
 
+(defn- organization-sso-callback-handler
+  "Handle the organization-SSO branch of the OIDC callback: state carries
+  :dest-url — exchange the authorization code with the OIDC provider to
+  verify authentication actually occurred, then redirect back to dest-url."
+  [cfg request state code]
+  (let [dest-url (:dest-url state)]
+    (try
+      (let [organization-id (:organization-id state)
+            sso             (nitrate/call cfg :get-organization-sso {:organization-id organization-id})
+            provider        (prepare-organization-sso-provider cfg sso)
+            info            (get-info cfg provider state code)
+            session         (session/get-session request)
+            exp             (or (:sso-token-exp info) (ct/in-future {:hours 48}))]
+        (when (and session organization-id)
+          (let [props (-> (or (:props session) {})
+                          (update :sso assoc organization-id exp))]
+            (session/update-session (::session/manager cfg) (assoc session :props props))))
+        (redirect-response dest-url))
+      (catch Throwable cause
+        (let [{:keys [code]} (ex-data cause)]
+          (binding [l/*context* (errors/request->context request)]
+            (if (some? code)
+              (l/warn :hint "organization sso callback failed"
+                      :code code
+                      :message (ex-message cause)
+                      :organization-id (:organization-id state))
+              (l/err :hint "unexpected error on organization sso callback"
+                     :organization-id (:organization-id state)
+                     :cause cause))))
+        (redirect-with-organization-sso-error
+         {:dest-url dest-url
+          :organization-id (:organization-id state)})))))
+
 (defn- callback-handler
   [cfg {:keys [params] :as request}]
   (if-let [error (get params :error)]
@@ -898,18 +938,8 @@
 
         ;; Organization SSO flow: state carries :dest-url — exchange the authorization
         ;; code with the OIDC provider to verify authentication actually occurred.
-        (if-let [dest-url (:dest-url state)]
-          (let [organization-id (:organization-id state)
-                sso             (nitrate/call cfg :get-organization-sso {:organization-id organization-id})
-                provider        (prepare-organization-sso-provider cfg sso)
-                info            (get-info cfg provider state code)
-                session         (session/get-session request)
-                exp             (or (:sso-token-exp info) (ct/in-future {:hours 48}))]
-            (when (and session organization-id)
-              (let [props (-> (or (:props session) {})
-                              (update :sso assoc organization-id exp))]
-                (session/update-session (::session/manager cfg) (assoc session :props props))))
-            (redirect-response dest-url))
+        (if (:dest-url state)
+          (organization-sso-callback-handler cfg request state code)
 
           (let [provider (resolve-provider cfg state)
                 info     (get-info cfg provider state code)

common/src/app/common/uri.cljc

@@ -67,6 +67,36 @@
               path
               (str path "/")))))
 
+(defn- update-query-params
+  "Apply `f` to the query-params map of `url`, returning the updated URL string.
+  Handles both plain query strings and fragment-based (hash) URLs."
+  [url f]
+  (let [transform (fn [parsed]
+                    (update parsed :query
+                            (fn [q]
+                              (-> (query-string->map (or q ""))
+                                  f
+                                  map->query-string))))
+        parsed    (uri url)
+        fragment  (:fragment parsed)]
+    (if (str/blank? fragment)
+      (str (transform parsed))
+      (-> parsed
+          (assoc :fragment (str (transform (parse fragment))))
+          str))))
+
+(defn append-query-param
+  "Return a new URL string with the given query parameter added or replaced.
+  Handles both plain query strings and fragment-based (hash) URLs."
+  [url key value]
+  (update-query-params url #(assoc % key value)))
+
+(defn remove-query-param
+  "Return a new URL string with the given query parameter removed.
+  Handles both plain query strings and fragment-based (hash) URLs."
+  [url key]
+  (update-query-params url #(dissoc % key)))
+
 #?(:clj
    (defmethod print-method lambdaisland.uri.URI [^URI this ^java.io.Writer writer]
      (.write writer "#")

frontend/src/app/main/data/nitrate.cljs

@@ -1,5 +1,6 @@
 (ns app.main.data.nitrate
   (:require
+   [app.common.data :as d]
    [app.common.data.macros :as dm]
    [app.common.types.organization :as cto]
    [app.common.uri :as u]
@@ -351,6 +352,24 @@
                      (rx/empty)))))))))))
 
 
+(defn retry-organization-sso
+  "Retries the organization SSO login flow after a failed attempt, reusing
+  the same check-nitrate-sso RPC used elsewhere to move the user through
+  the organization's identity provider. Passing `team-id` enables the
+  backend's non-member short-circuit. Falls back to navigating straight
+  to `dest-url` when no fresh SSO redirect is needed or available."
+  [{:keys [team-id organization-id dest-url]}]
+  (ptk/reify ::retry-organization-sso
+    ptk/WatchEvent
+    (watch [_ _ _]
+      (->> (rp/cmd! :check-nitrate-sso (d/without-nils {:team-id team-id
+                                                        :organization-id organization-id
+                                                        :url dest-url}))
+           (rx/map (fn [{:keys [redirect-uri]}]
+                     (rt/nav-raw :uri (or redirect-uri dest-url))))
+           (rx/catch (fn [_]
+                       (rx/of (rt/nav-raw :uri dest-url))))))))
+
 (defn- fetch-organizations-allowed
   "Returns an rx observable of an `organizations-allowed` map (organization-id -> boolean).
    Organizations where :add-anybody-to-team is permitted are pre-approved;

frontend/src/app/main/ui/routes.cljs

@@ -123,6 +123,29 @@
               (errors/on-error cause))))
       (st/emit! (rt/navigated match send-event-info?)))))
 
+(defn- handle-sso-error-and-navigate
+  "Check if the current route has an SSO error marker. If so, assign an
+  exception with type :sso-error and organization-id from query params,
+  and deliberately do NOT proceed with normal navigation: emitting
+  `rt/navigated` would clear the exception that was just assigned.
+  Otherwise, delegate to `check-sso-and-navigate`."
+  [match send-event-info? url]
+  (let [route-name      (name (get-in match [:data :name]))
+        sso-error?      (some? (get-in match [:query-params :sso-error]))
+        organization-id (some-> (get-in match [:query-params :organization-id]) uuid/parse*)
+        team-id-str     (or (get-in match [:query-params :team-id])
+                            (get-in match [:params :path :team-id])) ;; Fallback: team-id may be in path params for workspace routes
+        team-id         (some-> team-id-str uuid/parse*)
+        is-workspace?   (str/starts-with? route-name "workspace")
+        is-dashboard?   (str/starts-with? route-name "dashboard")]
+    (if sso-error?
+      (st/emit! (rt/assign-exception {:type :sso-error
+                                      :organization-id organization-id
+                                      :team-id team-id
+                                      :is-workspace is-workspace?
+                                      :is-dashboard is-dashboard?}))
+      (check-sso-and-navigate match send-event-info? url))))
+
 (defn on-navigate
   [router path send-event-info?]
   (let [location        (.-location js/document)
@@ -138,7 +161,7 @@
       (st/emit! (rt/assign-exception {:type :not-found}))
 
       (some? match)
-      (check-sso-and-navigate match send-event-info? (rt/get-current-href))
+      (handle-sso-error-and-navigate match send-event-info? (rt/get-current-href))
 
       :else
       ;; We just recheck with an additional profile request; this

frontend/src/app/main/ui/static.cljs

@@ -13,6 +13,7 @@
    [app.common.uuid :as uuid]
    [app.main.data.auth :refer [is-authenticated?]]
    [app.main.data.common :as dcm]
+   [app.main.data.nitrate :as dnt]
    [app.main.errors :as errors]
    [app.main.refs :as refs]
    [app.main.repo :as rp]
@@ -433,43 +434,6 @@
                        (rx/of default)
                        (rx/throw cause)))))))
 
-(mf/defc exception-section*
-  {::mf/private true}
-  [{:keys [data] :as props}]
-  (let [type   (get data :type)
-        cause  (get data ::errors/instance)
-
-        report (mf/with-memo [cause]
-                 (when (ex/exception? cause)
-                   (errors/generate-report cause)))
-
-        props  (mf/spread-props props {:report report})]
-
-    (mf/with-effect [report type cause]
-      (when (and (ex/exception? cause)
-                 (not (contains? #{:not-found :authentication} type)))
-        (errors/submit-report :event-name "exception-page"
-                              :report report
-                              :hint (ex/get-hint cause))))
-
-    (case type
-      :not-found
-      [:> not-found* {}]
-
-      :authentication
-      [:> not-found* {}]
-
-      :bad-gateway
-      [:> bad-gateway* props]
-
-      :service-unavailable
-      [:> service-unavailable*]
-
-      :nitrate-unavailable
-      [:> nitrate-unavailable*]
-
-      [:> internal-error* props])))
-
 (mf/defc context-wrapper*
   [{:keys [is-workspace is-dashboard is-viewer profile children]}]
   [:*
@@ -515,6 +479,99 @@
 
    children])
 
+(mf/defc sso-error-section*
+  "Shown in place of the dashboard/workspace (same static skeleton and
+  `request-dialog*` used by the no-permission dialogs) when the organization
+  SSO exchange with the identity provider fails."
+  {::mf/private true}
+  [{:keys [organization-id team-id profile is-workspace is-dashboard]}]
+  (let [clean-url
+        (mf/with-memo []
+          (-> (rt/get-current-href)
+              (dom/remove-query-param :sso-error)
+              (dom/remove-query-param :organization-id)))
+
+        _ (mf/with-effect []
+            ;; Consume the marker once: scrub it from the URL bar so a
+            ;; browser refresh doesn't keep re-showing this dialog.
+            (dom/replace-history-state! clean-url))
+
+        on-close
+        (mf/use-fn
+         (mf/deps profile)
+         (fn []
+           ;; Land on the user's own default team
+           (st/emit! (rt/assign-exception nil)
+                     (dcm/go-to-dashboard-recent :team-id (:default-team-id profile)))))
+
+        on-retry
+        (mf/use-fn
+         (mf/deps organization-id team-id clean-url)
+         (fn []
+           (st/emit! (rt/assign-exception nil))
+           (if (or team-id organization-id)
+             ;; Retry with team-id and/or organization-id to trigger SSO check
+             (st/emit! (dnt/retry-organization-sso {:team-id team-id
+                                                    :organization-id organization-id
+                                                    :dest-url clean-url}))
+             ;; Fallback: just navigate to clean URL
+             (st/emit! (rt/nav-raw :uri clean-url)))))]
+
+    [:> context-wrapper* {:is-dashboard (or is-dashboard (not is-workspace))
+                          :is-workspace is-workspace
+                          :profile profile}
+     [:> request-dialog* {:title (tr "labels.sso-error.title")
+                          :content [(tr "labels.sso-error.desc-message")]
+                          :button-text (tr "labels.sso-error.retry")
+                          :on-button-click on-retry
+                          :cancel-text (tr "not-found.no-permission.go-dashboard")
+                          :on-close on-close}]]))
+
+(mf/defc exception-section*
+  {::mf/private true}
+  [{:keys [data] :as props}]
+  (let [type   (get data :type)
+        cause  (get data ::errors/instance)
+        organization-id (get data :organization-id)
+
+        report (mf/with-memo [cause]
+                 (when (ex/exception? cause)
+                   (errors/generate-report cause)))
+
+        props  (mf/spread-props props {:report report})]
+
+    (mf/with-effect [report type cause]
+      (when (and (ex/exception? cause)
+                 (not (contains? #{:not-found :authentication} type)))
+        (errors/submit-report :event-name "exception-page"
+                              :report report
+                              :hint (ex/get-hint cause))))
+
+    (case type
+      :not-found
+      [:> not-found* {}]
+
+      :authentication
+      [:> not-found* {}]
+
+      :bad-gateway
+      [:> bad-gateway* props]
+
+      :service-unavailable
+      [:> service-unavailable*]
+
+      :nitrate-unavailable
+      [:> nitrate-unavailable*]
+
+      :sso-error
+      [:> sso-error-section* {:organization-id organization-id
+                              :team-id (get data :team-id)
+                              :profile (mf/deref refs/profile)
+                              :is-workspace (get data :is-workspace false)
+                              :is-dashboard (get data :is-dashboard true)}]
+
+      [:> internal-error* props])))
+
 (mf/defc exception-page*
   [{:keys [data route] :as props}]
 

frontend/src/app/util/dom.cljs

@@ -875,35 +875,17 @@
   [url]
   (.replaceState (.-history globals/window) nil "" url))
 
-(defn- update-query-params
-  "Apply `f` to the query-params map of `url`, returning the updated URL string.
-  Handles both plain query strings and fragment-based (hash) URLs."
-  [url f]
-  (let [transform (fn [parsed]
-                    (update parsed :query
-                            (fn [q]
-                              (-> (u/query-string->map (or q ""))
-                                  f
-                                  u/map->query-string))))
-        parsed    (u/uri url)
-        fragment  (:fragment parsed)]
-    (if (str/blank? fragment)
-      (str (transform parsed))
-      (-> parsed
-          (assoc :fragment (str (transform (u/parse fragment))))
-          str))))
-
 (defn append-query-param
   "Return a new URL string with the given query parameter added or replaced.
   Handles both plain query strings and fragment-based (hash) URLs."
   [url key value]
-  (update-query-params url #(assoc % key value)))
+  (u/append-query-param url key value))
 
 (defn remove-query-param
   "Return a new URL string with the given query parameter removed.
   Handles both plain query strings and fragment-based (hash) URLs."
   [url key]
-  (update-query-params url #(dissoc % key)))
+  (u/remove-query-param url key))
 
 (defn reload-current-window
   ([]

frontend/translations/en.po

@@ -10339,3 +10339,12 @@ msgstr "Click to close the path"
 
 msgid "notifications.invitation-canceled"
 msgstr "This invitation is no longer available."
+
+msgid "labels.sso-error.title"
+msgstr "We couldn't sign you in to your organization"
+
+msgid "labels.sso-error.desc-message"
+msgstr "Sign-in with your organization's identity provider didn't complete. The provider may be unavailable, or your account may not be in its directory yet. Your Penpot account isn't affected."
+
+msgid "labels.sso-error.retry"
+msgstr "Try again"

frontend/translations/es.po

@@ -9989,3 +9989,12 @@ msgstr "Pulsar para cerrar la ruta"
 
 msgid "notifications.invitation-canceled"
 msgstr "Esta invitación ya no está disponible."
+
+msgid "labels.sso-error.title"
+msgstr "No pudimos iniciar sesión en tu organización"
+
+msgid "labels.sso-error.desc-message"
+msgstr "El inicio de sesión con el proveedor de identidad de tu organización no se completó. Es posible que el proveedor no esté disponible o que tu cuenta aún no esté en su directorio. Tu cuenta de Penpot no se ha visto afectada."
+
+msgid "labels.sso-error.retry"
+msgstr "Intentar de nuevo"