-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[security] Moving set/merge perm to security manager #5684
Conversation
8cc320f
to
285a584
Compare
LGTM |
view_menu = self.find_view_menu(view_menu_name) | ||
pv = None | ||
|
||
if not permission: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
285a584
to
0480c1f
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
(cherry picked from commit 8992755)
[security] Moving set/merge perm to security manager (apache#5684)
This PR refactors (copy and paste) the helper
set_perm
andmerge_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 ofSupersetSecurityManager.set_perm
as there already exists aSupersetSecurityManager.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,hence the reason for simply copy-and-pasting the current logic. The one exception is the
permission
andview_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