You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/06/22 05:08:43 UTC
[GitHub] [incubator-superset] john-bodley opened a new pull request #10130: fix(security): dbs/clusters perm
john-bodley opened a new pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130
### SUMMARY
Though both the [`Database`](https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/models/core.py#L99) and [`DruidCluster`](https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/connectors/druid/models.py#L122) have a `perm` 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 the `Database` model.
The reason 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 the `DruidCluster` model is probably an oversight as this logic is rarely used.
Rather than storing the `perm` I thought there was merit in deprecating the `dbs.perm` column (akin to the `DruidCluster` model) and replacing it with a SQLAlchemy [hybrid attribute](https://docs.sqlalchemy.org/en/13/orm/extensions/hybrid.html#defining-expression-behavior-distinct-from-attribute-behavior). 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 the `id` column to a string).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
CI and added additional unit tests.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [x] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#discussion_r443417055
##########
File path: superset/models/core.py
##########
@@ -639,9 +639,19 @@ def sqlalchemy_uri_decrypted(self) -> str:
def sql_url(self) -> str:
return f"/superset/sql/{self.id}/"
- def get_perm(self) -> str:
+ @hybrid_property
+ def perm(self) -> str:
return f"[{self.database_name}].(id:{self.id})"
+ @perm.expression # type: ignore
+ def perm(cls) -> str: # pylint: disable=no-self-argument
+ return (
+ "[" + cls.database_name + "].(id:" + expression.cast(cls.id, String) + ")"
+ )
+
+ def get_perm(self) -> str:
+ return self.perm # type: ignore
Review comment:
Is there some reason we want to keep this around and not just reference `perm` wherever needed?
##########
File path: superset/security/manager.py
##########
@@ -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
Review comment:
I'm surprised type checking has to be muted. Apparently mypy isn't comfortable with hybrid properties?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#discussion_r443637216
##########
File path: superset/security/manager.py
##########
@@ -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
Review comment:
I think it struggles to identify which method this refers to.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley edited a comment on pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
john-bodley edited a comment on pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#issuecomment-647734367
@bkyryliuk could you elaborate some more in relation to:
> 1 thing would be useful to think about -> automatic FAB permission rename on the DB renames
and
> 1 more thing would be nice to see in this PR -> keeping FAB with DB permission in sync, e.g. cleaning
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#discussion_r443752796
##########
File path: superset/models/core.py
##########
@@ -639,9 +639,19 @@ def sqlalchemy_uri_decrypted(self) -> str:
def sql_url(self) -> str:
return f"/superset/sql/{self.id}/"
- def get_perm(self) -> str:
+ @hybrid_property
+ def perm(self) -> str:
return f"[{self.database_name}].(id:{self.id})"
+ @perm.expression # type: ignore
+ def perm(cls) -> str: # pylint: disable=no-self-argument
+ return (
+ "[" + cls.database_name + "].(id:" + expression.cast(cls.id, String) + ")"
+ )
+
+ def get_perm(self) -> str:
+ return self.perm # type: ignore
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley merged pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#discussion_r443636670
##########
File path: superset/models/core.py
##########
@@ -639,9 +639,19 @@ def sqlalchemy_uri_decrypted(self) -> str:
def sql_url(self) -> str:
return f"/superset/sql/{self.id}/"
- def get_perm(self) -> str:
+ @hybrid_property
+ def perm(self) -> str:
return f"[{self.database_name}].(id:{self.id})"
+ @perm.expression # type: ignore
+ def perm(cls) -> str: # pylint: disable=no-self-argument
+ return (
+ "[" + cls.database_name + "].(id:" + expression.cast(cls.id, String) + ")"
+ )
+
+ def get_perm(self) -> str:
+ return self.perm # type: ignore
Review comment:
@villbro it’s required [here](https://github.com/apache/incubator-superset/blob/550e78ff7c02491717960d39ef0bb861aba0f977/superset/security/manager.py#L801). 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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#issuecomment-648976056
@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.
> @bkyryliuk could you elaborate some more in relation to:
>
> > 1 thing would be useful to think about -> automatic FAB permission rename on the DB renames
>
> and
>
> > 1 more thing would be nice to see in this PR -> keeping FAB with DB permission in sync, e.g. cleaning
>
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on pull request #10130: fix(security): dbs/clusters perm
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10130:
URL: https://github.com/apache/incubator-superset/pull/10130#issuecomment-647734367
@bkyryliuk could you elaborate some more in relation to:
> 1 thing would be useful to think about -> automatic FAB permission rename on the DB renames
and
> 1 more thing would be nice to see in this PR -> keeping FAB with DB permission in sync, e.g. cleaning
If you referring to the possible cascading of renames, I think if we moved completely to hybrid attributes this would be solved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org