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: Alpha should not be able to edit datasets that they don't own #19854

Merged
merged 16 commits into from
Apr 29, 2022

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Apr 26, 2022

SUMMARY

There is no longer a config value for OLD_API_CHECK_DATASET_OWNERSHIP due to 2.0 release. This is now allowing all users who have write permissions to edit any dataset. Adding this logic will verify if this user is an admin or owner before completing this request.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-04-26.at.1.01.03.PM.mov

Screen Shot 2022-04-26 at 8 50 12 PM

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

@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #19854 (04925be) into master (e061955) will decrease coverage by 0.15%.
The diff coverage is 71.14%.

❗ Current head 04925be differs from pull request most recent head d33cc8b. Consider uploading reports for the commit d33cc8b to get more accurate results

@@            Coverage Diff             @@
##           master   #19854      +/-   ##
==========================================
- Coverage   66.51%   66.36%   -0.16%     
==========================================
  Files        1690     1714      +24     
  Lines       64614    65031     +417     
  Branches     6655     6717      +62     
==========================================
+ Hits        42978    43157     +179     
- Misses      19936    20167     +231     
- Partials     1700     1707       +7     
Flag Coverage Δ
hive ?
mysql 81.93% <85.52%> (-0.02%) ⬇️
postgres 81.98% <85.52%> (-0.02%) ⬇️
presto ?
python 82.06% <85.52%> (-0.37%) ⬇️
sqlite 81.74% <85.52%> (-0.02%) ⬇️
unit ?

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

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <0.00%> (-19.05%) ⬇️
...rset-ui-chart-controls/src/sections/chartTitle.tsx 100.00% <ø> (ø)
...omponents/ColumnConfigControl/ColumnConfigItem.tsx 0.00% <ø> (ø)
.../shared-controls/components/RadioButtonControl.tsx 0.00% <ø> (ø)
...et-ui-chart-controls/src/shared-controls/index.tsx 36.19% <ø> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 33.33% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 100.00% <ø> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
... and 150 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e061955...d33cc8b. Read the comment docs.

@hughhhh hughhhh changed the title fix: api for checking owners fix: Alpha should not be able to edit datasets that they don't own Apr 26, 2022
@ghost
Copy link

ghost commented Apr 26, 2022

@hughhhh Looking good! Only thing I'm seeing is the disabled button shouldn't have a hover state (it should remain grey even when the user hovers)

@hughhhh hughhhh force-pushed the fix-write-ds-alphs branch 2 times, most recently from 48a4cd1 to c1f4ff1 Compare April 26, 2022 22:14
@hughhhh
Copy link
Member Author

hughhhh commented Apr 26, 2022

@jess-dillard

Screen Shot 2022-04-26 at 3 14 17 PM

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 27, 2022
@hughhhh hughhhh force-pushed the fix-write-ds-alphs branch 2 times, most recently from 252d70c to 6663bc3 Compare April 27, 2022 03:54
@yousoph
Copy link
Member

yousoph commented Apr 27, 2022

/testenv up

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment spinning up at http://34.222.236.159:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@@ -344,6 +365,11 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
},
{
Cell: ({ row: { original } }: any) => {
// Verify owner or isAdmin
const allowEdit =
original.owners.map((o: any) => o.id).includes(user.userId) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

add Owners to for loop

@yousoph
Copy link
Member

yousoph commented Apr 29, 2022

/testenv up

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment spinning up at http://35.165.228.74:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@yousoph
Copy link
Member

yousoph commented Apr 29, 2022

Ephemeral looks good to me (and design + QA took a look as well) :)

@jinghua-qa
Copy link
Member

tested in the ephermal env, primary contributor can not edit/delete the dataset they did not own. LGTM!

@hughhhh hughhhh merged commit 8b15b68 into apache:master Apr 29, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request May 2, 2022
…pache#19854)

* fix api for checking owners

* fix styles for disabling

* fix styles for disabling

* fix lint

* fix lint

* add owners key

* plzz

* remove

* update test

* add tooltip

* add type

* fix test

* fix user reference

* lit

* fix test

* work

(cherry picked from commit 8b15b68)
@sadpandajoe
Copy link
Member

🏷️ preset:2022.17

hughhhh added a commit to hve-labs/superset that referenced this pull request May 11, 2022
…pache#19854)

* fix api for checking owners

* fix styles for disabling

* fix styles for disabling

* fix lint

* fix lint

* add owners key

* plzz

* remove

* update test

* add tooltip

* add type

* fix test

* fix user reference

* lit

* fix test

* work
villebro pushed a commit that referenced this pull request May 26, 2022
…19854)

* fix api for checking owners

* fix styles for disabling

* fix styles for disabling

* fix lint

* fix lint

* add owners key

* plzz

* remove

* update test

* add tooltip

* add type

* fix test

* fix user reference

* lit

* fix test

* work

(cherry picked from commit 8b15b68)
michael-s-molina pushed a commit that referenced this pull request May 26, 2022
…19854)

* fix api for checking owners

* fix styles for disabling

* fix styles for disabling

* fix lint

* fix lint

* add owners key

* plzz

* remove

* update test

* add tooltip

* add type

* fix test

* fix user reference

* lit

* fix test

* work

(cherry picked from commit 8b15b68)
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
…pache#19854)

* fix api for checking owners

* fix styles for disabling

* fix styles for disabling

* fix lint

* fix lint

* add owners key

* plzz

* remove

* update test

* add tooltip

* add type

* fix test

* fix user reference

* lit

* fix test

* work
@mistercrunch mistercrunch added 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset:2022.17 size/L 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants