fix(ui): Disable ability to export data when user does not have the correct permission

[edjannoo]

SUMMARY

Disables Export to .JSON and Export to Excel from the Download submenu when viewing a chart unless the user has can_csv on Superset permission. The same permission is already required for Download to .CSV.

Disables DOWNLOAD TO CSV and COPY TO CLIPBOARD from the query results pane in SQL Lab unless the user has can_export_csv on SQLLab permission. The permission should be applied to both download to CSV and copy to clipboard functions since both are means of exporting data.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Verify that a user with the can_csv on Superset permission (for example a user with the Alpha role) is able to export as CSV, JSON and Excel when viewing a chart. Verify that a user without that permission is not able to do any of those.

Verify that a user with the can_export_csv on SQLLab pemission (for example a user with the sql_lab role) is able to download to CSV or copy to clipboard from the SQL Lab query results pane. Verify that a user without that permission is not able to do either of those.

ADDITIONAL INFORMATION

• Has associated issue: Fixes Not possible to disable export to json on charts #19535
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[rusackas]

Approving CI 🤞

[edjannoo]

@rusackas Any chance I could get this PR reviewed please? I've had to update it a couple of times after merge conflicts have arisen due to other change. I'm worried that it will be hard to keep it up to date if any more wide ranging changes get merged.

[rusackas]

Thanks for your patience. I was hoping to get a second opinion on this since it messes with permissions and that always makes me nervous, personally ;)

Running CI again, hopefully it passes. I think this looks OK to me in general, so I'll go ahead and approve it. I would still like a second opinion from @dpgaspar or @villebro if possible.

My only other (minor, probably non-blocking) thought is that we're now using can_export_csv and can_csv to control a lot more than csv. From a permissions standpoint, that seems reasonable. From a naming standpoint, I can't help but wonder if there's something more... inclusive. Maybe can_export_data or something similar. But... making that kind of change might be considered a breaking change and would have to wait for 5.0.

[villebro]

I agree with the changes here, and I'm super impressed with the general quality of the PR, especially the awesome tests ❤️ LGTM, but let's do a quick round of eph env testing before merging. I'm not at my laptop right now, but I should be able to do testing within the next 24 hours.

[villebro]

nit: I feel three steps down would look better by just starting from src/...

[edjannoo]

Thanks - updated.

[edjannoo]

Thanks for your patience. I was hoping to get a second opinion on this since it messes with permissions and that always makes me nervous, personally ;)

Running CI again, hopefully it passes. I think this looks OK to me in general, so I'll go ahead and approve it. I would still like a second opinion from @dpgaspar or @villebro if possible.

My only other (minor, probably non-blocking) thought is that we're now using can_export_csv and can_csv to control a lot more than csv. From a permissions standpoint, that seems reasonable. From a naming standpoint, I can't help but wonder if there's something more... inclusive. Maybe can_export_data or something similar. But... making that kind of change might be considered a breaking change and would have to wait for 5.0.

Thanks for reviewing. I also felt that the permission name feels wrong however the same permission is already used for controlling export to Excel so it's a logical extension of that. Perhaps this will be improved by the new permission model I saw a SIP for?

[jkogut]

@rusackas

Hello,

running Superset 4.0.2 version and still observing COPY TO CLIPBOARD button.

Steps to reproduce:

1. Revoke can_export_csv on SQLLab permission
2. Run a query in SQLLab
3. Button DOWNLOAD TO CSV exists and when clicked results with {"message":"Forbidden"}
4. Button COPY TO CLIPBOARD exists and when clicked results with copying to clipboard.

So looks like it has half functionality described, and permission is not applied to both download to CSV and copy to clipboard functions.

p.s. also cannot find this: #28429 PR in the CHANGELOG files even though the code was merged and present in 4.0.2 version. Is that intentional ?

TIA

[jkogut]

@rusackas

Hello again,

just tested same in Superset 4.1.0rc2 and this issue is no longer present.

Steps to reproduce (tested on local superset instance deployed with docker-compose):

1. Create new user and add it to sql_lab group
2. Revoke can_export_csv on SQLLAB permission for sql_lab group
3. Run a query - both buttons DOWNLOAD TO CSV and COPY TO CLIPBOARD are not present.

So 🤞 for the successful 4.1.0 version release :)

TIA