superset icon indicating copy to clipboard operation
superset copied to clipboard

fix: all_database_access should enable access to all datasets/charts/dashboards

Open mistercrunch opened this issue 10 months ago • 6 comments

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.

mistercrunch avatar Apr 24 '24 22:04 mistercrunch

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.

request-info[bot] avatar Apr 24 '24 22:04 request-info[bot]

So, what do we do? Deprecate or fix?

mistercrunch avatar Apr 25 '24 19:04 mistercrunch

/testenv up

eschutho avatar Apr 25 '24 21:04 eschutho

@eschutho Ephemeral environment spinning up at http://34.221.9.95:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar Apr 25 '24 21:04 github-actions[bot]

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?

eschutho avatar Apr 25 '24 22:04 eschutho

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.

mistercrunch avatar Apr 26 '24 00:04 mistercrunch

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.

mistercrunch avatar Apr 30 '24 23:04 mistercrunch

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.

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.

dpgaspar avatar May 02 '24 10:05 dpgaspar

Ephemeral environment shutdown and build artifacts deleted.

github-actions[bot] avatar May 02 '24 16:05 github-actions[bot]