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

fix: permission checks on import #23200

Merged
merged 8 commits into from
Mar 15, 2023
Merged

fix: permission checks on import #23200

merged 8 commits into from
Mar 15, 2023

Conversation

betodealmeida
Copy link
Member

SUMMARY

When importing assets (databases, datasets, charts, dashboards), ensure that the user has the proper permissions to create the assets. Otherwise a user who can only create charts is able to import a chart and have a dataset and databases added.

@eschutho, @dpgaspar said it would be nice if we could include this in 2.1.0, not sure if there's still time.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Updated and added unit tests.

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

@codecov
Copy link

codecov bot commented Feb 25, 2023

Codecov Report

Merging #23200 (344a070) into master (a70b7ac) will increase coverage by 0.06%.
The diff coverage is 88.13%.

❗ Current head 344a070 differs from pull request most recent head 3a8d104. Consider uploading reports for the commit 3a8d104 to get more accurate results

@@            Coverage Diff             @@
##           master   #23200      +/-   ##
==========================================
+ Coverage   67.50%   67.56%   +0.06%     
==========================================
  Files        1899     1898       -1     
  Lines       73322    73149     -173     
  Branches     7930     7882      -48     
==========================================
- Hits        49494    49425      -69     
+ Misses      21796    21705      -91     
+ Partials     2032     2019      -13     
Flag Coverage Δ
hive 52.77% <62.28%> (+0.01%) ⬆️
mysql 78.44% <83.47%> (+0.01%) ⬆️
postgres 78.50% <83.89%> (+0.01%) ⬆️
presto 52.69% <62.28%> (+0.03%) ⬆️
python 82.28% <88.13%> (+0.02%) ⬆️
sqlite 76.98% <82.62%> (+0.01%) ⬆️
unit 52.49% <54.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-ui-chart-controls/src/components/ColumnOption.tsx 84.61% <ø> (-1.10%) ⬇️
...-ui-chart-controls/src/components/MetricOption.tsx 90.47% <ø> (-0.44%) ⬇️
...t-ui-chart-controls/src/operators/pivotOperator.ts 100.00% <ø> (ø)
...et-ui-chart-controls/src/operators/sortOperator.ts 100.00% <ø> (ø)
...rt-controls/src/shared-controls/customControls.tsx 50.00% <ø> (+25.00%) ⬆️
...s/superset-ui-core/src/components/SafeMarkdown.tsx 66.66% <ø> (+5.12%) ⬆️
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts 66.66% <ø> (-4.77%) ⬇️
...gin-chart-echarts/src/Timeseries/transformProps.ts 52.87% <ø> (+0.60%) ⬆️
...d/plugins/plugin-chart-echarts/src/utils/series.ts 84.29% <ø> (ø)
...s/plugin-chart-pivot-table/src/PivotTableChart.tsx 0.00% <ø> (ø)
... and 142 more

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@betodealmeida betodealmeida force-pushed the import_perms branch 3 times, most recently from dd5f2ab to 0bfe810 Compare February 28, 2023 00:03
@eschutho
Copy link
Member

eschutho commented Mar 9, 2023

@betodealmeida I'm packaging up 2.1 today, but if there's another RC we can try to get it in, otherwise we'll aim for 2.1.1

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks great!

@betodealmeida betodealmeida merged commit ec6318b into master Mar 15, 2023
@dpgaspar dpgaspar added the 2.1.1 label May 8, 2023
@eschutho eschutho added the v2.1 label May 12, 2023
eschutho pushed a commit that referenced this pull request May 23, 2023
eschutho pushed a commit that referenced this pull request May 25, 2023
eschutho pushed a commit that referenced this pull request May 31, 2023
eschutho pushed a commit that referenced this pull request Jun 13, 2023
@mistercrunch mistercrunch added 🍒 2.1.1 🍒 2.1.2 🍒 2.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the import_perms branch March 26, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.1.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L v2.1 🍒 2.1.1 🍒 2.1.2 🍒 2.1.3 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants