Skip to content
Snippets Groups Projects
Unverified Commit 9d6f180f authored by Cam Saul's avatar Cam Saul
Browse files

Save Card read_permissions

parent de519667
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 159 additions and 75 deletions
resources/migrations/000_migrations.yaml
+ 11
− 0
View file @ 9d6f180f
......@@ -4020,3 +4020,14 @@ databaseChangeLog:
remarks: 'True if a XLS of the data should be included for this pulse card'
constraints:
nullable: false
- changeSet:
id: 75
author: camsaul
comment: 'Added 0.28.2'
changes:
- addColumn:
tableName: report_card
columns:
name: read_permissions
type: text
remarks: 'Permissions required to view this Card and run its query.'
src/metabase/api/database.clj
+ 8
− 3
View file @ 9d6f180f
......@@ -55,7 +55,9 @@
native (e.g. SQL) queries. Will be one of `:write`, `:read`, or `:none`."
[dbs]
(for [db dbs]
(let [user-has-perms? (fn [path-fn] (perms/set-has-full-permissions? @api/*current-user-permissions-set* (path-fn (u/get-id db))))]
(let [user-has-perms? (fn [path-fn]
(perms/set-has-full-permissions? @api/*current-user-permissions-set*
(path-fn (u/get-id db))))]
(assoc db :native_permissions (cond
(user-has-perms? perms/native-readwrite-path) :write
(user-has-perms? perms/native-read-path) :read
......@@ -103,7 +105,8 @@
(defn- source-query-cards
"Fetch the Cards that can be used as source queries (e.g. presented as virtual tables)."
[]
(as-> (db/select [Card :name :description :database_id :dataset_query :id :collection_id :result_metadata]
(as-> (db/select [Card :name :description :database_id :dataset_query :id :collection_id :result_metadata
:read_permissions]
:result_metadata [:not= nil] :archived false
{:order-by [[:%lower.name :asc]]}) <>
(filter card-database-supports-nested-queries? <>)
......@@ -142,7 +145,9 @@
include-cards? add-virtual-tables-for-saved-cards)))
(api/defendpoint GET "/"
"Fetch all `Databases`."
"Fetch all `Databases`. `include_tables` means we should hydrate the Tables belonging to each DB. `include_cards` here
means we should also include virtual Table entries for saved Questions, e.g. so we can easily use them as source
Tables in queries. Default for both is `false`."
[include_tables include_cards]
{include_tables (s/maybe su/BooleanString)
include_cards (s/maybe su/BooleanString)}
......
src/metabase/db/migrations.clj
+ 13
− 0
View file @ 9d6f180f
......@@ -361,3 +361,16 @@
;; either way, delete the old value from the DB since we'll never be using it again.
;; use `simple-delete!` because `Setting` doesn't have an `:id` column :(
(db/simple-delete! Setting {:key "enable-advanced-humanization"})))
;; for every Card in the DB, pre-calculate the read permissions required to read the Card/run its query and save them
;; under the new `read_permissions` column. Calculating read permissions is too expensive to do on the fly for Cards,
;; since it requires parsing their queries and expanding things like FKs or Segment/Metric macros. Simply calling
;; `update!` on each Card will cause it to be saved with updated `read_permissions` as a side effect of Card's
;; `pre-update` implementation.
;;
;; Caching these permissions will prevent 1000+ DB call API calls. See https://github.com/metabase/metabase/issues/6889
(defmigration ^{:author "camsaul", :added "0.28.2"} populate-card-read-permissions
(run!
(fn [card]
(db/update! Card (u/get-id card) {}))
(db/select-reducible Card :archived false, :read_permissions nil)))
src/metabase/models/card.clj
+ 47
− 24
View file @ 9d6f180f
......@@ -114,28 +114,48 @@
(u/pprint-to-str (u/filtered-stacktrace e)))
#{"/db/0/"}))) ; DB 0 will never exist
;; it takes a lot of DB calls and function calls to expand/resolve a query, and since they're pure functions we can
;; save ourselves some a lot of DB calls by caching the results. Cache the permissions reqquired to run a given query
;; dictionary for up to 6 hours
;; Calculating Card read permissions is rather expensive, since we must parse and expand the Card's query in order to
;; find the Tables it references. Since we read Cards relatively often, these permissions are cached in the
;; `:read_permissions` column of Card. There should not be situtations where these permissions are not cached; but if
;; for some strange reason they are we will go ahead and recalculate them.
;; TODO - what if the query uses a source query, and that query changes? Not sure if that will cause an issue or not.
;; May need to revisit this
(defn- query-perms-set* [{query-type :type, database :database, :as query} read-or-write]
(defn query-perms-set
"Calculate the set of permissions required to `:read`/run or `:write` (update) a Card with `query`. In normal cases
for read permissions you should look at a Card's `:read_permissions`, which is precalculated. If you specifically
need to calculate permissions for a query directly, and ignore anything precomputed, use this function. Otherwise
you should rely on one of the optimized ones below."
[{query-type :type, database :database, :as query} read-or-write]
(cond
(= query {}) #{}
(empty? query) #{}
(= (keyword query-type) :native) #{(native-permissions-path read-or-write database)}
(= (keyword query-type) :query) (mbql-permissions-path-set read-or-write query)
:else (throw (Exception. (str "Invalid query type: " query-type)))))
(def ^{:arglists '([query read-or-write])} query-perms-set
"Return a set of required permissions for *running* QUERY (if READ-OR-WRITE is `:read`) or *saving* it (if
READ-OR-WRITE is `:write`)."
(memoize/ttl query-perms-set* :ttl/threshold (* 6 60 60 1000))) ; memoize for 6 hours
(defn- perms-objects-set
"Return a set of required permissions object paths for CARD.
Optionally specify whether you want `:read` or `:write` permissions; default is `:read`.
(`:write` permissions only affects native queries)."
[{query :dataset_query, collection-id :collection_id, public-uuid :public_uuid, in-public-dash? :in_public_dashboard}
(defn- card-perms-set-for-query
"Return the permissions required to `read-or-write` `card` based on its query, disregarding the collection the Card is
in, whether it is publicly shared, etc. This will return precalculated `:read_permissions` if they are present."
[{read-perms :read_permissions, id :id, query :dataset_query} read-or-write]
(cond
;; for WRITE permissions always recalculate since these will be determined relatively infrequently (hopefully)
;; e.g. when updating a Card
(= :write read-or-write) (query-perms-set query :write)
;; if the Card has populated `:read_permissions` and we're looking up read pems return those rather than calculating
;; on-the-fly. Cast to `set` to be extra-double-sure it's a set like we'd expect when it gets deserialized from JSON
read-perms (set read-perms)
;; otherwise if :read_permissions was NOT populated. This should not normally happen since the data migration
;; should have pre-populated values for all the Cards. If it does it might mean something like we fetched the Card
;; without its `read_permissions` column. Since that would be "doing something wrong" warn about it.
:else (do (log/warn "Card" id "is missing its read_permissions. Calculating them now...")
(query-perms-set query :read))))
(defn- card-perms-set-for-current-user
"Calculate the permissions required to `read-or-write` `card` *for the current user*. This takes into account whether
the Card is publicly available, or in a collection the current user can view; it also attempts to use precalcuated
`read_permissions` when possible. This is the function that should be used for general permissions checking for a
Card."
[{collection-id :collection_id, public-uuid :public_uuid, in-public-dash? :in_public_dashboard, :as card}
read-or-write]
(cond
;; you don't need any permissions to READ a public card, which is PUBLIC by definition :D
......@@ -148,7 +168,7 @@
(collection/perms-objects-set collection-id read-or-write)
:else
(query-perms-set query read-or-write)))
(card-perms-set-for-query card read-or-write)))
;;; -------------------------------------------------- Dependencies --------------------------------------------------
......@@ -201,15 +221,16 @@
:query_type (keyword query-type)})
card))
(defn- pre-insert [{:keys [dataset_query], :as card}]
(defn- pre-insert [{query :dataset_query, :as card}]
;; TODO - make sure if `collection_id` is specified that we have write permissions for that collection
(u/prog1 card
;; Save the new Card with read permissions since calculating them dynamically is so expensive.
(u/prog1 (assoc card :read_permissions (query-perms-set query :read))
;; for native queries we need to make sure the user saving the card has native query permissions for the DB
;; because users can always see native Cards and we don't want someone getting around their lack of permissions
;; that way
(when (and *current-user-id*
(= (keyword (:type dataset_query)) :native))
(let [database (db/select-one ['Database :id :name], :id (:database dataset_query))]
(= (keyword (:type query)) :native))
(let [database (db/select-one ['Database :id :name], :id (:database query))]
(qp-perms/throw-if-cannot-run-new-native-query-referencing-db database)))))
(defn- post-insert [card]
......@@ -220,8 +241,9 @@
(log/info "Card references Fields in params:" field-ids)
(field-values/update-field-values-for-on-demand-dbs! field-ids))))
(defn- pre-update [{archived? :archived, :as card}]
(u/prog1 card
(defn- pre-update [{archived? :archived, query :dataset_query, :as card}]
;; save the updated Card with updated read permissions.
(u/prog1 (assoc card :read_permissions (query-perms-set query :read))
;; if the Card is archived, then remove it from any Dashboards
(when archived?
(db/delete! 'DashboardCard :card_id (u/get-id card)))
......@@ -260,7 +282,8 @@
:embedding_params :json
:query_type :keyword
:result_metadata :json
:visualization_settings :json})
:visualization_settings :json
:read_permissions :json-set})
:properties (constantly {:timestamped? true})
:pre-update (comp populate-query-fields pre-update)
:pre-insert (comp populate-query-fields pre-insert)
......@@ -272,7 +295,7 @@
(merge i/IObjectPermissionsDefaults
{:can-read? (partial i/current-user-has-full-permissions? :read)
:can-write? (partial i/current-user-has-full-permissions? :write)
:perms-objects-set perms-objects-set})
:perms-objects-set card-perms-set-for-current-user})
revision/IRevisioned
(assoc revision/IRevisionedDefaults
......
src/metabase/models/common.clj
+ 1
− 1
View file @ 9d6f180f
(ns metabase.models.common)
(def ^:const timezones
(def timezones
"The different timezones supported by Metabase.
Presented as options for the `report-timezone` Setting in the admin panel."
["Africa/Algiers"
......
src/metabase/models/interface.clj
+ 6
− 0
View file @ 9d6f180f
......@@ -32,6 +32,12 @@
:in json-in
:out json-out)
;; json-set is just like json but calls `set` on it when coming out of the DB. Intended for storing things like a
;; permissions set
(models/add-type! :json-set
:in json-in
:out #(when % (set (json-out %))))
(models/add-type! :clob
:in identity
:out u/jdbc-clob->str)
......
src/metabase/task/sync_databases.clj
+ 26
− 21
View file @ 9d6f180f
......@@ -20,9 +20,9 @@
(:import metabase.models.database.DatabaseInstance
[org.quartz CronTrigger DisallowConcurrentExecution JobDetail JobKey TriggerKey]))
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | JOB LOGIC |
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | JOB LOGIC |
;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private job-context->database :- DatabaseInstance
"Get the Database referred to in JOB-CONTEXT. Guaranteed to return a valid Database."
......@@ -43,9 +43,9 @@
(when (:is_full_sync database)
(field-values/update-field-values! (job-context->database job-context)))))
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | TASK INFO AND GETTER FUNCTIONS |
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | TASK INFO AND GETTER FUNCTIONS |
;;; +----------------------------------------------------------------------------------------------------------------+
(def ^:private TaskInfo
"One-off schema for information about the various sync tasks we run for a DB."
......@@ -64,7 +64,8 @@
:job-class UpdateFieldValues})
;; These getter functions are not strictly neccesary but are provided primarily so we can get some extra validation by using them
;; These getter functions are not strictly neccesary but are provided primarily so we can get some extra validation by
;; using them
(s/defn ^:private job-key :- JobKey
"Return an appropriate string key for the job described by TASK-INFO for DATABASE-OR-ID."
......@@ -97,15 +98,16 @@
(format "%s for all databases" (name (:key task-info))))
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | DELETING TASKS FOR A DB |
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | DELETING TASKS FOR A DB |
;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private delete-task!
"Cancel a single sync task for DATABASE-OR-ID and TASK-INFO."
[database :- DatabaseInstance, task-info :- TaskInfo]
(let [trigger-key (trigger-key database task-info)]
(log/debug (u/format-color 'red "Unscheduling task for Database %d: trigger: %s" (u/get-id database) (.getName trigger-key)))
(log/debug (u/format-color 'red "Unscheduling task for Database %d: trigger: %s"
(u/get-id database) (.getName trigger-key)))
(task/delete-trigger! trigger-key)))
(s/defn unschedule-tasks-for-db!
......@@ -114,9 +116,10 @@
(delete-task! database sync-analyze-task-info)
(delete-task! database field-values-task-info))
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | (RE)SCHEDULING TASKS FOR A DB |
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | (RE)SCHEDULING TASKS FOR A DB |
;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private job :- JobDetail
"Build a durable Quartz Job for TASK-INFO. Durable in Quartz allows the job to exist even if there are no triggers
......@@ -158,17 +161,19 @@
;; unschedule any tasks that might already be scheduled
(unschedule-tasks-for-db! database)
(log/debug (u/format-color 'green "Scheduling sync/analyze and field-values task for database %d: trigger: %s and trigger: %s"
(u/get-id database) (.getName (.getKey sync-trigger))
(u/get-id database) (.getName (.getKey fv-trigger))))
(log/debug
(u/format-color 'green "Scheduling sync/analyze and field-values task for database %d: trigger: %s and trigger: %s"
(u/get-id database) (.getName (.getKey sync-trigger))
(u/get-id database) (.getName (.getKey fv-trigger))))
;; now (re)schedule all the tasks
(task/add-trigger! sync-trigger)
(task/add-trigger! fv-trigger)))
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; | TASK INITIALIZATION |
;;; +------------------------------------------------------------------------------------------------------------------------+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | TASK INITIALIZATION |
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- job-init
"Separated from `task-init` primarily as it's useful in testing. Adds the sync and field-values job that all of the
......@@ -179,7 +184,7 @@
(defn task-init
"Automatically called during startup; start the jobs for syncing/analyzing and updating FieldValues for all
Databases."
Databases."
[]
(job-init)
(doseq [database (db/select Database)]
......
test/metabase/api/card_test.clj
+ 4
− 2
View file @ 9d6f180f
......@@ -213,9 +213,10 @@
:database_id database-id ; these should be inferred automatically
:table_id table-id
:labels []
:can_write true,
:dashboard_count 0,
:can_write true
:dashboard_count 0
:collection nil
:read_permissions [(format "/db/%d/schema//table/%d/" database-id table-id)]
:creator (match-$ (fetch-user :rasta)
{:common_name "Rasta Toucan"
:is_superuser false
......@@ -295,6 +296,7 @@
:id $})
:updated_at $
:dataset_query $
:read_permissions [(format "/db/%d/schema//table/%d/" database-id table-id)]
:id $
:display "table"
:visualization_settings {}
......
test/metabase/api/dashboard_test.clj
+ 1
− 0
View file @ 9d6f180f
......@@ -133,6 +133,7 @@
:display "table"
:query_type nil
:dataset_query {}
:read_permissions []
:visualization_settings {}
:query_average_duration nil
:in_public_dashboard false
......
test/metabase/api/embed_test.clj
+ 6
− 4
View file @ 9d6f180f
......@@ -139,10 +139,12 @@
[{:id nil, :type "date/single", :target ["variable" ["template-tag" "d"]], :name "d", :slug "d", :default nil}]
(with-embedding-enabled-and-new-secret-key
(with-temp-card [card {:enable_embedding true
:dataset_query {:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
:b {:type "date", :name "b", :display_name "b"}
:c {:type "date", :name "c", :display_name "c"}
:d {:type "date", :name "d", :display_name "d"}}}}
:dataset_query {:database (data/id)
:type :native
:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
:b {:type "date", :name "b", :display_name "b"}
:c {:type "date", :name "c", :display_name "c"}
:d {:type "date", :name "d", :display_name "d"}}}}
:embedding_params {:a "locked", :b "disabled", :c "enabled", :d "enabled"}}]
(:parameters (http/client :get 200 (card-url card {:params {:c 100}}))))))
......
test/metabase/api/preview_embed_test.clj
+ 6
− 4
View file @ 9d6f180f
......@@ -48,10 +48,12 @@
[{:id nil, :type "date/single", :target ["variable" ["template-tag" "d"]], :name "d", :slug "d", :default nil}]
(embed-test/with-embedding-enabled-and-new-secret-key
(embed-test/with-temp-card [card {:dataset_query
{:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
:b {:type "date", :name "b", :display_name "b"}
:c {:type "date", :name "c", :display_name "c"}
:d {:type "date", :name "d", :display_name "d"}}}}}]
{:database (data/id)
:type :native
:native {:template_tags {:a {:type "date", :name "a", :display_name "a"}
:b {:type "date", :name "b", :display_name "b"}
:c {:type "date", :name "c", :display_name "c"}
:d {:type "date", :name "d", :display_name "d"}}}}}]
(-> ((test-users/user->client :crowberto) :get 200 (card-url card {:_embedding_params {:a "locked"
:b "disabled"
:c "enabled"
......
test/metabase/api/public_test.clj
+ 29
− 16
View file @ 9d6f180f
......@@ -53,7 +53,9 @@
(defn- add-card-to-dashboard! [card dashboard]
(db/insert! DashboardCard :dashboard_id (u/get-id dashboard), :card_id (u/get-id card)))
(defmacro with-temp-public-dashboard-and-card {:style/indent 1} [[dashboard-binding card-binding & [dashcard-binding]] & body]
(defmacro ^:private with-temp-public-dashboard-and-card
{:style/indent 1}
[[dashboard-binding card-binding & [dashcard-binding]] & body]
`(with-temp-public-dashboard [dash#]
(with-temp-public-card [card#]
(let [~dashboard-binding dash#
......@@ -98,18 +100,20 @@
{(data/id :categories :name) {:values 75
:human_readable_values {}
:field_id (data/id :categories :name)}}
(tt/with-temp Card [card {:dataset_query {:type :native
:native {:query (str "SELECT COUNT(*) "
"FROM venues "
"LEFT JOIN categories ON venues.category_id = categories.id "
"WHERE {{category}}")
:collection "CATEGORIES"
:template_tags {:category {:name "category"
:display_name "Category"
:type "dimension"
:dimension ["field-id" (data/id :categories :name)]
:widget_type "category"
:required true}}}}}]
(tt/with-temp Card [card {:dataset_query
{:database (data/id)
:type :native
:native {:query (str "SELECT COUNT(*) "
"FROM venues "
"LEFT JOIN categories ON venues.category_id = categories.id "
"WHERE {{category}}")
:collection "CATEGORIES"
:template_tags {:category {:name "category"
:display_name "Category"
:type "dimension"
:dimension ["field-id" (data/id :categories :name)]
:widget_type "category"
:required true}}}}}]
(-> (:param_values (#'public-api/public-card :id (u/get-id card)))
(update-in [(data/id :categories :name) :values] count))))
......@@ -313,7 +317,9 @@
(tu/with-temporary-setting-values [enable-public-sharing true]
(with-temp-public-dashboard-and-card [dash card]
(with-temp-public-card [card-2]
(tt/with-temp DashboardCardSeries [_ {:dashboardcard_id (db/select-one-id DashboardCard :card_id (u/get-id card), :dashboard_id (u/get-id dash))
(tt/with-temp DashboardCardSeries [_ {:dashboardcard_id (db/select-one-id DashboardCard
:card_id (u/get-id card)
:dashboard_id (u/get-id dash))
:card_id (u/get-id card-2)}]
(qp-test/rows (http/client :get 200 (dashcard-url-path dash card-2))))))))
......@@ -334,7 +340,8 @@
(db/update! Dashboard (u/get-id dashboard) :parameters [{:name "Price", :type "category", :slug "price"}]))
(defn- add-dimension-param-mapping-to-dashcard! [dashcard card dimension]
(db/update! DashboardCard (u/get-id dashcard) :parameter_mappings [{:card_id (u/get-id card), :target ["dimension" dimension]}]))
(db/update! DashboardCard (u/get-id dashcard) :parameter_mappings [{:card_id (u/get-id card)
:target ["dimension" dimension]}]))
(defn- GET-param-values [dashboard]
(tu/with-temporary-setting-values [enable-public-sharing true]
......@@ -344,7 +351,13 @@
(expect
(price-param-values)
(with-temp-public-dashboard-and-card [dash card dashcard]
(db/update! Card (u/get-id card) :dataset_query {:native {:template_tags {:price {:name "price", :display_name "Price", :type "dimension", :dimension ["field-id" (data/id :venues :price)]}}}})
(db/update! Card (u/get-id card)
:dataset_query {:database (data/id)
:type :native
:native {:template_tags {:price {:name "price"
:display_name "Price"
:type "dimension"
:dimension ["field-id" (data/id :venues :price)]}}}})
(add-price-param-to-dashboard! dash)
(add-dimension-param-mapping-to-dashcard! dashcard card ["template-tag" "price"])
(GET-param-values dash)))
......
test/metabase/events/revision_test.clj
+ 1
− 0
View file @ 9d6f180f
......@@ -33,6 +33,7 @@
:creator_id (:creator_id card)
:database_id (id)
:dataset_query (:dataset_query card)
:read_permissions (vec (:read_permissions card))
:description nil
:display "table"
:enable_embedding false
......
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