-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(security): dbs/clusters perm #10130
fix(security): dbs/clusters perm #10130
Conversation
8842a35
to
cd4da3e
Compare
cd4da3e
to
5d6be73
Compare
5d6be73
to
ff956af
Compare
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.
Very nice! First pass comments.
def get_perm(self) -> str: | ||
return self.perm # type: ignore |
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.
Is there some reason we want to keep this around and not just reference perm
wherever needed?
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.
@villbro it’s required here. Note the set_perm
callback is still required as it has other logic besides merely setting the perm
column. For the dbs
and clusters
table perm
and get_perm
are equivalent and hence no database record will be updated (this has always been the case for the clusters
table).
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.
it is also used here: https://github.com/apache/incubator-superset/blob/550e78ff7c02491717960d39ef0bb861aba0f977/superset/security/manager.py#L820
https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/models/core.py#L659
logic that keeps FAB security in sync with the other models
@@ -244,7 +244,7 @@ def can_access_database(self, database: Union["Database", "DruidCluster"]) -> bo | |||
return ( | |||
self.can_access_all_datasources() | |||
or self.can_access_all_databases() | |||
or self.can_access("database_access", database.perm) | |||
or self.can_access("database_access", database.perm) # type: ignore |
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.
I'm surprised type checking has to be muted. Apparently mypy isn't comfortable with hybrid properties?
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.
I think it struggles to identify which method this refers to.
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.
This is quite good step towards the cleanup, thanks!
1 thing would be useful to think about -> automatic FAB permission rename on the DB renames
1 more thing would be nice to see in this PR -> keeping FAB with DB permission in sync, e.g. cleaning
def get_perm(self) -> str: | ||
return self.perm # type: ignore |
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.
it is also used here: https://github.com/apache/incubator-superset/blob/550e78ff7c02491717960d39ef0bb861aba0f977/superset/security/manager.py#L820
https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/models/core.py#L659
logic that keeps FAB security in sync with the other models
@bkyryliuk could you elaborate some more in relation to:
and
If you referring to the possible cascading of renames (I'm not sure if the database perm relates to the datasource per), I think if we moved completely to hybrid attributes this would be solved. |
Co-authored-by: John Bodley <[email protected]> (cherry picked from commit 37777f3)
@john-bodley actually scratch that, looks like it is already happening in the sync. I've seen some abandoned permissions, but there is probably a difference reason for them.
|
Co-authored-by: John Bodley <[email protected]>
SUMMARY
Though both the
Database
andDruidCluster
have aperm
property (which is merely a concatenation of either the database or cluster name and record ID), the property only maps to a physical column for theDatabase
model.The
perm
column exists is so various permission based queries can be executed to determine if a user can access said data entities. The reason it is not a physical column for theDruidCluster
model is probably an oversight as this logic is rarely used. Note this PR addresses the inconsistency.Rather than storing the
perm
in the database I thought there was merit in deprecating thedbs.perm
column and replacing it with a SQLAlchemy hybrid attribute. These hybrid attributes act like virtual columns and function in both Python and SQL (a SQL expression has been provided to deal with the casting of theid
column to a string). If people are ok with these hybrid attributes I wonder if there's merit in deprecating all of the physicalperm
columns.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI and added additional unit tests.
ADDITIONAL INFORMATION