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

[security] Moving set/merge perm to security manager #5684

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Aug 21, 2018

This PR refactors (copy and paste) the helper set_perm and merge_perm methods to be encompassed within the security manager which ensures that the manager controls all reads/writes related to data permissions.

Note the helpers.merge_perm logic is part of SupersetSecurityManager.set_perm as there already exists a SupersetSecurityManager.merge_perm method. Though the logic is similar the database connection logic differs. When I tried to use the later, though I was able to read, writes failed with the following exception,

sqlalchemy.exc.ResourceClosedError: This transaction is closed

hence the reason for simply copy-and-pasting the current logic. The one exception is the permission and view_menu should only be refreshed only if updated.

Finally I'm also perplexed as to why the pylint warnings are present (which I've ignored).

to: @jeffreythewang @mistercrunch @timifasubaa

@john-bodley john-bodley force-pushed the john-bodley-sm-set-merge-perm branch 6 times, most recently from 8cc320f to 285a584 Compare August 21, 2018 18:36
@mistercrunch
Copy link
Member

LGTM

view_menu = self.find_view_menu(view_menu_name)
pv = None

if not permission:
Copy link
Member

Choose a reason for hiding this comment

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

Hey I think we can get rid of most of this by calling self.add_permission_view_menu which takes care of all of this I think

Copy link
Member Author

Choose a reason for hiding this comment

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

@mistercrunch I tried that approach earlier (see description) which lead to the following exception being thrown: sqlalchemy.exc.ResourceClosedError: This transaction is closed

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the context is funky here where we're inside some sort of SQLAlchemy trigger. Feel free to merge as is.

@codecov-io
Copy link

Codecov Report

Merging #5684 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5684      +/-   ##
==========================================
- Coverage    63.5%   63.49%   -0.01%     
==========================================
  Files         360      360              
  Lines       22922    22920       -2     
  Branches     2555     2555              
==========================================
- Hits        14556    14554       -2     
  Misses       8351     8351              
  Partials       15       15
Impacted Files Coverage Δ
superset/models/helpers.py 85.97% <ø> (-1.73%) ⬇️
superset/connectors/druid/models.py 82.56% <100%> (ø) ⬆️
superset/models/core.py 85.09% <100%> (ø) ⬆️
superset/security.py 75.55% <100%> (+2.64%) ⬆️
superset/connectors/sqla/models.py 78.62% <100%> (-0.05%) ⬇️

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 71e0c07...0480c1f. Read the comment docs.

@john-bodley john-bodley merged commit 8992755 into apache:master Aug 22, 2018
@john-bodley john-bodley deleted the john-bodley-sm-set-merge-perm branch August 22, 2018 18:15
john-bodley added a commit to john-bodley/superset that referenced this pull request Aug 22, 2018
john-bodley added a commit to airbnb/superset-fork that referenced this pull request Aug 22, 2018
[security] Moving set/merge perm to security manager (apache#5684)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants