Skip to content
Snippets Groups Projects
Unverified Commit 8a3e16b0 authored by Ngoc Khuat's avatar Ngoc Khuat Committed by GitHub
Browse files

Fix incorrect hash for Advanced fieldvalues (#24740) 

* fix incorrectly hash a fieldvalues for field that is not the field we use to define sandbox rule

* appease clj-kondo

* make the doc clearer

* remove the _id destructring
parent cc8439ab
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
enterprise/backend/src/metabase_enterprise/sandbox/models/params/field_values.clj
+ 8
− 5
View file @ 8a3e16b0
......@@ -40,24 +40,27 @@
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 applied to `field-id`
- 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 example we have an GTAP rules
{:card_id 1
{:card_id 1
:attribute_remappings {\"State\" [:dimension [:field 3 nil]]}}
And users with login-attributes {\"State\" \"CA\"}
;; (field-id->gtap-attributes-for-current-user (Field 3))
;; -> [1, {\"State\" \"CA\"}]"
[{:keys [table_id id] :as _field}]
[{:keys [table_id] :as _field}]
(when-let [gtap (table-id->gtap table_id)]
(let [login-attributes (:login_attributes @api/*current-user*)
attribute_remappings (:attribute_remappings gtap)]
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
:when (= (mbql.u/match-one v [:dimension [:field id _]] id) id)]
;; 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!*
......
enterprise/backend/test/metabase_enterprise/sandbox/models/params/field_values_test.clj
+ 50
− 33
View file @ 8a3e16b0
......@@ -51,39 +51,47 @@
;; copy at top level so that `with-gtaps-for-user` does not have to create a new copy every time it gets called
(mt/with-temp-copy-of-db
(testing "gtap with remappings"
(letfn [(hash-for-user-id [user-id login-attributes]
(letfn [(hash-for-user-id [user-id login-attributes field-id]
(mt/with-gtaps-for-user user-id
{:gtaps {:categories {:remappings {"State" [:dimension [:field (mt/id :categories :name) nil]]}}}
:attributes login-attributes}
(ee-params.field-values/hash-key-for-sandbox (mt/id :categories :name))))]
(ee-params.field-values/hash-key-for-sandbox field-id)))]
(mt/with-temp* [User [{user-id-1 :id}]
User [{user-id-2 :id}]]
(testing "2 users with the same attribute should have the same hash"
(is (= (hash-for-user-id user-id-1 {"State" "CA"})
(hash-for-user-id user-id-2 {"State" "CA"})))
(testing "2 users with the same attribute"
(testing "should have the same hash for the same field"
(is (= (hash-for-user-id user-id-1 {"State" "CA"} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "CA"} (mt/id :categories :name)))))
(testing "should have different hash for different fields"
(is (not= (hash-for-user-id user-id-1 {"State" "CA"} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "CA"} (mt/id :categories :id)))))
(testing "having extra login attributes won't effect the hash"
(is (= (hash-for-user-id user-id-1 {"State" "CA"
"City" "San Jose"})
(hash-for-user-id user-id-2 {"State" "CA"})))))
"City" "San Jose"} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "CA"} (mt/id :categories :name))))))
(testing "same users but the login_attributes change should have different hash"
(is (not= (hash-for-user-id user-id-1 {"State" "CA"})
(hash-for-user-id user-id-1 {"State" "NY"}))))
(testing "2 users with the same attribute should have the different hash for different "
(is (= (hash-for-user-id user-id-1 {"State" "CA"} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "CA"} (mt/id :categories :name)))))
(testing "2 users with different login_attributes should have different hash"
(is (not= (hash-for-user-id user-id-1 {"State" "CA"})
(hash-for-user-id user-id-2 {"State" "NY"})))
(is (not= (hash-for-user-id user-id-1 {})
(hash-for-user-id user-id-2 {"State" "NY"}))))))))
(testing "same users but the login_attributes change should have different hash"
(is (not= (hash-for-user-id user-id-1 {"State" "CA"} (mt/id :categories :name))
(hash-for-user-id user-id-1 {"State" "NY"} (mt/id :categories :name)))))
(testing "2 users with different login_attributes should have different hash"
(is (not= (hash-for-user-id user-id-1 {"State" "CA"} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "NY"} (mt/id :categories :name))))
(is (not= (hash-for-user-id user-id-1 {} (mt/id :categories :name))
(hash-for-user-id user-id-2 {"State" "NY"} (mt/id :categories :name)))))))))
(testing "gtap with card and remappings"
;; hack so that we don't have to setup all the sandbox permissions the table
(with-redefs [ee-params.field-values/field-is-sandboxed? (constantly true)]
(letfn [(hash-for-user-id-with-attributes [user-id login_attributes]
(letfn [(hash-for-user-id-with-attributes [user-id login_attributes field-id]
(mt/with-temp-vals-in-db User user-id {:login_attributes login_attributes}
(mw.session/with-current-user user-id
(ee-params.field-values/hash-key-for-sandbox (mt/id :categories :name)))))]
(ee-params.field-values/hash-key-for-sandbox field-id))))]
(testing "2 users in the same group"
(mt/with-temp*
[Card [{card-id :id}]
......@@ -98,13 +106,18 @@
:group_id group-id
:table_id (mt/id :categories)
:attribute_remappings {"State" [:dimension [:field (mt/id :categories :name) nil]]}}]]
(testing "if the have the same attributes, the hash should be the ssame"
(is (= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "CA"}))))
(testing "if the have the different attributes, the hash should be the different"
(is (not= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "NY"}))))))
(testing "with same attributes, the hash should be the same field"
(is (= (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" "CA"} (mt/id :categories :name)))))
(testing "with same attributes, the hash should different for different fields"
(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" "CA"} (mt/id :categories :id)))))
(testing "with different attributes, the hash should be the different"
(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 "2 users in different groups but gtaps use the same card"
(mt/with-temp*
......@@ -128,13 +141,17 @@
:group_id group-id-2
:table_id (mt/id :categories)
:attribute_remappings {"State" [:dimension [:field (mt/id :categories :name) nil]]}}]]
(testing "if the have the same attributes, the hash should be the ssame"
(is (= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "CA"}))))
(testing "with the same attributes, the hash should be the same"
(is (= (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" "CA"} (mt/id :categories :name)))))
(testing "with same attributes, the hash should different for different fields"
(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" "CA"} (mt/id :categories :id)))))
(testing "if the have the different attributes, the hash should be the different"
(is (not= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "NY"}))))))
(testing "with different attributes, the hash should be the different"
(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 "2 users in different groups and gtaps use 2 different cards"
(mt/with-temp*
......@@ -158,7 +175,7 @@
:table_id (mt/id :categories)
:attribute_remappings {"State" [:dimension [:field (mt/id :categories :name) nil]]}}]]
(testing "the hash are different even though they have the same attribute"
(is (not= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "CA"})))
(is (not= (hash-for-user-id-with-attributes user-id-1 {"State" "CA"})
(hash-for-user-id-with-attributes user-id-2 {"State" "NY"})))))))))))
(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" "CA"} (mt/id :categories :name))))
(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))))))))))))
src/metabase/api/dashboard.clj
+ 1
− 1
View file @ 8a3e16b0
......@@ -608,7 +608,7 @@
(try
(let [results (map (if (seq query)
#(chain-filter/chain-filter-search % constraints query :limit result-limit)
#(chain-filter/chain-filter % constraints :limit result-limit))
#(chain-filter/chain-filter % constraints :limit result-limit))
field-ids)
values (distinct (mapcat :values results))
has_more_values (boolean (some true? (map :has_more_values results)))]
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment