Skip to content

Comments

feat(ui): Disable user selection of data when user does not have export permissions#29654

Closed
MialLewis wants to merge 26 commits intoapache:masterfrom
MialLewis:disable_data_selection
Closed

feat(ui): Disable user selection of data when user does not have export permissions#29654
MialLewis wants to merge 26 commits intoapache:masterfrom
MialLewis:disable_data_selection

Conversation

@MialLewis
Copy link

@MialLewis MialLewis commented Jul 22, 2024

SUMMARY

Disables the user selection of data from the sqllab resultTable, plugin-chart-pivot-table and plugin-chart-table if the user does not have the can_export_csv / can_csv role. This is based off: #28429.

Relevant discussion: #29078

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@MialLewis
Copy link
Author

MialLewis commented Jul 22, 2024

@rusackas I'm pinging you as you merged a related PR #28429.

I'm working to disable user select for data, which is required for our sensitive data. The more blockers we can put in the place of data export the better, even if they can be circumvented.

We are willing to fork superset to do this, but getting it merged would obviously be better! Do you think there would be any appetite for this change, willing to put behind a feature flag if necessary.

Also, any suggestions/examples of how best to use the permissions model inside the frontend plugins?

Thanks!

@michael-s-molina michael-s-molina added review:draft review:checkpoint Last PR reviewed during the daily review standup and removed review:checkpoint Last PR reviewed during the daily review standup labels Jul 22, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 29, 2024
@MialLewis MialLewis force-pushed the disable_data_selection branch from c229b8d to 067a2e0 Compare July 31, 2024 02:29
@MialLewis MialLewis force-pushed the disable_data_selection branch 2 times, most recently from 2f36aa3 to b19c49b Compare August 2, 2024 01:31
@MialLewis MialLewis marked this pull request as ready for review August 2, 2024 01:38
@dosubot dosubot bot added authentication:access-control Rlated to access control change:frontend Requires changing the frontend labels Aug 2, 2024
@MialLewis
Copy link
Author

This is ready for someone to take a look. I'm very open to feedback as this is the first time I've used react. I do have very limited time to work on this though, it's already serving our purposes as it.

Thanks!

@MialLewis MialLewis changed the title fix(ui): Disable user selection of data when user does not have export permissions feat(ui): Disable user selection of data when user does not have export permissions Aug 2, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 2, 2024
Comment on lines 161 to 165
Copy link
Member

@justinpark justinpark Sep 5, 2024

Choose a reason for hiding this comment

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

For similar reasons above, adding a user state to ChartContainer and ChartRenderer and including this kind of logic would not be flexible in terms of component reusability. Therefore, it would be more appropriate to handle this at the dashboard or explore component level. (Additionally, in the case of the findPermission logic, if there are many charts in the dashboard, repeating the same n+1 search logic for each chart in the user permission list would lead to inefficiency.)

In other words, this permission logic should be included in DashboardBuilder and passed as a property value to DashboardComponent, allowing access through this.props.canExportData.
Similarly, if it's included in ExploreChartPanel and passed as a property to ChartContainer, it can be accessed in the same way.

Copy link
Author

@MialLewis MialLewis Sep 11, 2024

Choose a reason for hiding this comment

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

With regards to dashboards, there is already an equivalent property supersetCanCSV passed through to dashboard/components/gridCompnents/Chart.jsx, so we can pass that to ChartContainer: canExportData={supersetCanCSV}

@MialLewis MialLewis force-pushed the disable_data_selection branch from f49b835 to e52fe25 Compare September 9, 2024 05:09
@rusackas
Copy link
Member

rusackas commented Sep 9, 2024

Running CI. Ping us if it gets stuck!

@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 11, 2024
@MialLewis
Copy link
Author

Thanks for the review @justinpark, I've made the changes you suggested.

@rusackas would you mind running the CI again, it wasn't quite ready last time. I've run prettier locally so hopefully that will stop the failures.

@MialLewis
Copy link
Author

MialLewis commented Sep 24, 2024

@rusackas could you re-run the CI please? Turns out the "unneeded" test change I removed in an above commit was in fact needed.

I've built the js packages locally and see no test failures now. I haven't been able to run the cypress tests however as I see a message": "400 Bad Request: The CSRF token is missing when cypress attempts to login.

@rusackas
Copy link
Member

rusackas commented Oct 4, 2024

Running CI 🤞

@rusackas
Copy link
Member

rusackas commented May 1, 2025

Hi there... sorry this thing seems to have fallen under our radar. We're trying to thin the backlog of unmergeable things, so we can keep a better handle on the situation and have this happen less often. I'm not sure how hard this is to rebase, but if you want to give it a go, we can try to get it merged (finally). I'll convert it to a draft in the meantime, and nowadays when you mark it ready for review, it'll get our attention again.

@rusackas rusackas marked this pull request as draft May 1, 2025 04:18
@rusackas
Copy link
Member

This draft hasn't been updated in >90 days, so it will be closed. Please feel free to reopen it at any time, or ping us on this thread for further help.

@rusackas rusackas closed this Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants