delete v2 perm paths (#29408)

They were getting deleted, but only when granting permissions.

Now they will be deleted whenever their v1 path counterpart gets deleted.

src/metabase/models/permissions.clj

@@ -935,6 +935,37 @@
 ;;; |                                                  GRAPH UPDATE                                                  |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(letfn [(delete [s to-delete] (str/replace s to-delete ""))
+        (data-query-split [path] [(str "/data" path) (str "/query" path)])]
+  (def ^:private data-kind->rewrite-fn
+    "lookup table to generate v2 query + data permission from a v1 data permission."
+    {:dk/db                                 data-query-split
+     :dk/db-native                          (fn [path] (data-query-split (delete path "native/")))
+     :dk/db-schema                          (fn [path] [(str "/data" (delete path "schema/")) (str "/query" path)])
+     :dk/db-schema-name                     data-query-split
+     :dk/db-schema-name-and-table           data-query-split
+     :dk/db-schema-name-table-and-read      (constantly [])
+     :dk/db-schema-name-table-and-query     (fn [path] (data-query-split (delete path "query/")))
+     :dk/db-schema-name-table-and-segmented (fn [path] (data-query-split (delete path "query/segmented/")))}))
+
+(mu/defn ^:private ->v2-path :- [:vector [:re path-regex-v2]]
+  "Takes either a v1 or v2 path, and translates it into one or more v2 paths."
+  [path :- [:or [:re path-regex-v1] [:re path-regex-v2]]]
+  (let [kind (classify-path path)]
+    (case kind
+      :data (let [data-permission-kind (classify-data-path path)
+                  rewrite-fn (data-kind->rewrite-fn data-permission-kind)]
+              (rewrite-fn path))
+
+      :admin ["/"]
+      :block []
+
+      ;; for sake of idempotency, v2 perm-paths should be unchanged.
+      (:data-v2 :query-v2) [path]
+
+      ;; other paths should be unchanged too.
+      [path])))
+
 ;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
 
 (s/defn delete-related-permissions!
@@ -957,12 +988,14 @@
   `revoke-data-perms!` elsewhere instead of calling this directly."
   {:style/indent 2}
   [group-or-id :- (s/cond-pre su/Map su/IntGreaterThanZero) path :- PathSchema & other-conditions]
-  (let [where {:where (apply list
+  (let [paths (conj (->v2-path path) path)
+        where {:where (apply list
                              :and
                              [:= :group_id (u/the-id group-or-id)]
-                             [:or
-                              [:like path (h2x/concat :object (h2x/literal "%"))]
-                              [:like :object (str path "%")]]
+                             (into [:or
+                                    [:like path (h2x/concat :object (h2x/literal "%"))]]
+                                   (map (fn [path-form] [:like :object (str path-form "%")])
+                                        paths))
                              other-conditions)}]
     (when-let [revoked (t2/select-fn-set :object Permissions where)]
       (log/debug (u/format-color 'red "Revoking permissions for group %d: %s" (u/the-id group-or-id) revoked))
@@ -990,38 +1023,6 @@
   (delete-related-permissions! group-or-id (apply (partial feature-perms-path :download :full) path-components))
   (delete-related-permissions! group-or-id (apply (partial feature-perms-path :download :limited) path-components)))
 
-
-(letfn [(delete [s to-delete] (str/replace s to-delete ""))
-        (data-query-split [path] [(str "/data" path) (str "/query" path)])]
-  (def ^:private data-kind->rewrite-fn
-    "lookup table to generate v2 query + data permission from a v1 data permission."
-    {:dk/db                                 data-query-split
-     :dk/db-native                          (fn [path] (data-query-split (delete path "native/")))
-     :dk/db-schema                          (fn [path] [(str "/data" (delete path "schema/")) (str "/query" path)])
-     :dk/db-schema-name                     data-query-split
-     :dk/db-schema-name-and-table           data-query-split
-     :dk/db-schema-name-table-and-read      (constantly [])
-     :dk/db-schema-name-table-and-query     (fn [path] (data-query-split (delete path "query/")))
-     :dk/db-schema-name-table-and-segmented (fn [path] (data-query-split (delete path "query/segmented/")))}))
-
-(mu/defn ^:private ->v2-path :- [:vector [:re path-regex-v2]]
-  "Takes either a v1 or v2 path, and translates it into one or more v2 paths."
-  [path :- [:or [:re path-regex-v1] [:re path-regex-v2]]]
-  (let [kind (classify-path path)]
-    (case kind
-      :data (let [data-permission-kind (classify-data-path path)
-                  rewrite-fn (data-kind->rewrite-fn data-permission-kind)]
-              (rewrite-fn path))
-
-      :admin ["/"]
-      :block []
-
-      ;; for sake of idempotency, v2 perm-paths should be unchanged.
-      (:data-v2 :query-v2) [path]
-
-      ;; other paths should be unchanged too.
-      [path])))
-
 (defn grant-permissions!
   "Grant permissions for `group-or-id`. Two-arity grants any arbitrary Permissions `path`. With > 2 args, grants the
   data permissions from calling [[data-perms-path]]."

test/metabase/api/permissions_test.clj

@@ -223,9 +223,32 @@
       (mt/with-temp* [PermissionsGroup [group]]
         (mt/user-http-request :crowberto :put 200 "permissions/graph"
          (assoc-in (perms/data-perms-graph) [:groups (u/the-id group)] nil))
+        (is (empty? (db/select :permissions :group_id (u/the-id group))))
         (is (= nil (get-in (perms/data-perms-graph) [:groups (u/the-id group)])))
         (is (= nil (get-in (perms/data-perms-graph-v2) [:groups (u/the-id group)])))))))
 
+(deftest can-delete-permsissions-via-graph-test
+  (testing "PUT /api/permissions/graph"
+    (testing "permissions when group has no permissions"
+      (let [db-id (mt/id :venues)]
+        (mt/with-temp* [PermissionsGroup [group]]
+          (mt/user-http-request
+           :crowberto :put 200 "permissions/graph"
+           (assoc-in (perms/data-perms-graph)
+                     [:groups (u/the-id group) (mt/id) :data :schemas] {"PUBLIC" {db-id :all}}))
+          (is (= (set (for [template ["/data/db/%s/schema/PUBLIC/table/%s/"
+                                      "/query/db/%s/schema/PUBLIC/table/%s/"
+                                      "/db/%s/schema/PUBLIC/table/%s/"]]
+                        (format template (mt/id) db-id)))
+                 (set (mapv :object (db/select :permissions :group_id (u/the-id group))))))
+          (mt/user-http-request
+           :crowberto :put 200 "permissions/graph"
+           (assoc-in (perms/data-perms-graph)
+                     [:groups (u/the-id group) (mt/id)]
+                     {:data {:native "none" :schemas "none"}}))
+          (is (= #{}
+                 (set (mapv :object (db/select :permissions :group_id (u/the-id group)))))))))))
+
 (deftest update-perms-graph-error-test
   (testing "PUT /api/permissions/graph"
     (testing "make sure an error is thrown if the :sandboxes key is included in an OSS request"