Fixes for audit DB permission checks & defaults (#36697)

Co-authored-by: bryan <bryan.maass@gmail.com>

enterprise/backend/src/metabase_enterprise/audit_app/permissions.clj

@@ -2,6 +2,7 @@
   (:require
    [metabase-enterprise.audit-db :refer [default-audit-collection]]
    [metabase.lib.metadata :as lib.metadata]
+   [metabase.models.interface :as mi]
    [metabase.models.permissions :as perms]
    [metabase.models.query.permissions :as query-perms]
    [metabase.public-settings.premium-features :refer [defenterprise]]
@@ -28,10 +29,17 @@
     "v_view_log"})
 
 (defenterprise check-audit-db-permissions
-  "Checks that a given query is not a native query, and only includes table IDs corresponding to the audit views
-  listed above. (These should be the only tables present in the DB anyway, but this is an extra check as a fallback measure)."
+  "Performs a number of permission checks to ensure that a query on the Audit database can be run.
+   Causes for rejection are:
+      - if the current user does not have access to the analytics collection
+      - native queries
+      - queries that include tables that are not audit views"
   :feature :audit-app
   [{query-type :type, database-id :database, query :query :as outer-query}]
+  ;; Check if the user has access to the analytics collection, since this should be coupled with access to the
+  ;; audit database in general.
+  (when-not (mi/can-read? (default-audit-collection))
+    (throw (ex-info (tru "You do not have access to the audit database") outer-query)))
   ;; query->source-table-ids returns a set of table IDs and/or the ::query-perms/native keyword
   (when (= query-type :native)
     (throw (ex-info (tru "Native queries are not allowed on the audit database")
@@ -48,7 +56,9 @@
                           outer-query)))))))
 
 (defenterprise update-audit-collection-permissions!
-  "Will remove or grant audit db (AppDB) permissions, if the instance analytics permissions changes."
+  "Will remove or grant audit db (AppDB) permissions, if the instance analytics collection permissions changes. This
+  technically isn't necessary, because we block all audit DB queries if a user doesn't have collection permissions.
+  But it's cleaner to keep the audit DB permission paths in the database consistent."
   :feature :audit-app
   [group-id changes]
   (let [[change-id type] (first (filter #(= (first %) (:id (default-audit-collection))) changes))]

enterprise/backend/src/metabase_enterprise/audit_db.clj

@@ -100,8 +100,6 @@
   - This uses a weird ID because some tests were hardcoded to look for database with ID = 2, and inserting an extra db
   throws that off since these IDs are sequential."
   [engine id]
-  ;; guard against someone manually deleting the audit-db entry, but not removing the audit-db permissions.
-  (t2/delete! :permissions {:where [:like :object (str "%/db/" id "/%")]})
   (t2/insert! Database {:is_audit         true
                         :id               id
                         :name             "Internal Metabase Database"
@@ -110,7 +108,9 @@
                         :is_full_sync     true
                         :is_on_demand     false
                         :creator_id       nil
-                        :auto_run_queries true}))
+                        :auto_run_queries true})
+  ;; guard against someone manually deleting the audit-db entry, but not removing the audit-db permissions.
+  (t2/delete! :model/Permissions {:where [:like :object (str "%/db/" id "/%")]}))
 
 (defn- adjust-audit-db-to-source!
   [{audit-db-id :id}]

enterprise/backend/test/metabase_enterprise/audit_app/permissions_test.clj

@@ -36,29 +36,28 @@
            (into #{}
                  (map :table_name (t2/query view-query))))))))
 
-;; TODO (noahmoss): re-enable this test once it is no longer flaky
-#_(deftest audit-db-basic-query-test
-    (mt/test-drivers #{:postgres :h2 :mysql}
-      (audit-db-test/with-audit-db-restoration
-        (premium-features-test/with-premium-features #{:audit-app}
-          (mt/with-test-user :crowberto
-            (testing "A query using a saved audit model as the source table runs succesfully"
-              (let [audit-card (t2/select-one :model/Card :database_id perms/audit-db-id :dataset true)]
-                (is (partial=
-                     {:status :completed}
-                     (qp/process-query
-                      {:database perms/audit-db-id
-                       :type     :query
-                       :query    {:source-table (str "card__" (u/the-id audit-card))}})))))
+(deftest audit-db-basic-query-test
+  (mt/test-drivers #{:postgres :h2 :mysql}
+    (audit-db-test/with-audit-db-restoration
+      (premium-features-test/with-premium-features #{:audit-app}
+        (mt/with-test-user :crowberto
+          (testing "A query using a saved audit model as the source table runs succesfully"
+            (let [audit-card (t2/select-one :model/Card :database_id perms/audit-db-id :dataset true)]
+              (is (partial=
+                   {:status :completed}
+                   (qp/process-query
+                    {:database perms/audit-db-id
+                     :type     :query
+                     :query    {:source-table (str "card__" (u/the-id audit-card))}})))))
 
-            (testing "A non-native query can be run on views in the audit DB"
-              (let [audit-view (t2/select-one :model/Table :db_id perms/audit-db-id)]
-                (is (partial=
-                     {:status :completed}
-                     (qp/process-query
-                      {:database perms/audit-db-id
-                       :type     :query
-                       :query    {:source-table (u/the-id audit-view)}}))))))))))
+          (testing "A non-native query can be run on views in the audit DB"
+            (let [audit-view (t2/select-one :model/Table :db_id perms/audit-db-id)]
+              (is (partial=
+                   {:status :completed}
+                   (qp/process-query
+                    {:database perms/audit-db-id
+                     :type     :query
+                     :query    {:source-table (u/the-id audit-view)}}))))))))))
 
 (deftest audit-db-disallowed-queries-test
   (mt/test-drivers #{:postgres :h2 :mysql}
@@ -86,7 +85,19 @@
                    (qp/process-query
                     {:database perms/audit-db-id
                      :type     :query
-                     :query   {:source-table (u/the-id table)}}))))))))))
+                     :query   {:source-table (u/the-id table)}})))))
+
+          (testing "Users without access to the audit collection cannot run any queries on the audit DB, even if they
+                   have data perms for the audit DB"
+            (binding [api/*current-user-permissions-set* (delay #{(perms/data-perms-path perms/audit-db-id)})]
+              (let [audit-view (t2/select-one :model/Table :db_id perms/audit-db-id)]
+                (is (thrown-with-msg?
+                     clojure.lang.ExceptionInfo
+                     #"You do not have access to the audit database"
+                     (qp/process-query
+                      {:database perms/audit-db-id
+                       :type     :query
+                       :query    {:source-table (u/the-id audit-view)}})))))))))))
 
 (deftest permissions-instance-analytics-audit-v2-test
   (premium-features-test/with-premium-features #{:audit-app}

enterprise/backend/test/metabase_enterprise/audit_db_test.clj

@@ -48,6 +48,9 @@
       (is (not= 0 (t2/count :model/Card {:where [:= :database_id perms/audit-db-id]}))
           "Cards should be created for Audit DB when the content is there."))
 
+    (testing "Only admins have data perms for the audit DB after it is installed"
+      (is (not (t2/exists? :model/Permissions {:where [:like :object (str "%" perms/audit-db-id "%")]}))))
+
     (testing "Audit DB does not have scheduled syncs"
       (let [db-has-sync-job-trigger? (fn [db-id]
                                        (contains?