superset
superset copied to clipboard
fix: all_database_access should enable access to all datasets/charts/dashboards
When granting all_database_access
, one would expect to get access to all charts and datasets.
From my understanding all_database_access
is practically identical to all_datasource_access
, and perhaps the two perms should be collapsed into one. In the meantime, this addresses user confusion around this perm not working according to its name.
I'm guessing that the difference between the two could be that all_database_access
would also allow you to CRUD database objects(?)
Side mission:
Introducing a nifty context manager for tests to create temp user:
with self.temporary_user(clone_user=gamma_user, extra_pvms=[('all_database_access', 'all_database_access')],login=True) as user:
user.do_stuff()
The neat thing is it can clone a user, will delete it on exit, optionally can log user in/out. It's not mutating anything so it should work well for running tests in parallel and/or for tests that fail half way through.
I'm planning on a follow up refactoring PR to standardize on using temporary_user
everywhere where the current create_user is used.
We would appreciate it if you could provide us with more info about this issue/pr! Please do not leave the title
or description
empty.
So, what do we do? Deprecate or fix?
/testenv up
@eschutho Ephemeral environment spinning up at http://34.221.9.95:8080. Credentials are admin
/admin
. Please allow several minutes for bootstrapping and startup.
Also a few more places like here where it could break. Plus as @mistercrunch mentioned the filters present more complexity where we can remove specific access through configs even with all database access.
We can fix any outliers, so maybe the bigger question, is how do we want it to work? Most people probably logically assume that permissions work like Database -> Dataset -> Dashboard -> Chart, but they don't. I'm usually for small incremental changes, but the permissions models are so complex, that I expect even a small change could break quite a few things. I vote fix, but maybe for a larger 5.0 project?
I think this specific special permission is redundant with all_datasource_access
on one side (data access policy) and redundant with Database - can_read
on the CRUD side of managing databases.
I vote for deprecating this perm, though I'd rather get consensus prior to doing the bit of work.
After conversations with @eschutho and @yousoph , we decided that all_database_access
should remain and provide both access to all data (through all databases connections) and allow listing all said database connections. Adding a note to UPDATING.md to notify people upgrading, but it should't come as a surprise that all_database_access
give access to all databases.
After conversations with @eschutho and @yousoph , we decided that
all_database_access
should remain and provide both access to all data (through all databases connections) and allow listing all said database connections. Adding a note to UPDATING.md to notify people upgrading, but it should't come as a surprise thatall_database_access
give access to all databases.
I agree with this change, to my understanding before 2.1, all database access
or a specific database access on X
would not list any chart, dashboards, users would have database, schema and table access on SQLLab only. Currently granting database access on X
will give access to all charts and dashboards based on that database, but not all database access
so it does seem like a bug.
Ephemeral environment shutdown and build artifacts deleted.