fix(security): dbs/clusters perm #10130
Conversation
john-bodley
force-pushed
the
john-bodley--dbs-and-clusters-hybrid-perm
branch
from
8842a35
to
cd4da3e
Compare
john-bodley
force-pushed
the
john-bodley--dbs-and-clusters-hybrid-perm
branch
from
cd4da3e
to
5d6be73
Compare
john-bodley
force-pushed
the
john-bodley--dbs-and-clusters-hybrid-perm
branch
from
5d6be73
to
ff956af
Compare
| def get_perm(self) -> str: | ||
| return self.perm # type: ignore |
There was a problem hiding this comment.
Is there some reason we want to keep this around and not just reference perm wherever needed?
There was a problem hiding this comment.
@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.
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.
I'm surprised type checking has to be muted. Apparently mypy isn't comfortable with hybrid properties?
There was a problem hiding this comment.
I think it struggles to identify which method this refers to.
| def get_perm(self) -> str: | ||
| return self.perm # type: ignore |
There was a problem hiding this comment.
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 <john.bodley@airbnb.com> (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 <john.bodley@airbnb.com>
mistercrunch
added
🏷️ bot
SUMMARY
Though both the
DatabaseandDruidClusterhave apermproperty (which is merely a concatenation of either the database or cluster name and record ID), the property only maps to a physical column for theDatabasemodel.The
permcolumn 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 theDruidClustermodel is probably an oversight as this logic is rarely used. Note this PR addresses the inconsistency.Rather than storing the
permin the database I thought there was merit in deprecating thedbs.permcolumn 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 theidcolumn to a string). If people are ok with these hybrid attributes I wonder if there's merit in deprecating all of the physicalpermcolumns.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI and added additional unit tests.
ADDITIONAL INFORMATION