Delete appropriate GTAPs when permissions change (#16222)

enterprise/backend/src/metabase_enterprise/sandbox/models/permissions/delete_sandboxes.clj

+(ns metabase-enterprise.sandbox.models.permissions.delete-sandboxes
+  (:require [clojure.tools.logging :as log]
+            [metabase-enterprise.enhancements.ee-strategy-impl :as ee-strategy-impl]
+            [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
+            [metabase.models.permissions.delete-sandboxes :as delete-sandboxes]
+            [metabase.models.table :refer [Table]]
+            [metabase.public-settings.metastore :as settings.metastore]
+            [metabase.util :as u]
+            [metabase.util.i18n :refer [tru]]
+            [pretty.core :as pretty]
+            [toucan.db :as db]))
+
+(defn- delete-gtaps-with-condition! [group-or-id condition]
+  (when (seq condition)
+    (let [conditions (into
+                      [:and
+                       [:= :gtap.group_id (u/the-id group-or-id)]]
+                      [condition])]
+      (log/debugf "Deleting GTAPs for Group %d with conditions %s" (u/the-id group-or-id) (pr-str conditions))
+      (try
+        (if-let [gtap-ids (not-empty (set (map :id (db/query
+                                                      {:select    [[:gtap.id :id]]
+                                                       :from      [[GroupTableAccessPolicy :gtap]]
+                                                       :left-join [[Table :table]
+                                                                   [:= :gtap.table_id :table.id]]
+                                                       :where     conditions}))))]
+          (do
+            (log/debugf "Deleting %d matching GTAPs: %s" (count gtap-ids) (pr-str gtap-ids))
+            (db/delete! GroupTableAccessPolicy :id [:in gtap-ids]))
+          (log/debug "No matching GTAPs need to be deleted."))
+        (catch Throwable e
+          (throw (ex-info (tru "Error deleting Sandboxes: {0}" (ex-message e))
+                          {:group (u/the-id group-or-id), :conditions conditions}
+                          e)))))))
+
+(defn- delete-gtaps-for-group-table! [{:keys [group-id table-id], :as context} changes]
+  (log/debugf "Deleting unneeded GTAPs for Group %d for Table %d. Graph changes: %s"
+             group-id table-id (pr-str changes))
+  (cond
+    (= changes :none)
+    (do
+      (log/debugf "Group %d no longer has any permissions for Table %d, deleting GTAP for this Table if one exists"
+                 group-id table-id)
+      (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+    (= changes :all)
+    (do
+      (log/debugf "Group %d now has full data perms for Table %d, deleting GTAP for this Table if one exists"
+                 group-id table-id)
+      (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+    :else
+    (let [new-query-perms (get changes :query :none)]
+      (case new-query-perms
+        :none
+        (do
+          (log/debugf "Group %d no longer has any query perms for Table %d; deleting GTAP for this Table if one exists"
+                     group-id table-id)
+          (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+        :all
+        (do
+          (log/debugf "Group %d now has full non-sandboxed query perms for Table %d; deleting GTAP for this Table if one exists"
+                     group-id table-id)
+          (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+        :segmented
+        (log/debugf "Group %d now has full segmented query perms for Table %d. Do not need to delete GTAPs."
+                   group-id table-id)))))
+
+(defn- delete-gtaps-for-group-schema! [{:keys [group-id database-id schema-name], :as context} changes]
+  (log/debugf "Deleting unneeded GTAPs for Group %d for Database %d, schema %s. Graph changes: %s"
+             group-id database-id (pr-str schema-name) (pr-str changes))
+  (cond
+    (= changes :none)
+    (do
+      (log/debugf "Group %d no longer has any permissions for Database %d schema %s, deleting all GTAPs for this schema"
+                  group-id database-id (pr-str schema-name))
+      (delete-gtaps-with-condition! group-id [:and [:= :table.db_id database-id] [:= :table.schema schema-name]]))
+
+    (= changes :all)
+    (do
+      (log/debugf "Group %d changes has full data perms for Database %d schema %s, deleting all GTAPs for this schema"
+                  group-id database-id (pr-str schema-name))
+      (delete-gtaps-with-condition! group-id [:and [:= :table.db_id database-id] [:= :table.schema schema-name]]))
+
+    :else
+    (doseq [table-id (set (keys changes))]
+      (delete-gtaps-for-group-table! (assoc context :table-id table-id) (get changes table-id)))))
+
+(defn- delete-gtaps-for-group-database! [{:keys [group-id database-id], :as context} changes]
+  (log/debugf "Deleting unneeded GTAPs for Group %d for Database %d. Graph changes: %s"
+             group-id database-id (pr-str changes))
+  (cond
+    (= changes :none)
+    (do
+      (log/debugf "Group %d no longer has any perms for Database %d, deleting all GTAPs for this DB" group-id database-id)
+      (delete-gtaps-with-condition! group-id [:= :table.db_id database-id]))
+
+    (= changes :all)
+    (do
+      (log/debugf "Group %d now has full data perms for Database %d, deleting all GTAPs for this DB" group-id database-id)
+      (delete-gtaps-with-condition! group-id [:= :table.db_id database-id]))
+
+    :else
+    (doseq [schema-name (set (keys changes))]
+      (delete-gtaps-for-group-schema!
+       (assoc context :schema-name schema-name)
+       (get changes schema-name)))))
+
+(defn- delete-gtaps-for-group! [{:keys [group-id]} changes]
+  (log/debugf "Deleting unneeded GTAPs for Group %d. Graph changes: %s" group-id (pr-str changes))
+  (doseq [database-id (set (keys changes))]
+    (delete-gtaps-for-group-database!
+     {:group-id group-id, :database-id database-id}
+     (get-in changes [database-id :schemas]))))
+
+(defn- delete-gtaps-if-needed-after-permissions-change! [changes]
+  (log/debug "Permissions updated, deleting unneeded GTAPs...")
+  (doseq [group-id (set (keys changes))]
+    (delete-gtaps-for-group! {:group-id group-id} (get changes group-id)))
+  (log/debug "Done deleting unneeded GTAPs."))
+
+(def ^:private impl
+  (reify
+    delete-sandboxes/DeleteSandboxes
+    (delete-gtaps-if-needed-after-permissions-change!* [_ changes]
+      (delete-gtaps-if-needed-after-permissions-change! changes))
+
+    pretty/PrettyPrintable
+    (pretty [_]
+      `impl)))
+
+(def ee-strategy-impl
+  "EE impl for Sandbox (GTAP) deletion behavior. Don't use this directly."
+  (ee-strategy-impl/reify-ee-strategy-impl
+    #'settings.metastore/enable-sandboxes?
+    impl
+    delete-sandboxes/oss-default-impl
+    delete-sandboxes/DeleteSandboxes))

enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj

+(ns metabase-enterprise.sandbox.api.permissions-test
+  (:require [clojure.test :refer :all]
+            [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
+            [metabase.models :refer [Database PermissionsGroup Table]]
+            [metabase.models.permissions-group :as group]
+            [metabase.test :as mt]
+            [metabase.util :as u]
+            [metabase.util.schema :as su]
+            [schema.core :as s]
+            [toucan.db :as db]))
+
+(defn- id->keyword [id]
+  (keyword (str (u/the-id id))))
+
+(defn- db-graph-keypath [group]
+  [:groups (id->keyword group) (id->keyword (mt/id))])
+
+(defn- venues-perms-graph-keypath [group]
+  (concat
+   (db-graph-keypath group)
+   [:schemas :PUBLIC (id->keyword (mt/id :venues))]))
+
+(deftest revoke-perms-delete-gtaps-test
+  (testing "PUT /api/permissions/graph"
+    (testing "removing sandboxed permissions for a group should delete the associated GTAP (#16190)"
+      (doseq [[message {:keys [updated-db-perms expected-perms]}]
+              {"when revoking all permissions for DB"
+               {:updated-db-perms (constantly {:native :none, :schemas :none})
+                :expected-perms   (constantly nil)}
+
+               "when revoking all permissions for schema"
+               {:updated-db-perms (constantly {:native :none, :schemas {:PUBLIC :none}})
+                :expected-perms   (constantly nil)}
+
+               "when revoking all permissions for Table"
+               {:updated-db-perms (fn []
+                                    {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) :none
+                                                                       (id->keyword (mt/id :users))  :all}}})
+                :expected-perms   (fn []
+                                    {:schemas {:PUBLIC {(id->keyword (mt/id :users)) "all"}}})}
+
+               "when revoking segmented query permissions for Table"
+               {:updated-db-perms (fn []
+                                    {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:read :all}}}})
+                :expected-perms   (fn []
+                                    {:schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:read "all"}}}})}
+
+               "when changing permissions for DB to unrestricted access"
+               {:updated-db-perms (constantly {:native :none, :schemas :all})
+                :expected-perms   (constantly {:schemas "all"})}
+
+               "when changing permissions for schema to unrestricted access"
+               {:updated-db-perms (constantly {:native :none, :schemas {:PUBLIC :all}})
+                :expected-perms   (constantly {:schemas {:PUBLIC "all"}})}
+
+               "when changing permissions for Table to :query [grant full query perms]"
+               {:updated-db-perms (fn []
+                                    {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:query :all}}}})
+                :expected-perms   (fn []
+                                    {:schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:query "all"}}}})}}]
+        (mt/with-gtaps {:gtaps {:venues {}}}
+          (testing message
+            (testing "sanity check"
+              (testing "perms graph endpoint should return segmented perms for Venues table"
+                (is (= {:query "segmented"}
+                       (get-in (mt/user-http-request :crowberto :get 200 "permissions/graph")
+                               (venues-perms-graph-keypath &group)))))
+              (testing "GTAP should exist in application DB"
+                (is (schema= [(s/one {:id                   su/IntGreaterThanZero
+                                      :group_id             (s/eq (u/the-id &group))
+                                      :table_id             (s/eq (mt/id :venues))
+                                      :card_id              (s/eq nil)
+                                      :attribute_remappings (s/eq nil)
+                                      s/Keyword             s/Any}
+                                     "GTAP")]
+                             (db/select GroupTableAccessPolicy :group_id (u/the-id &group))))))
+            (let [graph    (mt/user-http-request :crowberto :get 200 "permissions/graph")
+                  graph'   (assoc-in graph (db-graph-keypath &group) (updated-db-perms))
+                  response (mt/user-http-request :crowberto :put 200 "permissions/graph" graph')]
+              (mt/with-temp* [Database               [db-2]
+                              Table                  [db-2-table {:db_id (u/the-id db-2)}]
+                              GroupTableAccessPolicy [_ {:group_id (u/the-id &group)
+                                                         :table_id (u/the-id db-2-table)}]
+                              PermissionsGroup       [other-group]
+                              GroupTableAccessPolicy [_ {:group_id (u/the-id other-group)
+                                                         :table_id (mt/id :venues)}]]
+                (testing "perms graph should be updated"
+                  (testing "in API request response"
+                    (is (= (expected-perms)
+                           (get-in response (db-graph-keypath &group)))))
+                  (testing "on subsequent fetch of the graph"
+                    (is (= (expected-perms)
+                           (get-in (mt/user-http-request :crowberto :get 200 "permissions/graph")
+                                   (db-graph-keypath &group))))))
+                (testing "GTAP should be deleted from application DB"
+                  (is (= []
+                         (db/select GroupTableAccessPolicy
+                           :group_id (u/the-id &group)
+                           :table_id (mt/id :venues)))))
+                (testing "GTAP for same group, other database should not be affected"
+                  (is (schema= [(s/one {:id                   su/IntGreaterThanZero
+                                        :group_id             (s/eq (u/the-id &group))
+                                        :table_id             (s/eq (u/the-id db-2-table))
+                                        :card_id              (s/eq nil)
+                                        :attribute_remappings (s/eq nil)}
+                                       "GTAP")]
+                               (db/select GroupTableAccessPolicy
+                                 :group_id (u/the-id &group)
+                                 :table_id (u/the-id db-2-table)))))
+                (testing "GTAP for same table, other group should not be affected"
+                  (is (schema= [(s/one {:id                   su/IntGreaterThanZero
+                                        :group_id             (s/eq (u/the-id other-group))
+                                        :table_id             (s/eq (mt/id :venues))
+                                        :card_id              (s/eq nil)
+                                        :attribute_remappings (s/eq nil)}
+                                       "GTAP")]
+                               (db/select GroupTableAccessPolicy :group_id (u/the-id other-group)))))))))))))
+
+(deftest grant-sandbox-perms-dont-delete-gtaps-test
+  (testing "PUT /api/permissions/graph"
+    (testing "granting sandboxed permissions for a group should *not* delete an associated GTAP (#16190)"
+      (mt/with-temp-copy-of-db
+        (mt/with-temp GroupTableAccessPolicy [_ {:group_id (u/the-id (group/all-users))
+                                                 :table_id (mt/id :venues)}]
+          (let [graph    (mt/user-http-request :crowberto :get 200 "permissions/graph")
+                graph'   (assoc-in graph (db-graph-keypath (group/all-users)) {:schemas
+                                                                               {"PUBLIC"
+                                                                                {(id->keyword (mt/id :venues))
+                                                                                 {:read :all, :query :segmented}}}})]
+            (mt/user-http-request :crowberto :put 200 "permissions/graph" graph')
+            (testing "GTAP should not have been deleted"
+              (is (db/exists? GroupTableAccessPolicy :group_id (u/the-id (group/all-users)), :table_id (mt/id :venues))))))))))

enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj

@@ -109,7 +109,9 @@
     (mt/with-gtaps {:gtaps      {:checkins {:query      {:database (data/id), ...}
                                             :remappings {:user_category [\"variable\" ...]}}}
                     :attributes {\"user_category\" 1}}
-      (mt/run-mbql-query checkins {:limit 2}))"
+      (mt/run-mbql-query checkins {:limit 2}))
+
+  Introduces `&group` anaphor, bound to the PermissionsGroup associated with this GTAP."
   {:style/indent 1}
   [gtaps-and-attributes-map & body]
   `(do-with-gtaps-for-user (fn [] ~gtaps-and-attributes-map) :rasta (fn [~'&group] ~@body)))

src/metabase/models/permissions.clj

@@ -9,6 +9,7 @@
             [metabase.models.interface :as i]
             [metabase.models.permissions-group :as group]
             [metabase.models.permissions-revision :as perms-revision :refer [PermissionsRevision]]
+            [metabase.models.permissions.delete-sandboxes :as delete-sandboxes]
             [metabase.models.permissions.parse :as perms-parse]
             [metabase.plugins.classloader :as classloader]
             [metabase.util :as u]
@@ -714,7 +715,8 @@
        (db/transaction
          (doseq [[group-id changes] new]
            (update-group-permissions! group-id changes))
-         (save-perms-revision! (:revision old-graph) old new)))))
+         (save-perms-revision! (:revision old-graph) old new)
+         (delete-sandboxes/delete-gtaps-if-needed-after-permissions-change! new)))))
 
   ;; The following arity is provided soley for convenience for tests/REPL usage
   ([ks :- [s/Any], new-value]

src/metabase/models/permissions/delete_sandboxes.clj

+(ns metabase.models.permissions.delete-sandboxes
+  (:require [metabase.plugins.classloader :as classloader]
+            [metabase.util :as u]
+            [potemkin :as p]
+            [pretty.core :as pretty]))
+
+(p/defprotocol+ DeleteSandboxes
+  "Protocol for Sandbox deletion behavior when permissions are granted or revoked."
+  (delete-gtaps-if-needed-after-permissions-change!* [this changes]
+    "For use only inside `metabase.models.permissions`; don't call this elsewhere. Delete GTAPs that are no longer
+needed after the permissions graph is updated. See docstring for `delete-gtaps-if-needed-after-permissions-change!` for
+ more information."))
+
+(def oss-default-impl
+  "OSS no-op impl for Sandbox (GTAP) deletion behavior. Don't use this directly."
+  (reify
+    DeleteSandboxes
+    (delete-gtaps-if-needed-after-permissions-change!* [_ _] nil)
+
+    pretty/PrettyPrintable
+    (pretty [_]
+      `oss-default-impl)))
+
+(def ^:private impl
+  (delay
+    (or (u/ignore-exceptions
+          (classloader/require 'metabase-enterprise.sandbox.models.permissions.delete-sandboxes)
+          (var-get (resolve 'metabase-enterprise.sandbox.models.permissions.delete-sandboxes/ee-strategy-impl)))
+        oss-default-impl)))
+
+(defn delete-gtaps-if-needed-after-permissions-change!
+  "For use only inside `metabase.models.permissions`; don't call this elsewhere. Delete GTAPs (sandboxes) that are no
+  longer needed after the permissions graph is updated. This is EE-specific -- OSS impl is a no-op, since sandboxes
+  are an EE-only feature. `changes` are the parts of the graph that have changed, i.e. the `things-only-in-new`
+  returned by `clojure.data/diff`."
+  [changes]
+  (delete-gtaps-if-needed-after-permissions-change!* @impl changes))