Skip to content

Commit

Permalink
Merge pull request metabase#3648 from metabase/fix-csv-download-witho…
Browse files Browse the repository at this point in the history 
…ut-write-permissions

Fix CSV Download for SQL queries without write permissions
  • Loading branch information
@camsaul
camsaul authored Oct 25, 2016
2 parents e8f1025 + 2ca5308 commit 1d0db46
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 32 deletions.
2 changes: 1 addition & 1 deletion frontend/src/metabase/query_builder/NativeQueryEditor.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export default class NativeQueryEditor extends Component {
let dataSelectors = [];
if (this.state.showEditor && this.props.nativeDatabases) {
// we only render a db selector if there are actually multiple to choose from
if (this.props.nativeDatabases.length > 1) {
if (this.props.nativeDatabases.length > 1 && (this.props.query.database === null || _.any(this.props.nativeDatabases, (db) => db.id === this.props.query.database))) {
dataSelectors.push(
<div key="db_selector" className="GuiBuilder-section GuiBuilder-data flex align-center">
<span className="GuiBuilder-section-label Query-label">Database</span>
Expand Down
6 changes: 4 additions & 2 deletions frontend/src/metabase/query_builder/QueryVisualization.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ export default class QueryVisualization extends Component {
renderDownloadButton() {
const { card, result } = this.props;

const csvUrl = card.id != null ? `/api/card/${card.id}/query/csv`: "/api/dataset/csv";

if (result && !result.error) {
if (result && result.data && result.data.rows_truncated) {
// this is a "large" dataset, so show a modal to inform users about this and make them click again to d/l
Expand All @@ -160,7 +162,7 @@ export default class QueryVisualization extends Component {
}}>Download CSV</button>);
} else {
downloadButton = (
<form ref={(c) => this._downloadCsvForm = c} method="POST" action="/api/dataset/csv">
<form ref={(c) => this._downloadCsvForm = c} method="POST" action={csvUrl}>
<input type="hidden" name="query" value="" />
<a className="Button Button--primary" onClick={() => {this.onDownloadCSV(); this.refs.downloadModal.toggle();}}>
Download CSV
Expand Down Expand Up @@ -200,7 +202,7 @@ export default class QueryVisualization extends Component {
);
} else {
return (
<form ref={(c) => this._downloadCsvForm = c} method="POST" action="/api/dataset/csv">
<form ref={(c) => this._downloadCsvForm = c} method="POST" action={csvUrl}>
<input type="hidden" name="query" value="" />
<a className="mx1" title="Download this data" onClick={() => this.onDownloadCSV()}>
<Icon name='download' size={16} />
Expand Down
25 changes: 16 additions & 9 deletions src/metabase/api/card.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,17 +262,24 @@

;;; ------------------------------------------------------------ Running a Query ------------------------------------------------------------

(defn- run-query-for-card [card-id parameters]
(let [card (read-check Card card-id)
query (assoc (:dataset_query card)
:parameters parameters
:constraints dataset-api/query-constraints)
options {:executed-by *current-user-id*
:card-id card-id}]
(qp/dataset-query query options)))

(defendpoint POST "/:card-id/query"
"Run the query associated with a Card."
[card-id :as {{:keys [parameters timeout]} :body}]
(let [card (read-check Card card-id)
query (assoc (:dataset_query card)
:parameters parameters
:constraints dataset-api/query-constraints)]
;; Now run the query!
(let [options {:executed-by *current-user-id*
:card-id card-id}]
(qp/dataset-query query options))))
[card-id :as {{:keys [parameters]} :body}]
(run-query-for-card card-id parameters))

(defendpoint POST "/:card-id/query/csv"
"Run the query associated with a Card, and return its results as CSV."
[card-id :as {{:keys [parameters]} :body}]
(dataset-api/as-csv (run-query-for-card card-id parameters)))


(define-routes)
32 changes: 19 additions & 13 deletions src/metabase/api/dataset.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,26 +48,32 @@
(float (/ (reduce + running-times)
(count running-times))))}))

(defn as-csv
"Return a CSV response containing the RESULTS of a query."
{:arglists '([results])}
[{{:keys [columns rows]} :data, :keys [status], :as response}]
(if (= status :completed)
;; successful query, send CSV file
{:status 200
:body (with-out-str
;; turn keywords into strings, otherwise we get colons in our output
(csv/write-csv *out* (into [(mapv name columns)] rows)))
:headers {"Content-Type" "text/csv; charset=utf-8"
"Content-Disposition" (str "attachment; filename=\"query_result_" (u/date->iso-8601) ".csv\"")}}
;; failed query, send error message
{:status 500
:body (:error response)}))

(defendpoint POST "/csv"
"Execute an MQL query and download the result data as a CSV file."
"Execute a query and download the result data as a CSV file."
[query]
{query [Required String->Dict]}
(read-check Database (:database query))
(let [{{:keys [columns rows]} :data :keys [status] :as response} (qp/dataset-query query {:executed-by *current-user-id*})
columns (map name columns)] ; turn keywords into strings, otherwise we get colons in our output
(if (= status :completed)
;; successful query, send CSV file
{:status 200
:body (with-out-str
(csv/write-csv *out* (into [columns] rows)))
:headers {"Content-Type" "text/csv; charset=utf-8"
"Content-Disposition" (str "attachment; filename=\"query_result_" (u/date->iso-8601) ".csv\"")}}
;; failed query, send error message
{:status 500
:body (:error response)})))
(as-csv (qp/dataset-query query {:executed-by *current-user-id*})))


;; TODO - AFAIK this endpoint is no longer used. Remove it? </3
;; (We have POST /api/card/:id/query now which is used instead)
(defendpoint GET "/card/:id"
"Execute the MQL query for a given `Card` and retrieve both the `Card` and the execution results as JSON.
This is a convenience endpoint which simplifies the normal 2 api calls to fetch the `Card` then execute its query."
Expand Down
1 change: 1 addition & 0 deletions src/metabase/models/permissions.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@

;;; ---------------------------------------- Helper Fns ----------------------------------------

;; TODO - why does this take a PATH when everything else takes PATH-COMPONENTS or IDs?
(defn delete-related-permissions!
"Delete all permissions for GROUP-OR-ID for ancestors or descendant objects of object with PATH.
You can optionally include OTHER-CONDITIONS, which are anded into the filter clause, to further restrict what is deleted."
Expand Down
46 changes: 39 additions & 7 deletions test/metabase/api/card_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
Card [{card-2-id :id} {:database_id db-id}]]
(let [card-returned? (fn [database-id card-id]
(contains? (set (for [card ((user->client :rasta) :get 200 "card", :f :database, :model_id database-id)]
(:id card)))
(u/get-id card)))
card-id))]
[(card-returned? (id) card-1-id)
(card-returned? db-id card-1-id)
Expand All @@ -60,7 +60,7 @@
Card [{card-2-id :id} {:table_id table-2-id}]]
(let [card-returned? (fn [table-id card-id]
(contains? (set (for [card ((user->client :rasta) :get 200 "card", :f :table, :model_id table-id)]
(:id card)))
(u/get-id card)))
card-id))]
[(card-returned? table-1-id card-1-id)
(card-returned? table-2-id card-1-id)
Expand Down Expand Up @@ -188,7 +188,7 @@
:query_type "query"
:archived false
:labels []})
((user->client :rasta) :get 200 (str "card/" (:id card))))
((user->client :rasta) :get 200 (str "card/" (u/get-id card))))

