Add Filter on DatabaseView that filters DBs Based on Role Access #7618
Conversation
dflionis
force-pushed
the
filter-databases-based-on-roles
branch
from
461a8b2 to
53193e3
Compare
dflionis
force-pushed
the
filter-databases-based-on-roles
branch
from
53193e3 to
c79a1cf
Compare
dflionis
changed the title
| @@ -127,12 +127,11 @@ def all_datasource_access(self): | |||
| return self.can_access( | |||
| 'all_datasource_access', 'all_datasource_access') | |||
|
|
|||
| def all_database_access(self): | |||
| return self.can_access('all_database_access', 'all_database_access') | |||
There was a problem hiding this comment.
In the original PR, this was sending a user keyword argument. However, can_access() does not currently accept a keyword arg so I removed it
| @@ -127,12 +127,11 @@ def all_datasource_access(self): | |||
| return self.can_access( | |||
| 'all_datasource_access', 'all_datasource_access') | |||
|
|
|||
| def all_database_access(self): | |||
There was a problem hiding this comment.
A different contributor came along and added some tests to this module, but since the tests were failing and it was after Maxime's approval, I did not attempt to port them over. Happy to try to write tests if there is demand for adding them.
superset/security.py
Outdated
| 'all_database_access', 'all_database_access') or | ||
| self.can_access('database_access', database.perm) | ||
| ) | ||
| return self.can_access('database_access', database.perm) |
There was a problem hiding this comment.
Can we make this:
return self.all_database_access() or self.can_access('database_access', database.perm) ?
There was a problem hiding this comment.
Wondering if we should also checkout for all_datasource_access (which is redundant with all_database_access) here for backwards compatibility reasons. Say if someone setup a local role Users with access to all data that has all_datasource_access, I feel like they should see all databases when opening SQL Lab.
There was a problem hiding this comment.
Thanks for checking this out--the PR has been updated. Regarding the last comment about a similar PR having gone in and gotten reverted, please let me know if that changes anything about how I should structure this PR and I would be happy to make the changes.
Codecov Report
@@ Coverage Diff @@
## master #7618 +/- ##
==========================================
+ Coverage 65.57% 65.58% +<.01%
==========================================
Files 435 435
Lines 21744 21753 +9
Branches 2393 2393
==========================================
+ Hits 14259 14266 +7
- Misses 7364 7366 +2
Partials 121 121
Continue to review full report at Codecov.
|
dflionis
force-pushed
the
filter-databases-based-on-roles
branch
from
4d9c89b to
61d4032
Compare
mistercrunch
added
🏷️ bot
This is an attempt to resurrect #6356 which had been approved by Maxime but needs to be rebased and may have been abandoned. I'm not sure if this replicates that functionality as I had to make some changes--but I'm happy to attempt to push this along as feedback comes in.