Skip to content

Commit

Permalink
Do not create personal collections for API keys (metabase#48638)
Browse files Browse the repository at this point in the history 
* Do not create personal collections for API keys

Co-authored-by: Noah Moss <[email protected]>
  • Loading branch information
@johnswanson @noahmoss
johnswanson and noahmoss authored Oct 23, 2024
1 parent d4bfd14 commit 3a05e07
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 45 deletions.
17 changes: 16 additions & 1 deletion src/metabase/models/api_key.clj
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
(ns metabase.models.api-key
(:require [crypto.random :as crypto-random]
(:require [clojure.core.memoize :as memoize]
[crypto.random :as crypto-random]
[metabase.db :as mdb]
[metabase.models.audit-log :as audit-log]
[metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
Expand Down Expand Up @@ -108,3 +110,16 @@
(defmethod audit-log/model-details :model/ApiKey
[entity _event-type]
(select-keys entity [:name :group :key_prefix :user_id]))

(def ^{:arglists '([user-id])} is-api-key-user?
"Cached function to determine whether the user with this ID is an API key user"
(memoize/ttl
^{::memoize/args-fn (fn [[user-id]]
[(mdb/unique-identifier) user-id])}
(fn is-api-key-user?*
[user-id]
(= :api-key (t2/select-one-fn :type :model/User user-id)))

;; cache the results for 60 minutes; TTL is here only to eventually clear out old entries/keep it from growing too
;; large
:ttl/threshold (* 60 60 1000)))
104 changes: 60 additions & 44 deletions src/metabase/models/collection.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
:as api
:refer [*current-user-id* *current-user-permissions-set*]]
[metabase.audit :as audit]
[metabase.config :refer [*request-id*]]
[metabase.config :as config :refer [*request-id*]]
[metabase.db :as mdb]
[metabase.events :as events]
[metabase.models.api-key :as api-key]
[metabase.models.collection.root :as collection.root]
[metabase.models.interface :as mi]
[metabase.models.permissions :as perms :refer [Permissions]]
Expand Down Expand Up @@ -385,19 +386,20 @@
[user-or-id]
(t2/select-one Collection :personal_owner_id (u/the-id user-or-id)))

(mu/defn user->personal-collection :- (ms/InstanceOf Collection)
(mu/defn user->personal-collection :- [:maybe (ms/InstanceOf Collection)]
"Return the Personal Collection for `user-or-id`, if it already exists; if not, create it and return it."
[user-or-id]
(or (user->existing-personal-collection user-or-id)
(try
(first (t2/insert-returning-instances! Collection
{:name (user->personal-collection-name user-or-id :site)
:personal_owner_id (u/the-id user-or-id)}))
;; if an Exception was thrown why trying to create the Personal Collection, we can assume it was a race
;; condition where some other thread created it in the meantime; try one last time to fetch it
(catch Throwable e
(or (user->existing-personal-collection user-or-id)
(throw e))))))
(when-not (api-key/is-api-key-user? (u/the-id user-or-id))
(or (user->existing-personal-collection user-or-id)
(try
(first (t2/insert-returning-instances! Collection
{:name (user->personal-collection-name user-or-id :site)
:personal_owner_id (u/the-id user-or-id)}))
;; if an Exception was thrown why trying to create the Personal Collection, we can assume it was a race
;; condition where some other thread created it in the meantime; try one last time to fetch it
(catch Throwable e
(or (user->existing-personal-collection user-or-id)
(throw e)))))))

(def ^:private ^{:arglists '([user-id])} user->personal-collection-id
"Cached function to fetch the ID of the Personal Collection belonging to User with `user-id`. Since a Personal
Expand All @@ -409,22 +411,24 @@
[(mdb/unique-identifier) user-id])}
(fn user->personal-collection-id*
[user-id]
(u/the-id (user->personal-collection user-id)))
(some-> user-id user->personal-collection u/the-id))
;; cache the results for 60 minutes; TTL is here only to eventually clear out old entries/keep it from growing too
;; large
:ttl/threshold (* 60 60 1000)))

(mu/defn user->personal-collection-and-descendant-ids :- [:sequential {:min 1} ms/PositiveInt]
(mu/defn user->personal-collection-and-descendant-ids :- [:sequential ms/PositiveInt]
"Somewhat-optimized function that fetches the ID of a User's Personal Collection as well as the IDs of all descendants
of that Collection. Exists because this needs to be known to calculate the Current User's permissions set, which is
done for every API call; this function is an attempt to make fetching this information as efficient as reasonably
possible."
[user-or-id]
(let [personal-collection-id (user->personal-collection-id (u/the-id user-or-id))]
(cons personal-collection-id
;; `descendant-ids` wants a CollectionWithLocationAndID, and luckily we know Personal Collections always go
;; in Root, so we can pass it what it needs without actually having to fetch an entire CollectionInstance
(descendant-ids {:location "/", :id personal-collection-id}))))
(into []
(when-let [personal-collection-id (user->personal-collection-id (u/the-id user-or-id))]
(conj
;; `descendant-ids` wants a CollectionWithLocationAndID, and luckily we know Personal Collections always go
;; in Root, so we can pass it what it needs without actually having to fetch an entire CollectionInstance
(descendant-ids {:location "/", :id personal-collection-id})
personal-collection-id))))

(mi/define-batched-hydration-method include-personal-collection-ids
:personal_collection_id
Expand All @@ -433,15 +437,19 @@
[users]
(when (seq users)
;; efficiently create a map of user ID -> personal collection ID
(let [user-id->collection-id (t2/select-fn->pk :personal_owner_id Collection
:personal_owner_id [:in (set (map u/the-id users))])]
(assert (map? user-id->collection-id))
(let [non-api-user-ids (t2/select-pks-set :model/User
:id [:in (set (map u/the-id users))]
:type [:not= :api-key])
user-id->collection-id (when (seq non-api-user-ids)
(t2/select-fn->pk :personal_owner_id Collection
:personal_owner_id [:in non-api-user-ids]))]
;; now for each User, try to find the corresponding ID out of that map. If it's not present (the personal
;; Collection hasn't been created yet), then instead call `user->personal-collection-id`, which will create it
;; as a side-effect. This will ensure this property never comes back as `nil`
(for [user users]
(assoc user :personal_collection_id (or (user-id->collection-id (u/the-id user))
(user->personal-collection-id (u/the-id user))))))))
(assoc user :personal_collection_id (when (contains? non-api-user-ids (u/the-id user))
(or (get user-id->collection-id (u/the-id user))
(user->personal-collection-id (u/the-id user)))))))))

