mirror of
https://github.com/penpot/penpot.git
synced 2026-08-27 15:19:49 +00:00
🐛 Fix bad managed error on backend sso failure (#11247)
This commit is contained in:
parent
57c9c3f6a4
commit
ed04d509ed
@ -261,23 +261,28 @@
|
|||||||
(defn- wrap-nitrate-sso
|
(defn- wrap-nitrate-sso
|
||||||
"Enforce Nitrate organization SSO authentication for RPC handlers.
|
"Enforce Nitrate organization SSO authentication for RPC handlers.
|
||||||
|
|
||||||
Resolves the organization/team context from request params using priority order:
|
Resolves the organization/team context from request params:
|
||||||
1. Explicit :organization-id param
|
1. Explicit :organization-id param identifies the organization directly
|
||||||
2. Explicit :team-id param
|
2. The team comes from the first available of: explicit :team-id, explicit
|
||||||
3. Explicit :project-id param -> lookup project.team_id
|
:project-id -> lookup project.team_id, explicit :file-id -> lookup file's
|
||||||
4. Explicit :file-id param -> lookup file's team via join
|
team via join, or the :id param dispatched by ::rpc/id-type metadata
|
||||||
5. :id param dispatched by ::rpc/id-type metadata (:team, :project, or :file)
|
(:team, :project, or :file)
|
||||||
|
|
||||||
Once the context is resolved, checks if the user is authorized within that organization's
|
Once the context is resolved, checks if the user is authorized within that organization's
|
||||||
SSO session using nitrate/sso-session-authorized?. Authorized results are cached
|
SSO session using nitrate/sso-session-authorized?, against the organization when it is
|
||||||
by [profile-id cache-ref] for 15 minutes to avoid repeated lookups.
|
known and against the team otherwise. The team is resolved either way, so the raised
|
||||||
|
error can carry it. Authorized results are cached by [profile-id cache-ref] for 15
|
||||||
|
minutes to avoid repeated lookups.
|
||||||
|
|
||||||
Only activates when:
|
Only activates when:
|
||||||
- Nitrate flag is enabled
|
- Nitrate flag is enabled
|
||||||
- Endpoint requires authentication (::auth true by default)
|
- Endpoint requires authentication (::auth true by default)
|
||||||
- Endpoint is not marked with ::nitrate/organization-sso false
|
- Endpoint is not marked with ::nitrate/organization-sso false
|
||||||
|
|
||||||
Raises :nitrate-sso-required error if user is not authorized in the organization."
|
Raises :nitrate-sso-required error if user is not authorized in the organization.
|
||||||
|
The error carries the resolved :organization-id and :team-id so the client can
|
||||||
|
restart the SSO flow (via :check-nitrate-sso) instead of reporting a plain
|
||||||
|
permission failure."
|
||||||
[_ f mdata]
|
[_ f mdata]
|
||||||
(if (and (contains? cf/flags :admin-console)
|
(if (and (contains? cf/flags :admin-console)
|
||||||
(::auth mdata true) ;; only for endpoints that needs auth
|
(::auth mdata true) ;; only for endpoints that needs auth
|
||||||
@ -302,17 +307,22 @@
|
|||||||
cached (cache/get organization-sso-auth-cache cache-key)
|
cached (cache/get organization-sso-auth-cache cache-key)
|
||||||
result (if (some? cached)
|
result (if (some? cached)
|
||||||
cached
|
cached
|
||||||
(let [team-id (when-not organization-id
|
;; The team is resolved even when the organization is
|
||||||
(or team-id
|
;; already known: the client needs it to restart the
|
||||||
(when project-id
|
;; SSO flow without sending non-members through the
|
||||||
(:team-id (db/get-by-id cfg :project project-id {:columns [:id :team-id]})))
|
;; organization's identity provider.
|
||||||
|
(let [team-id (or team-id
|
||||||
|
(when project-id
|
||||||
|
(:team-id (db/get-by-id cfg :project project-id {:columns [:id :team-id]})))
|
||||||
|
(when file-id
|
||||||
(:id (teams/get-team-for-file cfg file-id))))
|
(:id (teams/get-team-for-file cfg file-id))))
|
||||||
request (-> (meta params) (get ::http/request))
|
request (-> (meta params) (get ::http/request))
|
||||||
{:keys [authorized sso]} (if organization-id
|
{:keys [authorized sso]} (if organization-id
|
||||||
(nitrate/sso-session-authorized? cfg organization-id nil request)
|
(nitrate/sso-session-authorized? cfg organization-id nil request)
|
||||||
(nitrate/sso-session-authorized? cfg nil team-id request))
|
(nitrate/sso-session-authorized? cfg nil team-id request))
|
||||||
entry {:authorized authorized
|
entry {:authorized authorized
|
||||||
:organization-id (:organization-id sso)}]
|
:organization-id (or (:organization-id sso) organization-id)
|
||||||
|
:team-id team-id}]
|
||||||
(when authorized
|
(when authorized
|
||||||
(cache/get organization-sso-auth-cache cache-key (constantly entry)))
|
(cache/get organization-sso-auth-cache cache-key (constantly entry)))
|
||||||
entry))]
|
entry))]
|
||||||
@ -320,6 +330,8 @@
|
|||||||
(f cfg params)
|
(f cfg params)
|
||||||
(ex/raise :type :authentication
|
(ex/raise :type :authentication
|
||||||
:code :nitrate-sso-required
|
:code :nitrate-sso-required
|
||||||
|
:organization-id (:organization-id result)
|
||||||
|
:team-id (:team-id result)
|
||||||
:hint "organization SSO authentication required")))
|
:hint "organization SSO authentication required")))
|
||||||
(f cfg params))))
|
(f cfg params))))
|
||||||
f))
|
f))
|
||||||
|
|||||||
@ -673,10 +673,15 @@
|
|||||||
(sv/defmethod ::check-nitrate-sso
|
(sv/defmethod ::check-nitrate-sso
|
||||||
"Check if a user needs to login into the organization SSO.
|
"Check if a user needs to login into the organization SSO.
|
||||||
Accepts either team-id (to look up the organization via the team) or organization-id directly.
|
Accepts either team-id (to look up the organization via the team) or organization-id directly.
|
||||||
Returns {:authorized true} when SSO is not active or the user cannot access the team.
|
Returns {:authorized true :reason :sso-satisfied} when SSO is not active or the
|
||||||
|
session already holds a valid entry for the organization, and
|
||||||
|
{:authorized true :reason :no-team-access} when the gate was skipped because the
|
||||||
|
user cannot access the team; the reason lets the client tell a usable session
|
||||||
|
apart from a plain permission failure.
|
||||||
Returns {:authorized false :redirect-uri <url>} when SSO is active;
|
Returns {:authorized false :redirect-uri <url>} when SSO is active;
|
||||||
the client must redirect there. The OIDC provider itself handles
|
the client must redirect there. The OIDC provider itself handles
|
||||||
re-authentication transparently if the user already has an active SSO session."
|
re-authentication transparently if the user already has an active SSO session.
|
||||||
|
A nil :redirect-uri means SSO is required but the provider is not usable."
|
||||||
{::rpc/auth true
|
{::rpc/auth true
|
||||||
::doc/added "2.18"
|
::doc/added "2.18"
|
||||||
::sm/params schema:check-nitrate-sso
|
::sm/params schema:check-nitrate-sso
|
||||||
@ -687,11 +692,11 @@
|
|||||||
(not (teams/has-read-permissions? cfg profile-id team-id)))
|
(not (teams/has-read-permissions? cfg profile-id team-id)))
|
||||||
;; Let the destination RPC enforce its own permissions. Starting SSO before
|
;; Let the destination RPC enforce its own permissions. Starting SSO before
|
||||||
;; access is established sends unrelated users through the organization's IdP.
|
;; access is established sends unrelated users through the organization's IdP.
|
||||||
{:authorized true}
|
{:authorized true :reason :no-team-access}
|
||||||
(let [request (rph/get-request params)
|
(let [request (rph/get-request params)
|
||||||
{:keys [authorized sso]} (nitrate/sso-session-authorized? cfg organization-id team-id request)]
|
{:keys [authorized sso]} (nitrate/sso-session-authorized? cfg organization-id team-id request)]
|
||||||
(if authorized
|
(if authorized
|
||||||
{:authorized true}
|
{:authorized true :reason :sso-satisfied}
|
||||||
(if (oidc/organization-sso-discovery-uri sso)
|
(if (oidc/organization-sso-discovery-uri sso)
|
||||||
{:authorized false
|
{:authorized false
|
||||||
:redirect-uri (oidc/build-organization-sso-auth-redirect-uri cfg sso
|
:redirect-uri (oidc/build-organization-sso-auth-redirect-uri cfg sso
|
||||||
@ -699,4 +704,4 @@
|
|||||||
:organization-id organization-id)}
|
:organization-id organization-id)}
|
||||||
{:authorized false
|
{:authorized false
|
||||||
:redirect-uri nil}))))
|
:redirect-uri nil}))))
|
||||||
{:authorized true}))
|
{:authorized true :reason :sso-satisfied}))
|
||||||
|
|||||||
@ -15,15 +15,17 @@
|
|||||||
[app.db :as-alias db]
|
[app.db :as-alias db]
|
||||||
[app.email :as eml]
|
[app.email :as eml]
|
||||||
[app.http :as-alias http]
|
[app.http :as-alias http]
|
||||||
|
[app.http.errors :as http-errors]
|
||||||
[app.nitrate :as nitrate]
|
[app.nitrate :as nitrate]
|
||||||
[app.rpc :as-alias rpc]
|
[app.rpc :as rpc]
|
||||||
[app.rpc.commands.nitrate]
|
[app.rpc.commands.nitrate]
|
||||||
[app.rpc.commands.teams :as teams]
|
[app.rpc.commands.teams :as teams]
|
||||||
[app.rpc.helpers :as rph]
|
[app.rpc.helpers :as rph]
|
||||||
[backend-tests.helpers :as th]
|
[backend-tests.helpers :as th]
|
||||||
[buddy.core.codecs :as bc]
|
[buddy.core.codecs :as bc]
|
||||||
[clojure.test :as t]
|
[clojure.test :as t]
|
||||||
[cuerdas.core :as str]))
|
[cuerdas.core :as str]
|
||||||
|
[yetti.response :as-alias yres]))
|
||||||
|
|
||||||
(t/use-fixtures :once th/state-init)
|
(t/use-fixtures :once th/state-init)
|
||||||
(t/use-fixtures :each th/database-reset)
|
(t/use-fixtures :each th/database-reset)
|
||||||
@ -87,6 +89,31 @@
|
|||||||
|
|
||||||
nil)))
|
nil)))
|
||||||
|
|
||||||
|
(defn- unauthorized-sso-mock
|
||||||
|
"Creates a mock for nitrate/sso-session-authorized? that reports an active
|
||||||
|
SSO the session does not satisfy. Pass nil to leave the organization out of
|
||||||
|
the nitrate payload."
|
||||||
|
[organization-id]
|
||||||
|
(fn [_cfg _organization-id _team-id _request]
|
||||||
|
{:authorized false
|
||||||
|
:sso (cond-> {:active true
|
||||||
|
:issuer "https://idp.example.com"}
|
||||||
|
(some? organization-id)
|
||||||
|
(assoc :organization-id organization-id))}))
|
||||||
|
|
||||||
|
(defn- sso-gate-error
|
||||||
|
"Builds the SSO gate around a handler that must never be reached, and
|
||||||
|
returns the exception it raises for `params`."
|
||||||
|
[mdata params cfg]
|
||||||
|
(let [handler (fn [_cfg _params] ::handler-called)
|
||||||
|
wrapped (binding [cf/flags (conj cf/flags :admin-console)]
|
||||||
|
(#'rpc/wrap-nitrate-sso nil handler mdata))]
|
||||||
|
(try
|
||||||
|
(wrapped cfg (with-meta params {::http/request {}}))
|
||||||
|
nil
|
||||||
|
(catch Throwable cause
|
||||||
|
cause))))
|
||||||
|
|
||||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||||
;; Tests
|
;; Tests
|
||||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||||
@ -112,7 +139,9 @@
|
|||||||
(constantly "https://idp.example.com/authorize")]
|
(constantly "https://idp.example.com/authorize")]
|
||||||
(let [out (th/command! params)]
|
(let [out (th/command! params)]
|
||||||
(t/is (th/success? out))
|
(t/is (th/success? out))
|
||||||
(t/is (= {:authorized true} (:result out))))))))
|
;; The reason tells the client this is a permission problem, not a
|
||||||
|
;; usable SSO session.
|
||||||
|
(t/is (= {:authorized true :reason :no-team-access} (:result out))))))))
|
||||||
|
|
||||||
(t/deftest check-nitrate-sso-keeps-gate-for-team-member
|
(t/deftest check-nitrate-sso-keeps-gate-for-team-member
|
||||||
(let [team-owner (th/create-profile* 1 {:is-active true})
|
(let [team-owner (th/create-profile* 1 {:is-active true})
|
||||||
@ -169,6 +198,91 @@
|
|||||||
:redirect-uri redirect-uri}
|
:redirect-uri redirect-uri}
|
||||||
(:result out))))))))
|
(:result out))))))))
|
||||||
|
|
||||||
|
(t/deftest check-nitrate-sso-reports-a-satisfied-gate-for-a-valid-session
|
||||||
|
(let [team-owner (th/create-profile* 1 {:is-active true})
|
||||||
|
team (th/create-team* 1 {:profile-id (:id team-owner)})
|
||||||
|
organization-id (uuid/random)
|
||||||
|
params (with-meta
|
||||||
|
{::th/type :check-nitrate-sso
|
||||||
|
::rpc/profile-id (:id team-owner)
|
||||||
|
:team-id (:id team)
|
||||||
|
:url "https://penpot.example.com/#/workspace"}
|
||||||
|
{::http/request {}})]
|
||||||
|
(binding [cf/flags (conj cf/flags :admin-console)]
|
||||||
|
(with-redefs [nitrate/sso-session-authorized?
|
||||||
|
(fn [_cfg _organization-id _team-id _request]
|
||||||
|
{:authorized true
|
||||||
|
:sso {:active true
|
||||||
|
:issuer "https://idp.example.com"
|
||||||
|
:organization-id organization-id}})]
|
||||||
|
(let [out (th/command! params)]
|
||||||
|
(t/is (th/success? out))
|
||||||
|
(t/is (= {:authorized true :reason :sso-satisfied} (:result out))))))))
|
||||||
|
|
||||||
|
(t/deftest nitrate-sso-required-error-resolves-the-team-from-the-file
|
||||||
|
(t/testing "the workspace path, where the file id arrives as :id, still reports the team"
|
||||||
|
(let [profile (th/create-profile* 1 {:is-active true})
|
||||||
|
file (th/create-file* 1 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)})
|
||||||
|
organization-id (uuid/random)]
|
||||||
|
(with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock organization-id)]
|
||||||
|
(let [data (ex-data (sso-gate-error {::rpc/id-type :file}
|
||||||
|
{::rpc/profile-id (:id profile)
|
||||||
|
:id (:id file)}
|
||||||
|
th/*system*))]
|
||||||
|
(t/is (= :authentication (:type data)))
|
||||||
|
(t/is (= :nitrate-sso-required (:code data)))
|
||||||
|
(t/is (= organization-id (:organization-id data)))
|
||||||
|
(t/is (= (:default-team-id profile) (:team-id data))))))))
|
||||||
|
|
||||||
|
(t/deftest nitrate-sso-required-error-keeps-the-team-known-by-the-request
|
||||||
|
(t/testing "an explicit team-id is not dropped by an explicit organization-id"
|
||||||
|
(let [profile-id (uuid/random)
|
||||||
|
team-id (uuid/random)
|
||||||
|
organization-id (uuid/random)]
|
||||||
|
;; The nitrate payload carries no organization-id here, so the one from
|
||||||
|
;; the request params is the only one left to report.
|
||||||
|
(with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock nil)]
|
||||||
|
(let [data (ex-data (sso-gate-error {}
|
||||||
|
{::rpc/profile-id profile-id
|
||||||
|
:team-id team-id
|
||||||
|
:organization-id organization-id}
|
||||||
|
{}))]
|
||||||
|
(t/is (= organization-id (:organization-id data)))
|
||||||
|
(t/is (= team-id (:team-id data))))))))
|
||||||
|
|
||||||
|
(t/deftest nitrate-sso-required-error-resolves-the-team-with-a-known-organization
|
||||||
|
(t/testing "knowing the organization does not stop the team lookup"
|
||||||
|
(let [profile (th/create-profile* 1 {:is-active true})
|
||||||
|
file (th/create-file* 1 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)})
|
||||||
|
organization-id (uuid/random)]
|
||||||
|
(with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock nil)]
|
||||||
|
(let [data (ex-data (sso-gate-error {}
|
||||||
|
{::rpc/profile-id (:id profile)
|
||||||
|
:organization-id organization-id
|
||||||
|
:file-id (:id file)}
|
||||||
|
th/*system*))]
|
||||||
|
(t/is (= organization-id (:organization-id data)))
|
||||||
|
(t/is (= (:default-team-id profile) (:team-id data))))))))
|
||||||
|
|
||||||
|
(t/deftest nitrate-sso-required-error-reaches-the-client-in-the-401-body
|
||||||
|
(t/testing "the ids survive the http error response, not only the exception"
|
||||||
|
(let [profile-id (uuid/random)
|
||||||
|
team-id (uuid/random)
|
||||||
|
organization-id (uuid/random)]
|
||||||
|
(with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock organization-id)]
|
||||||
|
(let [cause (sso-gate-error {}
|
||||||
|
{::rpc/profile-id profile-id
|
||||||
|
:team-id team-id}
|
||||||
|
{})
|
||||||
|
response (http-errors/handle cause {})
|
||||||
|
body (::yres/body response)]
|
||||||
|
(t/is (= 401 (::yres/status response)))
|
||||||
|
(t/is (= :nitrate-sso-required (:code body)))
|
||||||
|
(t/is (= organization-id (:organization-id body)))
|
||||||
|
(t/is (= team-id (:team-id body))))))))
|
||||||
|
|
||||||
(t/deftest leave-organization-happy-path-no-extra-teams
|
(t/deftest leave-organization-happy-path-no-extra-teams
|
||||||
(let [profile-owner (th/create-profile* 1 {:is-active true})
|
(let [profile-owner (th/create-profile* 1 {:is-active true})
|
||||||
profile-user (th/create-profile* 2 {:is-active true})
|
profile-user (th/create-profile* 2 {:is-active true})
|
||||||
|
|||||||
@ -352,19 +352,30 @@
|
|||||||
(rx/empty)))))))))))
|
(rx/empty)))))))))))
|
||||||
|
|
||||||
|
|
||||||
|
(defn check-organization-sso
|
||||||
|
"Asks the backend whether the organization SSO gate can be satisfied for
|
||||||
|
`dest-url`, returning an observable of the raw `:check-nitrate-sso`
|
||||||
|
result: `:authorized` with a `:reason` of `:sso-satisfied` or
|
||||||
|
`:no-team-access`, or `:authorized false` with a `:redirect-uri` (nil
|
||||||
|
when SSO is required but the provider is unusable). Failures are not
|
||||||
|
caught, so a network blip stays a network error for the caller to
|
||||||
|
handle instead of masquerading as an answer."
|
||||||
|
[{:keys [team-id organization-id dest-url]}]
|
||||||
|
(rp/cmd! :check-nitrate-sso (d/without-nils {:team-id team-id
|
||||||
|
:organization-id organization-id
|
||||||
|
:url dest-url})))
|
||||||
|
|
||||||
(defn retry-organization-sso
|
(defn retry-organization-sso
|
||||||
"Retries the organization SSO login flow after a failed attempt, reusing
|
"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 same check-nitrate-sso RPC used elsewhere to move the user through
|
||||||
the organization's identity provider. Passing `team-id` enables the
|
the organization's identity provider. Passing `team-id` enables the
|
||||||
backend's non-member short-circuit. Falls back to navigating straight
|
backend's non-member short-circuit. Falls back to navigating straight
|
||||||
to `dest-url` when no fresh SSO redirect is needed or available."
|
to `dest-url` when no fresh SSO redirect is needed or available."
|
||||||
[{:keys [team-id organization-id dest-url]}]
|
[{:keys [dest-url] :as params}]
|
||||||
(ptk/reify ::retry-organization-sso
|
(ptk/reify ::retry-organization-sso
|
||||||
ptk/WatchEvent
|
ptk/WatchEvent
|
||||||
(watch [_ _ _]
|
(watch [_ _ _]
|
||||||
(->> (rp/cmd! :check-nitrate-sso (d/without-nils {:team-id team-id
|
(->> (check-organization-sso params)
|
||||||
:organization-id organization-id
|
|
||||||
:url dest-url}))
|
|
||||||
(rx/map (fn [{:keys [redirect-uri]}]
|
(rx/map (fn [{:keys [redirect-uri]}]
|
||||||
(rt/nav-raw :uri (or redirect-uri dest-url))))
|
(rt/nav-raw :uri (or redirect-uri dest-url))))
|
||||||
(rx/catch (fn [_]
|
(rx/catch (fn [_]
|
||||||
|
|||||||
@ -13,6 +13,7 @@
|
|||||||
[app.main.data.auth :as da]
|
[app.main.data.auth :as da]
|
||||||
[app.main.data.event :as ev]
|
[app.main.data.event :as ev]
|
||||||
[app.main.data.modal :as modal]
|
[app.main.data.modal :as modal]
|
||||||
|
[app.main.data.nitrate :as dnt]
|
||||||
[app.main.data.notifications :as ntf]
|
[app.main.data.notifications :as ntf]
|
||||||
[app.main.data.workspace :as-alias dw]
|
[app.main.data.workspace :as-alias dw]
|
||||||
[app.main.router :as rt]
|
[app.main.router :as rt]
|
||||||
@ -21,6 +22,7 @@
|
|||||||
[app.util.globals :as g]
|
[app.util.globals :as g]
|
||||||
[app.util.i18n :refer [tr]]
|
[app.util.i18n :refer [tr]]
|
||||||
[app.util.timers :as ts]
|
[app.util.timers :as ts]
|
||||||
|
[beicon.v2.core :as rx]
|
||||||
[cuerdas.core :as str]
|
[cuerdas.core :as str]
|
||||||
[potok.v2.core :as ptk]))
|
[potok.v2.core :as ptk]))
|
||||||
|
|
||||||
@ -234,9 +236,8 @@
|
|||||||
;; We receive a explicit authentication error; If the uri is for
|
;; We receive a explicit authentication error; If the uri is for
|
||||||
;; workspace, dashboard, viewer or settings, then assign the exception
|
;; workspace, dashboard, viewer or settings, then assign the exception
|
||||||
;; for show the error page. Otherwise this explicitly clears all
|
;; for show the error page. Otherwise this explicitly clears all
|
||||||
;; profile data and redirect the user to the login page. This is here
|
;; profile data and redirect the user to the login page.
|
||||||
;; and not in app.main.errors because of circular dependency.
|
(defn- show-authentication-error
|
||||||
(defmethod ptk/handle-error :authentication
|
|
||||||
[error]
|
[error]
|
||||||
(let [message (tr "errors.auth.unable-to-login")
|
(let [message (tr "errors.auth.unable-to-login")
|
||||||
uri (rt/get-current-href)
|
uri (rt/get-current-href)
|
||||||
@ -253,6 +254,85 @@
|
|||||||
(st/emit! (da/logout))
|
(st/emit! (da/logout))
|
||||||
(ts/schedule 500 #(st/emit! (ntf/warn message)))))))
|
(ts/schedule 500 #(st/emit! (ntf/warn message)))))))
|
||||||
|
|
||||||
|
;; The user does belong to an organization with SSO active, but there is
|
||||||
|
;; no provider to send them to (unusable or incomplete SSO config). Show
|
||||||
|
;; the SSO error dialog, which offers an explicit retry, rather than
|
||||||
|
;; claiming they have no access.
|
||||||
|
(defn- show-sso-error
|
||||||
|
[{:keys [organization-id team-id]}]
|
||||||
|
(let [uri (rt/get-current-href)]
|
||||||
|
(st/async-emit!
|
||||||
|
(rt/assign-exception {:type :sso-error
|
||||||
|
:organization-id organization-id
|
||||||
|
:team-id team-id
|
||||||
|
:is-workspace (str/includes? uri "workspace")
|
||||||
|
:is-dashboard (str/includes? uri "dashboard")}))))
|
||||||
|
|
||||||
|
;; A page issues many SSO-guarded requests at once, and all of them fail
|
||||||
|
;; together the moment the organization SSO session lapses; without this
|
||||||
|
;; only-one-in-flight guard each of them would start its own identity
|
||||||
|
;; provider round-trip.
|
||||||
|
(def ^:private sso-renewal-pending? (volatile! false))
|
||||||
|
|
||||||
|
(defn- renew-organization-sso
|
||||||
|
"Recover from a request rejected by the organization SSO gate.
|
||||||
|
|
||||||
|
Asks the backend what can be done for the current location and acts on
|
||||||
|
the answer: go through the identity provider when there is one (it
|
||||||
|
re-authenticates transparently while the user still has a live session
|
||||||
|
with it), retry the location when the gate turns out to be satisfied
|
||||||
|
already (another tab renewed the session, or SSO was turned off), show
|
||||||
|
the SSO error dialog when SSO is required but unusable, and report a
|
||||||
|
permission failure only when the user really has no access to the team.
|
||||||
|
A failing check is left to the generic error handling, so a network
|
||||||
|
blip is not turned into a permission error."
|
||||||
|
[{:keys [organization-id team-id] :as error}]
|
||||||
|
(when-not @sso-renewal-pending?
|
||||||
|
(vreset! sso-renewal-pending? true)
|
||||||
|
(let [dest-url (rt/get-current-href)]
|
||||||
|
(->> (dnt/check-organization-sso
|
||||||
|
{:organization-id organization-id
|
||||||
|
:team-id team-id
|
||||||
|
:dest-url dest-url})
|
||||||
|
;; Release the guard however the check ends, including an
|
||||||
|
;; unsubscription or a completion without a result: a stuck guard
|
||||||
|
;; would silently drop every later rejection.
|
||||||
|
(rx/finalize (fn [] (vreset! sso-renewal-pending? false)))
|
||||||
|
(rx/subs! (fn [{:keys [authorized reason redirect-uri]}]
|
||||||
|
(cond
|
||||||
|
;; SSO must be renewed and we know where to send them
|
||||||
|
(some? redirect-uri)
|
||||||
|
(st/emit! (rt/nav-raw :uri (str redirect-uri)))
|
||||||
|
|
||||||
|
;; The gate is satisfied after all, so the request
|
||||||
|
;; that failed can be retried. Only an affirmative
|
||||||
|
;; reason is accepted here: reloading on any
|
||||||
|
;; unrecognized "authorized" answer would spin
|
||||||
|
;; whenever the reload hits the same rejection.
|
||||||
|
(= :sso-satisfied reason)
|
||||||
|
(st/emit! (rt/reload false))
|
||||||
|
|
||||||
|
;; SSO is required but the provider is unusable
|
||||||
|
(not authorized)
|
||||||
|
(show-sso-error error)
|
||||||
|
|
||||||
|
;; No access to the team, so the gate was never
|
||||||
|
;; evaluated: this really is a permission failure
|
||||||
|
:else
|
||||||
|
(show-authentication-error error)))
|
||||||
|
on-error)))))
|
||||||
|
|
||||||
|
(defmethod ptk/handle-error :authentication
|
||||||
|
[error]
|
||||||
|
;; Without an organization or a team there is nothing to check, and asking
|
||||||
|
;; anyway would fail schema validation and report that instead of the
|
||||||
|
;; authentication problem the user actually hit.
|
||||||
|
(if (and (= :nitrate-sso-required (get error :code))
|
||||||
|
(or (some? (get error :organization-id))
|
||||||
|
(some? (get error :team-id))))
|
||||||
|
(renew-organization-sso error)
|
||||||
|
(show-authentication-error error)))
|
||||||
|
|
||||||
;; Error that happens on an active business model validation does not
|
;; Error that happens on an active business model validation does not
|
||||||
;; passes an validation (example: profile can't leave a team). From
|
;; passes an validation (example: profile can't leave a team). From
|
||||||
;; the user perspective a error flash message should be visualized but
|
;; the user perspective a error flash message should be visualized but
|
||||||
|
|||||||
@ -11,10 +11,17 @@
|
|||||||
- stale-asset-error? – pure predicate
|
- stale-asset-error? – pure predicate
|
||||||
- exception->error-data – pure transformer
|
- exception->error-data – pure transformer
|
||||||
- on-error re-entrancy guard – prevents recursive invocations
|
- on-error re-entrancy guard – prevents recursive invocations
|
||||||
- flash schedules async emit – ntf/show is not emitted synchronously"
|
- flash schedules async emit – ntf/show is not emitted synchronously
|
||||||
|
- organization SSO recovery – expired SSO sessions go back to the provider"
|
||||||
(:require
|
(:require
|
||||||
[app.main.errors :as errors]
|
[app.main.errors :as errors]
|
||||||
|
[app.main.repo :as rp]
|
||||||
|
[app.main.router :as rt]
|
||||||
|
[app.main.store :as st]
|
||||||
|
[app.util.timers :as tm]
|
||||||
|
[beicon.v2.core :as rx]
|
||||||
[cljs.test :as t :include-macros true]
|
[cljs.test :as t :include-macros true]
|
||||||
|
[frontend-tests.helpers.mock :as mock]
|
||||||
[potok.v2.core :as ptk]))
|
[potok.v2.core :as ptk]))
|
||||||
|
|
||||||
;; ---------------------------------------------------------------------------
|
;; ---------------------------------------------------------------------------
|
||||||
@ -134,3 +141,231 @@
|
|||||||
(errors/on-error (ex-info "test" {:type ::test-reentrant :hint "first"}))
|
(errors/on-error (ex-info "test" {:type ::test-reentrant :hint "first"}))
|
||||||
;; The guard must have allowed only the first invocation through.
|
;; The guard must have allowed only the first invocation through.
|
||||||
(t/is (= 1 @reentrant-call-count))))
|
(t/is (= 1 @reentrant-call-count))))
|
||||||
|
|
||||||
|
;; ---------------------------------------------------------------------------
|
||||||
|
;; Expired organization SSO session
|
||||||
|
;;
|
||||||
|
;; The backend rejects SSO-guarded requests with an :authentication error
|
||||||
|
;; coded :nitrate-sso-required once the organization SSO session lapses.
|
||||||
|
;; The user must be sent back through the identity provider instead of
|
||||||
|
;; being told they have no access to the file.
|
||||||
|
;; ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
(def ^:private workspace-href
|
||||||
|
"https://penpot.example.com/#/workspace?team-id=b8f8bb52-8b70-8144-8004-4a5085f0bdc9")
|
||||||
|
|
||||||
|
(def ^:private organization-id "d1a4c0f2-2f36-8114-8006-1b0e6d9d0c11")
|
||||||
|
|
||||||
|
(defn- sso-required-error
|
||||||
|
[]
|
||||||
|
{:type :authentication
|
||||||
|
:code :nitrate-sso-required
|
||||||
|
:organization-id organization-id
|
||||||
|
:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9"})
|
||||||
|
|
||||||
|
(t/deftest expired-organization-sso-navigates-to-identity-provider
|
||||||
|
(t/testing "the browser is sent to the identity provider instead of an error page"
|
||||||
|
(let [events (atom [])]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [_command _params]
|
||||||
|
(rx/of {:authorized false
|
||||||
|
:redirect-uri "https://idp.example.com/authorize"})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
st/emit!
|
||||||
|
(mock/stub (fn [& emitted] (swap! events into emitted)))]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= [::rt/nav-raw] (mapv ptk/type @events)))))))
|
||||||
|
|
||||||
|
(t/deftest expired-organization-sso-comes-back-to-the-current-location
|
||||||
|
(t/testing "the SSO check asks the provider to return the user where they were"
|
||||||
|
(let [rpc-calls (atom [])]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [command params]
|
||||||
|
(swap! rpc-calls conj {:command command :params params})
|
||||||
|
(rx/of {:authorized false
|
||||||
|
:redirect-uri "https://idp.example.com/authorize"})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
st/emit! mock/noop]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= [{:command :check-nitrate-sso
|
||||||
|
:params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9"
|
||||||
|
:organization-id organization-id
|
||||||
|
:url workspace-href}}]
|
||||||
|
@rpc-calls))))))
|
||||||
|
|
||||||
|
(t/deftest already-satisfied-organization-sso-retries-the-location
|
||||||
|
(t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring"
|
||||||
|
(let [events (atom [])]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [_command _params]
|
||||||
|
(rx/of {:authorized true :reason :sso-satisfied})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
st/emit!
|
||||||
|
(mock/stub (fn [& emitted] (swap! events into emitted)))]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= [::rt/reload] (mapv ptk/type @events)))))))
|
||||||
|
|
||||||
|
(t/deftest organization-sso-without-usable-provider-shows-the-sso-error-dialog
|
||||||
|
(t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error"
|
||||||
|
(let [assigned* (atom nil)]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [_command _params]
|
||||||
|
(rx/of {:authorized false :redirect-uri nil})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
rt/assign-exception
|
||||||
|
(fn [error]
|
||||||
|
(reset! assigned* error)
|
||||||
|
(ptk/data-event ::assigned error))]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= :sso-error (:type @assigned*)))
|
||||||
|
(t/is (= organization-id (:organization-id @assigned*)))
|
||||||
|
(t/is (true? (:is-workspace @assigned*)))))))
|
||||||
|
|
||||||
|
(t/deftest organization-sso-without-team-access-reports-a-permission-failure
|
||||||
|
(t/testing "a user who cannot reach the team keeps getting the authentication error"
|
||||||
|
(let [assigned* (atom nil)]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [_command _params]
|
||||||
|
(rx/of {:authorized true :reason :no-team-access})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
rt/assign-exception
|
||||||
|
(fn [error]
|
||||||
|
(reset! assigned* error)
|
||||||
|
(ptk/data-event ::assigned error))]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= :authentication (:type @assigned*)))
|
||||||
|
(t/is (= :nitrate-sso-required (:code @assigned*)))))))
|
||||||
|
|
||||||
|
(t/deftest organization-sso-does-not-retry-on-an-unexplained-authorization
|
||||||
|
(t/testing "reloading on an answer we don't understand would spin on the same rejection"
|
||||||
|
(let [events (atom [])]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub (fn [_command _params] (rx/of {:authorized true})))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
rt/assign-exception
|
||||||
|
(fn [error] (ptk/data-event ::assigned error))
|
||||||
|
|
||||||
|
;; async-emit! is variadic-only, so the replacement must be
|
||||||
|
;; variadic too for the compiled static dispatch to find it
|
||||||
|
st/async-emit!
|
||||||
|
(fn [& emitted] (swap! events into emitted))]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= [::assigned] (mapv ptk/type @events)))))))
|
||||||
|
|
||||||
|
(t/deftest organization-sso-error-without-context-is-reported-as-it-arrives
|
||||||
|
(t/testing "with no organization and no team there is nothing to check"
|
||||||
|
(let [rpc-calls (atom 0)
|
||||||
|
assigned* (atom nil)]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub (fn [_command _params]
|
||||||
|
(swap! rpc-calls inc)
|
||||||
|
(rx/empty)))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
rt/assign-exception
|
||||||
|
(fn [error]
|
||||||
|
(reset! assigned* error)
|
||||||
|
(ptk/data-event ::assigned error))]
|
||||||
|
|
||||||
|
(errors/on-error {:type :authentication
|
||||||
|
:code :nitrate-sso-required})
|
||||||
|
|
||||||
|
(t/is (zero? @rpc-calls))
|
||||||
|
(t/is (= :nitrate-sso-required (:code @assigned*)))))))
|
||||||
|
|
||||||
|
(t/deftest a-resultless-organization-sso-check-does-not-wedge-later-rejections
|
||||||
|
(t/testing "the one-in-flight guard is released even when no answer arrives"
|
||||||
|
(let [rpc-calls (atom 0)]
|
||||||
|
(with-redefs [rp/cmd!
|
||||||
|
(mock/stub (fn [_command _params]
|
||||||
|
(swap! rpc-calls inc)
|
||||||
|
(rx/empty)))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
st/emit! mock/noop]
|
||||||
|
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
|
||||||
|
(t/is (= 2 @rpc-calls))))))
|
||||||
|
|
||||||
|
;; A failing check must stay a failing check: the generic handling turns it
|
||||||
|
;; into a toast, whereas swallowing it would show a permission error for
|
||||||
|
;; what may be a momentary network blip. The mocked RPC fails on a later
|
||||||
|
;; tick, like a real request, so the handler is not inside on-error's
|
||||||
|
;; re-entrancy guard when the failure arrives.
|
||||||
|
|
||||||
|
(def ^:private check-failures (atom []))
|
||||||
|
|
||||||
|
(defmethod ptk/handle-error ::test-check-failure
|
||||||
|
[error]
|
||||||
|
(swap! check-failures conj error))
|
||||||
|
|
||||||
|
(t/deftest failing-organization-sso-check-is-not-reported-as-missing-access
|
||||||
|
(t/async done
|
||||||
|
(reset! check-failures [])
|
||||||
|
(let [assigned* (atom nil)]
|
||||||
|
(mock/with-mocks
|
||||||
|
{rp/cmd!
|
||||||
|
(mock/stub
|
||||||
|
(fn [_command _params]
|
||||||
|
(->> (rx/timer 0)
|
||||||
|
(rx/mapcat (fn [_]
|
||||||
|
(rx/throw (ex-info "boom" {:type ::test-check-failure})))))))
|
||||||
|
|
||||||
|
rt/get-current-href
|
||||||
|
(constantly workspace-href)
|
||||||
|
|
||||||
|
rt/assign-exception
|
||||||
|
(fn [error]
|
||||||
|
(reset! assigned* error)
|
||||||
|
(ptk/data-event ::assigned error))}
|
||||||
|
|
||||||
|
(fn [done']
|
||||||
|
(errors/on-error (sso-required-error))
|
||||||
|
(tm/schedule
|
||||||
|
50
|
||||||
|
(fn []
|
||||||
|
(t/is (= [::test-check-failure] (mapv :type @check-failures)))
|
||||||
|
(t/is (nil? @assigned*))
|
||||||
|
(done'))))
|
||||||
|
done))))
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user