Skip to content
Snippets Groups Projects
Unverified Commit 6db14952 authored by Noah Moss's avatar Noah Moss Committed by GitHub
Browse files

Track SCIM users + API response success/error counts (#48702)

parent 6ccadee0
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/src/metabase_enterprise/scim/v2/api.clj
+ 150
− 122
View file @ 6db14952
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
(:require (:require
[compojure.core :refer [GET POST]] [compojure.core :refer [GET POST]]
[metabase-enterprise.scim.api :as scim] [metabase-enterprise.scim.api :as scim]
[metabase.analytics.prometheus :as prometheus]
[metabase.api.common :as api :refer [defendpoint]] [metabase.api.common :as api :refer [defendpoint]]
[metabase.models.interface :as mi] [metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group] [metabase.models.permissions-group :as perms-group]
...@@ -106,6 +107,22 @@ ...@@ -106,6 +107,22 @@
:body object :body object
:headers {"Content-Type" "application/scim+json"}}) :headers {"Content-Type" "application/scim+json"}})
(defn- do-with-prometheus-counters
[thunk]
(try
(let [response (thunk)]
(prometheus/inc! :metabase-scim/response-ok)
response)
(catch Throwable e
(prometheus/inc! :metabase-scim/response-error)
(throw e))))
(defmacro with-prometheus-counters
"Macro to wrap SCIM endpoints and automatically increment Prometheus counters to track success and error API
responses."
[& body]
`(do-with-prometheus-counters (fn [] ~@body)))
;;; +----------------------------------------------------------------------------------------------------------------+ ;;; +----------------------------------------------------------------------------------------------------------------+
;;; | User operations | ;;; | User operations |
;;; +----------------------------------------------------------------------------------------------------------------+ ;;; +----------------------------------------------------------------------------------------------------------------+
...@@ -163,7 +180,8 @@ ...@@ -163,7 +180,8 @@
:last_name familyName :last_name familyName
:email email :email email
:is_active is-active? :is_active is-active?
:type :personal} :type :personal
:sso_source "scim"}
(when (and locale (i18n/available-locale? locale)) (when (and locale (i18n/available-locale? locale))
{:locale locale})))) {:locale locale}))))
...@@ -188,98 +206,103 @@ ...@@ -188,98 +206,103 @@
{start-index [:maybe ms/PositiveInt] {start-index [:maybe ms/PositiveInt]
c [:maybe ms/PositiveInt] c [:maybe ms/PositiveInt]
filter-param [:maybe ms/NonBlankString]} filter-param [:maybe ms/NonBlankString]}
(let [limit (or c default-pagination-limit) (with-prometheus-counters
;; SCIM start-index is 1-indexed, so we need to decrement it here (let [limit (or c default-pagination-limit)
offset (if start-index (dec start-index) default-pagination-offset) ;; SCIM start-index is 1-indexed, so we need to decrement it here
filter-param (when filter-param (codec/url-decode filter-param)) offset (if start-index (dec start-index) default-pagination-offset)
where-clause [:and [:= :type "personal"] filter-param (when filter-param (codec/url-decode filter-param))
(when filter-param (user-filter-clause filter-param))] where-clause [:and [:= :type "personal"]
users (t2/select (cons :model/User user-cols) (when filter-param (user-filter-clause filter-param))]
{:where where-clause users (t2/select (cons :model/User user-cols)
:limit limit {:where where-clause
:offset offset :limit limit
:order-by [[:id :asc]]}) :offset offset
hydrated-users (t2/hydrate users :scim_user_group_memberships) :order-by [[:id :asc]]})
results-count (count hydrated-users) hydrated-users (t2/hydrate users :scim_user_group_memberships)
items-per-page (if (< results-count limit) results-count limit) results-count (count hydrated-users)
result {:schemas [list-schema-uri] items-per-page (if (< results-count limit) results-count limit)
:totalResults (t2/count :model/User {:where where-clause}) result {:schemas [list-schema-uri]
:startIndex (inc offset) :totalResults (t2/count :model/User {:where where-clause})
:itemsPerPage items-per-page :startIndex (inc offset)
:Resources (map mb-user->scim hydrated-users)}] :itemsPerPage items-per-page
(scim-response result))) :Resources (map mb-user->scim hydrated-users)}]
(scim-response result))))
(defendpoint GET "/Users/:id" (defendpoint GET "/Users/:id"
"Fetch a single user." "Fetch a single user."
[id] [id]
{id ms/NonBlankString} {id ms/NonBlankString}
(-> (get-user-by-entity-id id) (with-prometheus-counters
(t2/hydrate :scim_user_group_memberships) (-> (get-user-by-entity-id id)
mb-user->scim)) (t2/hydrate :scim_user_group_memberships)
mb-user->scim)))
(defendpoint POST "/Users" (defendpoint POST "/Users"
"Create a single user." "Create a single user."
[:as {scim-user :body}] [:as {scim-user :body}]
{scim-user SCIMUser} {scim-user SCIMUser}
(let [mb-user (scim-user->mb scim-user) (with-prometheus-counters
email (:email mb-user)] (let [mb-user (scim-user->mb scim-user)
(when (t2/exists? :model/User :%lower.email (u/lower-case-en email)) email (:email mb-user)]
(throw-scim-error 409 "Email address is already in use")) (when (t2/exists? :model/User :%lower.email (u/lower-case-en email))
(let [new-user (t2/with-transaction [_] (throw-scim-error 409 "Email address is already in use"))
(user/insert-new-user! mb-user) (let [new-user (t2/with-transaction [_]
(-> (t2/select-one (cons :model/User user-cols) (user/insert-new-user! mb-user)
:email (u/lower-case-en email)) (-> (t2/select-one (cons :model/User user-cols)
mb-user->scim))] :email (u/lower-case-en email))
(scim-response new-user 201)))) mb-user->scim))]
(scim-response new-user 201)))))
(defendpoint PUT "/Users/:id" (defendpoint PUT "/Users/:id"
"Update a user." "Update a user."
[:as {scim-user :body {id :id} :params}] [:as {scim-user :body {id :id} :params}]
{scim-user SCIMUser} {scim-user SCIMUser}
(let [updates (scim-user->mb scim-user) (with-prometheus-counters
email (-> scim-user :emails first :value) (let [updates (scim-user->mb scim-user)
current-user (get-user-by-entity-id id)] email (-> scim-user :emails first :value)
(if (not= email (:email current-user)) current-user (get-user-by-entity-id id)]
(throw-scim-error 400 "You may not update the email of an existing user.") (if (not= email (:email current-user))
(try (throw-scim-error 400 "You may not update the email of an existing user.")
(t2/with-transaction [_conn] (try
(t2/update! :model/User (u/the-id current-user) updates) (t2/with-transaction [_conn]
(let [user (-> (t2/select-one (cons :model/User user-cols) (t2/update! :model/User (u/the-id current-user) updates)
:entity_id id) (let [user (-> (t2/select-one (cons :model/User user-cols)
mb-user->scim)] :entity_id id)
(scim-response user))) mb-user->scim)]
(catch Exception e (scim-response user)))
(let [message (format "Error updating user: %s" (ex-message e))] (catch Exception e
(throw (ex-info message (let [message (format "Error updating user: %s" (ex-message e))]
{:schemas [error-schema-uri] (throw (ex-info message
:detail message {:schemas [error-schema-uri]
:status 400 :detail message
:status-code 400})))))))) :status 400
:status-code 400})))))))))
(defendpoint PATCH "/Users/:id" (defendpoint PATCH "/Users/:id"
"Activate or deactivate a user. Supports specific replace operations, but not arbitrary patches." "Activate or deactivate a user. Supports specific replace operations, but not arbitrary patches."
[:as {patch-ops :body {id :id} :params}] [:as {patch-ops :body {id :id} :params}]
{patch-ops UserPatch} {patch-ops UserPatch}
{id ms/NonBlankString} {id ms/NonBlankString}
(t2/with-transaction [_conn] (with-prometheus-counters
(let [user (get-user-by-entity-id id) (t2/with-transaction [_conn]
updates (reduce (let [user (get-user-by-entity-id id)
(fn [acc operation] updates (reduce
(let [{:keys [op path value]} operation] (fn [acc operation]
(if (= (u/lower-case-en op) "replace") (let [{:keys [op path value]} operation]
(case path (if (= (u/lower-case-en op) "replace")
"active" (assoc acc :is_active (Boolean/valueOf (u/lower-case-en value))) (case path
"userName" (assoc acc :email value) "active" (assoc acc :is_active (Boolean/valueOf (u/lower-case-en value)))
"name.givenName" (assoc acc :first_name value) "userName" (assoc acc :email value)
"name.familyName" (assoc acc :last_name value) "name.givenName" (assoc acc :first_name value)
(throw-scim-error 400 (format "Unsupported path: %s" path))) "name.familyName" (assoc acc :last_name value)
acc))) (throw-scim-error 400 (format "Unsupported path: %s" path)))
{} acc)))
(:Operations patch-ops))] {}
(t2/update! :model/User (u/the-id user) updates) (:Operations patch-ops))]
(-> (get-user-by-entity-id id) (t2/update! :model/User (u/the-id user) updates)
mb-user->scim (-> (get-user-by-entity-id id)
scim-response)))) mb-user->scim
scim-response)))))
;;; +----------------------------------------------------------------------------------------------------------------+ ;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Group operations | ;;; | Group operations |
...@@ -344,35 +367,37 @@ ...@@ -344,35 +367,37 @@
{start-index [:maybe ms/PositiveInt] {start-index [:maybe ms/PositiveInt]
c [:maybe ms/PositiveInt] c [:maybe ms/PositiveInt]
filter-param [:maybe ms/NonBlankString]} filter-param [:maybe ms/NonBlankString]}
(let [limit (or c default-pagination-limit) (with-prometheus-counters
;; SCIM start-index is 1-indexed, so we need to decrement it here (let [limit (or c default-pagination-limit)
offset (if start-index (dec start-index) default-pagination-offset) ;; SCIM start-index is 1-indexed, so we need to decrement it here
filter-param (when filter-param (codec/url-decode filter-param)) offset (if start-index (dec start-index) default-pagination-offset)
where-clause [:and filter-param (when filter-param (codec/url-decode filter-param))
[:not= :id (:id perms-group/all-users)] where-clause [:and
[:not= :id (:id perms-group/admin)] [:not= :id (:id perms-group/all-users)]
(when filter-param (group-filter-clause filter-param))] [:not= :id (:id perms-group/admin)]
groups (t2/select (cons :model/PermissionsGroup group-cols) (when filter-param (group-filter-clause filter-param))]
{:where where-clause groups (t2/select (cons :model/PermissionsGroup group-cols)
:limit limit {:where where-clause
:offset offset :limit limit
:order-by [[:id :asc]]}) :offset offset
results-count (count groups) :order-by [[:id :asc]]})
items-per-page (if (< results-count limit) results-count limit) results-count (count groups)
result {:schemas [list-schema-uri] items-per-page (if (< results-count limit) results-count limit)
:totalResults (t2/count :model/PermissionsGroup {:where where-clause}) result {:schemas [list-schema-uri]
:startIndex (inc offset) :totalResults (t2/count :model/PermissionsGroup {:where where-clause})
:itemsPerPage items-per-page :startIndex (inc offset)
:Resources (map mb-group->scim groups)}] :itemsPerPage items-per-page
(scim-response result))) :Resources (map mb-group->scim groups)}]
(scim-response result))))
(defendpoint GET "/Groups/:id" (defendpoint GET "/Groups/:id"
"Fetch a single group." "Fetch a single group."
[id] [id]
{id ms/NonBlankString} {id ms/NonBlankString}
(-> (get-group-by-entity-id id) (with-prometheus-counters
(t2/hydrate :scim_group_members) (-> (get-group-by-entity-id id)
mb-group->scim)) (t2/hydrate :scim_group_members)
mb-group->scim)))
(defn- update-group-membership (defn- update-group-membership
"Updates the membership of `group-id` to be the set of users in the collection `user-entity-ids`. Clears "Updates the membership of `group-id` to be the set of users in the collection `user-entity-ids`. Clears
...@@ -389,41 +414,44 @@ ...@@ -389,41 +414,44 @@
"Create a single group, and populates it if necessary." "Create a single group, and populates it if necessary."
[:as {scim-group :body}] [:as {scim-group :body}]
{scim-group SCIMGroup} {scim-group SCIMGroup}
(let [group-name (:displayName scim-group) (with-prometheus-counters
entity-ids (map :value (:members scim-group))] (let [group-name (:displayName scim-group)
(when (t2/exists? :model/PermissionsGroup :%lower.name (u/lower-case-en group-name)) entity-ids (map :value (:members scim-group))]
(throw-scim-error 409 "A group with that name already exists")) (when (t2/exists? :model/PermissionsGroup :%lower.name (u/lower-case-en group-name))
(t2/with-transaction [_conn] (throw-scim-error 409 "A group with that name already exists"))
(let [new-group (first (t2/insert-returning-instances! :model/PermissionsGroup {:name group-name}))] (t2/with-transaction [_conn]
(when (seq entity-ids) (let [new-group (first (t2/insert-returning-instances! :model/PermissionsGroup {:name group-name}))]
(update-group-membership (:id new-group) entity-ids)) (when (seq entity-ids)
(-> new-group (update-group-membership (:id new-group) entity-ids))
(t2/hydrate :scim_group_members) (-> new-group
mb-group->scim (t2/hydrate :scim_group_members)
(scim-response 201)))))) mb-group->scim
(scim-response 201)))))))
(defendpoint PUT "/Groups/:id" (defendpoint PUT "/Groups/:id"
"Update a group." "Update a group."
[:as {scim-group :body {id :id} :params}] [:as {scim-group :body {id :id} :params}]
{scim-group SCIMGroup} {scim-group SCIMGroup}
(let [group-name (:displayName scim-group) (with-prometheus-counters
entity-ids (map :value (:members scim-group))] (let [group-name (:displayName scim-group)
(t2/with-transaction [_conn] entity-ids (map :value (:members scim-group))]
(let [group (get-group-by-entity-id id)] (t2/with-transaction [_conn]
(t2/update! :model/PermissionsGroup (u/the-id group) {:name group-name}) (let [group (get-group-by-entity-id id)]
(when (seq entity-ids) (t2/update! :model/PermissionsGroup (u/the-id group) {:name group-name})
(update-group-membership (u/the-id group) entity-ids)) (when (seq entity-ids)
(-> (get-group-by-entity-id id) (update-group-membership (u/the-id group) entity-ids))
(t2/hydrate :scim_group_members) (-> (get-group-by-entity-id id)
mb-group->scim (t2/hydrate :scim_group_members)
scim-response))))) mb-group->scim
scim-response))))))
(defendpoint DELETE "/Groups/:id" (defendpoint DELETE "/Groups/:id"
"Delete a group." "Delete a group."
[id] [id]
{id ms/NonBlankString} {id ms/NonBlankString}
(let [group (get-group-by-entity-id id)] (with-prometheus-counters
(t2/delete! :model/PermissionsGroup (u/the-id group)) (let [group (get-group-by-entity-id id)]
(scim-response nil 204))) (t2/delete! :model/PermissionsGroup (u/the-id group))
(scim-response nil 204))))
(api/define-routes) (api/define-routes)
enterprise/backend/test/metabase_enterprise/scim/v2/api_test.clj
+ 26
− 2
View file @ 6db14952
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
[clojure.test :refer :all] [clojure.test :refer :all]
[metabase-enterprise.scim.api :as scim] [metabase-enterprise.scim.api :as scim]
[metabase-enterprise.scim.v2.api :as scim-api] [metabase-enterprise.scim.v2.api :as scim-api]
[metabase.analytics.prometheus :as prometheus]
[metabase.http-client :as client] [metabase.http-client :as client]
[metabase.models.permissions-group :as perms-group] [metabase.models.permissions-group :as perms-group]
[metabase.test :as mt] [metabase.test :as mt]
...@@ -64,6 +65,28 @@ ...@@ -64,6 +65,28 @@
(testing "A SCIM API key cannot be passed via the x-api-key header" (testing "A SCIM API key cannot be passed via the x-api-key header"
(client/client :get 401 "ee/scim/v2/Users" {:request-options {:headers {"x-api-key" *scim-api-key*}}})))) (client/client :get 401 "ee/scim/v2/Users" {:request-options {:headers {"x-api-key" *scim-api-key*}}}))))
(deftest prometheus-metrics-test
(testing "Prometheus counters get incremented for success responses and errors"
(with-scim-setup!
(let [calls (atom nil)]
(with-redefs [prometheus/inc! #(swap! calls conj %)]
(testing "Success response"
(scim-client :get 200 "ee/scim/v2/Users")
(is (= 1 (count (filter #{:metabase-scim/response-ok} @calls))))
(is (= 0 (count (filter #{:metabase-scim/response-error} @calls)))))
(testing "Bad request (400)"
(scim-client :get 400 (format "ee/scim/v2/Users?filter=%s"
(codec/url-encode "id ne \"newuser@metabase.com\"")))
(is (= 1 (count (filter #{:metabase-scim/response-ok} @calls))))
(is (= 1 (count (filter #{:metabase-scim/response-error} @calls)))))
(testing "Unexpected server error (500)"
(with-redefs [scim-api/scim-response #(throw (Exception.))]
(scim-client :get 500 "ee/scim/v2/Users")
(is (= 1 (count (filter #{:metabase-scim/response-ok} @calls))))
(is (= 2 (count (filter #{:metabase-scim/response-error} @calls)))))))))))
(deftest fetch-user-test (deftest fetch-user-test
(with-scim-setup! (with-scim-setup!
(testing "A single user can be fetched in the SCIM format by entity ID with its groups" (testing "A single user can be fetched in the SCIM format by entity ID with its groups"
...@@ -142,8 +165,9 @@ ...@@ -142,8 +165,9 @@
:active true} :active true}
response (scim-client :post 201 "ee/scim/v2/Users" new-user)] response (scim-client :post 201 "ee/scim/v2/Users" new-user)]
(is (malli= scim-api/SCIMUser response)) (is (malli= scim-api/SCIMUser response))
(is (=? (select-keys user [:email :first_name :last_name :is_active]) (is (=? (assoc (select-keys user [:email :first_name :last_name :is_active])
(t2/select-one [:model/User :email :first_name :last_name :is_active] :sso_source :scim)
(t2/select-one [:model/User :email :first_name :last_name :is_active :sso_source]
:entity_id (:id response))))) :entity_id (:id response)))))
(finally (t2/delete! :model/User :email (:email user)))))) (finally (t2/delete! :model/User :email (:email user))))))
......
src/metabase/analytics/prometheus.clj
+ 4
− 0
View file @ 6db14952
...@@ -203,6 +203,10 @@ ...@@ -203,6 +203,10 @@
{:description (trs "Number of successful SDK requests.")}) {:description (trs "Number of successful SDK requests.")})
(prometheus/counter :metabase-sdk/response-error (prometheus/counter :metabase-sdk/response-error
{:description (trs "Number of errors when responding to SDK requests.")}) {:description (trs "Number of errors when responding to SDK requests.")})
(prometheus/counter :metabase-scim/response-ok
{:description (trs "Number of successful responses from SCIM endpoints")})
(prometheus/counter :metabase-scim/response-error
{:description (trs "Number of error responses from SCIM endpoints")})
(prometheus/counter :metabase-query-processor/metrics (prometheus/counter :metabase-query-processor/metrics
{:description (trs "Number of queries consuming metrics processed by the query processor.")}) {:description (trs "Number of queries consuming metrics processed by the query processor.")})
(prometheus/counter :metabase-query-processor/metric-errors (prometheus/counter :metabase-query-processor/metric-errors
......
src/metabase/analytics/stats.clj
+ 14
− 11
View file @ 6db14952
...@@ -633,17 +633,20 @@ ...@@ -633,17 +633,20 @@
(let [one-day-ago (->one-day-ago) (let [one-day-ago (->one-day-ago)
total-translation-count (:total (get-translation-count)) total-translation-count (:total (get-translation-count))
_ (clear-translation-count!)] _ (clear-translation-count!)]
{:models (t2/count :model/Card :type :model :archived false) {:models (t2/count :model/Card :type :model :archived false)
:new_embedded_dashboards (t2/count :model/Dashboard :new_embedded_dashboards (t2/count :model/Dashboard
:enable_embedding true :enable_embedding true
:archived false :archived false
:created_at [:>= one-day-ago]) :created_at [:>= one-day-ago])
:new_users_last_24h (t2/count :model/User :new_users_last_24h (t2/count :model/User
:is_active true :is_active true
:date_joined [:>= one-day-ago]) :date_joined [:>= one-day-ago])
:pivot_tables (t2/count :model/Card :display :pivot :archived false) :pivot_tables (t2/count :model/Card :display :pivot :archived false)
:query_executions_last_24h (t2/count :model/QueryExecution :started_at [:>= one-day-ago]) :query_executions_last_24h (t2/count :model/QueryExecution :started_at [:>= one-day-ago])
:entity_id_translations_last_24h total-translation-count})) :entity_id_translations_last_24h total-translation-count
:scim_users_last_24h (t2/count :model/User :sso_source :scim
:is_active true
:date_joined [:>= one-day-ago])}))
(mu/defn- snowplow-metrics (mu/defn- snowplow-metrics
[stats metric-info :- [:map [stats metric-info :- [:map
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment