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: Revert "fix(explore): let admin overwrite slice" #16408

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

rusackas
Copy link
Member

Reverts #16290

This PR gives Admin users a capability they did not have before, to meddle with another users' work. I think we need to discuss this a little more broadly and gain some degree of consensus before merging it, that we want to allow Admin users to have these added capabilities/privileges.

@rusackas
Copy link
Member Author

Note that reverting this will probably bring some of the bugs that it was intended to solve. There were some issues with (Admin) users seeing 403/404 errors when the clicking Save/Overwrite button on charts they did not own. This fix made it actually work, when perhaps the more prudent approach would be to figure out why the action was enabled in the first place, and find a more proper means to disable that Overwrite action. More research is warranted.

@rusackas rusackas requested a review from suddjian August 23, 2021 18:55
@rusackas rusackas changed the title Revert "fix(explore): let admin overwrite slice" fix: Revert "fix(explore): let admin overwrite slice" Aug 23, 2021
@rusackas rusackas requested a review from kgabryje August 23, 2021 18:56
@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #16408 (4a8e9e0) into master (c768941) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16408      +/-   ##
==========================================
+ Coverage   76.33%   76.42%   +0.09%     
==========================================
  Files        1000     1000              
  Lines       53483    53496      +13     
  Branches     6818     6816       -2     
==========================================
+ Hits        40827    40886      +59     
+ Misses      12418    12374      -44     
+ Partials      238      236       -2     
Flag Coverage Δ
mysql 81.52% <100.00%> (+0.03%) ⬆️
postgres 81.55% <100.00%> (+0.03%) ⬆️
python 81.63% <100.00%> (ø)
sqlite 81.15% <100.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...d/src/explore/components/PropertiesModal/index.tsx 81.94% <100.00%> (ø)
superset/views/core.py 75.30% <100.00%> (ø)
superset-frontend/src/components/Select/Select.tsx 73.42% <0.00%> (-3.16%) ⬇️
superset-frontend/src/components/Icons/Icon.tsx 100.00% <0.00%> (ø)
...erset-frontend/src/datasource/DatasourceEditor.jsx 74.81% <0.00%> (ø)
...rset-frontend/src/components/InfoTooltip/index.tsx 100.00% <0.00%> (ø)
...et-frontend/src/components/CertifiedIcon/index.tsx 100.00% <0.00%> (ø)
...nd/src/components/WarningIconWithTooltip/index.tsx 100.00% <0.00%> (ø)
...re/components/controls/DatasourceControl/index.jsx 79.06% <0.00%> (ø)
superset-frontend/src/SqlLab/actions/sqlLab.js 58.97% <0.00%> (+0.40%) ⬆️
... and 4 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 c768941...4a8e9e0. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I agree that this probably needs to be discussed more before introducing a change such as this (what first appeared to be a bug in hindsight makes sense to restrict admins from accidentally overwriting other people's charts). I'll continue to work on figuring out what may be the root cause of the 403/404 issue.

@rusackas rusackas merged commit 81241b6 into master Aug 24, 2021
@rusackas rusackas deleted the revert-16290-villebro/slice-overwrite branch August 24, 2021 15:38
henryyeh pushed a commit to preset-io/superset that referenced this pull request Aug 24, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Aug 26, 2021
* upstream/master: (25 commits)
  chore(ci): bump pylint to 2.10.2 (apache#16463)
  fix: prevent page crash when chart can't render (apache#16464)
  chore: fixed slack invite link (apache#16466)
  fix(native-filters): handle null values in value filter (apache#16460)
  feat: add function list to auto-complete to Clickhouse datasource (apache#16234)
  refactor(explore): improve typing for Dnd controls (apache#16362)
  fix(explore): update overwrite button on perm change (apache#16437)
  feat: Draggable and Resizable Modal (apache#16394)
  refactor: sql_json view endpoint (apache#16441)
  fix(dashboard): undo and redo buttons weird alignment  (apache#16417)
  fix: setupPlugin in chart list page (apache#16413)
  fix: Disable Slack notification method if no api token (apache#16367)
  feat: add Shillelagh DB engine spec (apache#16416)
  fix: copy to Clipboard order (apache#16299)
  docs: make FEATURE_FLAGS.md reference a link (apache#16415)
  chore(viz): bump superset-ui to 0.17.87 (apache#16420)
  feat: add activate command (apache#16404)
  Revert "fix(explore): let admin overwrite slice (apache#16290)" (apache#16408)
  fix(explore): retain chart ownership on query context update (apache#16419)
  chore: Removes the TODOs and uses the default page size (apache#16422)
  ...
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
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.

None yet

2 participants