Merge branch 'master' into table-level-native-query-perms

This reverts commit 2bc9996596d1ac3993f3d7c77f990936ea79f1b4.

.clj-kondo/config.edn

@@ -335,7 +335,8 @@
     metabase.pulse                 #{metabase.pulse.core}
     metabase.query-processor       :any                                                             ; TODO omg scream, 29 namespaces used outside of the module. WHAT THE HECC
     metabase.query-analysis        #{metabase.query-analysis
-                                     metabase.query-analysis.failure-map}
+                                     metabase.query-analysis.failure-map
+                                     metabase.query-analysis.native-query-analyzer}
     metabase.related               #{metabase.related}
     metabase.sample-data           #{metabase.sample-data}
     metabase.search                #{metabase.search}

e2e/test/scenarios/permissions/create-queries.cy.spec.js

@@ -1,15 +1,23 @@
-import { USER_GROUPS } from "e2e/support/cypress_data";
+import { SAMPLE_DB_ID, USER_GROUPS } from "e2e/support/cypress_data";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 import {
+  assertPermissionForItem,
   assertPermissionTable,
+  assertSameBeforeAndAfterSave,
+  focusNativeEditor,
   modal,
   modifyPermission,
   popover,
   restore,
+  runNativeQuery,
   selectPermissionRow,
   selectSidebarItem,
+  setTokenFeatures,
+  startNewNativeQuestion,
 } from "e2e/support/helpers";
 
-const { ALL_USERS_GROUP } = USER_GROUPS;
+const { ALL_USERS_GROUP, DATA_GROUP } = USER_GROUPS;
+const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
 
 const NATIVE_QUERIES_PERMISSION_INDEX = 0;
 
@@ -55,19 +63,19 @@ describe("scenarios > admin > permissions > create queries > granular", () => {
 
     // should also remove native permissions for all other tables
     assertPermissionTable([
-      ["Accounts", "Query builder only"],
-      ["Analytic Events", "Query builder only"],
-      ["Feedback", "Query builder only"],
-      ["Invoices", "Query builder only"],
+      ["Accounts", "Query builder and native"],
+      ["Analytic Events", "Query builder and native"],
+      ["Feedback", "Query builder and native"],
+      ["Invoices", "Query builder and native"],
       ["Orders", "No"],
-      ["People", "Query builder only"],
-      ["Products", "Query builder only"],
-      ["Reviews", "Query builder only"],
+      ["People", "Query builder and native"],
+      ["Products", "Query builder and native"],
+      ["Reviews", "Query builder and native"],
     ]);
 
-    // should not allow 'query builder and native' as a granular permissions permission options
+    // should allow 'query builder and native' as a granular permissions permission options
     selectPermissionRow("Orders", NATIVE_QUERIES_PERMISSION_INDEX);
-    popover().should("not.contain", "Query builder and native");
+    popover().should("contain", "Query builder and native");
 
     // should have db set to granular
     selectSidebarItem("All Users");
@@ -90,10 +98,10 @@ describe("scenarios > admin > permissions > create queries > granular", () => {
     modifyPermission(
       "Orders",
       NATIVE_QUERIES_PERMISSION_INDEX,
-      "Query builder only",
+      "Query builder and native",
     );
     selectSidebarItem("All Users");
-    assertPermissionTable([["Sample Database", "Query builder only"]]);
+    assertPermissionTable([["Sample Database", "Query builder and native"]]);
   });
 });
 
@@ -161,9 +169,9 @@ describe("scenarios > admin > permissions > create queries > query builder and n
       ["Reviews", "No"],
     ]);
 
-    // Test that query builder and native is not an option when it's not selected at table level
+    // test that query builder and native is an option when it's not selected at table level
     selectPermissionRow("Orders", NATIVE_QUERIES_PERMISSION_INDEX);
-    popover().should("not.contain", "Query builder and native");
+    popover().should("contain", "Query builder and native");
 
     // Navigate back
     selectSidebarItem("collection");
@@ -223,7 +231,7 @@ describe("scenarios > admin > permissions > create queries > query builder and n
     );
   });
 
