Skip to content

Commit 5fc29c4

Browse files
edpagetedpaget
committed
fix: improve db drop down perf by prefiltering (#60652)
1 parent 0ba5652 commit 5fc29c4

File tree

6 files changed

+275
-9
lines changed

6 files changed

+275
-9
lines changed

‎src/metabase/permissions/core.clj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
[metabase.permissions.models.data-permissions.sql
5858
UserInfo
5959
PermissionMapping
60+
visible-database-filter-select
6061
visible-table-filter-select]
6162
[metabase.permissions.models.permissions
6263
audit-namespace-clause

‎src/metabase/permissions/models/data_permissions/sql.clj

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,35 @@
135135
[(apply has-perms-for-table-as-honey-sql? user-id perm-type (cond-> perm-level
136136
(not (sequential? perm-level)) vector))]))
137137
permission-mapping))})
138+
139+
(mu/defn- has-perms-for-database-as-honey-sql?
140+
"Builds an EXISTS (SELECT ...) half-join to filter databases that a user has the required permissions for.
141+
Similar to has-perms-for-table-as-honey-sql? but specifically for database-level permissions."
142+
[user-id :- pos-int?
143+
perm-type :- :keyword
144+
required-level :- :keyword
145+
& [most-or-least :- [:maybe [:enum :most :least]]]]
146+
[:exists {:select [1]
147+
:from [[:data_permissions :dp]]
148+
:where [:and
149+
[:= :dp.perm_type (h2x/literal perm-type)]
150+
[:= :md.id :dp.db_id]
151+
(user-in-group-half-join user-id)]
152+
:group-by [:md.id]
153+
:having [(perm-condition perm-type required-level (or most-or-least :least))]}])
154+
155+
(mu/defn visible-database-filter-select
156+
"Selects database IDs that are visible to the provided user given a mapping of permission types to the required value.
157+
Similar to visible-table-filter-select but for databases."
158+
[{:keys [user-id is-superuser?]} :- UserInfo
159+
permission-mapping :- PermissionMapping]
160+
{:select [:md.id]
161+
:from [[:metabase_database :md]]
162+
:where (if is-superuser?
163+
[:= [:inline 1] [:inline 1]]
164+
(into [:and]
165+
(mapcat (fn [[perm-type perm-level]]
166+
[(apply has-perms-for-database-as-honey-sql?
167+
user-id perm-type (cond-> perm-level
168+
(not (sequential? perm-level)) vector))]))
169+
permission-mapping))})

