Skip to content
Snippets Groups Projects
Unverified Commit 2550abbb authored by John Swanson's avatar John Swanson Committed by GitHub
Browse files

Add `updated_by` to API Keys (#37014) 

This required a bit of modification of the DB.

First, I had named the column meant to hold the ID of the user who last updated an API key as `updated_by`. But it seemed like `updated_by` was the natural field to hold the user name that FE wanted, and I didn't want to clobber an existing field with `hydrate`.

So, I renamed the existing column to `updated_by_id` and used `updated_by` to hold the hydrated user that updated the ApiKey.

Second, for consistency, I renamed `created_by` to `creator_id`.

The logic for presenting an ApiKey was getting a bit complicated and spread out over many different endpoints, so I pulled it out into its own function that's called from almost every endpoint defined in the `metabase.api.api-key` namespace. Note that it calls `(t2/hydrate api-key :updated_by :group_name)` - but I verified that this will not result in additional DB calls if the model was already hydrated.
parent f59b18a1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
resources/migrations/001_update_migrations.yaml
+ 46
− 0
View file @ 2550abbb
......@@ -4390,6 +4390,52 @@ databaseChangeLog:
nullable: true
remarks: 'If true, the table requires a filter to be able to query it'
- changeSet:
id: v49.00-013
author: johnswanson
comment: Add `api_key.updated_by_id`
rollback: # will be removed with the table
changes:
- addColumn:
tableName: api_key
columns:
- column:
remarks: The ID of the user that last updated this API key
name: updated_by_id
type: integer
constraints:
# no default and nullable: false for a new column? yikes! But this should probably be ok, because
# we don't yet have any UI for creating a new API key.
nullable: false
referencedTableName: core_user
referencedColumnNames: id
foreignKeyName: fk_api_key_updated_by_id_user_id
- changeSet:
id: v49.00-014
author: johnswanson
comment: Add an index on `api_key.updated_by_id`
rollback: # not necessary, will be removed with the table
changes:
- createIndex:
tableName: api_key
indexName: idx_api_key_updated_by_id
columns:
column:
name: updated_by_id
- changeSet:
id: v49.00-015
author: johnswanson
comment: Rename `created_by` to `creator_id`
rollback: # not necessary, will be removed with the table
changes:
- renameColumn:
tableName: api_key
columnDataType: integer
oldColumnName: created_by
newColumnName: creator_id
- changeSet:
id: v49.00-016
author: noahmoss
......
src/metabase/api/api_key.clj
+ 43
− 24
View file @ 2550abbb
......@@ -12,6 +12,21 @@
[metabase.util.malli.schema :as ms]
[toucan2.core :as t2]))
(defn- present-api-key
"Takes an ApiKey and hydrates/selects keys as necessary to put it into a standard form for responses"
[api-key]
(-> api-key
(t2/hydrate :group_name :updated_by)
(select-keys [:created_at
:updated_at
:updated_by
:id
:group_name
:unmasked_key
:name
:masked_key])
(update :updated_by #(select-keys % [:common_name :id]))))
(defn- key-with-unique-prefix []
(u/auto-retry 5
(let [api-key (api-key/generate-key)
......@@ -22,6 +37,12 @@
api-key
(throw (ex-info (tru "could not generate key with unique prefix") {}))))))
(defn- with-updated-by [api-key]
(assoc api-key :updated_by_id api/*current-user-id*))
(defn- with-creator [api-key]
(assoc api-key :creator_id api/*current-user-id*))
(api/defendpoint POST "/"
"Create a new API key (and an associated `User`) with the provided name and group ID."
[:as {{:keys [group_id name] :as _body} :body}]
......@@ -38,18 +59,16 @@
:type :api-key})]
(user/set-permissions-groups! user [(perms-group/all-users) group_id])
(let [api-key (-> (t2/insert-returning-instance! :model/ApiKey
{:user_id (u/the-id user)
:name name
:unhashed_key unhashed-key
:created_by api/*current-user-id*})
(t2/hydrate :group_name))]
(-> {:user_id (u/the-id user)
:name name
:unhashed_key unhashed-key}
with-creator
with-updated-by))
(t2/hydrate :group_name :updated_by))]
(events/publish-event! :event/api-key-create
{:object api-key
:user-id api/*current-user-id*})
(-> api-key
(select-keys [:created_at :updated_at :id :group_name :name])
(assoc :unmasked_key unhashed-key
:masked_key (api-key/mask unhashed-key))))))))
(present-api-key (assoc api-key :unmasked_key unhashed-key)))))))
(api/defendpoint GET "/count"
"Get the count of API keys in the DB"
......@@ -68,19 +87,19 @@
;; hydrate the group_name for audit logging
(t2/hydrate :group_name))]
(t2/with-transaction [_conn]
(when group_id
(let [user (-> api-key-before (t2/hydrate :user) :user)]
(user/set-permissions-groups! user [(perms-group/all-users) {:id group_id}])))
(when name
;; A bit of a pain to keep these in sync, but oh well.
(t2/update! :model/User (:user_id api-key-before) {:first_name name})
(t2/update! :model/ApiKey id {:name name}))
(when group_id
(let [user (-> api-key-before (t2/hydrate :user) :user)]
(user/set-permissions-groups! user [(perms-group/all-users) {:id group_id}]))))
(t2/update! :model/ApiKey id (with-updated-by {:name name}))))
(let [updated-api-key (-> (t2/select-one :model/ApiKey :id id)
(t2/hydrate :group_name))]
(events/publish-event! :event/api-key-update {:object updated-api-key
(t2/hydrate :group_name :updated_by))]
(events/publish-event! :event/api-key-update {:object updated-api-key
:previous-object api-key-before
:user-id api/*current-user-id*})
(select-keys updated-api-key [:created_at :updated_at :id :name :masked_key :group_name]))))
:user-id api/*current-user-id*})
(present-api-key updated-api-key))))
(api/defendpoint PUT "/:id/regenerate"
"Regenerate an API Key"
......@@ -93,21 +112,21 @@
api-key-after (assoc api-key-before
:unhashed_key unhashed-key
:key_prefix (api-key/prefix unhashed-key))]
(t2/update! :model/ApiKey :id id (select-keys api-key-after [:unhashed_key]))
(t2/update! :model/ApiKey :id id (with-updated-by
(select-keys api-key-after [:unhashed_key])))
(events/publish-event! :event/api-key-regenerate
{:object api-key-after
:previous-object api-key-before
:user-id api/*current-user-id*})
(-> api-key-after
(select-keys [:created_at :updated_at :id :name :group_name])
(assoc :unmasked_key unhashed-key
:masked_key (api-key/mask unhashed-key)))))
(present-api-key (assoc api-key-after
:unmasked_key unhashed-key
:masked_key (api-key/mask unhashed-key)))))
(api/defendpoint GET "/"
"Get a list of API keys. Non-paginated."
[]
(api/check-superuser)
(let [api-keys (t2/hydrate (t2/select :model/ApiKey) :group_name)]
(map #(select-keys % [:created_at :updated_at :id :name :group_name]) api-keys)))
(let [api-keys (t2/hydrate (t2/select :model/ApiKey) :group_name :updated_by)]
(map present-api-key api-keys)))
(api/define-routes)
src/metabase/models/api_key.clj
+ 12
− 2
View file @ 2550abbb
(ns metabase.models.api-key
(:require [crypto.random :as crypto-random]
[metabase.api.common :as api]
[metabase.models.audit-log :as audit-log]
[metabase.models.interface :as mi]
[metabase.models.permissions-group :as perms-group]
......@@ -75,17 +76,26 @@
unhashed_key (assoc :key (u.password/hash-bcrypt unhashed_key))
true (dissoc :unhashed_key)))
(defn- add-updated-by-id [api-key]
(update api-key :updated_by_id #(or % api/*current-user-id*)))
(defn- add-created-by-id [api-key]
(update api-key :creator_id #(or % api/*current-user-id*)))
(t2/define-before-insert :model/ApiKey
[api-key]
(-> api-key
add-prefix
add-key))
add-key
add-updated-by-id
add-created-by-id))
(t2/define-before-update :model/ApiKey
[api-key]
(-> api-key
add-prefix
add-key))
add-key
add-updated-by-id))
(defn- add-masked-key [api-key]
(if-let [prefix (:key_prefix api-key)]
......
src/metabase/models/user.clj
+ 4
− 3
View file @ 2550abbb
......@@ -41,9 +41,10 @@
:model/User)
(methodical/defmethod t2/table-name :model/User [_model] :core_user)
(methodical/defmethod t2/model-for-automagic-hydration [:default :author] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :creator] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :user] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :author] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :creator] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :updated_by] [_original-model _k] :model/User)
(methodical/defmethod t2/model-for-automagic-hydration [:default :user] [_original-model _k] :model/User)
(doto :model/User
(derive :metabase/model)
......
test/metabase/api/api_key_test.clj
+ 40
− 28
View file @ 2550abbb
......@@ -17,14 +17,16 @@
resp (mt/user-http-request :crowberto :post 200 "api-key"
{:group_id group-id
:name name})]
(is (= #{:name :group_name :unmasked_key :masked_key :id :created_at :updated_at}
(is (= #{:name :group_name :unmasked_key :masked_key :id :created_at :updated_at :updated_by}
(-> resp keys set)))
(is (= (select-keys (mt/fetch-user :crowberto) [:id :common_name])
(:updated_by resp)))
(is (= "Cool Friends" (:group_name resp)))
(is (= name (:name resp)))))
(testing "Trying to create another API key with the same name fails"
(let [key-name (str (random-uuid))]
;; works once...
(is (= #{:unmasked_key :masked_key :group_name :name :id :created_at :updated_at}
(is (= #{:unmasked_key :masked_key :group_name :name :id :created_at :updated_at :updated_by}
(set (keys (mt/user-http-request :crowberto :post 200 "api-key"
{:group_id group-id
:name key-name})))))
......@@ -44,10 +46,11 @@
(swap! generated-keys next)
next-val))]
;; put an API Key in the database with that key.
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key generated-key
:name "my cool name"
:user_id (mt/user->id :crowberto)
:created_by (mt/user->id :crowberto)}]
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key generated-key
:name "my cool name"
:user_id (mt/user->id :crowberto)
:creator_id (mt/user->id :crowberto)
:updated_by_id (mt/user->id :crowberto)}]
;; this will try to generate a new API key
(mt/user-http-request :crowberto :post 200 "api-key"
{:group_id group-id
......@@ -57,10 +60,11 @@
(testing "We don't retry forever if prefix collision keeps happening"
(let [generated-key (api-key/generate-key)]
(with-redefs [api-key/generate-key (constantly generated-key)]
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key generated-key
:name "my cool name"
:user_id (mt/user->id :crowberto)
:created_by (mt/user->id :crowberto)}]
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key generated-key
:name "my cool name"
:user_id (mt/user->id :crowberto)
:creator_id (mt/user->id :crowberto)
:updated_by_id (mt/user->id :crowberto)}]
(is (= "could not generate key with unique prefix"
(:message (mt/user-http-request :crowberto :post 500 "api-key"
{:group_id group-id
......@@ -95,15 +99,17 @@
(deftest api-count-works
(mt/with-empty-h2-app-db
(is (zero? (mt/user-http-request :crowberto :get 200 "api-key/count")))
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key "prefix_key"
:name "my cool name"
:user_id (mt/user->id :crowberto)
:created_by (mt/user->id :crowberto)}]
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key "prefix_key"
:name "my cool name"
:user_id (mt/user->id :crowberto)
:creator_id (mt/user->id :crowberto)
:updated_by_id (mt/user->id :crowberto)}]
(is (= 1 (mt/user-http-request :crowberto :get 200 "api-key/count")))
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key "some_other_key"
:name "my cool OTHER name"
:user_id (mt/user->id :crowberto)
:created_by (mt/user->id :crowberto)}]
(t2.with-temp/with-temp [:model/ApiKey _ {:unhashed_key "some_other_key"
:name "my cool OTHER name"
:user_id (mt/user->id :crowberto)
:creator_id (mt/user->id :crowberto)
:updated_by_id (mt/user->id :crowberto)}]
(is (= 2 (mt/user-http-request :crowberto :get 200 "api-key/count")))))))
(deftest api-keys-work-e2e
......@@ -126,8 +132,8 @@
:post 200 "api-key"
{:group_id group-id-1
:name (str (random-uuid))})
_ (is (= "Cool Friends" (:group_name create-resp)))
api-user-id (:id (:user (t2/hydrate (t2/select-one :model/ApiKey :id id) :user)))
_ (is (= "Cool Friends" (:group_name create-resp)))
api-user-id (:id (:user (t2/hydrate (t2/select-one :model/ApiKey :id id) :user)))
member-of-group? (fn [group-id]
(t2/exists? :model/PermissionsGroupMembership
:user_id api-user-id
......@@ -140,19 +146,24 @@
(is (not (member-of-group? group-id-1)))
(is (member-of-group? group-id-2))))
(testing "You can change the name of an API key"
(let [name-1 (str "My First Name" (random-uuid))
name-2 (str "My Second Name" (random-uuid))
{id :id} (mt/user-http-request :crowberto
(let [name-1 (str "My First Name" (random-uuid))
name-2 (str "My Second Name" (random-uuid))
{id :id} (mt/user-http-request :crowberto
:post 200 "api-key"
{:group_id group-id-1
:name name-1})
:name name-1})
api-user-id (-> (t2/select-one :model/ApiKey :id id) (t2/hydrate :user) :user :id)]
(testing "before the change..."
(is (= name-1 (:common_name (t2/select-one :model/User api-user-id)))))
(testing "after the change..."
(mt/user-http-request :crowberto :put 200 (str "api-key/" id)
{:name name-2})
(is (= name-2 (:common_name (t2/select-one :model/User api-user-id)))))))))
(is (= name-2 (:common_name (t2/select-one :model/User api-user-id)))))
(testing "the shape of the response is correct"
(is (= #{:created_at :updated_at :updated_by :id :group_name :name :masked_key}
(set (keys (mt/user-http-request :crowberto :put 200 (str "api-key/" id)
{:name name-1}))))))))))
(deftest api-keys-can-be-regenerated
(testing "You can regenerate an API key"
......@@ -166,7 +177,7 @@
{:as resp new-key :unmasked_key}
(mt/user-http-request :crowberto
:put 200 (format "api-key/%s/regenerate" id))]
(is (= #{:created_at :updated_at :id :name :unmasked_key :masked_key :group_name}
(is (= #{:created_at :updated_at :id :name :unmasked_key :masked_key :group_name :updated_by}
(set (keys resp))))
(is (client/client :get 401 "user/current" {:request-options {:headers {"x-api-key" old-key}}}))
(is (client/client :get 200 "user/current" {:request-options {:headers {"x-api-key" new-key}}}))))))
......@@ -181,8 +192,9 @@
{:group_id group-id
:name "My First API Key"})
(is (= [{:name "My First API Key"
:group_name "Cool Friends"}]
(map #(select-keys % [:name :group_name])
:group_name "Cool Friends"
:updated_by (select-keys (mt/fetch-user :crowberto) [:common_name :id])}]
(map #(select-keys % [:name :group_name :updated_by])
(mt/user-http-request :crowberto :get 200 "api-key"))))
(mt/user-http-request :crowberto
......
test/metabase/server/middleware/session_test.clj
+ 23
− 20
View file @ 2550abbb
......@@ -247,10 +247,11 @@
(select-keys (wrapped-handler request) [:anti-csrf-token :cookies :metabase-session-id :uri]))))))
(deftest current-user-info-for-api-key-test
(t2.with-temp/with-temp [:model/ApiKey _ {:name "An API Key"
:user_id (mt/user->id :lucky)
:created_by (mt/user->id :lucky)
:unhashed_key "mb_foobar"}]
(t2.with-temp/with-temp [:model/ApiKey _ {:name "An API Key"
:user_id (mt/user->id :lucky)
:creator_id (mt/user->id :lucky)
:updated_by_id (mt/user->id :lucky)
:unhashed_key "mb_foobar"}]
(testing "A valid API key works, and user info is added to the request"
(let [req {:headers {"x-api-key" "mb_foobar"}}]
(is (= (merge req {:metabase-user-id (mt/user->id :lucky)
......@@ -286,27 +287,29 @@
(fn [e] (throw e)))))
(deftest user-data-is-correctly-bound-for-api-keys
(t2.with-temp/with-temp [:model/ApiKey _ {:name "An API Key"
:user_id (mt/user->id :lucky)
:created_by (mt/user->id :lucky)
:unhashed_key "mb_foobar"}
:model/ApiKey _ {:name "A superuser API Key"
:user_id (mt/user->id :crowberto)
:created_by (mt/user->id :crowberto)
:unhashed_key "mb_superuser"}]
(t2.with-temp/with-temp [:model/ApiKey _ {:name "An API Key"
:user_id (mt/user->id :lucky)
:creator_id (mt/user->id :lucky)
:updated_by_id (mt/user->id :lucky)
:unhashed_key "mb_foobar"}
:model/ApiKey _ {:name "A superuser API Key"
:user_id (mt/user->id :crowberto)
:creator_id (mt/user->id :lucky)
:updated_by_id (mt/user->id :lucky)
:unhashed_key "mb_superuser"}]
(testing "A valid API key works, and user info is added to the request"
(is (= {:is-superuser? false
(is (= {:is-superuser? false
:is-group-manager? false
:user-id (mt/user->id :lucky)
:user {:id (mt/user->id :lucky)
:email (:email (mt/fetch-user :lucky))}}
:user-id (mt/user->id :lucky)
:user {:id (mt/user->id :lucky)
:email (:email (mt/fetch-user :lucky))}}
(simple-auth-handler {:headers {"x-api-key" "mb_foobar"}}))))
(testing "A superuser API key has `*is-superuser?*` bound correctly"
(is (= {:is-superuser? true
(is (= {:is-superuser? true
:is-group-manager? false
:user-id (mt/user->id :crowberto)
:user {:id (mt/user->id :crowberto)
:email (:email (mt/fetch-user :crowberto))}}
:user-id (mt/user->id :crowberto)
:user {:id (mt/user->id :crowberto)
:email (:email (mt/fetch-user :crowberto))}}
(simple-auth-handler {:headers {"x-api-key" "mb_superuser"}}))))))
(deftest current-user-info-for-session-test
......
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