-  it("should allow setting create queries to 'query builder and native' in database view", () => {
+  it("should allow setting 'query builder and native' in database view", () => {
     cy.visit("/admin/permissions/");
     cy.get("label").contains("Databases").click();
 
@@ -279,6 +287,138 @@ describe("scenarios > admin > permissions > create queries > query builder and n
 
     assertPermissionTable(finalPermissions);
   });
+
+  it("should allow setting 'query builder and native' in table view and it should be enforced correctly", () => {
+    cy.log("should allow setting permissions value at table level");
+
+    cy.visit(`/admin/permissions/data/group/${DATA_GROUP}`);
+
+    const CREATE_QUERIES_PERM_IDX = 0;
+
+    modifyPermission(
+      "Sample Database",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder only",
+    );
+
+    cy.get("main").findByText("Sample Database").click();
+
+    modifyPermission(
+      "Orders",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder and native",
+    );
+
+    assertSameBeforeAndAfterSave(() => {
+      assertPermissionForItem(
+        "Accounts",
+        CREATE_QUERIES_PERM_IDX,
+        "Query builder only",
+      );
+      assertPermissionForItem(
+        "Orders",
+        CREATE_QUERIES_PERM_IDX,
+        "Query builder and native",
+      );
+    });
+
+    cy.log("should enforce the new settings");
+    cy.signOut();
+    cy.signInAsNormalUser();
+
+    cy.log("should work for table with native query access");
+    startNewNativeQuestion();
+    focusNativeEditor().type("SELECT * FROM Orders");
+    runNativeQuery();
+    cy.findAllByTestId("cell-data").should("contain", "1");
+
+    cy.log("should not work for table without native query access");
+    startNewNativeQuestion();
+    focusNativeEditor()
+      .type("{selectall}{backspace}", { delay: 50 })
+      .type("SELECT * FROM Accounts");
+    runNativeQuery();
+    cy.findByTestId("query-visualization-root")
+      .findByText(/You do not have permissions to run this query/)
+      .should("exist");
+    cy.findByTestId("qb-save-button").should("be.enabled").click();
+    cy.findByTestId("save-question-modal").within(() => {
+      cy.findByLabelText("Name").type("Save should fail");
+      cy.intercept("POST", "/api/card").as("saveQuestion");
+      cy.findByText("Save").click();
+      cy.wait("@saveQuestion");
+      cy.findByText(
+        /You cannot save this Question because you do not have permissions to run its query/,
+      ).should("exist");
+    });
+  });
+
+  it("should only allow users to create native queries for tables they have permissions for", () => {
+    cy.visit(`/admin/permissions/data/group/${DATA_GROUP}`);
+
+    const CREATE_QUERIES_PERM_IDX = 0;
+
+    modifyPermission(
+      "Sample Database",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder only",
+    );
+
+    cy.get("main").findByText("Sample Database").click();
+
+    modifyPermission(
+      "Orders",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder and native",
+    );
+  });
+
+  it("should prompt users you can not have sandboxed permissions when granting native query access", () => {
+    setTokenFeatures("all");
+
+    cy.sandboxTable({
+      table_id: ORDERS_ID,
+      group_id: DATA_GROUP,
+      attribute_remappings: {
+        "User ID": ["dimension", ["field", ORDERS.USER_ID, null]],
+      },
+    });
+
+    cy.visit(
+      `/admin/permissions/data/group/${DATA_GROUP}/database/${SAMPLE_DB_ID}`,
+    );
+
+    const DATA_ACCESS_PERM_IDX = 0;
+    const CREATE_QUERIES_PERM_IDX = 1;
+
+    assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Sandboxed");
+    assertPermissionForItem(
+      "Orders",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder only",
+    );
+    modifyPermission(
+      "Orders",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder and native",
+    );
+
+    modal()
+      .should("exist")
+      .within(() => {
+        cy.findByText(/Remove “Sandboxed” access from this table/).should(
+          "exist",
+        );
+        cy.findByText("Change").click();
+      });
+
+    assertPermissionForItem("Orders", DATA_ACCESS_PERM_IDX, "Can view");
+    assertPermissionForItem(
+      "Orders",
+      CREATE_QUERIES_PERM_IDX,
+      "Query builder and native",
+    );
+  });
 });
 
 describe("scenarios > admin > permissions > create queries > query builder only", () => {
@@ -365,79 +505,6 @@ describe("scenarios > admin > permissions > create queries > query builder only"
     );
   });
 
-  it("should set entire database to 'query builder only' if a table is changed to it while db is 'query builder only'", () => {
-    cy.visit("/admin/permissions");
-    selectSidebarItem("collection");
-
-    assertPermissionTable([["Sample Database", "No"]]);
-
-    modifyPermission(
-      "Sample Database",
-      NATIVE_QUERIES_PERMISSION_INDEX,
-      "Query builder and native",
-    );
-
-    // Drill down to tables permissions
-    cy.findByTextEnsureVisible("Sample Database").click();
-
-    assertPermissionTable([
-      ["Accounts", "Query builder and native"],
-      ["Analytic Events", "Query builder and native"],
-      ["Feedback", "Query builder and native"],
-      ["Invoices", "Query builder and native"],
-      ["Orders", "Query builder and native"],
-      ["People", "Query builder and native"],
-      ["Products", "Query builder and native"],
-      ["Reviews", "Query builder and native"],
-    ]);
-
-    modifyPermission(
-      "Orders",
-      NATIVE_QUERIES_PERMISSION_INDEX,
-      "Query builder only",
-    );
-
-    modal().within(() => {
-      cy.findByText("Change access to this database to “Granular”?");
-      cy.findByText("Change").click();
-    });
-
-    const finalTablePermissions = [
-      ["Accounts", "Query builder only"],
-      ["Analytic Events", "Query builder only"],
-      ["Feedback", "Query builder only"],
-      ["Invoices", "Query builder only"],
-      ["Orders", "Query builder only"],
-      ["People", "Query builder only"],
-      ["Products", "Query builder only"],
-      ["Reviews", "Query builder only"],
-    ];
-
-    assertPermissionTable(finalTablePermissions);
-
-    // Navigate back
-    selectSidebarItem("collection");
-    assertPermissionTable([["Sample Database", "Query builder only"]]);
-
-    cy.button("Save changes").click();
-
-    modal().within(() => {
-      cy.findByText("Save permissions?");
-      cy.contains(
-        "collection will only be able to use the query builder for Sample Database.",
-      );
-      cy.button("Yes").click();
-    });
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Save changes").should("not.exist");
-
-    // Drill down to tables permissions
-    cy.findByTextEnsureVisible("Sample Database").click();
-
-    assertPermissionTable(finalTablePermissions);
-  });
-
   it("should allow setting create queries to 'query builder only' in group view", () => {
     cy.visit("/admin/permissions");
     selectSidebarItem("collection");

e2e/test/scenarios/permissions/download-permissions.cy.spec.js

@@ -7,6 +7,7 @@ import {
 import {
   assertPermissionForItem,
   assertSheetRowsCount,
+  createNativeQuestion,
   describeEE,
   downloadAndAssert,
   modal,
@@ -173,21 +174,31 @@ describeEE("scenarios > admin > permissions > data > downloads", () => {
     beforeEach(() => {
       cy.intercept("POST", "/api/dataset").as("dataset");
 
-      cy.createNativeQuestion(
+      createNativeQuestion(
         {
           name: "Native Orders",
           native: {
             query: "select * from orders",
           },
         },
-        { wrapId: true, idAlias: "nativeQuestionId" },
+        { wrapId: true, idAlias: "nativeOrdersQuestionId" },
+      );
+
+      createNativeQuestion(
+        {
+          name: "Native Products",
+          native: {
+            query: "select * from products",
+          },
+        },
+        { wrapId: true, idAlias: "nativeProductsQuestionId" },
       );
     });
 
     it("lets user download results from native queries", () => {
       cy.signInAsNormalUser();
 
-      cy.get("@nativeQuestionId").then(id => {
+      cy.get("@nativeOrdersQuestionId").then(id => {
         visitQuestion(id);
 
         downloadAndAssert(
@@ -213,12 +224,12 @@ describeEE("scenarios > admin > permissions > data > downloads", () => {
       });
     });
 
-    it("prevents user from downloading a native question even if only one table doesn't have download permissions", () => {
-      setDownloadPermissionsForProductsTable("none");
+    it("prevents user from downloading a native question if a referenced table doesn't have download permissions", () => {
+      setDownloadPermissionsForOrdersTable("none");
 
       cy.signInAsNormalUser();
 
-      cy.get("@nativeQuestionId").then(id => {
+      cy.get("@nativeOrdersQuestionId").then(id => {
         visitQuestion(id);
 
         cy.findByText("Showing first 2,000 rows");
@@ -232,8 +243,6 @@ describeEE("scenarios > admin > permissions > data > downloads", () => {
         cy.icon("download").should("not.exist");
 
         // Convert question to a model, which also shouldn't be downloadable
-        cy.request("PUT", `/api/card/${id}`, { name: "Native Model" });
-
         visitQuestion(id);
 
         cy.findByText("Showing first 2,000 rows");
@@ -241,12 +250,12 @@ describeEE("scenarios > admin > permissions > data > downloads", () => {
       });
     });
 
-    it("limits download results for a native question even if only one table has `limited` download permissions", () => {
-      setDownloadPermissionsForProductsTable("limited");
+    it("limits download results for a native question if a referenced table has `limited` download permissions", () => {
+      setDownloadPermissionsForOrdersTable("limited");
 
       cy.signInAsNormalUser();
 
-      cy.get("@nativeQuestionId").then(id => {
+      cy.get("@nativeOrdersQuestionId").then(id => {
         visitQuestion(id);
 
         downloadAndAssert(
@@ -274,15 +283,15 @@ describeEE("scenarios > admin > permissions > data > downloads", () => {
   });
 });
 
-function setDownloadPermissionsForProductsTable(permission) {
+function setDownloadPermissionsForOrdersTable(permission) {
   cy.updatePermissionsGraph({
     [ALL_USERS_GROUP]: {
       [SAMPLE_DB_ID]: {
         download: {
           schemas: {
             PUBLIC: {
-              [PRODUCTS_ID]: permission,
-              [ORDERS_ID]: "full",
+              [PRODUCTS_ID]: "full",
+              [ORDERS_ID]: permission,
               [PEOPLE_ID]: "full",
               [REVIEWS_ID]: "full",
               [ACCOUNTS_ID]: "full",

e2e/test/scenarios/permissions/view-data.cy.spec.js

@@ -5,7 +5,6 @@ import {
   assertPermissionForItem,
   assertPermissionTable,
   assertSameBeforeAndAfterSave,
-  createNativeQuestion,
   createTestRoles,
   describeEE,
   getPermissionRowPermissions,
@@ -58,20 +57,6 @@ describeEE("scenarios > admin > permissions > view data > blocked", () => {
       assertPermissionForItem(g, DOWNLOAD_PERM_IDX, "No", true);
     });
 
-    cy.log(
-      "assert that user properly sees native query warning related to table level blocking",
-    );
-    getPermissionRowPermissions("All Users")
-      .eq(DATA_ACCESS_PERM_IDX)
-      .findByLabelText("warning icon")
-      .realHover();
-
-    cy.findByRole("tooltip")
-      .findByText(
-        /Groups with a database, schema, or table set to Blocked can't view native queries on this database/,
-      )
-      .should("exist");
-
     cy.visit(`/admin/permissions/data/database/${SAMPLE_DB_ID}`); // database level
 
     assertPermissionForItem(g, DATA_ACCESS_PERM_IDX, "Granular", false);
@@ -678,20 +663,56 @@ describeEE("scenarios > admin > permissions > view data > sandboxed", () => {
     modal().button("Save").click();
 
     const expectedFinalPermissions = [
-      ["Accounts", "Can view", "Query builder only", "1 million rows", "No"],
       [
-        "Analytic Events",
+        "Accounts",
         "Can view",
-        "Query builder only",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
+      [
+        "Analytic Events",
+        "Can view",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
+      [
+        "Feedback",
+        "Can view",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
+      [
+        "Invoices",
+        "Can view",
+        "Query builder and native",
         "1 million rows",
         "No",
       ],
-      ["Feedback", "Can view", "Query builder only", "1 million rows", "No"],
-      ["Invoices", "Can view", "Query builder only", "1 million rows", "No"],
       ["Orders", "Sandboxed", "Query builder only", "1 million rows", "No"],
-      ["People", "Can view", "Query builder only", "1 million rows", "No"],
-      ["Products", "Can view", "Query builder only", "1 million rows", "No"],
-      ["Reviews", "Can view", "Query builder only", "1 million rows", "No"],
+      [
+        "People",
+        "Can view",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
+      [
+        "Products",
+        "Can view",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
+      [
+        "Reviews",
+        "Can view",
+        "Query builder and native",
+        "1 million rows",
+        "No",
+      ],
     ];
 
     assertPermissionTable(expectedFinalPermissions);
@@ -886,19 +907,6 @@ describeEE(
       removeCollectionGroupPermissions();
       assertCollectionGroupHasNoAccess(ORDERS_QUESTION_ID, true);
     });
-
-    it("should deny view access to any native question if the user has blocked view data for any table or database", () => {
-      createNativeQuestion({
-        native: { query: "select 1" },
-      }).then(({ body: { id: nativeQuestionId } }) => {
-        assertCollectionGroupUserHasAccess(nativeQuestionId, false);
-        cy.visit(
-          `/admin/permissions/data/database/${SAMPLE_DB_ID}/schema/PUBLIC/table/${ORDERS_ID}`,
-        );
-        removeCollectionGroupPermissions();
-        assertCollectionGroupHasNoAccess(nativeQuestionId, false);
-      });
-    });
   },
 );
 

enterprise/backend/src/metabase_enterprise/advanced_permissions/query_processor/middleware/permissions.clj

@@ -10,6 +10,8 @@
    [metabase.query-processor.error-type :as qp.error-type]
    [metabase.util.i18n :refer [tru]]))
 
+(set! *warn-on-reflection* true)
+
 (def ^:private max-rows-in-limited-downloads 10000)
 
 (defn- is-download?
@@ -17,31 +19,22 @@
   [query]
   (some-> query :info :context name (str/includes? "download")))
 
-(defmulti ^:private current-user-download-perms-level :type)
-
-(defmethod current-user-download-perms-level :default
-  [_]
-  :one-million-rows)
-
-(defmethod current-user-download-perms-level :native
-  [{database :database}]
-  (data-perms/native-download-permission-for-user api/*current-user-id* database))
-
-(defmethod current-user-download-perms-level :query
+(defn- current-user-download-perms-level
   [{db-id :database, :as query}]
-  ;; Remove the :native key (containing the transpiled MBQL) so that this helper function doesn't think the query is
-  ;; a native query. Actual native queries are dispatched to a different method by the :type key.
-  (let [table-ids   (query-perms/query->source-table-ids (dissoc query :native))
-        table-perms (into #{}
-                          (map (fn [table-id]
-                                 (if (= table-id ::query-perms/native)
-                                   (data-perms/native-download-permission-for-user api/*current-user-id* db-id)
-                                   (data-perms/table-permission-for-user api/*current-user-id* :perms/download-results db-id table-id)))
-                               table-ids))]
-    ;; The download perm level for a query should be equal to the lowest perm level of any table referenced by the query.
-    (or (table-perms :no)
-        (table-perms :ten-thousand-rows)
-        :one-million-rows)))
+  (let [table-ids          (query-perms/query->source-table-ids query)
+        full-native-perms? (contains? table-ids ::query-perms/native)]
+    (if full-native-perms?
+      ;; If we can't parse individual tables from a native query, use the lowest permission for *any* table in the DB
+      (data-perms/fallback-native-download-permission-for-user api/*current-user-id* db-id)
+      (let [table-perms
+            (into #{}
+                  (map (fn [table-id]
+                         (data-perms/table-permission-for-user api/*current-user-id* :perms/download-results db-id table-id))
+                       table-ids))]
+        ;; The download perm level for a query should be equal to the lowest perm level of any table referenced by the query.
+        (or (table-perms :no)
+            (table-perms :ten-thousand-rows)
+            :one-million-rows)))))
 
 (defenterprise apply-download-limit
   "Pre-processing middleware to apply row limits to MBQL export queries if the user has `ten-thousand-rows` download

enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj

@@ -195,9 +195,9 @@
 (t2/define-before-insert :model/GroupTableAccessPolicy
   [{:keys [table_id group_id], :as gtap}]
   (let [db-id (database/table-id->database-id table_id)]
-    ;; Remove native query access to the DB when saving a sandbox
+    ;; Remove native query access to the table when saving a sandbox
     (when (= (data-perms/table-permission-for-groups #{group_id} :perms/create-queries db-id table_id) :query-builder-and-native)
-      (data-perms/set-database-permission! group_id db-id :perms/create-queries :query-builder)))
+      (data-perms/set-table-permission! group_id table_id :perms/create-queries :query-builder)))
   (u/prog1 gtap
     (check-columns-match-table gtap)))
 

enterprise/backend/src/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions.clj

@@ -276,26 +276,41 @@
        :perms/create-queries (zipmap table-ids (repeat :query-builder))})))
 
 (defn- merge-perms
-  "The shape of permissions maps is a little odd, and using `m/deep-merge` doesn't give us exactly what we want.
-  In particular, if we need query-builder-and-native at the *database* level, but :query-builder at the *table* level,
-  the permissions maps will look like:
+  "In general, we merge conflicting values in two permission maps by taking the more permissive value using
+  (`data-perms/coalesce`).
 
-  - `{:perms/create-queries :query-builder-and-native}`
-  - `{:perms/create-queries {1 :query-builder}}`
+  However, if a permission type is defined at the *database* level in one map and the *table* level in another, we
+  always take the database value:
 
-  Currently, we never require a *lower* level permission at the database level, so it's ok to just say that the
-  db-level permissions always win. If we ever wanted to merge something like `{:perms/create-queries
-  {1 :query-builder-and-native}}` with `{:perms/create-queries :query-builder}`, this would break down and we'd
-  probably want to modify the shape of the permissions-maps themselves."
+  => (merge-perms
+      {:perms/create-queries :query-builder-and-native}
+      {:perms/create-queries {1 :query-builder}})
+  => {:perms/create-queries :query-builder-and-native}
+
+  WARNING: This technically returns an incorrect result if the *database* perm is more restrictive than the *table* perm:
+
+  => (merge-perms
+      {:perms/create-queries :query-builder}
+      {:perms/create-queries {1 :query-builder-and-native}})
+  => {:perms/create-queries :query-builder}
+
+  For the sandboxing use case this is acceptable, because :create-queries should never be :query-builder for the entire
+  database. Perms required for sandboxing are always table-level, unless it is a native question which can't be parsed,
+  in which case database-level :query-builder-and-native is required."
   ([perms-a] perms-a)
   ([perms-a perms-b]
    (reduce (fn [merged [k v]]
              (update merged k (fn [old-v]
                                 (cond
+                                  (and (keyword? old-v) (keyword? v))
+                                  (data-perms/coalesce k [old-v v])
+
                                   (keyword? old-v) old-v
-                                  (keyword? v) v
+                                  (keyword? v)     v
+
                                   (and (map? old-v) (map? v))
-                                  (merge old-v v)
+                                  (merge-with #(data-perms/coalesce k [%1 %2]) old-v v)
+
                                   :else v))))
            (or perms-a {})
            (seq perms-b)))
@@ -360,8 +375,8 @@
       (-> sandboxed-query
           (assoc ::original-metadata (expected-cols original-query))
           (update-in [::query-perms/perms :gtaps]
-                     (fn [required-perms] (merge required-perms
-                                                 (sandboxes->required-perms (vals table-id->gtap)))))))))
+                     (fn [required-perms] (merge-perms required-perms
+                                                       (sandboxes->required-perms (vals table-id->gtap)))))))))
 
 (def ^:private default-recursion-limit 20)
 (def ^:private ^:dynamic *recursion-limit* default-recursion-limit)
@@ -377,7 +392,7 @@
           (let [gtapped-query (gtapped-query query table-id->gtap)]
             (if (not= query gtapped-query)
               ;; Applying GTAPs to the query may have introduced references to tables that are also sandboxed,
-              ;; so we need to recursively appby the middleware until new queries are not returned.
+              ;; so we need to recursively apply the middleware until new queries are not returned.
               (if (= *recursion-limit* 0)
                 (throw (ex-info (trs "Reached recursion limit of {0} in \"apply-sandboxing\" middleware"
                                      default-recursion-limit)

enterprise/backend/test/metabase_enterprise/advanced_permissions/query_processor/middleware/permissions_test.clj

@@ -215,10 +215,16 @@
             :assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}})
 
           (streaming-test/do-test!
-           "A user with limited download perms for a table has limited download perms for native queries on all tables"
+           "A user with limited download perms for a table has them enforced for native queries"
+           {:query      (mt/native-query {:query "SELECT * FROM orders LIMIT 10;"})
+            :endpoints  [:card :dataset]
+            :assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}})
+
+          (streaming-test/do-test!
+           "A user with limited download perms for a table still has full download perms for native queries on other tables"
            {:query      (mt/native-query {:query "SELECT * FROM checkins LIMIT 10;"})
             :endpoints  [:card :dataset]
-            :assertions {:csv (fn [results] (is (= 3 (csv-row-count results))))}}))))))
+            :assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}}))))))
 
 (deftest no-download-perms-test
   (testing "Users with no download perms cannot run download queries"
@@ -285,13 +291,19 @@
           :assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}})
 
         (streaming-test/do-test!
-         "A user with no download perms for a table has no download perms for native queries on all tables"
-         {:query      (mt/native-query {:query "SELECT * FROM checkins LIMIT 10;"})
+         "A user with no download perms for a table has it enforced for native queries on the table"
+         {:query      (mt/native-query {:query "SELECT * FROM venues LIMIT 10;"})
           :endpoints  [:card :dataset]
           :assertions {:csv (fn [results]
                               (is (partial=
                                    {:error "You do not have permissions to download the results of this query."}
-                                   results)))}})))))
+                                   results)))}})
+
+        (streaming-test/do-test!
+         "A user with no download perms for a table still has full download perms for native queries on other tables"
+         {:query      (mt/native-query {:query "SELECT * FROM checkins LIMIT 10;"})
+          :endpoints  [:card :dataset]
+          :assertions {:csv (fn [results] (is (= 10 (csv-row-count results))))}})))))
 
 (deftest joins-test
   (mt/with-full-data-perms-for-all-users!

enterprise/backend/test/metabase_enterprise/sandbox/models/group_table_access_policy_test.clj

@@ -5,6 +5,7 @@
     :as sandboxes
     :refer [GroupTableAccessPolicy]]
    [metabase.models :refer [Card]]
+   [metabase.models.data-permissions :as data-perms]
    [metabase.models.permissions-group :as perms-group]
    [metabase.query-processor.preprocess :as qp.preprocess]
    [metabase.test :as mt]
@@ -166,3 +167,25 @@
                 {(u/the-id (perms-group/all-users))
                  {(mt/id)
                   {:view-data :unrestricted}}}))))))))
+
+(deftest saving-sandbox-disables-native-access
+  (mt/with-premium-features #{:sandboxes}
+    (mt/with-full-data-perms-for-all-users!
+      (testing "Saving a sandbox automatically disables native query access to the sandboxed table"
+        (mt/with-temp [:model/User                   user {}
+                       :model/GroupTableAccessPolicy _    {:table_id (mt/id :venues)
+                                                           :group_id (u/the-id (perms-group/all-users))}]
+          (is (= {(mt/id) {:perms/create-queries
+                           {(mt/id :venues)     :query-builder
+                            (mt/id :orders)     :query-builder-and-native
+                            (mt/id :users)      :query-builder-and-native
+                            (mt/id :categories) :query-builder-and-native
+                            (mt/id :checkins)   :query-builder-and-native
+                            (mt/id :products)   :query-builder-and-native
+                            (mt/id :people)     :query-builder-and-native
+                            (mt/id :reviews)    :query-builder-and-native}}}
+                 (data-perms/permissions-for-user (u/the-id user)
+                                                  :db-id
+                                                  (mt/id)
+                                                  :perm-type
+                                                  :perms/create-queries))))))))

enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj

@@ -261,11 +261,58 @@
                                                                 :display_name  "Count"
                                                                 :source        :aggregation
                                                                 :field_ref     [:aggregation 0]}]
-                   ::query-perms/perms                        {:gtaps {:perms/create-queries :query-builder-and-native}}})
+                   ::query-perms/perms                        {:gtaps {:perms/create-queries {(mt/id :venues) :query-builder-and-native}
+                                                                       :perms/view-data      {(mt/id :venues) :unrestricted}}}})
                 (apply-row-level-permissions
                  (mt/mbql-query venues
                    {:aggregation [[:count]]}))))))))
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                                  UNIT TESTS                                                    |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(deftest merge-perms-test
+  (testing "merge-perms correctly combines two sandboxing permission graphs, taking the more permissive value"
+    (is (= {:perms/create-queries :query-builder-and-native}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries :query-builder}
+            {:perms/create-queries :query-builder-and-native})))
+
+    (is (= {:perms/create-queries :query-builder-and-native}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries :query-builder-and-native}
+            {:perms/create-queries :query-builder})))
+
+    (is (= {:perms/create-queries :query-builder-and-native}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries :query-builder-and-native}
+            {:perms/create-queries {1 :query-builder}})))
+
+    (is (= {:perms/create-queries {1 :query-builder-and-native}}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries {1 :query-builder-and-native}}
+            {:perms/create-queries {1 :query-builder}})))
+
+    (is (= {:perms/create-queries {1 :query-builder-and-native}}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries {1 :query-builder}}
+            {:perms/create-queries {1 :query-builder-and-native}})))
+
+    (is (= {:perms/create-queries {1 :query-builder-and-native
+                                   2 :query-builder}}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries {1 :query-builder-and-native}}
+            {:perms/create-queries {2 :query-builder}}))))
+
+  (testing "The database-level perm always wins, even if it is less permissive than the table-level perm"
+    ;; Note: This case shouldn't arise in the context of sandboxing, so it's OK that the behavior is technically
+    ;; incorrect. Sandboxing queries might require :query-builder-and-native at the DB-level, but :query-builder
+    ;; will only ever be requried at the table-level.
+    (is (= {:perms/create-queries :query-builder}
+           (@#'row-level-restrictions/merge-perms
+            {:perms/create-queries {1 :query-builder-and-native}}
+            {:perms/create-queries :query-builder})))))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                END-TO-END TESTS                                                |
 ;;; +----------------------------------------------------------------------------------------------------------------+

enterprise/frontend/src/metabase-enterprise/advanced_permissions/graph.ts

@@ -5,13 +5,9 @@ import {
   DataPermission,
   DataPermissionValue,
 } from "metabase/admin/permissions/types";
-import {
-  isSchemaEntityId,
-  isTableEntityId,
-} from "metabase/admin/permissions/utils/data-entity-id";
+import { isTableEntityId } from "metabase/admin/permissions/utils/data-entity-id";
 import {
   getEntityPermission,
-  getSchemasPermission,
   hasPermissionValueInSubgraph,
   updateEntityPermission,
 } from "metabase/admin/permissions/utils/graph";
@@ -26,29 +22,18 @@ export function shouldRestrictNativeQueryPermissions(
   value: DataPermissionValue,
   _database: Database,
 ) {
-  const currDbNativePermission = getSchemasPermission(
+  const createQueriesPermissions = getEntityPermission(
     permissions,
     groupId,
-    { databaseId: entityId.databaseId },
+    entityId,
     DataPermission.CREATE_QUERIES,
   );
 
-  if (isTableEntityId(entityId)) {
-    return (
-      (value === DataPermissionValue.SANDBOXED ||
-        value === DataPermissionValue.BLOCKED) &&
-      currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
-    );
-  }
-
-  if (isSchemaEntityId(entityId)) {
-    return (
-      value === DataPermissionValue.BLOCKED &&
-      currDbNativePermission === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
-    );
-  }
-
-  return false;
+  return (
+    (value === DataPermissionValue.SANDBOXED ||
+      value === DataPermissionValue.BLOCKED) &&
+    createQueriesPermissions === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
+  );
 }
 
 export function upgradeViewPermissionsIfNeeded(
@@ -58,23 +43,17 @@ export function upgradeViewPermissionsIfNeeded(
   value: NativePermissions,
   database: Database,
 ) {
-  // get permission for item up one level or db if we're already at the top most entity:
-  // table -> schema, schema -> database, database -> database
-  const parentOrDbEntityId = isTableEntityId(entityId)
-    ? _.pick(entityId, ["databaseId", "schemaName"])
-    : _.pick(entityId, ["databaseId"]);
-
-  const parentOrDbPermission = getEntityPermission(
+  const viewDataPermission = getEntityPermission(
     permissions,
     groupId,
-    parentOrDbEntityId,
+    entityId,
     DataPermission.VIEW_DATA,
   );
 
   const isGrantingNativeQueryAccessWithoutProperViewAccess =
     value === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
-    parentOrDbPermission !== DataPermissionValue.UNRESTRICTED &&
-    parentOrDbPermission !== DataPermissionValue.IMPERSONATED;
+    viewDataPermission !== DataPermissionValue.UNRESTRICTED &&
+    viewDataPermission !== DataPermissionValue.IMPERSONATED;
 
   const isGrantingQueryAccessWithBlockedChild =
     value !== DataPermissionValue.NO &&
@@ -95,7 +74,7 @@ export function upgradeViewPermissionsIfNeeded(
     permissions = updateEntityPermission(
       permissions,
       groupId,
-      parentOrDbEntityId,
+      entityId,
       DataPermissionValue.UNRESTRICTED,
       database,
       DataPermission.VIEW_DATA,

enterprise/frontend/src/metabase-enterprise/sandboxes/confirmations.ts

@@ -1,10 +1,11 @@
 import { t } from "ttag";
 
 import {
+  DataPermission,
   DataPermissionValue,
   type EntityId,
 } from "metabase/admin/permissions/types";
-import { hasPermissionValueInGraph } from "metabase/admin/permissions/utils/graph";
+import { getEntityPermission } from "metabase/admin/permissions/utils/graph";
 import type { GroupsPermissions } from "metabase-types/api/permissions";
 
 export function getSandboxedTableWarningModal(
@@ -13,19 +14,41 @@ export function getSandboxedTableWarningModal(
   entityId: EntityId,
   value: DataPermissionValue,
 ) {
-  // if the user is sandboxing the table while there is create-queries permissions set to
-  // query builder and native for that group's access to the database being modified, we
-  // should prompt them that we will have to remove native access for all tables/schemas
+  const createQueriesPermissions = getEntityPermission(
+    permissions,
+    groupId,
+    entityId,
+    DataPermission.CREATE_QUERIES,
+  );
+
+  // warn we will remove native query access if user is sandboxing a table
   if (
     value === DataPermissionValue.SANDBOXED &&
-    hasPermissionValueInGraph(
-      permissions[groupId][entityId.databaseId],
-      DataPermissionValue.QUERY_BUILDER_AND_NATIVE,
-    )
+    createQueriesPermissions === DataPermissionValue.QUERY_BUILDER_AND_NATIVE
   ) {
     return {
       title: t`Change access to this database to “Sandboxed”?`,
-      message: t`This group's native querying permissions will be removed from all tables and schemas in this database.`,
+      message: t`This group's native querying permissions will be removed from this table.`,
+      confirmButtonText: t`Change`,
+      cancelButtonText: t`Cancel`,
+    };
+  }
+
+  const viewDataPermissions = getEntityPermission(
+    permissions,
+    groupId,
+    entityId,
+    DataPermission.VIEW_DATA,
+  );
+
+  // warn we will remove sandboxing if user gives native query access
+  if (
+    value === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
+    viewDataPermissions === DataPermissionValue.SANDBOXED
+  ) {
+    return {
+      title: t`Remove “Sandboxed” access from this table?`,
+      message: t`Giving this group native querying permissions will remove sandboxing and grant “Can view” View data access.`,
       confirmButtonText: t`Change`,
       cancelButtonText: t`Cancel`,
     };

frontend/src/metabase/admin/permissions/selectors/confirmations.tsx

@@ -90,23 +90,6 @@ export function getPermissionWarning(
   return null;
 }
 
-export function getBlockWarning(
-  dbValue: DataPermissionValue,
-  schemaValue: DataPermissionValue,
-  tableValue?: DataPermissionValue,
-) {
-  if (dbValue === DataPermissionValue.BLOCKED) {
-    return;
-  }
-
-  if (
-    schemaValue === DataPermissionValue.BLOCKED ||
-    tableValue === DataPermissionValue.BLOCKED
-  ) {
-    return t`Groups with a database, schema, or table set to Blocked can't view native queries on this database.`;
-  }
-}
-
 function getEntityTypeFromId(entityId: EntityId): [string, string] {
   return isTableEntityId(entityId)
     ? [t`table`, t`tables`]

frontend/src/metabase/admin/permissions/selectors/data-permissions/fields.ts

@@ -1,11 +1,7 @@
 import _ from "underscore";
 
 import { getNativePermissionDisabledTooltip } from "metabase/admin/permissions/selectors/data-permissions/shared";
-import {
-  getFieldsPermission,
-  getSchemasPermission,
-  getTablesPermission,
-} from "metabase/admin/permissions/utils/graph";
+import { getFieldsPermission } from "metabase/admin/permissions/utils/graph";
 import {
   PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_ACTIONS,
   PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_CONFIRMATIONS,
@@ -19,14 +15,13 @@ import type { Group, GroupsPermissions } from "metabase-types/api";
 
 import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
 import { UNABLE_TO_CHANGE_ADMIN_PERMISSIONS } from "../../constants/messages";
-import type { PermissionSectionConfig, TableEntityId } from "../../types";
-import {
-  DataPermission,
-  DataPermissionType,
+import type {
   DataPermissionValue,
+  PermissionSectionConfig,
+  TableEntityId,
 } from "../../types";
+import { DataPermission, DataPermissionType } from "../../types";
 import {
-  getBlockWarning,
   getPermissionWarning,
   getPermissionWarningModal,
   getRevokingAccessToAllTablesWarningModal,
@@ -63,21 +58,7 @@ const buildAccessPermission = (
     DataPermission.VIEW_DATA,
   );
 
-  const dbValue = getSchemasPermission(
-    permissions,
-    groupId,
-    entityId,
-    DataPermission.VIEW_DATA,
-  );
-
-  const schemaValue = getTablesPermission(
-    permissions,
-    groupId,
-    entityId,
-    DataPermission.VIEW_DATA,
-  );
-
-  const permissionWarning = getPermissionWarning(
+  const warning = getPermissionWarning(
     value,
     defaultGroupValue,
     "fields",
@@ -85,11 +66,6 @@ const buildAccessPermission = (
     groupId,
   );
 
-  const blockWarning = getBlockWarning(dbValue, schemaValue, value);
-
-  // permissionWarning should always trump a blockWarning
-  const warning = permissionWarning || blockWarning;
-
   const confirmations = (newValue: DataPermissionValue) => [
     getPermissionWarningModal(
       newValue,
@@ -147,13 +123,6 @@ const buildNativePermission = (
   permissions: GroupsPermissions,
   accessPermissionValue: DataPermissionValue,
 ): PermissionSectionConfig => {
-  const schemaValue = getTablesPermission(
-    permissions,
-    groupId,
-    entityId,
-    DataPermission.CREATE_QUERIES,
-  );
-
   const value = getFieldsPermission(
     permissions,
     groupId,
@@ -173,14 +142,16 @@ const buildNativePermission = (
     disabledTooltip,
     isHighlighted: isAdmin,
     value,
-    options: _.compact([
-      schemaValue === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
-        DATA_PERMISSION_OPTIONS.queryBuilderAndNative,
+    options: [
+      DATA_PERMISSION_OPTIONS.queryBuilderAndNative,
       DATA_PERMISSION_OPTIONS.queryBuilder,
       DATA_PERMISSION_OPTIONS.no,
-    ]),
-    confirmations: () => [
+    ],
+    confirmations: (newValue: DataPermissionValue) => [
       getWillRevokeNativeAccessWarningModal(permissions, groupId, entityId),
+      ...PLUGIN_ADMIN_PERMISSIONS_TABLE_FIELDS_CONFIRMATIONS.map(confirmation =>
+        confirmation(permissions, groupId, entityId, newValue),
+      ),
     ],
   };
 };

frontend/src/metabase/admin/permissions/selectors/data-permissions/tables.ts

@@ -1,10 +1,7 @@
 import _ from "underscore";
 
 import { getNativePermissionDisabledTooltip } from "metabase/admin/permissions/selectors/data-permissions/shared";
-import {
-  getSchemasPermission,
-  getTablesPermission,
-} from "metabase/admin/permissions/utils/graph";
+import { getTablesPermission } from "metabase/admin/permissions/utils/graph";
 import {
   PLUGIN_ADMIN_PERMISSIONS_TABLE_OPTIONS,
   PLUGIN_ADVANCED_PERMISSIONS,
@@ -16,14 +13,13 @@ import type { Group, GroupsPermissions } from "metabase-types/api";
 import { DATA_PERMISSION_OPTIONS } from "../../constants/data-permissions";
 import { UNABLE_TO_CHANGE_ADMIN_PERMISSIONS } from "../../constants/messages";
 import { navigateToGranularPermissions } from "../../permissions";
-import type { PermissionSectionConfig, SchemaEntityId } from "../../types";
-import {
-  DataPermission,
-  DataPermissionType,
+import type {
   DataPermissionValue,
+  PermissionSectionConfig,
+  SchemaEntityId,
 } from "../../types";
+import { DataPermission, DataPermissionType } from "../../types";
 import {
-  getBlockWarning,
   getPermissionWarning,
   getPermissionWarningModal,
   getViewDataPermissionsTooRestrictiveWarningModal,
@@ -59,14 +55,7 @@ const buildAccessPermission = (
     DataPermission.VIEW_DATA,
   );
 
-  const dbValue = getSchemasPermission(
-    permissions,
-    groupId,
-    entityId,
-    DataPermission.VIEW_DATA,
-  );
-
-  const permissionWarning = getPermissionWarning(
+  const warning = getPermissionWarning(
     value,
     defaultGroupValue,
     "tables",
@@ -74,11 +63,6 @@ const buildAccessPermission = (
     groupId,
   );
 
-  const blockWarning = getBlockWarning(dbValue, value);
-
-  // permissionWarning should always trump a blockWarning
-  const warning = permissionWarning || blockWarning;
-
   const confirmations = (newValue: DataPermissionValue) => [
     getPermissionWarningModal(
       newValue,
@@ -129,13 +113,6 @@ const buildNativePermission = (
   accessPermissionValue: DataPermissionValue,
   database: Database,
 ): PermissionSectionConfig => {
-  const dbValue = getSchemasPermission(
-    permissions,
-    groupId,
-    { databaseId: entityId.databaseId },
-    DataPermission.CREATE_QUERIES,
-  );
-
   const value = getTablesPermission(
     permissions,
     groupId,
@@ -156,8 +133,7 @@ const buildNativePermission = (
     isHighlighted: isAdmin,
     value,
     options: _.compact([
-      dbValue === DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
-        DATA_PERMISSION_OPTIONS.queryBuilderAndNative,
+      DATA_PERMISSION_OPTIONS.queryBuilderAndNative,
       DATA_PERMISSION_OPTIONS.queryBuilder,
       DATA_PERMISSION_OPTIONS.controlled,
       DATA_PERMISSION_OPTIONS.no,

frontend/src/metabase/admin/permissions/utils/graph/data-permissions/update.ts

@@ -24,11 +24,7 @@ import type Database from "metabase-lib/v1/metadata/Database";
 import type Table from "metabase-lib/v1/metadata/Table";
 import type { GroupsPermissions } from "metabase-types/api";
 
-import {
-  getFieldsPermission,
-  getPermissionPath,
-  getSchemasPermission,
-} from "./get";
+import { getFieldsPermission, getPermissionPath } from "./get";
 import { isRestrictivePermission } from "./utils";
 
 export function updatePermission(
@@ -205,21 +201,7 @@ export function restrictCreateQueriesPermissionsIfNeeded(
   value: DataPermissionValue,
   database: Database,
 ) {
-  const currDbCreateQueriesPermission = getSchemasPermission(
-    permissions,
-    groupId,
-    { databaseId: entityId.databaseId },
-    DataPermission.CREATE_QUERIES,
-  );
-
-  const isMakingGranularCreateQueriesChange =
-    permission === DataPermission.CREATE_QUERIES &&
-    value !== DataPermissionValue.QUERY_BUILDER_AND_NATIVE &&
-    (entityId.tableId != null || entityId.schemaName != null) &&
-    currDbCreateQueriesPermission ===
-      DataPermissionValue.QUERY_BUILDER_AND_NATIVE;
-
-  const shouldRestrictForSomeReason =
+  const shouldRestrictNative =
     PLUGIN_DATA_PERMISSIONS.shouldRestrictNativeQueryPermissions(
       permissions,
       groupId,
@@ -229,25 +211,15 @@ export function restrictCreateQueriesPermissionsIfNeeded(
       database,
     );
 
-  const shouldRestrictNative =
-    isMakingGranularCreateQueriesChange || shouldRestrictForSomeReason;
-
   if (shouldRestrictNative) {
-    const schemaNames = (database && database.schemaNames()) ?? [null];
-
-    schemaNames.forEach(schemaName => {
-      permissions = updateTablesPermission(
-        permissions,
-        groupId,
-        {
-          databaseId: entityId.databaseId,
-          schemaName,
-        },
-        DataPermissionValue.QUERY_BUILDER,
-        database,
-        DataPermission.CREATE_QUERIES,
-      );
-    });
+    permissions = updateEntityPermission(
+      permissions,
+      groupId,
+      entityId,
+      DataPermissionValue.QUERY_BUILDER,
+      database,
+      DataPermission.CREATE_QUERIES,
+    );
   }
 
   if (

resources/migrations/001_update_migrations.yaml

@@ -10087,6 +10087,23 @@ databaseChangeLog:
                   constraints:
                     nullable: true
 
+  - changeSet:
+      id: v52.2024-10-15T23:14:11
+      author: noahmoss
+      comment: downgrade migration for table-level native perms
+      changes:
+        - sql: "SELECT 1" # do nothing on forward migration
+      rollback:
+        - sqlFile:
+            dbms: postgresql,h2
+            path: permissions/rollback/table_level_native_perms.sql
+            relativeToChangelogFile: true
+        - sqlFile:
+            dbms: mysql,mariadb
+            path: permissions/rollback/mysql_table_level_native_perms.sql
+            relativeToChangelogFile: true
+
+
   # >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
 
 ########################################################################################################################

resources/migrations/permissions/rollback/mysql_table_level_native_perms.sql

@@ -0,0 +1,48 @@
+-- Downgrade table-level native access to query-builder-only access
+UPDATE data_permissions
+SET perm_value = 'query-builder'
+WHERE perm_value = 'query-builder-and-native'
+AND table_id IS NOT NULL;
+
+-- Coalesce table-level perms to DB perms: if all tables in a DB have 'query-builder' access, insert a DB-level perm
+-- instead, with a NULL schema and table ID
+INSERT INTO data_permissions (
+    group_id,
+    perm_type,
+    db_id,
+    schema_name,
+    table_id,
+    perm_value
+)
+SELECT
+  dp.group_id,
+  'perms/create-queries' AS perm_type,
+  dp.db_id,
+  NULL AS schema_name,
+  NULL AS table_id,
+  'query-builder' AS perm_value
+FROM data_permissions dp
+WHERE dp.table_id IS NOT NULL
+  AND dp.perm_value = 'query-builder'
+  AND NOT EXISTS (
+      SELECT 1
+      FROM data_permissions dp2
+      WHERE dp2.group_id = dp.group_id
+        AND dp2.db_id = dp.db_id
+        AND dp2.schema_name IS NOT NULL
+        AND dp2.perm_value = 'no'
+        AND dp2.table_id IS NOT NULL
+  )
+GROUP BY dp.group_id, dp.db_id;
+
+-- Delete table-level permissions if a DB-level permission was created
+DELETE dp1
+FROM data_permissions dp1
+JOIN data_permissions dp2
+  ON dp1.group_id = dp2.group_id
+  AND dp1.db_id = dp2.db_id
+  AND dp2.schema_name IS NULL
+  AND dp2.table_id IS NULL
+  AND dp2.perm_value = 'query-builder'
+WHERE dp1.perm_value = 'query-builder'
+  AND dp1.table_id IS NOT NULL;

resources/migrations/permissions/rollback/table_level_native_perms.sql

@@ -0,0 +1,50 @@
+-- Downgrade table-level native access to query-builder-only access
+UPDATE data_permissions
+SET perm_value = 'query-builder'
+WHERE perm_value = 'query-builder-and-native'
+AND table_id IS NOT NULL;
+
+-- Coalesce table-level perms to DB perms: if all tables in a DB have 'query-builder' access, insert a DB-level perm
+-- instead, with a NULL schema and table ID
+INSERT INTO data_permissions (
+    group_id,
+    perm_type,
+    db_id,
+    schema_name,
+    table_id,
+    perm_value
+)
+SELECT
+  dp.group_id,
+  'perms/create-queries' AS perm_type,
+  dp.db_id,
+  NULL AS schema_name,
+  NULL AS table_id,
+  'query-builder' AS perm_value
+FROM data_permissions dp
+WHERE dp.table_id IS NOT NULL
+  AND dp.perm_value = 'query-builder'
+  AND NOT EXISTS (
+      SELECT 1
+      FROM data_permissions dp2
+      WHERE dp2.group_id = dp.group_id
+        AND dp2.db_id = dp.db_id
+        AND dp2.schema_name IS NOT NULL
+        AND dp2.perm_value = 'no'
+        AND dp2.table_id IS NOT NULL
+  )
+GROUP BY dp.group_id, dp.db_id;
+
+-- Delete table-level permissions if a DB-level permission was created
+DELETE FROM data_permissions
+WHERE perm_value = 'query-builder'
+  AND table_id IS NOT NULL
+  AND EXISTS (
+      SELECT 1
+      FROM data_permissions dp2
+      WHERE dp2.group_id = data_permissions.group_id
+        AND dp2.db_id = data_permissions.db_id
+        AND dp2.schema_name IS NULL
+        AND dp2.table_id IS NULL
+        AND dp2.perm_value = 'query-builder'
+  );

src/metabase/api/database.clj

@@ -81,8 +81,8 @@
                                      [:map
                                       [:native_permissions [:enum :write :none]]]]]
   "For each database in DBS add a `:native_permissions` field describing the current user's permissions for running
-  native (e.g. SQL) queries. Will be either `:write` or `:none`. `:write` means you can run ad-hoc native queries,
-  and save new Cards with native queries; `:none` means you can do neither.
+  native (e.g. SQL) queries. Will be either `:write` or `:none`. `:write` means you can run ad-hoc native queries on at
+  least one table, and save new Cards with native queries; `:none` means you can do neither.
 
   For the curious: the use of `:write` and `:none` is mainly for legacy purposes, when we had data-access-based
   permissions; there was a specific option where you could give a Perms Group permissions to run existing Cards with
@@ -93,7 +93,7 @@
     (assoc db
            :native_permissions
            (if (= :query-builder-and-native
-                  (data-perms/full-db-permission-for-user
+                  (data-perms/most-permissive-database-permission-for-user
                    api/*current-user-id*
                    :perms/create-queries
                    (u/the-id db)))

src/metabase/api/permission_graph.clj

@@ -56,7 +56,7 @@
 (def ^:private Perms
   "Perms that get reused for TablePerms and SchemaPerms"
   [:enum
-   :all :segmented :none :full :limited :unrestricted :legacy-no-self-service :sandboxed :query-builder :no :blocked])
+   :all :segmented :none :full :limited :unrestricted :legacy-no-self-service :sandboxed :query-builder-and-native :query-builder :no :blocked])
 
 (def ^:private TablePerms
   [:or Perms [:map
@@ -114,7 +114,6 @@
      [:map
       [:view-data {:optional true} Schemas]
       [:create-queries {:optional true} Schemas]
-      [:data {:optional true} "StrictDataPerms"]
       [:download {:optional true} "StrictDataPerms"]
       [:data-model {:optional true} "StrictDataPerms"]
       [:details {:optional true} [:enum :yes :no]]]
@@ -128,7 +127,7 @@
   [:map
    [:groups [:map-of GroupId [:maybe StrictDbGraph]]]])
 
-(def StrictApiPermissionsGraph
+(def ApiDataPermissionsGraphph
   "Top level strict data graph schema expected over the API. Includes revision ID for avoiding concurrent updates."
   [:map
    [:groups [:map-of GroupId [:maybe StrictDbGraph]]]

src/metabase/api/permissions.clj

@@ -131,7 +131,7 @@
   {body :map
    skip-graph [:maybe :boolean]}
   (api/check-superuser)
-  (let [new-graph (mc/decode api.permission-graph/StrictApiPermissionsGraph
+  (let [new-graph (mc/decode api.permission-graph/ApiDataPermissionsGraphph
                              body
                              (mtx/transformer
                               mtx/string-transformer

src/metabase/driver/common/parameters/values.clj

@@ -18,7 +18,6 @@
    [metabase.lib.schema.id :as lib.schema.id]
    [metabase.lib.schema.template-tag :as lib.schema.template-tag]
    [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
-   [metabase.query-processor.compile :as qp.compile]
    [metabase.query-processor.error-type :as qp.error-type]
    [metabase.query-processor.middleware.limit :as limit]
    [metabase.query-processor.store :as qp.store]
@@ -203,7 +202,9 @@
                       {:query (qp.persistence/persisted-info-native-query
                                (u/the-id (lib.metadata/database (qp.store/metadata-provider)))
                                persisted-info)})
-                    (qp.compile/compile (limit/disable-max-results query))))))
+                    ;; Resolve namespace at runtime to avoid circular dependency
+                    ((requiring-resolve 'metabase.query-processor.compile/compile)
+                     (limit/disable-max-results query))))))
       (catch ExceptionInfo e
         (throw (ex-info
                 (tru "The sub-query from referenced question #{0} failed with the following error: {1}"

src/metabase/models/data_permissions.clj

@@ -453,8 +453,10 @@
       (or (coalesce perm-type perm-values)
           (least-permissive-value perm-type)))))
 
-(mu/defn native-download-permission-for-user :- PermissionValue
-  "Returns the effective download permission value for a given user and database ID, for native queries on the database.
+(mu/defn fallback-native-download-permission-for-user :- PermissionValue
+  "Returns the download permission value to use for native queries, if we don't have information available about the
+  tables referenced by the native query.
+
   For each group, the native download permission for a database is equal to the lowest permission level of any table in
   the database."
   [user-id database-id]
@@ -809,11 +811,7 @@
                     other-new-perms (map (fn [table]
                                            {:perm_type   perm-type
                                             :group_id    group-id
-                                            :perm_value  (case existing-db-perm-value
-                                                           ;; If the previous database-level permission can't be set at
-                                                           ;; the table-level, we need to provide a new default
-                                                           :query-builder-and-native :query-builder
-                                                           existing-db-perm-value)
+                                            :perm_value  existing-db-perm-value
                                             :db_id       db-id
                                             :table_id    (:id table)
                                             :schema_name (:schema table)})

src/metabase/models/query/permissions.clj

@@ -15,6 +15,7 @@
    [metabase.models.interface :as mi]
    [metabase.models.permissions :as perms]
    [metabase.permissions.util :as perms.u]
+   [metabase.query-analysis.native-query-analyzer :as nqa]
    [metabase.query-processor.error-type :as qp.error-type]
    [metabase.query-processor.store :as qp.store]
    [metabase.query-processor.util :as qp.util]
@@ -59,21 +60,50 @@
 ;;           {:perms/view-data {table-id :unrestricted}}  source-card-read-perms
 ;;
 
-(mu/defn query->source-table-ids :- [:set [:or [:= ::native] ::lib.schema.id/table]]
-  "Return a sequence of all Table IDs referenced by `query`, and/or the ::native keyword for native queries."
+(mu/defn query->native-table-ids :- [:set [:or [:= ::native] ::lib.schema.id/table]]
+  "Returns a sequence of all Table IDs referenced by any native query strings in `query`, based on the output of the
+  Native Query Analyzer. Returns the placeholder value ::native if the query can't be parsed, or the parser doesn't
+  detect any tables, to indicate that full native access to the database should be required."
   [query :- :map]
   (set
    (flatten
     (lib.util.match/match query
-      ;; if we come across a native query just put a placeholder (`::native`) there so we know we need to
-      ;; add native permissions to the complete set below.
+      ;; if we come across a native query, try to parse its tables using the Native Query Analyzer. If it errors or
+      ;; otherwise can't parse the query, we insert a ::native placeholder so that full native query access will be
+      ;; required for the DB.
       (m :guard (every-pred map? :native))
-      [::native]
+      (try
+        (let [refs      (nqa/tables-for-native (assoc m :database (:database query)))
+              table-ids (->> refs
+                             :tables
+                             (map :table-id)
+                             (filter some?))]
+          (if (seq table-ids)
+            table-ids
+            [::native]))
+        (catch Throwable e
+          (log/error e)
+          [::native]))))))
 
+(mu/defn query->mbql-table-ids :- [:set ::lib.schema.id/table]
+  "Return a sequence of all Table IDs referenced by `query`, excluding tables referenced in native query strings.
+  (Use `query->native-table-ids` for native query parsing.)"
+  [query :- :map]
+  (set
+   (flatten
+    (lib.util.match/match query
       (m :guard (every-pred map? #(pos-int? (:source-table %))))
       (cons
        (:source-table m)
-       (query->source-table-ids (dissoc m :source-table)))))))
+       (query->mbql-table-ids (dissoc m :source-table)))))))
+
+(mu/defn query->source-table-ids :- [:set [:or [:= ::native] ::lib.schema.id/table]]
+  "Returns a set of all Table IDs referenced by `query`, including both MBQL and native references. Returns the
+  placeholder value ::native if a native query can't be parsed, or the parser doesn't detect any tables, to indicate
+  that full native access to the database should be required."
+  [query :- :map]
+  (set/union (query->mbql-table-ids query)
+             (query->native-table-ids query)))
 
 (def ^:dynamic *card-instances*
   "A map from card IDs to card instances with the collection_id (possibly nil).
@@ -117,7 +147,7 @@
      query)
     (not-empty @all-ids)))
 
-(defn- native-query-perms
+(defn- full-native-query-perms
   [query]
   (merge
    {:perms/create-queries :query-builder-and-native
@@ -128,6 +158,22 @@
                              (mi/perms-objects-set (card-instance card-id) :read)))
                    card-ids)})))
 
+(defn- native-query-perms
+  [query]
+  (merge
+   (let [table-ids                  (vec (query->native-table-ids query))
+         require-full-native-perms? (.contains ^clojure.lang.PersistentVector table-ids ::native)]
+     (if require-full-native-perms?
+       (full-native-query-perms query)
+       (when (seq table-ids)
+         {:perms/create-queries (zipmap table-ids (repeat :query-builder-and-native))
+          :perms/view-data      (zipmap table-ids (repeat :unrestricted))})))
+   (when-let [card-ids (referenced-card-ids query)]
+     {:paths (into #{}
+                   (mapcat (fn [card-id]
+                             (mi/perms-objects-set (card-instance card-id) :read)))
+                   card-ids)})))
+
 (defn- legacy-mbql-required-perms
   [query {:keys [throw-exceptions? already-preprocessed?]}]
   (try
@@ -136,17 +182,20 @@
       (if-let [source-card-id (qp.util/query->source-card-id query)]
         {:paths (source-card-read-perms source-card-id)}
         ;; otherwise if there's no source card then calculate perms based on the Tables referenced in the query
-        (let [query               (cond-> query
-                                    (not already-preprocessed?) preprocess-query)
-              table-ids-or-native (vec (query->source-table-ids query))
-              table-ids           (filter integer? table-ids-or-native)
-              native?             (.contains ^clojure.lang.PersistentVector table-ids-or-native ::native)]
-          (merge
-           (when (seq table-ids)
-             {:perms/create-queries (zipmap table-ids (repeat :query-builder))
-              :perms/view-data      (zipmap table-ids (repeat :unrestricted))})
-           (when native?
-             (native-query-perms query))))))
+        (let [query                      (cond-> query
+                                           (not already-preprocessed?) preprocess-query)
+              mbql-table-ids             (vec (query->mbql-table-ids query))
+              native-table-ids           (vec (query->native-table-ids query))
+              require-full-native-perms? (.contains ^clojure.lang.PersistentVector native-table-ids ::native)]
+          (if require-full-native-perms?
+            (full-native-query-perms query)
+            (merge
+             (when (seq mbql-table-ids)
+               {:perms/create-queries (zipmap mbql-table-ids (repeat :query-builder))
+                :perms/view-data      (zipmap mbql-table-ids (repeat :unrestricted))})
+             (when (seq native-table-ids)
+               {:perms/create-queries (zipmap native-table-ids (repeat :query-builder-and-native))
+                :perms/view-data      (zipmap native-table-ids (repeat :unrestricted))}))))))
     ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card) just return a set of permissions
     ;; that means no one will ever get to see it
     (catch Throwable e

src/metabase/query_analysis/native_query_analyzer.clj

@@ -274,7 +274,8 @@
   [driver query]
   (let [db-id      (:database query)
         macaw-opts (nqa.impl/macaw-options driver)
-        sql-string (:query (nqa.sub/replace-tags query))
+        sql-string (let [replaced (nqa.sub/replace-tags query)]
+                     (:query replaced replaced))
         result     (macaw/query->components (macaw/parsed-query sql-string macaw-opts) macaw-opts)]
     (if (:error result)
       ;; We may want to do some error translation here in the future, to decrease coupling to Macaw.
@@ -297,7 +298,8 @@
   (let [db-id      (:database query)
         macaw-opts (nqa.impl/macaw-options driver)
         table-opts (assoc macaw-opts :mode mode)
-        sql-string (:query (nqa.sub/replace-tags query))
+        sql-string (let [replaced (nqa.sub/replace-tags query)]
+                     (:query replaced replaced))
         result     (macaw/query->tables sql-string table-opts)]
     (u/update-if-exists result :tables table-refs-for-query db-id)))
 

src/metabase/query_analysis/native_query_analyzer/parameter_substitution.clj

@@ -1,7 +1,5 @@
 (ns metabase.query-analysis.native-query-analyzer.parameter-substitution
-  "Replace {{variable}}s and {{field filters}} in SQL queries with parse-able equivalents."
-  (:require
-   [metabase.query-processor.compile :as qp.compile]))
+  "Replace {{variable}}s and {{field filters}} in SQL queries with parse-able equivalents.")
 
 (def default-values
   "Map of default values for each type"
@@ -56,5 +54,7 @@
   template tags."
   [query]
   (if-let [name->tag (seq (get-in query [:native :template-tags]))]
-    (qp.compile/compile (assoc-in query [:native :template-tags] (update-vals name->tag tag-default)))
+    ;; Resolve namespace at runtime to avoid circular dependency
+    ((requiring-resolve 'metabase.query-processor.compile/compile)
+     (assoc-in query [:native :template-tags] (update-vals name->tag tag-default)))
     (:native query)))

test/metabase/api/card_test.clj

@@ -2657,7 +2657,7 @@
           (update-card card {:description "a new description"})
           (is (empty? (reviews card)))))
       (testing "Does not add nil moderation reviews when there are reviews but not verified"
-        ;; testing that we aren't just adding a nil moderation each time we update a card
+       ;; testing that we aren't just adding a nil moderation each time we update a card
         (with-card :verified
           (is (verified? card))
           (moderation-review/create-review! {:moderated_item_id   (u/the-id card)
@@ -3759,44 +3759,6 @@
                 (is (= [[1] [2]] (mt/rows (process-query-for-card child-card)))
                     "view-data = unrestricted is sufficient to allow running the query")))))))))
 
-(deftest cannot-run-any-native-queries-when-blocked-test
-  (mt/with-non-admin-groups-no-root-collection-perms
-    (mt/with-temp [:model/Collection allowed-collection    {}
-                   :model/Collection disallowed-collection {}
-                   :model/Card       parent-card           {:dataset_query {:database (mt/id)
-                                                                            :type     :native
-                                                                            :native   {:query "SELECT id FROM venues ORDER BY id ASC LIMIT 2;"}}
-                                                            :database_id   (mt/id)
-                                                            :collection_id (u/the-id disallowed-collection)}
-                   :model/Card       child-card            {:dataset_query {:database (mt/id)
-                                                                            :type     :query
-                                                                            :query    {:source-table (format "card__%d" (u/the-id parent-card))}}
-                                                            :collection_id (u/the-id allowed-collection)}]
-      (letfn [(process-query-for-card [card]
-                (mt/user-http-request :rasta :post (format "card/%d/query" (u/the-id card))))]
-        (testing "Cannot run native queries when a single table is unrestricted and the rest are blocked"
-          (mt/with-no-data-perms-for-all-users!
-            (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :blocked)
-            (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :unrestricted)
-            (perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
-            (is (thrown-with-msg?
-                 clojure.lang.ExceptionInfo
-                 #"You do not have permissions to run this query."
-                 (mt/rows (process-query-for-card child-card)))
-                "Someone with `:blocked` permissions on ANY table in the database cannot run ANY card with native queries, including as a source for another card.")))
-        ;; update collection perms in place:
-        (perms/revoke-collection-permissions! (perms-group/all-users) allowed-collection)
-        (testing "Cannot run native queries when a single table is blocked and the rest are unrestricted"
-          (mt/with-no-data-perms-for-all-users!
-            (data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/view-data :unrestricted)
-            (data-perms/set-table-permission! (perms-group/all-users) (mt/id :venues) :perms/view-data :blocked)
-            (perms/grant-collection-read-permissions! (perms-group/all-users) allowed-collection)
-            (is (thrown-with-msg?
-                 clojure.lang.ExceptionInfo
-                 #"You do not have permissions to run this query."
-                 (mt/rows (process-query-for-card child-card)))
-                "Someone with `:blocked` permissions on ANY table in the database cannot run ANY card with native queries, including as a source for another card.")))))))
-
 (deftest query-metadata-test
   (mt/with-temp
     [Card {card-id-1 :id} {:dataset_query (mt/mbql-query products)

test/metabase/api/database_test.clj

@@ -1095,6 +1095,33 @@
           (check-tables-included response (virtual-table-for-card ok-card))
           (check-tables-not-included response (virtual-table-for-card bad-card)))))))
 
+(deftest database-list-native-perms-info-test
+  (testing "GET /api/database"
+    (testing "A native_permissions key is set to `write` if any tables have native query permissions"
+      (mt/with-temp [:model/Database                   {db-id :id}      {}
+                     :model/Table                      {table-id-1 :id} {:db_id db-id}
+                     :model/Table                      {table-id-2 :id} {:db_id db-id}
+                     :model/User                       {user-id :id}    {}
+                     :model/PermissionsGroup           {group-id :id}   {}
+                     :model/PermissionsGroupMembership {}               {:user_id user-id
+                                                                         :group_id group-id}]
+        (mt/with-no-data-perms-for-all-users!
+          (let [fetch-db (fn [] (->> (mt/user-http-request user-id :get 200 "database")
+                                     :data
+                                     (filter #(= (:id %) db-id))
+                                     first))]
+            (data-perms/set-database-permission! group-id db-id :perms/create-queries :query-builder)
+            (is (= "none" (:native_permissions (fetch-db))))
+
+            (data-perms/set-database-permission! group-id db-id :perms/create-queries :query-builder-and-native)
+            (is (= "write" (:native_permissions (fetch-db))))
+
+            (data-perms/set-table-permission! group-id table-id-1 :perms/create-queries :query-builder)
+            (is (= "write" (:native_permissions (fetch-db))))
+
+            (data-perms/set-table-permission! group-id table-id-2 :perms/create-queries :query-builder)
+            (is (= "none" (:native_permissions (fetch-db))))))))))
+
 (deftest ^:parallel db-metadata-saved-questions-db-test
   (testing "GET /api/database/:id/metadata works for the Saved Questions 'virtual' database"
     (t2.with-temp/with-temp [Card card (assoc (card-with-native-query "Birthday Card")

test/metabase/db/schema_migrations_test.clj

@@ -2714,3 +2714,53 @@
           (is (= "1.2.3.4:5555" (t2/select-one-fn :value :model/Setting :key "embedding-app-origin")))
           (migrate!)
           (is (= "1.2.3.4:5555" (t2/select-one-fn :value :model/Setting :key "embedding-app-origins-interactive"))))))))
+
+(deftest table-level-native-perms-rollback-test
+  (impl/test-migrations "v52.2024-10-15T23:14:11" [migrate!]
+    (let [migrate-up!  (fn []
+                         (migrate!)
+                         (clear-permissions!))
+          group-id     (first (t2/insert-returning-pks! (t2/table-name PermissionsGroup) {:name "Test Group"}))
+          db-id        (first (t2/insert-returning-pks! (t2/table-name Database) {:name       "db"
+                                                                                  :engine     "postgres"
+                                                                                  :created_at :%now
+                                                                                  :updated_at :%now
+                                                                                  :details    "{}"}))
+          table-id-1   (first (t2/insert-returning-pks! (t2/table-name Table) {:db_id      db-id
+                                                                               :name       "Table 1"
+                                                                               :schema     "PUBLIC"
+                                                                               :created_at :%now
+                                                                               :updated_at :%now
+                                                                               :active     true}))
+          table-id-2   (first (t2/insert-returning-pks! (t2/table-name Table) {:db_id      db-id
+                                                                               :name       "Table 2"
+                                                                               :schema     "PUBLIC"
+                                                                               :created_at :%now
+                                                                               :updated_at :%now
+                                                                               :active     true}))
+          insert-perm! (fn [perm-type perm-value & [table-id]]
+                         (t2/insert! (t2/table-name :model/DataPermissions)
+                                     :db_id db-id
+                                     :group_id group-id
+                                     :table_id table-id
+                                     :schema_name "PUBLIC"
+                                     :perm_type perm-type
+                                     :perm_value perm-value))]
+      (testing "Test that table-level native create-queries permissions get correctly cleared when rolling back from 52->51"
+        ;; If all tables end up as `query-builder`, they're replaced with a DB-level perm
+        (migrate-up!)
+        (insert-perm! "perms/create-queries" "query-builder-and-native" table-id-1)
+        (insert-perm! "perms/create-queries" "query-builder" table-id-2)
+        (migrate! :down 51)
+        (is (= #{["query-builder" nil]}
+               (t2/select-fn-set (juxt :perm_value :table_id)
+                                 (t2/table-name :model/DataPermissions) :db_id db-id :group_id group-id)))
+
+        ;; If at least one table is `no`, we keep perms at the table-level
+        (migrate-up!)
+        (insert-perm! "perms/create-queries" "query-builder-and-native" table-id-1)
+        (insert-perm! "perms/create-queries" "no" table-id-2)
+        (migrate! :down 51)
+        (is (= #{["query-builder" table-id-1] ["no" table-id-2]}
+               (t2/select-fn-set (juxt :perm_value :table_id)
+                                 (t2/table-name :model/DataPermissions) :db_id db-id :group_id group-id)))))))

test/metabase/models/data_permissions/graph_test.clj

@@ -112,6 +112,14 @@
             {:perms/create-queries {"PUBLIC" {table-id-1 :query-builder}}
              :perms/view-data {"PUBLIC" {table-id-1 :unrestricted}}}}}
 
+          {group-id-1
+           {database-id-1
+            {:create-queries {"PUBLIC" {table-id-1 :query-builder-and-native}}}}}
+          {group-id-1
+           {database-id-1
+            {:perms/create-queries {"PUBLIC" {table-id-1 :query-builder-and-native}}
+             :perms/view-data {"PUBLIC" {table-id-1 :unrestricted}}}}}
+
           {group-id-1
            {database-id-1
             {:create-queries {"PUBLIC" {table-id-1 :no}}}}}

test/metabase/models/data_permissions_test.clj

@@ -129,24 +129,6 @@
           (is (nil?  (create-queries-perm-value table-id-2)))
           (is (nil?  (create-queries-perm-value table-id-3))))))))
 
-(deftest native-queries-against-db-with-some-blocked-table-is-illegal-test
-  (mt/with-temp [:model/Card {card-id :id {db-id :database} :dataset_query} {:dataset_query (mt/native-query {:query "select 1"})}]
-    (mt/with-no-data-perms-for-all-users!
-      (data-perms/set-database-permission! (perms-group/all-users) db-id :perms/create-queries (data-perms/most-permissive-value :perms/create-queries))
-      (data-perms/set-database-permission! (perms-group/all-users) db-id :perms/view-data (data-perms/most-permissive-value :perms/view-data))
-      ;; rasta has access to the database:
-      (is (= "Can Run Query"
-             (:error (mt/user-http-request :rasta :post 202 (format "card/%d/query" card-id))
-                     "Can Run Query")))
-
-      ;; block a single table on the db:
-      (let [tables-in-db (map :id (:tables (t2/hydrate (t2/select-one :model/Database db-id) :tables)))
-            table-id (rand-nth tables-in-db)]
-        (data-perms/set-table-permissions! (perms-group/all-users) :perms/view-data {table-id :blocked}))
-
-      (is (= "You do not have permissions to run this query."
-             (:error (mt/user-http-request :rasta :post 202 (format "card/%d/query" card-id))))))))
-
 (deftest database-permission-for-user-test
   (mt/with-temp [:model/PermissionsGroup           {group-id-1 :id}    {}
                  :model/PermissionsGroup           {group-id-2 :id}    {}

test/metabase/models/query/permissions_test.clj

@@ -96,16 +96,24 @@
               (is (= false
                      (mi/can-write? card))))))))))
 
-(defn- native [query]
-  {:database 1
-   :type     :native
-   :native   {:query query}})
+(deftest ^:parallel native-query-test
+  (testing (str "By default, native queries are parsed and perms are only required for referenced tables")
+    (is (= {:perms/view-data      {(mt/id :checkins) :unrestricted}
+            :perms/create-queries {(mt/id :checkins) :query-builder-and-native}}
+           (query-perms/required-perms-for-query
+            {:database (mt/id)
+             :type     :native
+             :native   "SELECT * FROM CHECKINS"})))))
 
-(deftest ^:parallel native-query-perms-test
-  (is (= {:perms/create-queries :query-builder-and-native
-          :perms/view-data :unrestricted}
-         (query-perms/required-perms-for-query
-          (native "SELECT count(*) FROM toucan_sightings;")))))
+(deftest ^:parallel invalid-native-query-test
+  (testing (str "If the native query can't be parsed, or we can't map table references to table IDs in Metabase, we "
+                "require full data + native query access to the database")
+    (is (= {:perms/view-data      :unrestricted
+            :perms/create-queries :query-builder-and-native}
+           (query-perms/required-perms-for-query
+            {:database (mt/id)
+             :type     :native
+             :native   "SELECT * FROM INVALID_TABLE"})))))
 
 (deftest ^:parallel mbql-query-test
   (is (= {:perms/view-data      {(mt/id :venues) :unrestricted}
@@ -202,14 +210,25 @@
 (deftest ^:parallel nested-native-query-test-2
   (testing (str "However if you just pass in the same query directly as a `:source-query` you will still require "
                 "READWRITE permissions to save the query since we can't verify that it belongs to a Card that you can view.")
-    (is (= {:perms/view-data :unrestricted
-            :perms/create-queries :query-builder-and-native}
+    (is (= {:perms/view-data      {(mt/id :checkins) :unrestricted}
+            :perms/create-queries {(mt/id :checkins) :query-builder-and-native}}
            (query-perms/required-perms-for-query
             {:database (mt/id)
              :type     :query
              :query    {:source-query {:native "SELECT * FROM CHECKINS"}}}
             :throw-exceptions? true)))))
 
+(deftest ^:parallel invalid-nested-native-query-test
+  (testing (str "If you pass in a native query as the `:source-query`, but we can't detect the correct table IDs "
+                "by parsing the SQL, then default to requiring full native access.")
+    (is (= {:perms/view-data      :unrestricted
+            :perms/create-queries :query-builder-and-native}
+           (query-perms/required-perms-for-query
+            {:database (mt/id)
+             :type     :query
+             :query    {:source-query {:native "SELECT * FROM non_existant_table"}}}
+            :throw-exceptions? true)))))
+
 (deftest ^:parallel invalid-queries-test
   (testing "invalid/legacy queries should return perms for something that doesn't exist so no one gets to see it"
     (is (= {:perms/create-queries {0 :query-builder}}