From 6db14952636e507835ac23723a4e58ce2adc4be2 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Thu, 17 Oct 2024 18:23:34 -0400
Subject: [PATCH] Track SCIM users + API response success/error counts (#48702)
---
.../src/metabase_enterprise/scim/v2/api.clj | 272 ++++++++++--------
.../metabase_enterprise/scim/v2/api_test.clj | 28 +-
src/metabase/analytics/prometheus.clj | 4 +
src/metabase/analytics/stats.clj | 25 +-
4 files changed, 194 insertions(+), 135 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/scim/v2/api.clj b/enterprise/backend/src/metabase_enterprise/scim/v2/api.clj
index 24c8a0ac93f..dff41dec686 100644
--- a/enterprise/backend/src/metabase_enterprise/scim/v2/api.clj
+++ b/enterprise/backend/src/metabase_enterprise/scim/v2/api.clj
@@ -6,6 +6,7 @@
(:require
[compojure.core :refer [GET POST]]
[metabase-enterprise.scim.api :as scim]
+ [metabase.analytics.prometheus :as prometheus]
[metabase.api.common :as api :refer [defendpoint]]
[metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
@@ -106,6 +107,22 @@
:body object
: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 |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -163,7 +180,8 @@
:last_name familyName
:email email
:is_active is-active?
- :type :personal}
+ :type :personal
+ :sso_source "scim"}
(when (and locale (i18n/available-locale? locale))
{:locale locale}))))
@@ -188,98 +206,103 @@
{start-index [:maybe ms/PositiveInt]
c [:maybe ms/PositiveInt]
filter-param [:maybe ms/NonBlankString]}
- (let [limit (or c default-pagination-limit)
- ;; SCIM start-index is 1-indexed, so we need to decrement it here
- offset (if start-index (dec start-index) default-pagination-offset)
- filter-param (when filter-param (codec/url-decode filter-param))
- where-clause [:and [:= :type "personal"]
- (when filter-param (user-filter-clause filter-param))]
- users (t2/select (cons :model/User user-cols)
- {:where where-clause
- :limit limit
- :offset offset
- :order-by [[:id :asc]]})
- hydrated-users (t2/hydrate users :scim_user_group_memberships)
- results-count (count hydrated-users)
- items-per-page (if (< results-count limit) results-count limit)
- result {:schemas [list-schema-uri]
- :totalResults (t2/count :model/User {:where where-clause})
- :startIndex (inc offset)
- :itemsPerPage items-per-page
- :Resources (map mb-user->scim hydrated-users)}]
- (scim-response result)))
+ (with-prometheus-counters
+ (let [limit (or c default-pagination-limit)
+ ;; SCIM start-index is 1-indexed, so we need to decrement it here
+ offset (if start-index (dec start-index) default-pagination-offset)
+ filter-param (when filter-param (codec/url-decode filter-param))
+ where-clause [:and [:= :type "personal"]
+ (when filter-param (user-filter-clause filter-param))]
+ users (t2/select (cons :model/User user-cols)
+ {:where where-clause
+ :limit limit
+ :offset offset
+ :order-by [[:id :asc]]})
+ hydrated-users (t2/hydrate users :scim_user_group_memberships)
+ results-count (count hydrated-users)
+ items-per-page (if (< results-count limit) results-count limit)
+ result {:schemas [list-schema-uri]
+ :totalResults (t2/count :model/User {:where where-clause})
+ :startIndex (inc offset)
+ :itemsPerPage items-per-page
+ :Resources (map mb-user->scim hydrated-users)}]
+ (scim-response result))))
(defendpoint GET "/Users/:id"
"Fetch a single user."
[id]
{id ms/NonBlankString}
- (-> (get-user-by-entity-id id)
- (t2/hydrate :scim_user_group_memberships)
- mb-user->scim))
+ (with-prometheus-counters
+ (-> (get-user-by-entity-id id)
+ (t2/hydrate :scim_user_group_memberships)
+ mb-user->scim)))
(defendpoint POST "/Users"
"Create a single user."
[:as {scim-user :body}]
{scim-user SCIMUser}
- (let [mb-user (scim-user->mb scim-user)
- email (:email mb-user)]
- (when (t2/exists? :model/User :%lower.email (u/lower-case-en email))
- (throw-scim-error 409 "Email address is already in use"))
- (let [new-user (t2/with-transaction [_]
- (user/insert-new-user! mb-user)
- (-> (t2/select-one (cons :model/User user-cols)
- :email (u/lower-case-en email))
- mb-user->scim))]
- (scim-response new-user 201))))
+ (with-prometheus-counters
+ (let [mb-user (scim-user->mb scim-user)
+ email (:email mb-user)]
+ (when (t2/exists? :model/User :%lower.email (u/lower-case-en email))
+ (throw-scim-error 409 "Email address is already in use"))
+ (let [new-user (t2/with-transaction [_]
+ (user/insert-new-user! mb-user)
+ (-> (t2/select-one (cons :model/User user-cols)
+ :email (u/lower-case-en email))
+ mb-user->scim))]
+ (scim-response new-user 201)))))
(defendpoint PUT "/Users/:id"
"Update a user."
[:as {scim-user :body {id :id} :params}]
{scim-user SCIMUser}
- (let [updates (scim-user->mb scim-user)
- email (-> scim-user :emails first :value)
- current-user (get-user-by-entity-id id)]
- (if (not= email (:email current-user))
- (throw-scim-error 400 "You may not update the email of an existing user.")
- (try
- (t2/with-transaction [_conn]
- (t2/update! :model/User (u/the-id current-user) updates)
- (let [user (-> (t2/select-one (cons :model/User user-cols)
- :entity_id id)
- mb-user->scim)]
- (scim-response user)))
- (catch Exception e
- (let [message (format "Error updating user: %s" (ex-message e))]
- (throw (ex-info message
- {:schemas [error-schema-uri]
- :detail message
- :status 400
- :status-code 400}))))))))
+ (with-prometheus-counters
+ (let [updates (scim-user->mb scim-user)
+ email (-> scim-user :emails first :value)
+ current-user (get-user-by-entity-id id)]
+ (if (not= email (:email current-user))
+ (throw-scim-error 400 "You may not update the email of an existing user.")
+ (try
+ (t2/with-transaction [_conn]
+ (t2/update! :model/User (u/the-id current-user) updates)
+ (let [user (-> (t2/select-one (cons :model/User user-cols)
+ :entity_id id)
+ mb-user->scim)]
+ (scim-response user)))
+ (catch Exception e
+ (let [message (format "Error updating user: %s" (ex-message e))]
+ (throw (ex-info message
+ {:schemas [error-schema-uri]
+ :detail message
+ :status 400
+ :status-code 400})))))))))
(defendpoint PATCH "/Users/:id"
"Activate or deactivate a user. Supports specific replace operations, but not arbitrary patches."
[:as {patch-ops :body {id :id} :params}]
{patch-ops UserPatch}
{id ms/NonBlankString}
- (t2/with-transaction [_conn]
- (let [user (get-user-by-entity-id id)
- updates (reduce
- (fn [acc operation]
- (let [{:keys [op path value]} operation]
- (if (= (u/lower-case-en op) "replace")
- (case path
- "active" (assoc acc :is_active (Boolean/valueOf (u/lower-case-en value)))
- "userName" (assoc acc :email value)
- "name.givenName" (assoc acc :first_name value)
- "name.familyName" (assoc acc :last_name value)
- (throw-scim-error 400 (format "Unsupported path: %s" path)))
- acc)))
- {}
- (:Operations patch-ops))]
- (t2/update! :model/User (u/the-id user) updates)
- (-> (get-user-by-entity-id id)
- mb-user->scim
- scim-response))))
+ (with-prometheus-counters
+ (t2/with-transaction [_conn]
+ (let [user (get-user-by-entity-id id)
+ updates (reduce
+ (fn [acc operation]
+ (let [{:keys [op path value]} operation]
+ (if (= (u/lower-case-en op) "replace")
+ (case path
+ "active" (assoc acc :is_active (Boolean/valueOf (u/lower-case-en value)))
+ "userName" (assoc acc :email value)
+ "name.givenName" (assoc acc :first_name value)
+ "name.familyName" (assoc acc :last_name value)
+ (throw-scim-error 400 (format "Unsupported path: %s" path)))
+ acc)))
+ {}
+ (:Operations patch-ops))]
+ (t2/update! :model/User (u/the-id user) updates)
+ (-> (get-user-by-entity-id id)
+ mb-user->scim
+ scim-response)))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Group operations |
@@ -344,35 +367,37 @@
{start-index [:maybe ms/PositiveInt]
c [:maybe ms/PositiveInt]
filter-param [:maybe ms/NonBlankString]}
- (let [limit (or c default-pagination-limit)
- ;; SCIM start-index is 1-indexed, so we need to decrement it here
- offset (if start-index (dec start-index) default-pagination-offset)
- filter-param (when filter-param (codec/url-decode filter-param))
- where-clause [:and
- [:not= :id (:id perms-group/all-users)]
- [:not= :id (:id perms-group/admin)]
- (when filter-param (group-filter-clause filter-param))]
- groups (t2/select (cons :model/PermissionsGroup group-cols)
- {:where where-clause
- :limit limit
- :offset offset
- :order-by [[:id :asc]]})
- results-count (count groups)
- items-per-page (if (< results-count limit) results-count limit)
- result {:schemas [list-schema-uri]
- :totalResults (t2/count :model/PermissionsGroup {:where where-clause})
- :startIndex (inc offset)
- :itemsPerPage items-per-page
- :Resources (map mb-group->scim groups)}]
- (scim-response result)))
+ (with-prometheus-counters
+ (let [limit (or c default-pagination-limit)
+ ;; SCIM start-index is 1-indexed, so we need to decrement it here
+ offset (if start-index (dec start-index) default-pagination-offset)
+ filter-param (when filter-param (codec/url-decode filter-param))
+ where-clause [:and
+ [:not= :id (:id perms-group/all-users)]
+ [:not= :id (:id perms-group/admin)]
+ (when filter-param (group-filter-clause filter-param))]
+ groups (t2/select (cons :model/PermissionsGroup group-cols)
+ {:where where-clause
+ :limit limit
+ :offset offset
+ :order-by [[:id :asc]]})
+ results-count (count groups)
+ items-per-page (if (< results-count limit) results-count limit)
+ result {:schemas [list-schema-uri]
+ :totalResults (t2/count :model/PermissionsGroup {:where where-clause})
+ :startIndex (inc offset)
+ :itemsPerPage items-per-page
+ :Resources (map mb-group->scim groups)}]
+ (scim-response result))))
(defendpoint GET "/Groups/:id"
"Fetch a single group."
[id]
{id ms/NonBlankString}
- (-> (get-group-by-entity-id id)
- (t2/hydrate :scim_group_members)
- mb-group->scim))
+ (with-prometheus-counters
+ (-> (get-group-by-entity-id id)
+ (t2/hydrate :scim_group_members)
+ mb-group->scim)))
(defn- update-group-membership
"Updates the membership of `group-id` to be the set of users in the collection `user-entity-ids`. Clears
@@ -389,41 +414,44 @@
"Create a single group, and populates it if necessary."
[:as {scim-group :body}]
{scim-group SCIMGroup}
- (let [group-name (:displayName scim-group)
- entity-ids (map :value (:members scim-group))]
- (when (t2/exists? :model/PermissionsGroup :%lower.name (u/lower-case-en group-name))
- (throw-scim-error 409 "A group with that name already exists"))
- (t2/with-transaction [_conn]
- (let [new-group (first (t2/insert-returning-instances! :model/PermissionsGroup {:name group-name}))]
- (when (seq entity-ids)
- (update-group-membership (:id new-group) entity-ids))
- (-> new-group
- (t2/hydrate :scim_group_members)
- mb-group->scim
- (scim-response 201))))))
+ (with-prometheus-counters
+ (let [group-name (:displayName scim-group)
+ entity-ids (map :value (:members scim-group))]
+ (when (t2/exists? :model/PermissionsGroup :%lower.name (u/lower-case-en group-name))
+ (throw-scim-error 409 "A group with that name already exists"))
+ (t2/with-transaction [_conn]
+ (let [new-group (first (t2/insert-returning-instances! :model/PermissionsGroup {:name group-name}))]
+ (when (seq entity-ids)
+ (update-group-membership (:id new-group) entity-ids))
+ (-> new-group
+ (t2/hydrate :scim_group_members)
+ mb-group->scim
+ (scim-response 201)))))))
(defendpoint PUT "/Groups/:id"
"Update a group."
[:as {scim-group :body {id :id} :params}]
{scim-group SCIMGroup}
- (let [group-name (:displayName scim-group)
- entity-ids (map :value (:members scim-group))]
- (t2/with-transaction [_conn]
- (let [group (get-group-by-entity-id id)]
- (t2/update! :model/PermissionsGroup (u/the-id group) {:name group-name})
- (when (seq entity-ids)
- (update-group-membership (u/the-id group) entity-ids))
- (-> (get-group-by-entity-id id)
- (t2/hydrate :scim_group_members)
- mb-group->scim
- scim-response)))))
+ (with-prometheus-counters
+ (let [group-name (:displayName scim-group)
+ entity-ids (map :value (:members scim-group))]
+ (t2/with-transaction [_conn]
+ (let [group (get-group-by-entity-id id)]
+ (t2/update! :model/PermissionsGroup (u/the-id group) {:name group-name})
+ (when (seq entity-ids)
+ (update-group-membership (u/the-id group) entity-ids))
+ (-> (get-group-by-entity-id id)
+ (t2/hydrate :scim_group_members)
+ mb-group->scim
+ scim-response))))))
(defendpoint DELETE "/Groups/:id"
"Delete a group."
[id]
{id ms/NonBlankString}
- (let [group (get-group-by-entity-id id)]
- (t2/delete! :model/PermissionsGroup (u/the-id group))
- (scim-response nil 204)))
+ (with-prometheus-counters
+ (let [group (get-group-by-entity-id id)]
+ (t2/delete! :model/PermissionsGroup (u/the-id group))
+ (scim-response nil 204))))
(api/define-routes)
diff --git a/enterprise/backend/test/metabase_enterprise/scim/v2/api_test.clj b/enterprise/backend/test/metabase_enterprise/scim/v2/api_test.clj
index 20dd7d6ca68..8e7dbc1382e 100644
--- a/enterprise/backend/test/metabase_enterprise/scim/v2/api_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/scim/v2/api_test.clj
@@ -3,6 +3,7 @@
[clojure.test :refer :all]
[metabase-enterprise.scim.api :as scim]
[metabase-enterprise.scim.v2.api :as scim-api]
+ [metabase.analytics.prometheus :as prometheus]
[metabase.http-client :as client]
[metabase.models.permissions-group :as perms-group]
[metabase.test :as mt]
@@ -64,6 +65,28 @@
(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*}}}))))
+(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
(with-scim-setup!
(testing "A single user can be fetched in the SCIM format by entity ID with its groups"
@@ -142,8 +165,9 @@
:active true}
response (scim-client :post 201 "ee/scim/v2/Users" new-user)]
(is (malli= scim-api/SCIMUser response))
- (is (=? (select-keys user [:email :first_name :last_name :is_active])
- (t2/select-one [:model/User :email :first_name :last_name :is_active]
+ (is (=? (assoc (select-keys 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)))))
(finally (t2/delete! :model/User :email (:email user))))))
diff --git a/src/metabase/analytics/prometheus.clj b/src/metabase/analytics/prometheus.clj
index d427d039df3..0279513c344 100644
--- a/src/metabase/analytics/prometheus.clj
+++ b/src/metabase/analytics/prometheus.clj
@@ -203,6 +203,10 @@
{:description (trs "Number of successful SDK requests.")})
(prometheus/counter :metabase-sdk/response-error
{: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
{:description (trs "Number of queries consuming metrics processed by the query processor.")})
(prometheus/counter :metabase-query-processor/metric-errors
diff --git a/src/metabase/analytics/stats.clj b/src/metabase/analytics/stats.clj
index e8f378fb556..d87b9fc82c3 100644
--- a/src/metabase/analytics/stats.clj
+++ b/src/metabase/analytics/stats.clj
@@ -633,17 +633,20 @@
(let [one-day-ago (->one-day-ago)
total-translation-count (:total (get-translation-count))
_ (clear-translation-count!)]
- {:models (t2/count :model/Card :type :model :archived false)
- :new_embedded_dashboards (t2/count :model/Dashboard
- :enable_embedding true
- :archived false
- :created_at [:>= one-day-ago])
- :new_users_last_24h (t2/count :model/User
- :is_active true
- :date_joined [:>= one-day-ago])
- :pivot_tables (t2/count :model/Card :display :pivot :archived false)
- :query_executions_last_24h (t2/count :model/QueryExecution :started_at [:>= one-day-ago])
- :entity_id_translations_last_24h total-translation-count}))
+ {:models (t2/count :model/Card :type :model :archived false)
+ :new_embedded_dashboards (t2/count :model/Dashboard
+ :enable_embedding true
+ :archived false
+ :created_at [:>= one-day-ago])
+ :new_users_last_24h (t2/count :model/User
+ :is_active true
+ :date_joined [:>= one-day-ago])
+ :pivot_tables (t2/count :model/Card :display :pivot :archived false)
+ :query_executions_last_24h (t2/count :model/QueryExecution :started_at [:>= one-day-ago])
+ :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
[stats metric-info :- [:map
--
GitLab