chore: Migrate /superset/schemas_access_for_file_upload to v1 #23227
Conversation
…/migrate-schemas-access-for-file-upload-to-v1
Codecov Report
@@ Coverage Diff @@
## master #23227 +/- ##
==========================================
+ Coverage 67.47% 67.51% +0.03%
==========================================
Files 1906 1907 +1
Lines 73403 73472 +69
Branches 7958 7976 +18
==========================================
+ Hits 49527 49601 +74
+ Misses 21835 21822 -13
- Partials 2041 2049 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| mock_can_access_database, | ||
| mock_schemas_accessible, | ||
| ): | ||
| self.login(username="admin") |
There was a problem hiding this 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
Outdated
|
|
||
| try: | ||
| schemas_allowed = database.get_schema_access_for_file_upload() | ||
| if security_manager.can_access_database(database): |
There was a problem hiding this 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
Outdated
| # 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 |
There was a problem hiding this 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
Outdated
| database, schemas_allowed, False | ||
| ) | ||
| return self.response(200, schemas=schemas_allowed_processed) | ||
| except Exception as ex: # pylint: disable=broad-except |
There was a problem hiding this 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
| ): | ||
| self.login(username="gamma") | ||
| self.create_fake_db() | ||
| mock_can_access_all_datasources.return_value = False |
There was a problem hiding this comment.
using user gamma we should not need to mock can_access_database and can_access_all_datasources
| mock_schemas_accessible.return_value = ["this_schema_is_allowed_too"] | ||
| rv = self.client.get(f"/api/v1/database/120ff/schemas_access_for_file_upload") | ||
| self.assertEqual(rv.status_code, 404) | ||
| self.delete_fake_db() |
There was a problem hiding this comment.
can you add another simple test to assert a database id that does not exist returns a 404 from this endpoint?
mistercrunch
added
🏷️ bot
mistercrunch
deleted the
hugh/migrate-schemas-access-for-file-upload-to-v1
branch
SUMMARY
replacing #23033
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION