Merge pull request #3762 from metabase/fix-inactive-users-being-returned-by-perms-endpoints

Don't return inactive users with permissions endpoints 

src/metabase/api/permissions.clj

@@ -83,11 +83,12 @@
   (check-superuser)
   (db/query {:select    [:pg.id :pg.name [:%count.pgm.id :members]]
              :from      [[:permissions_group :pg]]
-             :left-join [[:permissions_group_membership :pgm]
-                         [:= :pg.id :pgm.group_id]]
-             :where     (if (metabot/metabot-enabled)
-                          true
-                          [:not= :pg.id (:id (group/metabot))])
+             :left-join [[:permissions_group_membership :pgm] [:= :pg.id :pgm.group_id]
+                         [:core_user :user]                   [:= :pgm.user_id :user.id]]
+             :where     [:and [:= :user.is_active true]
+                              (if (metabot/metabot-enabled)
+                                true
+                                [:not= :pg.id (:id (group/metabot))])]
              :group-by  [:pg.id :pg.name]
              :order-by  [:%lower.pg.name]}))
 

src/metabase/models/permissions_group.clj

@@ -90,13 +90,14 @@
 (defn ^:hydrate members
   "Return `Users` that belong to GROUP-OR-ID, ordered by their name (case-insensitive)."
   [group-or-id]
-  (db/query {:select    [:core_user.first_name
-                         :core_user.last_name
-                         :core_user.email
-                         [:core_user.id :user_id]
-                         [:permissions_group_membership.id :membership_id]]
-             :from      [:core_user]
-             :left-join [:permissions_group_membership [:= :core_user.id :permissions_group_membership.user_id]]
-             :where     [:= :permissions_group_membership.group_id (u/get-id group-or-id)]
-             :order-by  [[:%lower.core_user.first_name :asc]
-                         [:%lower.core_user.last_name :asc]]}))
+  (db/query {:select    [:user.first_name
+                         :user.last_name
+                         :user.email
+                         [:user.id :user_id]
+                         [:pgm.id :membership_id]]
+             :from      [[:core_user :user]]
+             :left-join [[:permissions_group_membership :pgm] [:= :user.id :pgm.user_id]]
+             :where     [:and [:= :user.is_active true]
+                              [:= :pgm.group_id (u/get-id group-or-id)]]
+             :order-by  [[:%lower.user.first_name :asc]
+                         [:%lower.user.last_name :asc]]}))

test/metabase/api/permissions_test.clj

+(ns metabase.api.permissions-test
+  "Tests for `api/permissions` endpoints."
+  (:require [expectations :refer :all]
+            [metabase.test.data.users :as tu]
+            [metabase.models.permissions-group :as group]
+            [metabase.util :as u]))
+
+
+;; GET /group
+;; Should *not* include inactive users in the counts.
+;; It should also *not* include the MetaBot group because MetaBot should *not* be enabled
+(expect
+  #{{:id (u/get-id (group/all-users)), :name "All Users",      :members 3}
+    {:id (u/get-id (group/admin)),     :name "Administrators", :members 1}}
+  (do
+    (tu/delete-temp-users!)
+    (set ((tu/user->client :crowberto) :get 200 "permissions/group"))))
+
+;; GET /group/:id
+;; Should *not* include inactive users
+(expect
+  #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (tu/user->id :crowberto), :membership_id true}
+    {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (tu/user->id :lucky),     :membership_id true}
+    {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (tu/user->id :rasta),     :membership_id true}}
+  (do
+    (tu/delete-temp-users!)
+    (set (for [member (:members ((tu/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
+           (update member :membership_id (complement nil?))))))

test/metabase/test/data/users.clj

@@ -125,3 +125,10 @@
           ;; If we got a 401 unauthenticated clear the tokens cache + recur
           (reset! tokens {})
           (apply client-fn args))))))
+
+
+(defn delete-temp-users!
+  "Delete all users besides the 4 persistent test users.
+   This is a HACK to work around tests that don't properly clean up after themselves; one day we should be able to remove this. (TODO)"
+  []
+  (db/cascade-delete! 'User :id [:not-in (map user->id [:crowberto :lucky :rasta :trashbird])]))