Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSV appends: permissions checks #36950

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

calherries
Copy link
Contributor

@calherries calherries commented Dec 19, 2023

This PR will merge into the feature branch for Merge 1 of Milestone 0 (PR)

Epic: Allow appending more data to CSV uploads
product doc
eng doc

This PR implements the permissions checks required for appending CSV data to an existing upload table via the endpoint.

It corresponds to the following section of the eng doc (block link):

Permissions errors (status: 403):

  • Message: “You don’t have permissions to do that.”
  • Same permissions checks as uploading a CSV (POST /api/card/from-csv) and unrestricted data permissions on the table
  • Test: if user has block permissions: native query editing is required to upload CSV
  • Test: if user doesn’t have unrestricted data permissions then upload fails
  • Test: if the user does have unrestricted data permissions but no native query editing permissions then upload succeeds
  • Test: if user is sandboxed then upload fails

@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Dec 19, 2023
@calherries calherries requested a review from a team December 19, 2023 19:03
(t2/update! :model/Table (:id table) {:is_upload true})
(binding [upload/*sync-synchronously?* true]
(#'upload/scan-and-sync-table! (mt/db) table))
table))

(defn append-csv-with-defaults!
"Upload a small CSV file to the given collection ID. Default args can be overridden"
"Upload a small CSV file to a newly created default table, or an existing table if `table-id` is provided. Default args can be overridden"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring was just wrong before

@@ -606,7 +606,7 @@
(ex-info (tru "The table must be an uploaded table.")
{:status-code 422})

(not (mi/can-write? table))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this wrong before. You can read more about table permissions here:

;; To read (e.g., fetch metadata) a Table you must have either self-service data permissions for the Table, or write
;; permissions for the Table (detailed below). `can-read?` checks the former, while `can-write?` checks the latter;
;; the permission-checking function to call when reading a Table depends on the context of the request. When reading
;; Tables to power the admin data model page; `can-write?` should be called; in other contexts, `can-read?` should
;; be called. (TODO: is there a way to clear up the semantics here?)
;;
;; To write a Table (e.g. update its metadata):
;; * If Enterprise Edition code is available and the :advanced-permissions feature is enabled, you must have
;; data-model permissions for othe table
;; * Else, you must be an admin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the doc

unrestricted data permissions on the table

could you explain how can-write resonates with that?

Copy link
Contributor Author

@calherries calherries Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't. That's why I changed it from can-write? to can-read?.

And from the comments on table permissions I linked above:
1bc1857/src/metabase/models/table.clj#L70-L79

  ;; To read (e.g., fetch metadata) a Table you must have either self-service data permissions for the Table, or write
  ;; permissions for the Table (detailed below). `can-read?` checks the former, while `can-write?` checks the latter;

"self-service data permissions" is the same thing as "unrestricted data permissions" as I wrote in the eng doc.

Copy link
Contributor

@qnkhuat qnkhuat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice tests!

@calherries calherries merged commit cf06a4c into appends/milestone-0-endpoint Dec 21, 2023
102 checks passed
@calherries calherries deleted the appends/endpoint-permissions branch December 21, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants