Add table-privileges driver feature and sync table-privileges on sync (#34128)

docs/developers-guide/driver-changelog.md

@@ -9,10 +9,13 @@ title: Driver interface changelog
 - The MBQL schema in `metabase.mbql.schema` now uses [Malli](https://github.com/metosin/malli) instead of
   [Schema](https://github.com/plumatic/schema). If you were using this namespace in combination with Schema, you'll
   want to update your code to use Malli instead.
-
-- The multimethod `metabase.driver/current-user-table-privileges` has been added. This method is used to get
-  the set of privileges the database connection's current user has. It needs to be implemented if the database
-  supports the `:actions` feature.
+  
+- Another driver feature has been added: `:table-privileges`. This feature signals whether we can store
+  the table-level privileges for the database on database sync.
+  
+- The multimethod `metabase.driver/current-user-table-privileges` has been added. This method is used to get 
+  the set of privileges the database connection's current user has. It needs to be implemented if the database 
+  supports the `:table-privileges` feature.
 
 - The following functions in `metabase.query-processor.store` (`qp.store`) are now deprecated
 

src/metabase/driver.clj

@@ -507,6 +507,9 @@
     ;; Does the driver support experimental "writeback" actions like "delete this row" or "insert a new row" from 44+?
     :actions
 
+    ;; Does the driver support storing table privileges in the application database for the current user?
+    :table-privileges
+
     ;; Does the driver support uploading files
     :uploads
 

src/metabase/driver/mysql.clj

@@ -85,6 +85,10 @@
   [database]
   (-> database :dbms_version :flavor (= "MariaDB")))
 
+(defmethod driver/database-supports? [:mysql :table-privileges]
+  [driver _feat db]
+  (and (= driver :mysql) (not (mariadb? db))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                             metabase.driver impls                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+

src/metabase/driver/postgres.clj

@@ -70,7 +70,10 @@
 
 (defmethod driver/display-name :postgres [_] "PostgreSQL")
 
-(doseq [feature [:actions :actions/custom :uploads]]
+(doseq [feature [:actions
+                 :actions/custom
+                 :table-privileges
+                 :uploads]]
   (defmethod driver/database-supports? [:postgres feature]
     [driver _feat _db]
     ;; only supported for Postgres for right now. Not supported for child drivers like Redshift or whatever.

src/metabase/sync/sync_metadata.clj

@@ -13,6 +13,7 @@
    [metabase.sync.sync-metadata.fields :as sync-fields]
    [metabase.sync.sync-metadata.fks :as sync-fks]
    [metabase.sync.sync-metadata.metabase-metadata :as metabase-metadata]
+   [metabase.sync.sync-metadata.sync-table-privileges :as sync-table-privileges]
    [metabase.sync.sync-metadata.sync-timezone :as sync-tz]
    [metabase.sync.sync-metadata.tables :as sync-tables]
    [metabase.sync.util :as sync-util]
@@ -50,7 +51,9 @@
    ;; Now for each table, sync the FKS. This has to be done after syncing all the fields to make sure target fields exist
    (sync-util/create-sync-step "sync-fks" sync-fks/sync-fks! sync-fks-summary)
    ;; finally, sync the metadata metadata table if it exists.
-   (sync-util/create-sync-step "sync-metabase-metadata" #(metabase-metadata/sync-metabase-metadata! % db-metadata))])
+   (sync-util/create-sync-step "sync-metabase-metadata" #(metabase-metadata/sync-metabase-metadata! % db-metadata))
+   ;; Now sync the table privileges
+   (sync-util/create-sync-step "sync-table-privileges" sync-table-privileges/sync-table-privileges!)])
 
 (mu/defn sync-db-metadata!
   "Sync the metadata for a Metabase `database`. This makes sure child Table & Field objects are synchronized."

src/metabase/sync/sync_metadata/sync_table_privileges.clj

@@ -14,9 +14,7 @@
    This is a cache of the data returned from `driver/table-privileges`, but it's stored in the database for performance."
   [database :- (mi/InstanceOf :model/Database)]
   (let [driver (driver.u/database->driver database)]
-    ;; The following expression should be replaced by (when (driver/supports? :actions driver) ...)
-    ;; when table_privileges are implemented for all the actions drivers
-    (when (contains? (methods driver/current-user-table-privileges) driver)
+    (when (driver/database-supports? driver :table-privileges database)
       (let [rows               (driver/current-user-table-privileges driver database)
             schema+table->id   (t2/select-fn->pk (fn [t] {:schema (:schema t), :table (:name t)}) :model/Table :db_id (:id database))
             rows-with-table-id (keep (fn [row]

test/metabase/driver_test.clj

@@ -66,3 +66,7 @@
         (is (nil? (->> (driver/describe-database driver/*driver* (mt/db))
                        :tables
                        (some :schema))))))))
+
+(deftest supports-table-privileges-matches-implementations-test
+  (mt/test-drivers (mt/normal-drivers-with-feature :table-privileges)
+    (is (some? (driver/current-user-table-privileges driver/*driver* (mt/db))))))

test/metabase/sync/sync_metadata/sync_table_privileges_test.clj

 (ns metabase.sync.sync-metadata.sync-table-privileges-test
   (:require
    [clojure.java.jdbc :as jdbc]
+   [clojure.set :as set]
    [clojure.test :refer :all]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.sync.sync-metadata.sync-table-privileges :as sync-table-privileges]
@@ -11,11 +12,9 @@
 (set! *warn-on-reflection* true)
 
 (deftest sync-table-privileges!-test
-  ;; TODO: The following expression should be replaced by
-  ;; (mt/test-drivers (mt/normal-drivers-with-feature :table-privileges) ...)
-  ;; and we should use more general DDL functions to create tables
-  (mt/test-drivers #{:postgres}
-    (testing "`TablePrivileges` should store the correct data for current_user and role privileges for postgres"
+  (mt/test-drivers (set/intersection (mt/normal-drivers-with-feature :table-privileges)
+                                     (mt/normal-drivers-with-feature :schemas))
+    (testing "`TablePrivileges` should store the correct data for current_user and role privileges for databases with schemas"
       (mt/with-empty-db
         (let [conn-spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
           (jdbc/execute! conn-spec (str "CREATE SCHEMA foo; "
@@ -30,19 +29,19 @@
                       :insert          true
                       :update          true}]
                     (t2/select :model/TablePrivileges :table_id table-id :role nil))))))))
-  (mt/test-drivers #{:mysql}
-    (when (-> (mt/db) :dbms_version :flavor (not= "MariaDB"))
-      (testing "`TablePrivileges` should store the correct data for current_user and role privileges for postgres"
-        (mt/with-empty-db
-          (let [conn-spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
-            (jdbc/execute! conn-spec (str "CREATE TABLE baz (id INTEGER);"))
-            (sync-tables/sync-tables-and-database! (mt/db))
-            (sync-table-privileges/sync-table-privileges! (mt/db))
-            (let [table-id (t2/select-one-pk :model/Table :name "baz" :schema nil)]
-              (is (=? [{:table_id        table-id
-                        :role            nil
-                        :select          true
-                        :delete          true
-                        :insert          true
-                        :update          true}]
-                      (t2/select :model/TablePrivileges :table_id table-id :role nil))))))))))
+  (mt/test-drivers (set/difference (mt/normal-drivers-with-feature :table-privileges)
+                                   (mt/normal-drivers-with-feature :schemas))
+    (testing "`TablePrivileges` should store the correct data for current_user and role privileges for databases without schemas"
+      (mt/with-empty-db
+        (let [conn-spec (sql-jdbc.conn/db->pooled-connection-spec (mt/db))]
+          (jdbc/execute! conn-spec (str "CREATE TABLE baz (id INTEGER);"))
+          (sync-tables/sync-tables-and-database! (mt/db))
+          (sync-table-privileges/sync-table-privileges! (mt/db))
+          (let [table-id (t2/select-one-pk :model/Table :name "baz" :schema nil)]
+            (is (=? [{:table_id        table-id
+                      :role            nil
+                      :select          true
+                      :delete          true
+                      :insert          true
+                      :update          true}]
+                    (t2/select :model/TablePrivileges :table_id table-id :role nil)))))))))