‎src/metabase/warehouses/api.clj

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -256,23 +256,28 @@
256256
(let [filter-on-router-database-id (when (some->> router-database-id
257257
(perms/user-has-permission-for-database? api/*current-user-id* :perms/manage-database :yes))
258258
router-database-id)
259-
dbs (t2/select :model/Database {:order-by [:%lower.name :%lower.engine]
260-
:where [:and
261-
(when-not include-analytics?
262-
[:= :is_audit false])
263-
(if filter-on-router-database-id
264-
[:= :router_database_id router-database-id]
265-
[:= :router_database_id nil])]})
266259
filter-by-data-access? (not (or include-editable-data-model?
267260
exclude-uneditable-details?
268-
filter-on-router-database-id))]
261+
filter-on-router-database-id))
262+
user-info {:user-id api/*current-user-id* :is-superuser? (mi/superuser?)}
263+
permission-mapping {:perms/create-queries :query-builder}
264+
base-where [:and
265+
(when-not include-analytics?
266+
[:= :is_audit false])
267+
(if filter-on-router-database-id
268+
[:= :router_database_id router-database-id]
269+
[:= :router_database_id nil])]
270+
where-clause (if filter-by-data-access?
271+
[:and base-where (mi/visible-filter-clause :model/Database :id user-info permission-mapping)]
272+
base-where)
273+
dbs (t2/select :model/Database {:order-by [:%lower.name :%lower.engine]
274+
:where where-clause})]
269275
(cond-> (add-native-perms-info dbs)
270276
include-tables? add-tables
271277
true add-can-upload-to-dbs
272278
true (t2/hydrate :router_user_attribute)
273279
include-editable-data-model? filter-databases-by-data-model-perms
274280
exclude-uneditable-details? (#(filter (some-fn :is_attached_dwh mi/can-write?) %))
275-
filter-by-data-access? (#(filter mi/can-read? %))
276281
include-saved-questions-db? (add-saved-questions-virtual-database :include-tables? include-saved-questions-tables?)
277282
;; Perms checks for uploadable DBs are handled by exclude-uneditable-details? (see below)
278283
include-only-uploadable? (#(filter uploadable-db? %)))))

‎src/metabase/warehouses/models/database.clj

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@
122122
(and (can-write? pk)
123123
(not (:is_attached_dwh (t2/select-one :model/Database :id pk))))))
124124

125+
(mu/defmethod mi/visible-filter-clause :model/Database
126+
[_model column-or-exp user-info permission-mapping]
127+
[:in column-or-exp
128+
(perms/visible-database-filter-select user-info permission-mapping)])
129+
125130
(defn- infer-db-schedules
126131
"Infer database schedule settings based on its options."
127132
[{:keys [details is_full_sync is_on_demand cache_field_values_schedule metadata_sync_schedule] :as database}]

‎test/metabase/permissions/models/data_permissions_test.clj

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,88 @@
516516
(is (= :query-builder (data-perms/most-permissive-database-permission-for-user
517517
user-id-1 :perms/create-queries database-id-1))))))))
518518

519+
(deftest most-permissive-database-permission-for-user-multiple-groups-test
520+
(testing "most-permissive-database-permission-for-user with multiple groups"
521+
(mt/with-temp [:model/PermissionsGroup {group-id-1 :id} {}
522+
:model/PermissionsGroup {group-id-2 :id} {}
523+
:model/PermissionsGroup {group-id-3 :id} {}
524+
:model/User {user-id :id} {}
525+
:model/PermissionsGroupMembership {} {:user_id user-id
526+
:group_id group-id-1}
527+
:model/PermissionsGroupMembership {} {:user_id user-id
528+
:group_id group-id-2}
529+
:model/PermissionsGroupMembership {} {:user_id user-id
530+
:group_id group-id-3}
531+
:model/Database {database-id :id} {}
532+
:model/Table {table-id-1 :id} {:db_id database-id}
533+
:model/Table {table-id-2 :id} {:db_id database-id}
534+
:model/Table {table-id-3 :id} {:db_id database-id}]
535+
(mt/with-no-data-perms-for-all-users!
536+
;; Clear the default permissions for all groups
537+
(doseq [group-id [group-id-1 group-id-2 group-id-3]]
538+
(t2/delete! :model/DataPermissions :group_id group-id))
539+
540+
(testing "Returns most permissive permission when user has different levels across groups"
541+
;; Group 1: no permissions (least permissive)
542+
(data-perms/set-table-permission! group-id-1 table-id-1 :perms/create-queries :no)
543+
(data-perms/set-table-permission! group-id-1 table-id-2 :perms/create-queries :no)
544+
(data-perms/set-table-permission! group-id-1 table-id-3 :perms/create-queries :no)
545+
546+
;; Group 2: query-builder permissions (medium permissive)
547+
(data-perms/set-table-permission! group-id-2 table-id-1 :perms/create-queries :query-builder)
548+
(data-perms/set-table-permission! group-id-2 table-id-2 :perms/create-queries :query-builder)
549+
(data-perms/set-table-permission! group-id-2 table-id-3 :perms/create-queries :no)
550+
551+
;; Group 3: native permissions (most permissive)
552+
(data-perms/set-table-permission! group-id-3 table-id-1 :perms/create-queries :query-builder-and-native)
553+
(data-perms/set-table-permission! group-id-3 table-id-2 :perms/create-queries :no)
554+
(data-perms/set-table-permission! group-id-3 table-id-3 :perms/create-queries :no)
555+
556+
;; Should return the most permissive permission found across all tables and groups
557+
(is (= :query-builder-and-native
558+
(data-perms/most-permissive-database-permission-for-user
559+
user-id :perms/create-queries database-id))))
560+
561+
(testing "Coalesces permissions correctly for :perms/view-data"
562+
;; Group 1: blocked for all tables
563+
(data-perms/set-table-permission! group-id-1 table-id-1 :perms/view-data :blocked)
564+
(data-perms/set-table-permission! group-id-1 table-id-2 :perms/view-data :blocked)
565+
(data-perms/set-table-permission! group-id-1 table-id-3 :perms/view-data :blocked)
566+
567+
;; Group 2: unrestricted for one table
568+
(data-perms/set-table-permission! group-id-2 table-id-1 :perms/view-data :unrestricted)
569+
(data-perms/set-table-permission! group-id-2 table-id-2 :perms/view-data :blocked)
570+
(data-perms/set-table-permission! group-id-2 table-id-3 :perms/view-data :blocked)
571+
572+
;; Group 3: legacy-no-self-service for remaining tables
573+
(data-perms/set-table-permission! group-id-3 table-id-1 :perms/view-data :blocked)
574+
(data-perms/set-table-permission! group-id-3 table-id-2 :perms/view-data :legacy-no-self-service)
575+
(data-perms/set-table-permission! group-id-3 table-id-3 :perms/view-data :legacy-no-self-service)
576+
577+
;; Should return :unrestricted (most permissive) as per coalesce logic
578+
(is (= :unrestricted
579+
(data-perms/most-permissive-database-permission-for-user
580+
user-id :perms/view-data database-id))))
581+
582+
(testing "Returns correct permission when all groups have same level"
583+
;; All groups have query-builder permission
584+
(data-perms/set-table-permission! group-id-1 table-id-1 :perms/create-queries :query-builder)
585+
(data-perms/set-table-permission! group-id-2 table-id-1 :perms/create-queries :query-builder)
586+
(data-perms/set-table-permission! group-id-3 table-id-1 :perms/create-queries :query-builder)
587+
588+
(is (= :query-builder
589+
(data-perms/most-permissive-database-permission-for-user
590+
user-id :perms/create-queries database-id))))
591+
592+
(testing "Returns least permissive value when no permissions are granted"
593+
;; Remove all permissions
594+
(doseq [group-id [group-id-1 group-id-2 group-id-3]]
595+
(t2/delete! :model/DataPermissions :group_id group-id))
596+
597+
(is (= :no
598+
(data-perms/most-permissive-database-permission-for-user
599+
user-id :perms/create-queries database-id))))))))
600+
519601
(deftest set-new-database-permissions!-test
520602
(mt/with-temp [:model/PermissionsGroup {group-id :id} {}
521603
:model/Database {db-id-1 :id} {}

‎test/metabase/warehouses/models/database_test.clj

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
[metabase.lib.test-util :as lib.tu]
1212
[metabase.models.interface :as mi]
1313
[metabase.models.serialization :as serdes]
14+
[metabase.permissions.core :as perms]
15+
[metabase.permissions.models.data-permissions :as data-perms]
1416
[metabase.query-processor.store :as qp.store]
1517
[metabase.request.core :as request]
1618
[metabase.secrets.core :as secret]
@@ -627,3 +629,142 @@
627629
(t2/hydrate :tables)
628630
:tables
629631
(#(map :name %))))))
632+
633+
(deftest visible-filter-clause-test
634+
(testing "Database visible-filter-clause generates a HoneySQL clause that filters databases based on user permissions"
635+
(mt/with-no-data-perms-for-all-users!
636+
(mt/with-temp [:model/Database {db1-id :id} {:name "Database 1"}
637+
:model/Database {db2-id :id} {:name "Database 2"}
638+
:model/Database {db3-id :id} {:name "Database 3"}
639+
:model/Table _ {:db_id db1-id :name "Table1"}
640+
:model/Table _ {:db_id db2-id :name "Table2"}
641+
:model/Table _ {:db_id db3-id :name "Table3"}
642+
:model/PermissionsGroup pg1 {}
643+
:model/PermissionsGroup pg2 {}]
644+
645+
;; Set up permissions: user has access to db1 and db2 via different groups, but not db3
646+
(perms/add-user-to-group! (mt/user->id :rasta) pg1)
647+
(perms/add-user-to-group! (mt/user->id :rasta) pg2)
648+
649+
;; Clear existing permissions for our test databases only
650+
(t2/delete! :model/DataPermissions :db_id [:in [db1-id db2-id db3-id]])
651+
652+
;; Grant permissions to specific databases
653+
(data-perms/set-database-permission! pg1 db1-id :perms/view-data :unrestricted)
654+
(data-perms/set-database-permission! pg1 db1-id :perms/create-queries :query-builder)
655+
656+
(data-perms/set-database-permission! pg2 db2-id :perms/view-data :unrestricted)
657+
(data-perms/set-database-permission! pg2 db2-id :perms/create-queries :query-builder)
658+
659+
;; No permissions for db3
660+
(data-perms/set-database-permission! pg1 db3-id :perms/view-data :blocked)
661+
(data-perms/set-database-permission! pg2 db3-id :perms/view-data :blocked)
662+
663+
(let [user-id (mt/user->id :rasta)
664+
fetch-visible-ids (fn [user-info permission-mapping column-field]
665+
(let [filter-clause (mi/visible-filter-clause :model/Database column-field user-info permission-mapping)]
666+
(t2/select-pks-set :model/Database
667+
{:where [:and
668+
filter-clause
669+
[:in :id [db1-id db2-id db3-id]]]})))] ; Only check our test databases
670+
671+
(testing "Superuser should see all databases"
672+
(let [user-info {:user-id user-id :is-superuser? true}
673+
permission-mapping {:perms/view-data :unrestricted
674+
:perms/create-queries :query-builder}]
675+
(is (= #{db1-id db2-id db3-id}
676+
(fetch-visible-ids user-info permission-mapping :id))
677+
"Superuser should see all databases")))
678+
679+
(testing "Non-superuser should only see databases they have permissions for"
680+
(let [user-info {:user-id user-id :is-superuser? false}
681+
permission-mapping {:perms/view-data :unrestricted
682+
:perms/create-queries :query-builder}]
683+
(is (= #{db1-id db2-id}
684+
(fetch-visible-ids user-info permission-mapping :id))
685+
"Non-superuser should only see databases with sufficient permissions")))
686+
687+
(testing "Using qualified column name"
688+
(let [user-info {:user-id user-id :is-superuser? false}
689+
permission-mapping {:perms/view-data :unrestricted
690+
:perms/create-queries :query-builder}]
691+
(is (= #{db1-id db2-id}
692+
(fetch-visible-ids user-info permission-mapping :metabase_database.id))
693+
"Should work with qualified column names")))
694+
695+
(testing "Using column expression"
696+
(let [user-info {:user-id user-id :is-superuser? false}
697+
permission-mapping {:perms/view-data :unrestricted
698+
:perms/create-queries :query-builder}]
699+
(is (= #{db1-id db2-id}
700+
(fetch-visible-ids user-info permission-mapping [:coalesce :id :metabase_database.id]))
701+
"Should work with column expressions")))
702+
703+
(testing "Different permission levels"
704+
(testing "Requiring only view-data permissions"
705+
(let [user-info {:user-id user-id :is-superuser? false}
706+
permission-mapping {:perms/view-data :unrestricted
707+
:perms/create-queries :no}]
708+
(is (= #{db1-id db2-id}
709+
(fetch-visible-ids user-info permission-mapping :id))
710+
"Should include databases where user has view permissions")))
711+
712+
(testing "Requiring blocked level permissions (most permissive)"
713+
(let [user-info {:user-id user-id :is-superuser? false}
714+
permission-mapping {:perms/view-data :blocked
715+
:perms/create-queries :no}]
716+
(is (= #{db1-id db2-id db3-id}
717+
(fetch-visible-ids user-info permission-mapping :id))
718+
"Should include all databases when requiring only blocked level"))))
719+
720+
(testing "User with no permissions"
721+
;; Remove user from groups we added (avoid touching All Users group)
722+
(t2/delete! :model/PermissionsGroupMembership
723+
:user_id user-id
724+
:group_id [:in [(:id pg1) (:id pg2)]])
725+
726+
(let [user-info {:user-id user-id :is-superuser? false}
727+
permission-mapping {:perms/view-data :unrestricted
728+
:perms/create-queries :query-builder}]
729+
(is (empty? (fetch-visible-ids user-info permission-mapping :id))
730+
"User with no group memberships should see no databases"))))))))
731+
732+
(deftest visible-filter-clause-table-level-permissions-test
733+
(testing "Database visible-filter-clause with table-level permissions"
734+
(mt/with-no-data-perms-for-all-users!
735+
(mt/with-temp [:model/Database {db-id :id} {:name "Test Database"}
736+
:model/Table {table1-id :id} {:db_id db-id :name "Table1"}
737+
:model/Table {table2-id :id} {:db_id db-id :name "Table2"}
738+
:model/Table _ {:db_id db-id :name "Table3"}
739+
:model/PermissionsGroup pg {}]
740+
741+
(perms/add-user-to-group! (mt/user->id :rasta) pg)
742+
743+
;; Clear existing permissions for our test database only
744+
(t2/delete! :model/DataPermissions :db_id db-id)
745+
746+
;; Block database-level access
747+
(data-perms/set-database-permission! pg db-id :perms/view-data :blocked)
748+
(data-perms/set-database-permission! pg db-id :perms/create-queries :no)
749+
750+
;; Grant table-level permissions to only table1 and table2
751+
(data-perms/set-table-permission! pg table1-id :perms/view-data :unrestricted)
752+
(data-perms/set-table-permission! pg table1-id :perms/create-queries :query-builder)
753+
754+
(data-perms/set-table-permission! pg table2-id :perms/view-data :unrestricted)
755+
(data-perms/set-table-permission! pg table2-id :perms/create-queries :query-builder)
756+
757+
;; table3 remains blocked
758+
759+
(let [user-id (mt/user->id :rasta)
760+
user-info {:user-id user-id :is-superuser? false}
761+
permission-mapping {:perms/view-data :unrestricted
762+
:perms/create-queries :query-builder}
763+
filter-clause (mi/visible-filter-clause :model/Database :id user-info permission-mapping)
764+
visible-db-ids (t2/select-pks-set :model/Database
765+
{:where [:and
766+
filter-clause
767+
[:= :id db-id]]})] ; Only check our test database
768+
769+
(is (contains? visible-db-ids db-id)
770+
"Database should be visible when user has access to at least one table"))))))

0 commit comments

Comments
 (0)