Fix incorrectness issue for advanced FieldValues when sandboxing use native question (#24967)

* use login-attributes when hash advanced field-values if sandbox use native query

* add a e2e test

* Add repro for #24966

Co-authored-by: Nemanja <31325167+nemanjaglumac@users.noreply.github.com>

enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj

@@ -9,7 +9,8 @@
             [metabase.models.field-values :as field-values]
             [metabase.models.params.field-values :as params.field-values]
             [metabase.public-settings.premium-features :refer [defenterprise]]
-            [toucan.db :as db]))
+            [toucan.db :as db]
+            [toucan.hydrate :refer [hydrate]]))
 
 (comment api/keep-me)
 
@@ -32,7 +33,7 @@
       (row-level-restrictions/assert-one-gtap-per-table gtaps)
       ;; there shold be only one gtap per table and we only need one table here
       ;; see docs in [[metabase.models.permissions]] for more info
-      (first gtaps))))
+      (hydrate (first gtaps) :card))))
 
 (defn- field->gtap-attributes-for-current-user
   "Returns the gtap attributes for current user that applied to `field`.
@@ -40,11 +41,13 @@
   The gtap-attributes is a list with 2 elements:
   1. card-id - for GTAP that use a saved question
   2. a map:
-    - with key is the user-attribute that applied to the table that `field` is in
-    - value is the user-attribute of current user corresponding to the key
+    if query is mbql query:
+      - with key is the user-attribute that applied to the table that `field` is in
+      - value is the user-attribute of current user corresponding to the key
+    for native query, this map will be the login-attributes of user
 
   For example we have an GTAP rules
-  {:card_id              1
+  {:card_id              1 ;; a mbql query
    :attribute_remappings {\"State\" [:dimension [:field 3 nil]]}}
 
   And users with login-attributes {\"State\" \"CA\"}
@@ -57,11 +60,17 @@
           attribute_remappings (:attribute_remappings gtap)
           field-ids            (db/select-field :id Field :table_id table_id)]
       [(:card_id gtap)
-       (into {} (for [[k v] attribute_remappings
-                      ;; get attribute that map to fields of the same table
-                      :when (contains? field-ids
-                                       (mbql.u/match-one v [:dimension [:field field-id _]] field-id))]
-                  {k (get login-attributes k)}))])))
+       (if (= :native (get-in gtap [:card :query_type]))
+         ;; For sandbox that uses native query, we can't narrow down to the exact attribute
+         ;; that affect the current table. So we just hash the whole login-attributes of users.
+         ;; This makes hashing a bit less efficient but it ensures that user get a new hash
+         ;; if they change login attributes
+         login-attributes
+         (into {} (for [[k v] attribute_remappings
+                        ;; get attribute that map to fields of the same table
+                        :when (contains? field-ids
+                                         (mbql.u/match-one v [:dimension [:field field-id _]] field-id))]
+                    {k (get login-attributes k)})))])))
 
 (defenterprise get-or-create-field-values-for-current-user!*
   "Fetch cached FieldValues for a `field`, creating them if needed if the Field should have FieldValues. These

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

@@ -22,67 +22,82 @@
 
 (deftest field-values-test
   (testing "GET /api/field/:id/values"
-    (mt/with-gtaps {:gtaps      {:venues {:query      (mt.tu/restricted-column-query (mt/id))
-                                          :remappings {:cat [:dimension (mt/id :venues :category_id)]}}}
-                    :attributes {:cat 50}}
-      (testing (str "When I call the FieldValues API endpoint for a Field that I have segmented table access only "
-                    "for, will I get ad-hoc values?\n")
-        (letfn [(fetch-values [user field]
-                  (-> (mt/user-http-request user :get 200 (format "field/%d/values" (mt/id :venues field)))
-                      (update :values (partial take 3))))]
-          ;; Rasta Toucan is only allowed to see Venues that are in the "Mexican" category [category_id = 50]. So
-          ;; fetching FieldValues for `venue.name` should do an ad-hoc fetch and only return the names of venues in
-          ;; that category.
-          (is (= {:field_id        (mt/id :venues :name)
-                  :values          [["Garaje"]
-                                    ["Gordo Taqueria"]
-                                    ["La Tortilla"]]
-                  :has_more_values false}
-                 (fetch-values :rasta :name)))
+    (mt/with-temp-copy-of-db
+      (doseq [[query-type gtap-rule]
+              [["MBQL"
+                {:gtaps      {:venues {:query      (mt.tu/restricted-column-query (mt/id))
+                                       :remappings {:cat [:dimension (mt/id :venues :category_id)]}}}
+                 :attributes {:cat 50}}]
+               ["native"
+                {:gtaps      {:venues {:query
+                                       (mt/native-query
+                                         {:query "SELECT id, name, category_id FROM venues WHERE category_id = {{cat}}"
+                                          :template-tags {"cat" {:id           "__MY_CAT__"
+                                                                 :name         "cat"
+                                                                 :display-name "Cat id"
+                                                                 :type         :number}}})
+                                       :remappings {:cat [:variable [:template-tag "cat"]]}}}
+                 :attributes {:cat 50}}]]]
+        (testing (format "GTAP rule is a %s query" query-type)
+          (mt/with-gtaps gtap-rule
+            (testing (str "When I call the FieldValues API endpoint for a Field that I have segmented table access only "
+                          "for, will I get ad-hoc values?\n")
+              (letfn [(fetch-values [user field]
+                        (-> (mt/user-http-request user :get 200 (format "field/%d/values" (mt/id :venues field)))
+                            (update :values (partial take 3))))]
+                ;; Rasta Toucan is only allowed to see Venues that are in the "Mexican" category [category_id = 50]. n
+                ;; fetching FieldValues for `venue.name` should do an ad-hoc fetch and only return the names of venues in
+                ;; that category.
+                (is (= {:field_id        (mt/id :venues :name)
+                        :values          [["Garaje"]
+                                          ["Gordo Taqueria"]
+                                          ["La Tortilla"]]
+                        :has_more_values false}
+                       (fetch-values :rasta :name)))
 
-          (testing (str "Now in this case recall that the `restricted-column-query` GTAP we're using does *not* include "
-                        "`venues.price` in the results. (Toucan isn't allowed to know the number of dollar signs!) So "
-                        "make sure if we try to fetch the field values instead of seeing `[[1] [2] [3] [4]]` we get no "
-                        "results")
-            (is (= {:field_id        (mt/id :venues :price)
-                    :values          []
-                    :has_more_values false}
-                   (fetch-values :rasta :price))))
+                (testing (str "Now in this case recall that the `restricted-column-query` GTAP we're using does *not* include "
+                              "`venues.price` in the results. (Toucan isn't allowed to know the number of dollar signs!) So "
+                              "make sure if we try to fetch the field values instead of seeing `[[1] [2] [3] [4]]` we get no "
+                              "results")
+                  (is (= {:field_id        (mt/id :venues :price)
+                          :values          []
+                          :has_more_values false}
+                         (fetch-values :rasta :price))))
 
-          (testing "Reset field values; if another User fetches them first, do I still see sandboxed values? (metabase/metaboat#128)"
-            (field-values/clear-field-values-for-field! (mt/id :venues :name))
-            ;; fetch Field values with an admin
-            (testing "Admin should see all Field values"
-              (is (= {:field_id        (mt/id :venues :name)
-                      :values          [["20th Century Cafe"]
-                                        ["25°"]
-                                        ["33 Taps"]]
-                      :has_more_values false}
-                     (fetch-values :crowberto :name))))
-            (testing "Sandboxed User should still see only their values after an admin fetches the values"
-              (is (= {:field_id        (mt/id :venues :name)
-                      :values          [["Garaje"]
-                                        ["Gordo Taqueria"]
-                                        ["La Tortilla"]]
-                      :has_more_values false}
-                     (fetch-values :rasta :name))))
-            (testing "A User with a *different* sandbox should see their own values"
-              (let [password (mt/random-name)]
-                (mt/with-temp User [another-user {:password password}]
-                  (mt/with-gtaps-for-user another-user {:gtaps      {:venues
-                                                                     {:remappings
-                                                                      {:cat
-                                                                       [:dimension (mt/id :venues :category_id)]}}}
-                                                        :attributes {:cat 5 #_BBQ}}
+                (testing "Reset field values; if another User fetches them first, do I still see sandboxed values? (metabase/metaboat#128)"
+                  (field-values/clear-field-values-for-field! (mt/id :venues :name))
+                  ;; fetch Field values with an admin
+                  (testing "Admin should see all Field values"
+                    (is (= {:field_id        (mt/id :venues :name)
+                            :values          [["20th Century Cafe"]
+                                              ["25°"]
+                                              ["33 Taps"]]
+                            :has_more_values false}
+                           (fetch-values :crowberto :name))))
+                  (testing "Sandboxed User should still see only their values after an admin fetches the values"
                     (is (= {:field_id        (mt/id :venues :name)
-                            :values          [["Baby Blues BBQ"]
-                                              ["Bludso's BBQ"]
-                                              ["Boneyard Bistro"]]
+                            :values          [["Garaje"]
+                                              ["Gordo Taqueria"]
+                                              ["La Tortilla"]]
                             :has_more_values false}
-                           (-> (mt/client {:username (:email another-user), :password password}
-                                          :get 200
-                                          (format "field/%d/values" (mt/id :venues :name)))
-                               (update :values (partial take 3)))))))))))))))
+                           (fetch-values :rasta :name))))
+                  (testing "A User with a *different* sandbox should see their own values"
+                    (let [password (mt/random-name)]
+                      (mt/with-temp User [another-user {:password password}]
+                        (mt/with-gtaps-for-user another-user {:gtaps      {:venues
+                                                                           {:remappings
+                                                                            {:cat
+                                                                             [:dimension (mt/id :venues :category_id)]}}}
+                                                              :attributes {:cat 5 #_BBQ}}
+                          (is (= {:field_id        (mt/id :venues :name)
+                                  :values          [["Baby Blues BBQ"]
+                                                    ["Bludso's BBQ"]
+                                                    ["Boneyard Bistro"]]
+                                  :has_more_values false}
+                                 (-> (mt/client {:username (:email another-user), :password password}
+                                                :get 200
+                                                (format "field/%d/values" (mt/id :venues :name)))
+                                     (update :values (partial take 3))))))))))))))))))
 
 (deftest human-readable-values-test
   (testing "GET /api/field/:id/values should returns correct human readable mapping if exists"

enterprise/backend/test/metabase_enterprise/sandbox/models/params/field_values_test.clj

@@ -119,6 +119,32 @@
                (is (not= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"} (mt/id :categories :name))
                          (hash-for-user-id-with-attributes user-id-2 {"State" "NY"} (mt/id :categories :name)))))))
 
