diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj b/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
index 30c60503207523bee2d07fb0f0c76fd442d30001..07616a89b4157d519618b0b593a7517b17d48fd4 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
+++ b/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
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
index 478d32f3eade75cb6364d12175219f79d0cb6b8a..d280eab60405df73ab581fb55fd0919a85994f14 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/field_test.clj
+++ b/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"
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/models/params/field_values_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/models/params/field_values_test.clj
index c326667f22d767c56de1757b1a8ca7ee068d6c7b..67a44199f48cc172fc1ab5c519dae1a6ba345415 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/models/params/field_values_test.clj
+++ b/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}]
diff --git a/frontend/test/metabase/scenarios/permissions/reproductions/24966-saved-native-q-field-values.cy.spec.js b/frontend/test/metabase/scenarios/permissions/reproductions/24966-saved-native-q-field-values.cy.spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..2150b3a9caf2afd1017db80f1c5a297a8400b1f8
--- /dev/null
+++ b/frontend/test/metabase/scenarios/permissions/reproductions/24966-saved-native-q-field-values.cy.spec.js
@@ -0,0 +1,121 @@
+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");
+ });
+ });
+});