Skip to content
Snippets Groups Projects
Unverified Commit 6837c2ef authored by metabase-bot[bot]'s avatar metabase-bot[bot] Committed by GitHub
Browse files

Allow group managers to view all people (#40619) (#40682) 

Fixes #40328

[Our documentation](https://www.metabase.com/docs/latest/people-and-groups/managing#group-managers

) states that:

> Group managers can:
>
> - View all people in the Admin settings > People tab.

This fixes enforcement to be aligned with the documentation. This behavior makes sense, because as the docs also state, Group Managers should be allowed to *add* people to the group they manage, which they can only do if they can see those people!

Initially, I also removed a faulty test, which was described as:
```
;; Non-segmented users are allowed to ask for a list of all of the users in the Metabase instance. Pulse email lists
;; are an example usage of this. Segmented users should not have that ability. Instead they should only see
;; themselves. This test checks that GET /api/user for a segmented user only returns themselves
```

but actually failed to test this in a relevant way (because it was testing the `GET /api/user` endpoint rather than the `GET /api/user/recipients` endpoint). The test had continued to pass only because the user was the only member of the group they managed.

I initially thought this behavior wasn't desired, but as it turns out, it *is* in fact documented behavior to disallow sandboxed users from seeing any email suggestions. Investigating, I found that a bug was allowing sandboxed users to see all users on the `/api/user/recipients` endpoint if the user-visibility setting was set to `:all`.

Thus, the second commit fixes the bug and re-adds the (fixed) test. A sandboxed user now only sees their own user when hitting `/api/user/recipients`.

Co-authored-by: default avatarJohn Swanson <john.swanson@metabase.com>
parent 015355da
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/test/metabase_enterprise/sandbox/api/user_test.clj
+ 36
− 27
View file @ 6837c2ef
......@@ -3,8 +3,6 @@
(:require
[clojure.test :refer :all]
[metabase-enterprise.test :as met]
[metabase.models.permissions-group-membership
:refer [PermissionsGroupMembership]]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[metabase.util :as u]
......@@ -15,31 +13,41 @@
;; Non-segmented users are allowed to ask for a list of all of the users in the Metabase instance. Pulse email lists
;; are an example usage of this. Segmented users should not have that ability. Instead they should only see
;; themselves. This test checks that GET /api/user for a segmented user only returns themselves
;; themselves. This test checks that GET /api/user/recipients for a segmented user only returns themselves, including
;; for Permissions Group Managers.
(deftest segmented-user-list-test
(testing "GET /api/user for a segmented user should not return data."
(met/with-gtaps {:gtaps {:venues {}}}
;; Now do the request
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :get 403 "user")))
(testing "Should return themselves when the user is a segmented group manager"
(mt/with-premium-features #{:advanced-permissions}
(mt/with-group [group {:name "a group"}]
(let [membership (t2/select-one PermissionsGroupMembership
:group_id (u/the-id group)
:user_id (mt/user->id :rasta))]
(t2/update! PermissionsGroupMembership :id (:id membership)
{:is_group_manager true}))
(let [result (mt/user-http-request :rasta :get 200 "user")]
(is (= ["rasta@metabase.com"]
(map :email (:data result))))
(is (= 1 (count (:data result))))
(is (= 1 (:total result))))))))))
(testing "GET /api/user/recipients"
(testing "sanity check: normally returns more than just me"
(is (seq (disj (->> (mt/user-http-request :rasta :get 200 "user/recipients")
:data
(map :email)
set)
"rasta@metabase.com"))))
(testing "a sandboxed user will see only themselves"
(met/with-gtaps {:gtaps {:venues {}}}
(is (= ["rasta@metabase.com"]
(->> (mt/user-http-request :rasta :get 200 "user/recipients")
:data
(map :email))))
(testing "... even if they are a group manager"
(mt/with-premium-features #{:advanced-permissions :sandboxes}
(mt/with-group [group {:name "a group"}]
(let [membership (t2/select-one :model/PermissionsGroupMembership
:group_id (u/the-id group)
:user_id (mt/user->id :rasta))]
(t2/update! :model/PermissionsGroupMembership :id (:id membership)
{:is_group_manager true}))
(let [result (mt/user-http-request :rasta :get 200 "user/recipients")]
(is (= ["rasta@metabase.com"]
(map :email (:data result))))
(is (= 1 (count (:data result))))
(is (= 1 (:total result)))))))))))
(deftest get-user-attributes-test
(testing "requires sandbox enabled"
(is (= "Sandboxes is a paid feature not currently available to your instance. Please upgrade to use it. Learn more at metabase.com/upgrade/"
(mt/user-http-request :crowberto :get 402 "mt/user/attributes"))))
(mt/with-premium-features #{}
(testing "requires sandbox enabled"
(is (= "Sandboxes is a paid feature not currently available to your instance. Please upgrade to use it. Learn more at metabase.com/upgrade/"
(mt/user-http-request :crowberto :get 402 "mt/user/attributes")))))
(mt/with-premium-features #{:sandboxes}
(testing "requires admin"
......@@ -56,9 +64,10 @@
(deftest update-user-attributes-test
(testing "requires sandbox enabled"
(is (= "Sandboxes is a paid feature not currently available to your instance. Please upgrade to use it. Learn more at metabase.com/upgrade/"
(mt/user-http-request :crowberto :put 402 (format "mt/user/%d/attributes" (mt/user->id :crowberto)) {}))))
(mt/with-premium-features #{}
(testing "requires sandbox enabled"
(is (= "Sandboxes is a paid feature not currently available to your instance. Please upgrade to use it. Learn more at metabase.com/upgrade/"
(mt/user-http-request :crowberto :put 402 (format "mt/user/%d/attributes" (mt/user->id :crowberto)) {})))))
(mt/with-premium-features #{:sandboxes}
(testing "requires admin"
......
src/metabase/api/user.clj
+ 35
− 42
View file @ 6837c2ef
......@@ -168,16 +168,6 @@
[clauses]
(dissoc clauses :order-by :limit :offset))
(defn- group-ids-for-manager
"Given a `user-id` return a list of group-ids of which the user is a group manager."
[user-id]
(t2/select-fn-set
:group_id
:model/PermissionsGroupMembership
{:where [:and [:= :user_id user-id]
[:= :is_group_manager true]
[:not= :group_id (:id (perms-group/all-users))]]}))
(api/defendpoint GET "/"
"Fetch a list of `Users` for admins or group managers.
By default returns only active users for admins and only active users within groups that the group manager is managing for group managers.
......@@ -205,14 +195,7 @@
(validation/check-manager-of-group group_id)
(validation/check-group-manager)))
(let [include_deactivated (Boolean/parseBoolean include_deactivated)
manager-group-ids (set (group-ids-for-manager api/*current-user-id*))
group-id-clause (cond
;; We know that the user is either admin or group manager of the given group_id (if it exists)
group_id [group_id]
;; Superuser can see all users, so don't filter by group ID
api/*is-superuser?* nil
;; otherwise, if the user is a group manager, only show them users in the groups they manage
api/*is-group-manager?* (vec manager-group-ids))
group-id-clause (when group_id [group_id])
clauses (user-clauses status query group-id-clause include_deactivated)]
{:data (cond-> (t2/select
(vec (cons User (user-visible-columns)))
......@@ -261,30 +244,40 @@
- If user-visibility is :group, include only users in the same group (excluding the all users group).
- If user-visibility is :none or the user is sandboxed, include only themselves."
[]
(cond
(or (= :all (user-visibility)) api/*is-superuser?*)
(let [clauses (-> (user-clauses nil nil nil nil)
(sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]))]
{:data (t2/select (vec (cons User (user-visible-columns))) clauses)
:total (t2/count :model/User (filter-clauses-without-paging clauses))
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*})
(and (= :group (user-visibility)) (not (premium-features/sandboxed-or-impersonated-user?)))
(let [user-ids (same-groups-user-ids api/*current-user-id*)
clauses (cond-> (user-clauses nil nil nil nil)
(seq user-ids) (sql.helpers/where [:in :core_user.id user-ids])
true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]))]
{:data (t2/select (vec (cons User (user-visible-columns))) clauses)
:total (t2/count :model/User (filter-clauses-without-paging clauses))
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*})
:else
{:data [(fetch-user :id api/*current-user-id*)]
:total 1
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*}))
;; defining these functions so the branching logic below can be as clear as possible
(letfn [(all [] (let [clauses (-> (user-clauses nil nil nil nil)
(sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]))]
{:data (t2/select (vec (cons User (user-visible-columns))) clauses)
:total (t2/count :model/User (filter-clauses-without-paging clauses))
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*}))
(within-group [] (let [user-ids (same-groups-user-ids api/*current-user-id*)
clauses (cond-> (user-clauses nil nil nil nil)
(seq user-ids) (sql.helpers/where [:in :core_user.id user-ids])
true (sql.helpers/order-by [:%lower.last_name :asc] [:%lower.first_name :asc]))]
{:data (t2/select (vec (cons User (user-visible-columns))) clauses)
:total (t2/count :model/User (filter-clauses-without-paging clauses))
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*}))
(just-me [] {:data [(fetch-user :id api/*current-user-id*)]
:total 1
:limit mw.offset-paging/*limit*
:offset mw.offset-paging/*offset*})]
(cond
;; if they're sandboxed OR if they're a superuser, ignore the setting and just give them nothing or everything,
;; respectively.
(premium-features/sandboxed-user?)
(just-me)
api/*is-superuser?*
(all)
;; otherwise give them what the setting says on the tin
:else
(case (user-visibility)
:none (just-me)
:group (within-group)
:all (all)))))
(defn- maybe-add-advanced-permissions
"If `advanced-permissions` is enabled, add to `user` a permissions map."
......
test/metabase/api/user_test.clj
+ 13
− 15
View file @ 6837c2ef
......@@ -128,24 +128,22 @@
(mt/user-http-request :rasta :get 403 "user" :group_id group-id2))))
(if config/ee-available?
;; Group management is an EE feature
(testing "manager can get users from their groups"
(is (= #{"lucky@metabase.com"
"rasta@metabase.com"}
(->> ((mt/user-http-request :rasta :get 200 "user" :group_id group-id1) :data)
(filter mt/test-user?)
(map :email)
set)))
(is (= #{"lucky@metabase.com"
"rasta@metabase.com"}
(->> ((mt/user-http-request :rasta :get 200 "user") :data)
(filter mt/test-user?)
(map :email)
set)))
(testing "see users from all groups the user manages"
(do
(testing "manager can get all users in their group"
(is (= #{"lucky@metabase.com"
"rasta@metabase.com"}
(->> ((mt/user-http-request :rasta :get 200 "user" :group_id group-id1) :data)
(filter mt/test-user?)
(map :email)
set))))
(testing "manager can't get all users in another group"
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :get 403 "user" :group_id group-id2))))
(testing "manager can get all users"
(is (= #{"lucky@metabase.com"
"rasta@metabase.com"
"crowberto@metabase.com"}
(->> ((mt/user-http-request :lucky :get 200 "user") :data)
(->> ((mt/user-http-request :rasta :get 200 "user") :data)
(filter mt/test-user?)
(map :email)
set)))))
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment