Only check create-queries perms when fetching DB/schema metadata (#45045)

enterprise/backend/src/metabase_enterprise/advanced_permissions/common.clj

@@ -25,8 +25,9 @@
      db-id
      (:table_id instance))))
 
-(defenterprise current-user-can-read-schema?
-  "Enterprise version. Returns a boolean whether the current user can read the given schema"
+(defenterprise current-user-can-manage-schema-metadata?
+  "Enterprise version. Returns a boolean whether the current user has permission to edit table metadata for any tables
+  in the schema"
   :feature :advanced-permissions
   [db-id schema-name]
   (data-perms/user-has-permission-for-schema?

src/metabase/api/database.clj

@@ -176,7 +176,7 @@
                                            [:not= :result_metadata nil]
                                            [:= :archived false]
                                            ;; always return metrics for now
-                                                  [:in :type [(u/qualified-name card-type) "metric"]]
+                                           [:in :type [(u/qualified-name card-type) "metric"]]
                                            [:in :database_id ids-of-dbs-that-support-source-queries]
                                            (collection/visible-collection-ids->honeysql-filter-clause
                                             (collection/permissions-set->visible-collection-ids
@@ -1085,8 +1085,9 @@
 
 ;;; ------------------------------------------ GET /api/database/:id/schemas -----------------------------------------
 
-(defenterprise current-user-can-read-schema?
-  "OSS implementation. Returns a boolean whether the current user can write the given field."
+(defenterprise current-user-can-manage-schema-metadata?
+  "Returns a boolean whether the current user has permission to edit table metadata for any tables in the schema.
+  On OSS, this is only available to admins."
   metabase-enterprise.advanced-permissions.common
   [_db-id _schema-name]
   (mi/superuser?))
@@ -1096,15 +1097,12 @@
   at least some of its tables?)"
   [database-id schema-name]
   (or
-   (and (= :unrestricted (data-perms/full-db-permission-for-user api/*current-user-id*
-                                                                 :perms/view-data
-                                                                 database-id))
-        (contains? #{:query-builder :query-builder-and-native}
-                   (data-perms/schema-permission-for-user api/*current-user-id*
-                                                          :perms/create-queries
-                                                          database-id
-                                                          schema-name)))
-   (current-user-can-read-schema? database-id schema-name)))
+   (contains? #{:query-builder :query-builder-and-native}
+              (data-perms/schema-permission-for-user api/*current-user-id*
+                                                     :perms/create-queries
+                                                     database-id
+                                                     schema-name))
+   (current-user-can-manage-schema-metadata? database-id schema-name)))
 
 (api/defendpoint GET "/:id/syncable_schemas"
   "Returns a list of all syncable schemas found for the database `id`."

src/metabase/models/database.clj

@@ -76,15 +76,11 @@
   ([_model pk]
    (if (should-read-audit-db? pk)
      false
-     (and (= :unrestricted (data-perms/full-db-permission-for-user
-                            api/*current-user-id*
-                            :perms/view-data
-                            pk))
-          (contains? #{:query-builder :query-builder-and-native}
-                    (data-perms/most-permissive-database-permission-for-user
-                     api/*current-user-id*
-                     :perms/create-queries
-                     pk))))))
+     (contains? #{:query-builder :query-builder-and-native}
+                (data-perms/most-permissive-database-permission-for-user
+                 api/*current-user-id*
+                 :perms/create-queries
+                 pk)))))
 
 (defenterprise current-user-can-write-db?
   "OSS implementation. Returns a boolean whether the current user can write the given field."

test/metabase/api/database_test.clj

@@ -194,6 +194,22 @@
               :specific-errors {:include ["should be either tables or tables.fields, received: \"schemas\""]}}
              (mt/user-http-request :lucky :get 400 (format "database/%d?include=schemas" (mt/id))))))))
 
+(deftest get-database-legacy-no-self-service-test
+  (testing "GET /api/database/:id"
+    (testing "A database can be fetched even if one table has legacy-no-self-service permissions"
+        (mt/with-user-in-groups [group {:name "Legacy no-self-service group"}
+                                 user  [group]]
+          (mt/with-temp [:model/Database         {db-id :id}      {}
+                         :model/Table            {table-id-1 :id} {:db_id  db-id}
+                         :model/Table            {table-id-2 :id} {:db_id  db-id}]
+            (mt/with-no-data-perms-for-all-users!
+              ;; Query permissions for a single table is enough to fetch the DB
+              (data-perms/set-table-permission! group table-id-1 :perms/view-data :legacy-no-self-service)
+              (data-perms/set-table-permission! group table-id-1 :perms/create-queries :no)
+              (data-perms/set-table-permission! group table-id-2 :perms/view-data :unrestricted)
+              (data-perms/set-table-permission! group table-id-2 :perms/create-queries :query-builder)
+              (mt/user-http-request user :get 200 (format "database/%d" db-id))))))))
+
 (deftest get-database-can-upload-test
   (testing "GET /api/database"
     (mt/with-discard-model-updates [:model/Database] ; to restore any existing metabase_database.uploads_enabled=true
@@ -1548,7 +1564,6 @@
     (mt/with-temp [Database {db-id :id} {}
                    Table    t1 {:db_id db-id :schema "schema1"}
                    Table    t2 {:db_id db-id :schema "schema1"}]
-
       (testing "should work if user has full DB perms..."
         (is (= ["schema1"]
                (mt/with-full-data-perms-for-all-users!
@@ -1738,17 +1753,25 @@
                    Table    t1  {:db_id db-id :schema "schema1" :name "t1"}
                    Table    _t2 {:db_id db-id :schema "schema2"}
                    Table    t3  {:db_id db-id :schema "schema1" :name "t3"}]
-      (testing "if we have full DB perms"
+      (testing "if we have full data perms for the DB"
         (mt/with-full-data-perms-for-all-users!
           (is (= ["t1" "t3"]
                (map :name (mt/user-http-request :rasta :get 200 (format "database/%d/schema/%s" db-id "schema1")))))))
 
-      (testing "if we have Table perms for all tables in the schema"
+      (testing "if we have query perms for all tables in the schema"
         (mt/with-no-data-perms-for-all-users!
-          (data-perms/set-database-permission! (perms-group/all-users) db-id :perms/view-data :unrestricted)
           (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t1) :perms/create-queries :query-builder)
           (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t3) :perms/create-queries :query-builder)
           (is (= ["t1" "t3"]
+                 (map :name (mt/user-http-request :rasta :get 200 (format "database/%d/schema/%s" db-id "schema1")))))))
+
+      (testing "if we have query perms for one table in the schema, and legacy-no-self-service data perms for another"
+        (mt/with-no-data-perms-for-all-users!
+          (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t1) :perms/view-data :legacy-no-self-service)
+          (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t1) :perms/create-queries :no)
+          (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t3) :perms/view-data :unrestricted)
+          (data-perms/set-table-permission! (perms-group/all-users) (u/the-id t3) :perms/create-queries :query-builder)
+          (is (= ["t3"]
                  (map :name (mt/user-http-request :rasta :get 200 (format "database/%d/schema/%s" db-id "schema1"))))))))
 
     (testing "should return a 403 for a user that doesn't have read permissions"