Fix `revoke-db-schema-permissions!` helper function so that it never revokes native perms (#31882)

e2e/test/scenarios/permissions/impersonated.cy.spec.js

@@ -52,51 +52,60 @@ describeEE("impersonated permission", () => {
           "No",
           "No",
         ],
-        [
-          "QA Postgres12",
-          "Impersonated",
-          "No", // FIXME: should be "Yes"
-          "1 million rows",
-          "No",
-          "No",
-        ],
+        ["QA Postgres12", "Impersonated", "Yes", "1 million rows", "No", "No"],
       ]);
 
       // Checking it shows the right state on the tables level
       cy.get("main").findByText("QA Postgres12").click();
 
       assertPermissionTable([
-        ["Accounts", "Impersonated", "No", "1 million rows", "No", "No"],
-        ["Analytic Events", "Impersonated", "No", "1 million rows", "No", "No"],
-        ["Feedback", "Impersonated", "No", "1 million rows", "No", "No"],
-        ["Invoices", "Impersonated", "No", "1 million rows", "No", "No"],
-        ["Orders", "Impersonated", "No", "1 million rows", "No", "No"],
+        ["Accounts", "Impersonated", "Yes", "1 million rows", "No", "No"],
         [
-          "People",
+          "Analytic Events",
           "Impersonated",
-          "No", // FIXME: should be "Yes"
+          "Yes",
           "1 million rows",
           "No",
           "No",
         ],
+        ["Feedback", "Impersonated", "Yes", "1 million rows", "No", "No"],
+        ["Invoices", "Impersonated", "Yes", "1 million rows", "No", "No"],
+        ["Orders", "Impersonated", "Yes", "1 million rows", "No", "No"],
+        ["People", "Impersonated", "Yes", "1 million rows", "No", "No"],
+        ["Products", "Impersonated", "Yes", "1 million rows", "No", "No"],
+        ["Reviews", "Impersonated", "Yes", "1 million rows", "No", "No"],
+      ]);
+
+      // Return back to the database view
+      cy.get("main").findByText("All Users group").click();
+
+      // Edit impersonated permission
+      modifyPermission(
+        "QA Postgres12",
+        DATA_ACCESS_PERMISSION_INDEX,
+        "Edit Impersonated",
+      );
+
+      selectImpersonatedAttribute("attr_uid");
+      saveImpersonationSettings();
+      savePermissions();
+
+      assertPermissionTable([
         [
-          "Products",
-          "Impersonated",
-          "No", // FIXME: should be "Yes"
-          "1 million rows",
-          "No",
+          "Sample Database",
+          "No self-service",
           "No",
-        ],
-        [
-          "Reviews",
-          "Impersonated",
-          "No", // FIXME: should be "Yes"
           "1 million rows",
           "No",
           "No",
         ],
+        ["QA Postgres12", "Impersonated", "Yes", "1 million rows", "No", "No"],
       ]);
 
+      // Checking table permissions when the native access is disabled for impersonated users
+      modifyPermission("QA Postgres12", NATIVE_QUERIES_PERMISSION_INDEX, "No");
+      cy.get("main").findByText("QA Postgres12").click();
+
       cy.get("main")
         .findByText("Orders")
         .closest("tr")
@@ -116,38 +125,9 @@ describeEE("impersonated permission", () => {
         "Native query editor access requires full data access.",
       ).should("not.exist");
 
+      // Return back to the database view
       cy.get("main").findByText("All Users group").click();
 
-      // Edit impersonated permission
-      modifyPermission(
-        "QA Postgres12",
-        DATA_ACCESS_PERMISSION_INDEX,
-        "Edit Impersonated",
-      );
-
-      selectImpersonatedAttribute("attr_uid");
-      saveImpersonationSettings();
-      savePermissions();
-
-      assertPermissionTable([
-        [
-          "Sample Database",
-          "No self-service",
-          "No",
-          "1 million rows",
-          "No",
-          "No",
-        ],
-        [
-          "QA Postgres12",
-          "Impersonated",
-          "No", // FIXME: should be "Yes"
-          "1 million rows",
-          "No",
-          "No",
-        ],
-      ]);
-
       // Change from impersonated permission
       modifyPermission(
         "QA Postgres12",
@@ -245,8 +225,6 @@ describeEE("impersonated permission", () => {
       createTestRoles({ type: "postgres" });
       cy.signInAsAdmin();
 
-      // FIXME: two calls is a hack because BE will set the native permission to "write" only from the second call
-      setImpersonatedPermission();
       setImpersonatedPermission();
 
       cy.signInAsImpersonatedUser();

src/metabase/models/permissions.clj

@@ -1067,13 +1067,22 @@
   [group-or-id database-or-id]
   (grant-permissions! group-or-id (adhoc-native-query-path database-or-id)))
 
+(defn- group-has-native-perms?
+  [group-or-id database-or-id]
+  (set-has-full-permissions?
+   (t2/select-fn-set :object Permissions :group_id (u/the-id group-or-id))
+   (adhoc-native-query-path database-or-id)))
+
 (defn revoke-db-schema-permissions!
   "Remove all permissions entries for a DB and *any* child objects.
    This does *not* revoke native permissions; use `revoke-native-permssions!` to do that."
   [group-or-id database-or-id]
-  ;; TODO - if permissions for this DB are DB root entries like `/db/1/` won't this end up removing our native perms?
-  (delete-related-permissions! group-or-id (data-perms-path database-or-id)
-    [:not= :object (adhoc-native-query-path database-or-id)]))
+  (let [has-native-perms? (group-has-native-perms? group-or-id database-or-id)]
+    (delete-related-permissions! group-or-id (data-perms-path database-or-id)
+      [:not= :object (adhoc-native-query-path database-or-id)])
+    ;; If we've removed native perms as a consequence of deleting a root database path like `/db/1/`, add them back
+    (when (and has-native-perms? (not (group-has-native-perms? group-or-id database-or-id)))
+      (grant-native-readwrite-permissions! group-or-id database-or-id))))
 
 (defn revoke-application-permissions!
   "Remove all permissions entries for a Group to access a Application permisisons"

test/metabase/models/permissions_test.clj

@@ -9,6 +9,7 @@
     :as perms-group
     :refer [PermissionsGroup]]
    [metabase.models.table :refer [Table]]
+   [metabase.models.user :as user]
    [metabase.test :as mt]
    [metabase.test.fixtures :as fixtures]
    [metabase.util :as u]
@@ -797,6 +798,19 @@
 ;;; |                                 Granting/Revoking Permissions Helper Functions                                 |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(deftest revoke-db-schema-permissions-test
+  (mt/with-temp* [Database [database]]
+    (testing "revoke-db-schema-permissions! should revoke all non-native permissions on a database"
+      (is (perms/set-has-full-permissions? (user/permissions-set (mt/user->id :rasta))
+                                           (perms/data-perms-path database)))
+      (is (perms/set-has-full-permissions? (user/permissions-set (mt/user->id :rasta))
+                                           (perms/adhoc-native-query-path database)))
+      (perms/revoke-db-schema-permissions! (perms-group/all-users) database)
+      (is (not (perms/set-has-full-permissions? (user/permissions-set (mt/user->id :rasta))
+                                                (perms/data-perms-path database))))
+      (is (perms/set-has-full-permissions? (user/permissions-set (mt/user->id :rasta))
+                                           (perms/adhoc-native-query-path database))))))
+
 (deftest revoke-permissions-helper-function-test
   (testing "Make sure if you try to use the helper function to *revoke* perms for a Personal Collection, you get an Exception"
     (is (thrown? Exception