;; Check that a user without permissions isn't allowed to fetch the card
(expect-with-temp [Database [{database-id :id}]
Expand All @@ -201,7 +201,7 @@
;; revoke permissions for default group to this database
(perms/delete-related-permissions! (perms-group/all-users) (perms/object-path database-id))
;; now a non-admin user shouldn't be able to fetch this card
((user->client :rasta) :get 403 (str "card/" (:id card)))))
((user->client :rasta) :get 403 (str "card/" (u/get-id card)))))

;; ## PUT /api/card/:id

Expand Down Expand Up @@ -252,13 +252,13 @@

;; Helper Functions
(defn- fave? [card]
((user->client :rasta) :get 200 (format "card/%d/favorite" (:id card))))
((user->client :rasta) :get 200 (format "card/%d/favorite" (u/get-id card))))

(defn- fave! [card]
((user->client :rasta) :post 200 (format "card/%d/favorite" (:id card))))
((user->client :rasta) :post 200 (format "card/%d/favorite" (u/get-id card))))

(defn- unfave! [card]
((user->client :rasta) :delete 204 (format "card/%d/favorite" (:id card))))
((user->client :rasta) :delete 204 (format "card/%d/favorite" (u/get-id card))))

;; ## GET /api/card/:id/favorite
;; Can we see if a Card is a favorite ?
Expand Down Expand Up @@ -308,3 +308,35 @@
[(get-labels) ; (1)
(update-labels [label-1-id label-2-id]) ; (2)
(update-labels [])])) ; (3)


;;; POST /api/:card-id/query/csv

(defn- do-with-temp-native-card {:style/indent 0} [f]
(with-temp* [Database [{database-id :id} {:details (:details (Database (id))), :engine :h2}]
Table [{table-id :id} {:db_id database-id, :name "CATEGORIES"}]
Card [card {:dataset_query {:database database-id
:type :native
:native {:query "SELECT COUNT(*) FROM CATEGORIES;"}
:query {:source-table table-id, :aggregation {:aggregation-type :count}}}}]]
;; delete all permissions for this DB
(perms/delete-related-permissions! (perms-group/all-users) (perms/object-path database-id))
(f database-id card)))

;; can someone with native query *read* permissions see a CSV card? (Issue #3648)
(expect
(str "COUNT(*)\n"
"75\n")
(do-with-temp-native-card
(fn [database-id card]
;; insert new permissions for native read access
(perms/grant-native-read-permissions! (perms-group/all-users) database-id)
;; now run the query
((user->client :rasta) :post 200 (format "card/%d/query/csv" (u/get-id card))))))

;; does someone without *read* permissions get DENIED?
(expect
"You don't have permissions to do that."
(do-with-temp-native-card
(fn [database-id card]
((user->client :rasta) :post 403 (format "card/%d/query/csv" (u/get-id card))))))

0 comments on commit 1d0db46

Please sign in to comment.