[GitHub] [incubator-superset] john-bodley commented on a change in pull request #10034: chore(security): Updating assert logic

[GitBox <gi...@apache.org>]

john-bodley commented on a change in pull request #10034:
URL: https://github.com/apache/incubator-superset/pull/10034#discussion_r438417720



##########
File path: superset/security/manager.py
##########
@@ -858,38 +852,65 @@ def set_perm(
                     )
                 )
 
-    def assert_datasource_permission(self, datasource: "BaseDatasource") -> None:
+    def raise_for_access(
+        self,
+        database: Optional["Database"] = None,
+        datasource: Optional["BaseDatasource"] = None,
+        query_context: Optional["QueryContext"] = None,
+        table: Optional["Table"] = None,
+        viz: Optional["BaseViz"] = None,
+    ) -> None:
         """
-        Assert the the user has permission to access the Superset datasource.
+        Raise an exception if the user cannot access the resource.
 
+        :param database: The Superset database (see table)
         :param datasource: The Superset datasource
-        :raises SupersetSecurityException: If the user does not have permission
+        :param query_context: The query context
+        :param table: The Superset table (see database)
+        :param viz: The visualization
+        :raises SupersetSecurityException: If the user cannot access the resource
         """
 
-        if not self.datasource_access(datasource):
-            raise SupersetSecurityException(
-                self.get_datasource_access_error_object(datasource),
+        from superset import db

Review comment:
       I wonder if there should be any assertion logic to ensure that the right combination of parameters are defined. Note except for `database` and `table` these are all mutually exclusive. I did consider having an argument `database_and_table` which would be an `Optional[Tuple["Database, "Table']]` but  I wasn't sold on the idea and thus opted for the additional docstring context.

##########
File path: superset/views/core.py
##########
@@ -2656,8 +2668,7 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
         if not datasource:
             return json_error_response(DATASOURCE_MISSING_ERR)
 
-        # Check permission for datasource

Review comment:
       Annexed self explanatory comment.




----------------------------------------------------------------
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