+          (testing "gtap with native question"
+            (mt/with-temp*
+              [Card                       [{card-id :id} {:query_type :native
+                                                          :name "A native query"
+                                                          :dataset_query
+                                                          {:type :native
+                                                           :database (mt/id)
+                                                           :native
+                                                           {:query "SELECT * FROM People WHERE state = {{STATE}}"
+                                                            :template-tags
+                                                            {"STATE" {:id "72461b3b-3877-4538-a5a3-7a3041924517"
+                                                                      :name "STATE"
+                                                                      :display-name "STATE"
+                                                                      :type "text"}}}}}]
+               PermissionsGroup           [{group-id :id}]
+               User                       [{user-id :id}]
+               PermissionsGroupMembership [_ {:group_id group-id
+                                              :user_id user-id}]
+               GroupTableAccessPolicy     [_ {:card_id card-id
+                                              :group_id group-id
+                                              :table_id (mt/id :categories)
+                                              :attribute_remappings {"State" [:variable [:template-tag "STATE"]]}}]]
+              (testing "same users but if the login_attributes change, they should have different hash (#24966)"
+                (is (not= (hash-for-user-id-with-attributes user-id {"State" "CA"} (mt/id :categories :name))
+                          (hash-for-user-id-with-attributes user-id {"State" "NY"} (mt/id :categories :name)))))))
+
           (testing "2 users in different groups but gtaps use the same card"
             (mt/with-temp*
               [Card                       [{card-id :id}]

frontend/test/metabase/scenarios/permissions/reproductions/24966-saved-native-q-field-values.cy.spec.js

+import {
+  restore,
+  visitQuestion,
+  visitDashboard,
+  filterWidget,
+} from "__support__/e2e/helpers";
+
+import { SAMPLE_DATABASE } from "__support__/e2e/cypress_sample_database";
+
+const { PRODUCTS, PRODUCTS_ID } = SAMPLE_DATABASE;
+
+const sandboxingQuestion = {
+  name: "geadsfasd",
+  native: {
+    query:
+      "select products.category,PRODUCTS.title from PRODUCTS where true [[AND products.CATEGORY = {{category}}]]",
+    "template-tags": {
+      category: {
+        id: "411b40bb-1374-9787-6ffb-20604df56d73",
+        name: "category",
+        "display-name": "Category",
+        type: "text",
+      },
+    },
+  },
+  parameters: [
+    {
+      id: "411b40bb-1374-9787-6ffb-20604df56d73",
+      type: "category",
+      target: ["variable", ["template-tag", "category"]],
+      name: "Category",
+      slug: "category",
+    },
+  ],
+};
+
+const dashboardFilter = {
+  name: "Text",
+  slug: "text",
+  id: "ec00b255",
+  type: "string/=",
+  sectionId: "string",
+};
+
+const dashboardDetails = { parameters: [dashboardFilter] };
+
+describe("issue 24966", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+
+    // Add user attribute to existing ("nodata" / id:3 user
+    cy.request("PUT", "/api/user/3", {
+      login_attributes: { attr_cat: "Gizmo" },
+    });
+
+    cy.createNativeQuestion(sandboxingQuestion).then(({ body: { id } }) => {
+      visitQuestion(id);
+
+      cy.sandboxTable({
+        table_id: PRODUCTS_ID,
+        card_id: id,
+        attribute_remappings: {
+          attr_cat: ["variable", ["template-tag", "category"]],
+        },
+      });
+    });
+
+    // Add the saved products table to the dashboard
+    cy.createQuestionAndDashboard({
+      questionDetails: {
+        query: {
+          "source-table": PRODUCTS_ID,
+          limit: 10,
+        },
+      },
+      dashboardDetails,
+    }).then(({ body: { id, card_id, dashboard_id } }) => {
+      cy.wrap(dashboard_id).as("dashboardId");
+
+      // Connect the filter to the card
+      cy.request("PUT", `/api/dashboard/${dashboard_id}/cards`, {
+        cards: [
+          {
+            id,
+            card_id,
+            col: 0,
+            row: 0,
+            sizeX: 12,
+            sizeY: 8,
+            parameter_mappings: [
+              {
+                parameter_id: dashboardFilter.id,
+                card_id,
+                target: ["dimension", ["field", PRODUCTS.CATEGORY, null]],
+              },
+            ],
+          },
+        ],
+      });
+    });
+  });
+
+  it("should correctly fetch field values for a filter when native question is used for sandboxing (metabase#24966)", () => {
+    cy.get("@dashboardId").then(id => {
+      cy.signIn("nodata");
+      visitDashboard(id);
+      filterWidget().click();
+      cy.findByTestId("Gizmo-filter-value").click();
+      cy.button("Add filter").click();
+      cy.location("search").should("eq", "?text=Gizmo");
+
+      cy.signInAsSandboxedUser();
+      visitDashboard(id);
+      filterWidget().click();
+      cy.findByTestId("Widget-filter-value").click();
+      cy.button("Add filter").click();
+      cy.location("search").should("eq", "?text=Widget");
+    });
+  });
+});