dpgaspar commented on code in PR #23227:
URL: https://github.com/apache/superset/pull/23227#discussion_r1126140144
##########
tests/integration_tests/databases/api_tests.py:
##########
@@ -3619,3 +3619,45 @@ def test_validate_sql_endpoint_failure(self,
get_validator_by_name):
return
self.assertEqual(rv.status_code, 422)
self.assertIn("Kaboom!", response["errors"][0]["message"])
+
+ @mock.patch(
+
"superset.security.SupersetSecurityManager.get_schemas_accessible_by_user"
+ )
+
@mock.patch("superset.security.SupersetSecurityManager.can_access_database")
+
@mock.patch("superset.security.SupersetSecurityManager.can_access_all_datasources")
+ def test_schemas_access_for_csv_upload_not_found_endpoint(
+ self,
+ mock_can_access_all_datasources,
+ mock_can_access_database,
+ mock_schemas_accessible,
+ ):
+ self.login(username="admin")
Review Comment:
nit: we could have used `gamma` user, since it has no predefined access to
any database.
So we should have a test with `gamma` user testing data permissions.
And a simple test with `admin` (or not) to a non existing database.
##########
superset/databases/api.py:
##########
@@ -1455,3 +1458,61 @@ def delete_ssh_tunnel(self, pk: int) -> Response:
exc_info=True,
)
return self.response_400(message=str(ex))
+
+ @expose("/<int:pk>/schemas_access_for_file_upload/")
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".schemas_access_for_file_upload",
+ log_to_statsd=False,
+ )
+ def schemas_access_for_file_upload(self, pk: int) -> Response:
+ """The list of the database schemas where to upload information
+ ---
+ get:
+ summary:
+ The list of the database schemas where to upload information
+ parameters:
+ - in: path
+ name: pk
+ schema:
+ type: integer
+ responses:
+ 200:
+ description: The list of the database schemas where to upload
information
+ content:
+ application/json:
+ schema:
+ $ref:
"#/components/schemas/DatabaseSchemaAccessForFileUploadResponse"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ database = DatabaseDAO.find_by_id(pk)
+ if not database:
+ return self.response_404()
+
+ try:
+ schemas_allowed = database.get_schema_access_for_file_upload()
+ if security_manager.can_access_database(database):
Review Comment:
I found it a bit confusing at first, why do we need to check
`can_access_database` again, but the initial filter applied to
`DatabaseDAO.find_by_id` will show the databases that the user has access
directly and by child dependency, datasets and schemas.
On the other hand `get_schemas_accessible_by_user` will check
`security_manager.can_access_database(database)` and return `schemas_allowed`
has is, so the following check is not needed
##########
superset/databases/api.py:
##########
@@ -1455,3 +1458,61 @@ def delete_ssh_tunnel(self, pk: int) -> Response:
exc_info=True,
)
return self.response_400(message=str(ex))
+
+ @expose("/<int:pk>/schemas_access_for_file_upload/")
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".schemas_access_for_file_upload",
+ log_to_statsd=False,
+ )
+ def schemas_access_for_file_upload(self, pk: int) -> Response:
+ """The list of the database schemas where to upload information
+ ---
+ get:
+ summary:
+ The list of the database schemas where to upload information
+ parameters:
+ - in: path
+ name: pk
+ schema:
+ type: integer
+ responses:
+ 200:
+ description: The list of the database schemas where to upload
information
+ content:
+ application/json:
+ schema:
+ $ref:
"#/components/schemas/DatabaseSchemaAccessForFileUploadResponse"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ database = DatabaseDAO.find_by_id(pk)
+ if not database:
+ return self.response_404()
+
+ try:
+ schemas_allowed = database.get_schema_access_for_file_upload()
+ if security_manager.can_access_database(database):
+ return self.response(200, schemas=schemas_allowed)
+ # the list schemas_allowed should not be empty here
+ # and the list schemas_allowed_processed returned from
security_manager
+ # should not be empty either,
+ # otherwise the database should have been filtered out
+ # in CsvToDatabaseForm
Review Comment:
if we call it using:
```
security_manager.get_schemas_accessible_by_user(
database, schemas_allowed, True
)
```
we can remove the previous is
`security_manager.can_access_database(database)`
##########
superset/databases/api.py:
##########
@@ -1455,3 +1458,61 @@ def delete_ssh_tunnel(self, pk: int) -> Response:
exc_info=True,
)
return self.response_400(message=str(ex))
+
+ @expose("/<int:pk>/schemas_access_for_file_upload/")
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".schemas_access_for_file_upload",
+ log_to_statsd=False,
+ )
+ def schemas_access_for_file_upload(self, pk: int) -> Response:
+ """The list of the database schemas where to upload information
+ ---
+ get:
+ summary:
+ The list of the database schemas where to upload information
+ parameters:
+ - in: path
+ name: pk
+ schema:
+ type: integer
+ responses:
+ 200:
+ description: The list of the database schemas where to upload
information
+ content:
+ application/json:
+ schema:
+ $ref:
"#/components/schemas/DatabaseSchemaAccessForFileUploadResponse"
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ database = DatabaseDAO.find_by_id(pk)
+ if not database:
+ return self.response_404()
+
+ try:
+ schemas_allowed = database.get_schema_access_for_file_upload()
+ if security_manager.can_access_database(database):
+ return self.response(200, schemas=schemas_allowed)
+ # the list schemas_allowed should not be empty here
+ # and the list schemas_allowed_processed returned from
security_manager
+ # should not be empty either,
+ # otherwise the database should have been filtered out
+ # in CsvToDatabaseForm
+ schemas_allowed_processed =
security_manager.get_schemas_accessible_by_user(
+ database, schemas_allowed, False
+ )
+ return self.response(200, schemas=schemas_allowed_processed)
+ except Exception as ex: # pylint: disable=broad-except
Review Comment:
We can remove the broad exception catch, since Superset has a catch all
exceptions on the API, so the broad exception will return HTTP 500 using our
defaults
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