(mi/define-batched-hydration-method collection-is-personal
:is_personal
Expand Down Expand Up @@ -592,26 +600,27 @@
;; c) their personal collection and its descendants
:from [(if is-superuser?
[:collection :c]
[{:union-all [{:select [:c.*]
:from [[:collection :c]]
:join [[:permissions :p]
[:= :c.id :p.collection_id]
[:permissions_group :pg] [:= :pg.id :p.group_id]
[:permissions_group_membership :pgm] [:= :pgm.group_id :pg.id]]
:where [:and
[:= :pgm.user_id current-user-id]
[:= :p.perm_type "perms/collection-access"]
[:or
[:= :p.perm_value "read-and-write"]
(when (= :read (:permission-level visibility-config))
[:= :p.perm_value "read"])]]}
{:select [[:c.*]]
:from [[:collection :c]]
:where [:= :type "trash"]}
(when-let [personal-collection-and-descendant-ids (user->personal-collection-and-descendant-ids current-user-id)]
{:select [:c.*]
:from [[:collection :c]]
:where [:in :id personal-collection-and-descendant-ids]})]}
[{:union-all (keep identity [{:select [:c.*]
:from [[:collection :c]]
:join [[:permissions :p]
[:= :c.id :p.collection_id]
[:permissions_group :pg] [:= :pg.id :p.group_id]
[:permissions_group_membership :pgm] [:= :pgm.group_id :pg.id]]
:where [:and
[:= :pgm.user_id current-user-id]
[:= :p.perm_type "perms/collection-access"]
[:or
[:= :p.perm_value "read-and-write"]
(when (= :read (:permission-level visibility-config))
[:= :p.perm_value "read"])]]}
{:select [[:c.*]]
:from [[:collection :c]]
:where [:= :type "trash"]}
(when-let [personal-collection-and-descendant-ids
(seq (user->personal-collection-and-descendant-ids current-user-id))]
{:select [:c.*]
:from [[:collection :c]]
:where [:in :id personal-collection-and-descendant-ids]})])}
:c])]
;; The `WHERE` clause is where we apply the other criteria we were given:
:where [:and
Expand Down Expand Up @@ -1122,9 +1131,16 @@

;;; ----------------------------------------------------- INSERT -----------------------------------------------------

(defn- assert-not-personal-collection-for-api-key [collection]
(when-not config/is-prod?
(when-let [user-id (:personal_owner_id collection)]
(when (= :api-key (t2/select-one-fn :type :model/User user-id))
(throw (ex-info "Can't create a personal collection for an API key" {:user user-id}))))))

(t2/define-before-insert :model/Collection
[{collection-name :name, :as collection}]
(assert-valid-location collection)
(assert-not-personal-collection-for-api-key collection)
(assert-valid-namespace (merge {:namespace nil} collection))
(assoc collection :slug (slugify collection-name)))

Expand Down
2 changes: 2 additions & 0 deletions test/metabase/api/api_key_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@
(is (client/client :get 200 "user/current" {:request-options {:headers {"x-api-key" api-key}}}))
(is (= "Unauthenticated"
(client/client :get 401 "user/current" {:request-options {:headers {"x-api-key" "mb_not_an_api_key"}}})))
(let [user-id (:id (client/client :get 200 "user/current" {:request-options {:headers {"x-api-key" api-key}}}))]
(is (not (t2/exists? :model/Collection :personal_owner_id user-id))))
(testing "A deleted API Key can no longer be used"
(mt/user-http-request :crowberto :delete 204 (format "api-key/%s" id))
(is (= "Unauthenticated"
Expand Down

0 comments on commit 3a05e07

Please sign in to comment.