Skip to content
Snippets Groups Projects
Commit 3a64d2a1 authored by Ryan Senior's avatar Ryan Senior
Browse files

Ensure is_active is included in user PUT/POST responses

parent 539ade1a
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/user.clj
+ 11
− 5
View file @ 3a64d2a1
......@@ -19,10 +19,16 @@
(api/check-403 (or (= user-id api/*current-user-id*)
(:is_superuser @api/*current-user*))))
(def ^:private all-user-fields
(vec (cons User user/all-user-fields)))
(api/defendpoint GET "/"
"Fetch a list of all `Users` for the admin People page."
[]
(db/select [User :id :first_name :last_name :email :is_superuser :google_auth :ldap_auth :last_login :is_active]))
(db/select all-user-fields))
(defn- fetch-user [& query-criteria]
(apply db/select-one all-user-fields query-criteria))
(defn- reactivate-user! [existing-user]
(db/update! User (u/get-id existing-user)
......@@ -36,7 +42,7 @@
:ldap_auth (boolean (and (:ldap_auth existing-user)
(ldap/ldap-configured?))))
;; now return the existing user whether they were originally active or not
(User (u/get-id existing-user)))
(fetch-user :id (u/get-id existing-user)))
(api/defendpoint POST "/"
......@@ -60,7 +66,7 @@
"Fetch a `User`. You must be fetching yourself *or* be a superuser."
[id]
(check-self-or-superuser id)
(api/check-404 (User :id id, :is_active true)))
(api/check-404 (fetch-user :id id, :is_active true)))
(api/defendpoint PUT "/:id"
......@@ -81,7 +87,7 @@
:last_name last_name
:is_superuser (when (:is_superuser @api/*current-user*)
is_superuser)))
(User id))
(fetch-user :id id))
(api/defendpoint PUT "/:id/reactivate"
"Reactivate user at `:id`"
......@@ -109,7 +115,7 @@
"Invalid password")))
(user/set-password! id password)
;; return the updated User
(User id))
(fetch-user :id id))
;; TODO - This could be handled by PUT /api/user/:id, we don't need a separate endpoint
......
src/metabase/middleware.clj
+ 2
− 1
View file @ 3a64d2a1
......@@ -119,7 +119,8 @@
response-unauthentic)))
(def ^:private current-user-fields
(vec (concat [User :is_active :google_auth :ldap_auth] (models/default-fields User))))
(vec (cons User user/all-user-fields)))
(defn bind-current-user
"Middleware that binds `metabase.api.common/*current-user*`, `*current-user-id*`, `*is-superuser?*`, and
......
src/metabase/models/user.clj
+ 8
− 1
View file @ 3a64d2a1
......@@ -96,10 +96,17 @@
['ViewLog :user_id]]]
(db/delete! model k id))))
(def ^:private default-user-fields
[:id :email :date_joined :first_name :last_name :last_login :is_superuser :is_qbnewb])
(def all-user-fields
"Seq of all the columns stored for a user"
(vec (concat default-user-fields [:google_auth :ldap_auth :is_active])))
(u/strict-extend (class User)
models/IModel
(merge models/IModelDefaults
{:default-fields (constantly [:id :email :date_joined :first_name :last_name :last_login :is_superuser :is_qbnewb])
{:default-fields (constantly default-user-fields)
:hydration-keys (constantly [:author :creator :user])
:pre-insert pre-insert
:post-insert post-insert
......
test/metabase/api/alert_test.clj
+ 2
− 2
View file @ 3a64d2a1
......@@ -22,8 +22,8 @@
(defn- user-details [user-kwd]
(-> user-kwd
fetch-user
(select-keys [:email :first_name :last_login :is_qbnewb :is_superuser :last_name :date_joined :common_name])
(assoc :id true)))
(select-keys [:email :first_name :last_login :is_qbnewb :is_superuser :last_name :common_name])
(assoc :id true :date_joined true)))
(defn- pulse-card-details [card]
(-> card
......
test/metabase/api/user_test.clj
+ 82
− 85
View file @ 3a64d2a1
......@@ -21,54 +21,51 @@
(expect (get middleware/response-unauthentic :body) (http/client :get 401 "user"))
(expect (get middleware/response-unauthentic :body) (http/client :get 401 "user/current"))
(def user-defaults
{:ldap_auth false
:is_active true
:is_superuser false
:google_auth false
:is_qbnewb true})
;; ## GET /api/user
;; Check that anyone can get a list of all active Users
(expect
#{(match-$ (fetch-user :trashbird)
{:email "trashbird@metabase.com"
:ldap_auth false
:first_name "Trash"
:last_login $
:is_active false
:is_superuser false
:id $
:last_name "Bird"
:common_name "Trash Bird"
:google_auth false})
(match-$ (fetch-user :crowberto)
{:common_name "Crowberto Corv"
:last_name "Corv"
:id $
:is_superuser true
:is_active true
:last_login $
:first_name "Crowberto"
:email "crowberto@metabase.com"
:google_auth false
:ldap_auth false})
(match-$ (fetch-user :lucky)
{:common_name "Lucky Pigeon"
:last_name "Pigeon"
:id $
:is_superuser false
:is_active true
:last_login $
:first_name "Lucky"
:email "lucky@metabase.com"
:google_auth false
:ldap_auth false})
(match-$ (fetch-user :rasta)
{:common_name "Rasta Toucan"
:last_name "Toucan"
:id $
:is_superuser false
:is_active true
:last_login $
:first_name "Rasta"
:email "rasta@metabase.com"
:google_auth false
:ldap_auth false})}
(set (map #(merge user-defaults %)
[(match-$ (fetch-user :trashbird)
{:email "trashbird@metabase.com"
:first_name "Trash"
:last_login $
:is_active false
:id $
:last_name "Bird"
:common_name "Trash Bird"
:date_joined $})
(match-$ (fetch-user :crowberto)
{:common_name "Crowberto Corv"
:last_name "Corv"
:id $
:is_superuser true
:last_login $
:first_name "Crowberto"
:email "crowberto@metabase.com"
:date_joined $})
(match-$ (fetch-user :lucky)
{:common_name "Lucky Pigeon"
:last_name "Pigeon"
:id $
:last_login $
:first_name "Lucky"
:email "lucky@metabase.com"
:date_joined $})
(match-$ (fetch-user :rasta)
{:common_name "Rasta Toucan"
:last_name "Toucan"
:id $
:last_login $
:first_name "Rasta"
:email "rasta@metabase.com"
:date_joined $})]))
(do
;; Delete all the other random Users we've created so far
(let [user-ids (set (map user->id [:crowberto :rasta :lucky :trashbird]))]
......@@ -87,12 +84,13 @@
:last_name user-name
:common_name (str user-name " " user-name)
:is_superuser false
:is_qbnewb true}
:is_qbnewb true
:is_active true}
(et/with-fake-inbox
((user->client :crowberto) :post 200 "user" {:first_name user-name
:last_name user-name
:email email})
(u/prog1 (db/select-one [User :email :first_name :last_name :is_superuser :is_qbnewb]
(u/prog1 (db/select-one [User :email :first_name :last_name :is_superuser :is_qbnewb :is_active]
:email email)
;; clean up after ourselves
(db/delete! User :email email)))))
......@@ -158,35 +156,30 @@
;; ## GET /api/user/current
;; Check that fetching current user will return extra fields like `is_active` and will return OrgPerms
(expect
(match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_name "Toucan"
:common_name "Rasta Toucan"
:date_joined $
:last_login $
:is_active true
:is_superuser false
:is_qbnewb true
:google_auth false
:ldap_auth false
:id $})
(merge user-defaults
(match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_name "Toucan"
:common_name "Rasta Toucan"
:date_joined $
:last_login $
:id $}))
((user->client :rasta) :get 200 "user/current"))
;; ## GET /api/user/:id
;; Should return a smaller set of fields, and should *not* return OrgPerms
(expect
(match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_login $
:is_superuser false
:is_qbnewb true
:id $
:last_name "Toucan"
:date_joined $
:common_name "Rasta Toucan"})
(merge user-defaults
(match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_login $
:id $
:last_name "Toucan"
:date_joined $
:common_name "Rasta Toucan"}))
((user->client :rasta) :get 200 (str "user/" (user->id :rasta))))
;; Check that a non-superuser CANNOT fetch someone else's user details
......@@ -194,16 +187,16 @@
((user->client :rasta) :get 403 (str "user/" (user->id :trashbird))))
;; A superuser should be allowed to fetch another users data
(expect (match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_login $
:is_superuser false
:is_qbnewb true
:id $
:last_name "Toucan"
:date_joined $
:common_name "Rasta Toucan"})
(expect
(merge user-defaults
(match-$ (fetch-user :rasta)
{:email "rasta@metabase.com"
:first_name "Rasta"
:last_login $
:id $
:last_name "Toucan"
:date_joined $
:common_name "Rasta Toucan"}))
((user->client :crowberto) :get 200 (str "user/" (user->id :rasta))))
;; We should get a 404 when trying to access a disabled account
......@@ -215,14 +208,18 @@
;; Test that we can edit a User
(expect
[{:first_name "Cam", :last_name "Era", :is_superuser true, :email "cam.era@metabase.com"}
(merge user-defaults
{:last_login nil, :common_name "Cam Eron", :id true, :date_joined true :email "cam.eron@metabase.com",
:first_name "Cam", :is_superuser true, :last_name "Eron"})
{:first_name "Cam", :last_name "Eron", :is_superuser true, :email "cam.eron@metabase.com"}]
(tt/with-temp User [{user-id :id} {:first_name "Cam", :last_name "Era", :email "cam.era@metabase.com", :is_superuser true}]
(let [user (fn [] (into {} (dissoc (db/select-one [User :first_name :last_name :is_superuser :email], :id user-id)
:common_name)))]
[(user)
(do ((user->client :crowberto) :put 200 (str "user/" user-id) {:last_name "Eron"
:email "cam.eron@metabase.com"})
(user))])))
(tu/boolean-ids-and-timestamps
((user->client :crowberto) :put 200 (str "user/" user-id) {:last_name "Eron"
:email "cam.eron@metabase.com"}))
(user)])))
;; ## PUT /api/user/:id
;; Test that updating a user's email to an existing inactive user's email fails
......@@ -290,9 +287,9 @@
[{:success true}
false]
(tt/with-temp User [{:keys [id]} {:first_name (random-name)
:last_name (random-name)
:email "def@metabase.com"
:password "def123"}]
:last_name (random-name)
:email "def@metabase.com"
:password "def123"}]
(let [creds {:username "def@metabase.com"
:password "def123"}]
[(metabase.http-client/client creds :put 200 (format "user/%d/qbnewb" id))
......
test/metabase/test/util.clj
+ 1
− 1
View file @ 3a64d2a1
......@@ -105,7 +105,7 @@
([data]
(boolean-ids-and-timestamps
(every-pred (some-fn keyword? string?)
(some-fn #{:id :created_at :updated_at :last_analyzed :created-at :updated-at :field-value-id :field-id}
(some-fn #{:id :created_at :updated_at :last_analyzed :created-at :updated-at :field-value-id :field-id :date_joined :date-joined}
#(.endsWith (name %) "_id")))
data))
([pred data]
......
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