diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 286ff3be45..4f7583d39d 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -761,6 +761,16 @@ ;; ORG SSO HELPERS ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +(defn- non-blank-uri + [value] + (when-not (str/blank? value) value)) + +(defn org-sso-discovery-uri + "Return the OIDC discovery URI from an org SSO config, preferring :issuer." + [sso] + (or (non-blank-uri (:issuer sso)) + (non-blank-uri (:base-url sso)))) + (defn prepare-org-sso-provider "Build an OIDC provider map dynamically from the Nitrate org SSO config. Uses OIDC discovery via :base-url (or :issuer as fallback) when @@ -770,12 +780,96 @@ {:type "oidc" :client-id client-id :client-secret client-secret - :base-uri (some-> (or base-url issuer) + :base-uri (some-> (or (non-blank-uri base-url) + (non-blank-uri issuer)) (str/rtrim "/") (str "/")) :scopes (into default-oidc-scopes (or scopes #{})) :skip-ssrf-check? true})) +(defn build-org-sso-auth-redirect-uri + "Build the OIDC authorization redirect URI for an organization SSO config. + Raises if the config is incomplete or OIDC discovery fails." + [cfg sso & {:keys [dest-url organization-id provider]}] + (let [organization-id (or organization-id (:organization-id sso)) + issuer (org-sso-discovery-uri sso) + dest-url (or dest-url (str (cf/get :public-uri)))] + (when-not issuer + (ex/raise :type :validation + :code :invalid-sso-config + :hint "missing issuer or base-url")) + (let [oidc-provider (or provider (prepare-org-sso-provider cfg sso)) + state-token (tokens/generate cfg {:iss "oidc" + :dest-url dest-url + :organization-id organization-id + :issuer issuer + :exp (ct/in-future "4h")})] + (build-auth-redirect-uri oidc-provider state-token)))) + +(def ^:private probe-auth-code "penpot-sso-config-probe") + +(defn- decode-token-error-response + [body] + (when (and (string? body) (pos? (count body))) + (try + (json/decode body) + (catch Throwable _ nil)))) + +(defn- token-endpoint-error + [response] + (some-> response :body decode-token-error-response :error d/name)) + +(defn- token-endpoint-error-description + [response] + (some-> response :body decode-token-error-response :error-description)) + +(defn- token-endpoint-valid-client-error? + "Token endpoint rejected the dummy auth code but accepted the client credentials." + [response] + (= "invalid_grant" (token-endpoint-error response))) + +(defn- token-endpoint-invalid-client-error? + "Token endpoint rejected the client credentials." + [{:keys [status] :as response}] + (let [error (token-endpoint-error response) + description (str/lower (or (token-endpoint-error-description response) ""))] + (or (= status 401) + (#{"invalid_client" "unauthorized_client"} error) + (and (= error "access_denied") + (str/includes? description "unauthorized"))))) + +(defn- probe-org-sso-client-credentials + "Probe the token endpoint with a dummy authorization code. + Valid client credentials are expected to answer with `invalid_grant`." + [cfg provider] + (let [params {:client_id (:client-id provider) + :client_secret (:client-secret provider) + :code probe-auth-code + :grant_type "authorization_code" + :redirect_uri (build-redirect-uri)} + req {:method :post + :headers {"content-type" "application/x-www-form-urlencoded" + "accept" "application/json"} + :uri (:token-uri provider) + :body (u/map->query-string params)} + response (http/req cfg req {:skip-ssrf-check? (:skip-ssrf-check? provider)})] + (cond + (token-endpoint-valid-client-error? response) true + (token-endpoint-invalid-client-error? response) false + :else false))) + +(defn is-organization-sso-config-valid? + "Return true when the SSO config can be discovered, can build a login URL, + and the client credentials are accepted by the token endpoint." + [cfg sso] + (try + (if (org-sso-discovery-uri sso) + (let [provider (prepare-org-sso-provider cfg sso)] + (and (build-org-sso-auth-redirect-uri cfg sso :provider provider) + (probe-org-sso-client-credentials cfg provider))) + false) + (catch Throwable _ false))) + (defn- auth-handler [cfg {:keys [params] :as request}] (let [provider (resolve-provider cfg params) diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index 9de442a48a..c1b63c9aff 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -88,7 +88,8 @@ #{:session-id :password :old-password - :token}) + :token + :client-secret}) (defn extract-utm-params "Extracts additional data from params and namespace them under diff --git a/backend/src/app/nitrate.clj b/backend/src/app/nitrate.clj index c99451d6df..1f54c47121 100644 --- a/backend/src/app/nitrate.clj +++ b/backend/src/app/nitrate.clj @@ -14,7 +14,8 @@ [app.common.schema :as sm] [app.common.schema.generators :as sg] [app.common.time :as ct] - [app.common.types.organization :as cto] + [app.common.types.organization :as cto + :refer [schema:nitrate-sso]] [app.common.uri :as u] [app.config :as cf] [app.http.client :as http] @@ -430,17 +431,6 @@ [:permissions [:map-of :keyword :string]]] params)) -(def ^:private schema:nitrate-sso - [:map - [:organization-id ::sm/uuid] - [:active [:maybe :boolean]] - [:provider [:maybe :string]] - [:client-id [:maybe :string]] - [:base-url [:maybe :string]] - [:client-secret [:maybe :string]] - [:issuer [:maybe :string]] - [:scopes [:maybe [::sm/set ::sm/text]]]]) - (defn- get-org-sso-api "Fetches the SSO configuration for an organization from Nitrate." [cfg {:keys [organization-id] :as params}] diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index 2103452c17..71a2e93b17 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -24,7 +24,6 @@ [app.rpc.nitrate.emails-helper :as neh] [app.rpc.nitrate.organization-helper :as noh] [app.rpc.notifications :as notifications] - [app.tokens :as tokens] [app.util.services :as sv])) @@ -647,17 +646,11 @@ {:keys [authorized sso]} (nitrate/sso-session-authorized? cfg organization-id team-id request)] (if authorized {:authorized true} - (if-let [issuer (or (:issuer sso) (:base-url sso))] - (let [oidc-provider (oidc/prepare-org-sso-provider cfg sso) - org-id (or organization-id (:organization-id sso)) - state-token (tokens/generate cfg {:iss "oidc" - :dest-url url - :organization-id org-id - :issuer issuer - :exp (ct/in-future "4h")}) - redirect-uri (oidc/build-auth-redirect-uri oidc-provider state-token)] - {:authorized false - :redirect-uri redirect-uri}) + (if (oidc/org-sso-discovery-uri sso) + {:authorized false + :redirect-uri (oidc/build-org-sso-auth-redirect-uri cfg sso + :dest-url url + :organization-id organization-id)} {:authorized false :redirect-uri nil}))) {:authorized true})) diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 9168c0ea63..700cf59cac 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -8,11 +8,12 @@ "Internal Nitrate HTTP RPC API. Provides authenticated access to organization management and token validation endpoints." (:require + [app.auth.oidc :as oidc] [app.common.data :as d] [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.time :as ct] - [app.common.types.organization :refer [schema:team-with-organization schema:organization-with-avatar]] + [app.common.types.organization :refer [schema:team-with-organization schema:organization-with-avatar schema:nitrate-sso]] [app.common.types.profile :refer [schema:profile, schema:basic-profile]] [app.common.types.team :refer [schema:team]] [app.config :as cf] @@ -878,6 +879,23 @@ RETURNING id, deleted_at;") photo-id (assoc :photo-url (files/resolve-public-uri photo-id)) owner-photo-id (assoc :owner-photo-url (files/resolve-public-uri owner-photo-id)))))))))))) +;; ---- API: check-organization-sso + +(def ^:private schema:check-organization-sso-result + [:map + [:valid ::sm/boolean]]) + +(sv/defmethod ::check-organization-sso + "Validate an organization SSO configuration by generating a login redirect URL. + Nitrate calls this while configuring SSO to verify client credentials and OIDC + discovery before saving the settings." + {::doc/added "2.20" + ::sm/params schema:nitrate-sso + ::sm/result schema:check-organization-sso-result + ::rpc/auth false} + [cfg params] + {:valid (oidc/is-organization-sso-config-valid? cfg params)}) + ;; ---- API: notify-org-sso-change (sv/defmethod ::notify-org-sso-change "Nitrate notifies that an organization sso values have changed" @@ -895,3 +913,4 @@ RETURNING id, deleted_at;") (when became-active (neh/send-organization-setup-sso-emails! cfg organization-id)) nil) + diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index bdf18e5541..7ec2f4bdaf 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -53,3 +53,20 @@ ;; not silently slip through as if it were the matching string. (t/is (= :auto (#'oidc/select-user-info-source :token))) (t/is (= :auto (#'oidc/select-user-info-source :userinfo))))) + +(t/deftest token-endpoint-errors-detect-valid-client-credentials + (let [response {:status 403 + :body "{\"error\":\"invalid_grant\",\"error_description\":\"Invalid authorization code\"}"}] + (t/is (#'oidc/token-endpoint-valid-client-error? response)) + (t/is (not (#'oidc/token-endpoint-invalid-client-error? response))))) + +(t/deftest token-endpoint-errors-detect-invalid-client-credentials + (t/is (#'oidc/token-endpoint-invalid-client-error? + {:status 401 + :body "{\"error\":\"access_denied\",\"error_description\":\"Unauthorized\"}"})) + (t/is (#'oidc/token-endpoint-invalid-client-error? + {:status 400 + :body "{\"error\":\"invalid_client\"}"})) + (t/is (not (#'oidc/token-endpoint-valid-client-error? + {:status 400 + :body "{\"error\":\"invalid_client\"}"})))) diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index 460471e639..2691514952 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -6,6 +6,7 @@ (ns backend-tests.rpc-management-nitrate-test (:require + [app.auth.oidc :as oidc] [app.common.data :as d] [app.common.time :as ct] [app.common.uuid :as uuid] @@ -1277,3 +1278,40 @@ (with-redefs [eml/send! (fn [params] (swap! sent conj params))] (management-command-with-nitrate! params)) (t/is (empty? @sent)))) + +(t/deftest check-organization-sso-returns-valid-true + (let [org-id (uuid/random) + out (with-redefs [oidc/is-organization-sso-config-valid? (constantly true)] + (management-command-with-nitrate! + {::th/type :check-organization-sso + :organization-id org-id + :client-id "test-client" + :client-secret "test-secret" + :base-url "https://idp.example.com"}))] + (t/is (th/success? out)) + (t/is (true? (-> out :result :valid))))) + +(t/deftest check-organization-sso-returns-valid-false-on-invalid-config + (let [out (management-command-with-nitrate! + {::th/type :check-organization-sso + :organization-id (uuid/random) + :client-id "test-client" + :client-secret "test-secret"})] + (t/is (th/success? out)) + (t/is (false? (-> out :result :valid))))) + +(t/deftest check-organization-sso-uses-issuer-when-base-url-is-blank + (let [org-id (uuid/random) + out (with-redefs [oidc/is-organization-sso-config-valid? + (fn [_cfg sso] + (and (= "test-client" (:client-id sso)) + (= "https://idp.example.com/" (:issuer sso))))] + (management-command-with-nitrate! + {::th/type :check-organization-sso + :organization-id org-id + :client-id "test-client" + :client-secret "test-secret" + :base-url "" + :issuer "https://idp.example.com/"}))] + (t/is (th/success? out)) + (t/is (true? (-> out :result :valid))))) diff --git a/common/src/app/common/types/organization.cljc b/common/src/app/common/types/organization.cljc index ca13e7de5b..16c5ab22aa 100644 --- a/common/src/app/common/types/organization.cljc +++ b/common/src/app/common/types/organization.cljc @@ -63,3 +63,14 @@ [:logo [:maybe ::sm/uri]] [:avatar-bg-url [:maybe ::sm/uri]] [:sso-active {:optional true} [:maybe :boolean]]]) + +(def schema:nitrate-sso + [:map {:title "NitrateOrganizationSso"} + [:organization-id ::sm/uuid] + [:active {:optional true} [:maybe :boolean]] + [:provider {:optional true} [:maybe :string]] + [:client-id {:optional true} [:maybe :string]] + [:base-url {:optional true} [:maybe :string]] + [:client-secret {:optional true} [:maybe :string]] + [:issuer {:optional true} [:maybe :string]] + [:scopes {:optional true} [:maybe [::sm/set ::sm/text]]]])