-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: database permissions on update and delete (avoid orphaned perms) #20081
fix: database permissions on update and delete (avoid orphaned perms) #20081
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20081 +/- ##
===========================================
- Coverage 66.25% 54.80% -11.46%
===========================================
Files 1758 1758
Lines 67048 67058 +10
Branches 7118 7092 -26
===========================================
- Hits 44423 36748 -7675
- Misses 20808 28497 +7689
+ Partials 1817 1813 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
…base-connection-is-delete
…base-connection-is-delete
for schema in schemas: | ||
old_view_menu_name = security_manager.get_schema_perm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't these logic sit inside database_after_update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very valid point, I would prefer to place it on the event listener also, but my decision is that database.get_all_schema_names()
will query the analytics DB for all available schemas, so it needs the database to be online, it seems unexpected to place this operation on a "background" task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also if we move this to after update SQLAlchemy event we loose the capability of correctly identifying any possible failure has a db connection failure and reply with a proper response back to the requester
…base-connection-is-delete
…base-connection-is-delete
sqlatable_table.update() | ||
.where( | ||
sqlatable_table.c.id == dataset.id, | ||
sqlatable_table.c.perm == old_dataset_vm_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this line is unnecessary as you already filter by id
…#20081) * fix: database permissions on update and delete (avoid orphaned perms) * fix event transaction * fix test * fix lint * update datasource access permissions * add tests * fix import * fix tests * update slice and dataset perms also * fix lint * fix tests * fix lint * fix lint * add test for edge case, small refactor * add test for edge case, small refactor * improve code * fix lint
SUMMARY
Currently updating a
database_name
or deleting a database does not update or delete any associated permissions.Examples:
Creating a database named
db1
with 2 datasources named: (ds1
,ds2
), creates the following permissions:database_access
->[db1](id:1)
datasource_access
->[db1].[ds1](id:1)
datasource_access
->[db1].[ds2](id:2)
Then updating the database name to
db1_updated
, leaves the permissions at the following state:database_access
->[db1](id:1)
datasource_access
->[db1].[ds1](id:1)
datasource_access
->[db1].[ds2](id:2)
database_access
->[db2](id:1)
datasource_access
->[db2].[ds1](id:1)
datasource_access
->[db2].[ds2](id:2)
This means that changing a database_name will remove access to all non admin users to that database on SQLLab. Besides leaving orphaned permissions on the DB.
This PR updates the permissions has expected, final result:
database_access
->[db2](id:1)
datasource_access
->[db2].[ds1](id:1)
datasource_access
->[db2].[ds2](id:2)
cc: @zephyring
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION