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.

Sign up for GitHub

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

Jump to bottom

Users with collections "edit" permissions and no data access permissions can't edit question metadata #11719

Closed
tlrobinson opened this issue Jan 15, 2020 · 2 comments
Closed

Users with collections "edit" permissions and no data access permissions can't edit question metadata #11719

tlrobinson opened this issue Jan 15, 2020 · 2 comments
Assignees
@camsaul
Labels
Administration/Permissions Collection or Data permissions .Backend Priority:P2 Average run of the mill bug .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Projects
Cypress Testing
Milestone
0.38.3

Comments

@tlrobinson
Copy link
Contributor

Describe the bug
A user with no "data access" permissions but "edit" collection permissions should be able to view and edit the question metadata (name, description, collection_id, and archived) but not the question definition (unless they are allowed to run the resulting ad-hoc question?). Currently they can view the question but not edit anything about the question.

Logs

01-14 23:56:07 WARN middleware.log :: PUT /api/card/40 403 41.5 ms (33 DB calls)
"You don't have permissions to do that."

To Reproduce
Steps to reproduce the behavior:

  1. Create a non-admin user and revoke their data access permissions from "All Users"
  2. Have an admin save a question to a collection the non-admin user does have "edit" collection permissions for
  3. Have the non-admin user view the question, which should succeed. Try having them edit the question name or description (via "Edit this question" entity menu), collection_id (via "Move" entity menu item), or archived (via "Archive" entity menu item), but you will get a permissions error.

Expected behavior
The user should be able to edit the question metadata, but not the question definition (because they have no data permissions)

Screenshots
If applicable, add screenshots to help explain your problem.

Information about your Metabase Installation:

You're on version v0.35.0-snapshot
Built on 2020-01-13
Hash: 2807015
Branch: master

Severity

P2?
@tlrobinson tlrobinson added Type:Bug Product defects .Backend Administration/Permissions Collection or Data permissions labels Jan 15, 2020
@flamber flamber added the Priority:P2 Average run of the mill bug label Mar 18, 2021
@nemanjaglumac nemanjaglumac added this to Backlog in Cypress Testing Mar 19, 2021
nemanjaglumac added a commit that referenced this issue Mar 19, 2021
@nemanjaglumac
Add repro(s) for #11719
6e62df2
@nemanjaglumac nemanjaglumac mentioned this issue Mar 19, 2021
Merged
@flamber flamber mentioned this issue Mar 19, 2021
Closed
nemanjaglumac added a commit that referenced this issue Mar 19, 2021
@nemanjaglumac
#11719 Repro: Users with curate access can't edit question's metadata (…
b7ae197 
…#15262)
@nemanjaglumac nemanjaglumac added the .Reproduced Issues reproduced in test (usually Cypress) label Mar 19, 2021
@camsaul camsaul mentioned this issue Mar 31, 2021
Closed
@camsaul
Copy link
Member

camsaul commented Mar 31, 2021

You actually can edit the metadata right now as long as you don't pass in dataset_query. I'm working on a fix tho so it lets you pass in dataset_query as long as it's unchanged from its current value

@camsaul camsaul self-assigned this Mar 31, 2021
@camsaul camsaul added this to the 0.38.3 milestone Mar 31, 2021
camsaul added a commit that referenced this issue Mar 31, 2021
@camsaul
Fix #11719
2a64002
camsaul added a commit that referenced this issue Mar 31, 2021
@camsaul
Add tests for #11719 and #14931
b7d8db7
@camsaul camsaul mentioned this issue Mar 31, 2021
Merged
camsaul added a commit that referenced this issue Apr 2, 2021
@camsaul
Improve card permissions errors (#15424)
8332830 
* Explicitly specify %throwable in Log4j logging pattern so the ex-data gets printed

This tweaks means Exceptions get rendered for logging with .toString instead .getMessage.

See https://github.com/clojure/tools.logging/tree/31073fe1fbb0533f3c775fa667de70809f24b127#log4j2
and https://logging.apache.org/log4j/2.x/manual/layouts.html#Patterns

* Remove ad-hoc query perms check on save logic from metabase.models.card.

We already do this in metabase.api.card, so in practice this is redundant code.

* Better permissions exceptions when missing ad-hoc perms for creating/updating a Card

* Fix #11719

* Return more info with most API exception responses

* Add tests for #11719 and #14931

* Fix namespace lint errors

* Fix error message grammar (thanks @jeff303)
@camsaul
Copy link
Member

camsaul commented Apr 2, 2021

Fixed by #15424

@camsaul camsaul closed this as completed Apr 2, 2021
tsmacdonald pushed a commit that referenced this issue Apr 2, 2021
@camsaul @tsmacdonald
Improve card permissions errors (#15424)
1a46cc6 
* Explicitly specify %throwable in Log4j logging pattern so the ex-data gets printed

This tweaks means Exceptions get rendered for logging with .toString instead .getMessage.

See https://github.com/clojure/tools.logging/tree/31073fe1fbb0533f3c775fa667de70809f24b127#log4j2
and https://logging.apache.org/log4j/2.x/manual/layouts.html#Patterns

* Remove ad-hoc query perms check on save logic from metabase.models.card.

We already do this in metabase.api.card, so in practice this is redundant code.

* Better permissions exceptions when missing ad-hoc perms for creating/updating a Card

* Fix #11719

* Return more info with most API exception responses

* Add tests for #11719 and #14931

* Fix namespace lint errors

* Fix error message grammar (thanks @jeff303)
@deploysentinel deploysentinel bot mentioned this issue Nov 28, 2022
Merged
@deploysentinel deploysentinel bot mentioned this issue Dec 6, 2022
Merged
@deploysentinel deploysentinel bot mentioned this issue Dec 16, 2022
Merged
@deploysentinel deploysentinel bot mentioned this issue Jan 11, 2023
Merged
This was referenced Jan 19, 2023
Merged
Merged
Closed
This was referenced Jan 27, 2023
Merged
Merged
This was referenced Feb 5, 2024
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Open
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@camsaul camsaul

Labels
Administration/Permissions Collection or Data permissions .Backend Priority:P2 Average run of the mill bug .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Projects
Cypress Testing
Backlog
Milestone
0.38.3
Development

No branches or pull requests
4 participants
@tlrobinson @flamber @camsaul @nemanjaglumac