-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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: all_database_access should enable access to all datasets/charts/dashboards #28205
Conversation
We would appreciate it if you could provide us with more info about this issue/pr! Please do not leave the |
return self.can_access_all_databases() or self.can_access( | ||
"all_datasource_access", "all_datasource_access" | ||
) |
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.
This makes sense and looks fine, I'm just worried about unintended consequences. Would be nice to get a signoff from @dpgaspar and/or @villebro and/or @john-bodley here, just in case.
Also, it would be really nice if we could decouple the data permissions from the API permissions. Maybe we should create a workgroup to brainstorm how this should look like.
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.
I agree, grepping for all_database_access
there's very little justifying its existence as AFAIU, two choices here:
- make it work as "expected" based on name which would be what's in this PR, essentially making
all_database_access
==all_datasource_access
- deprecate
all_database_access
and [either]- migrate
all_datasource_access
where it's defined - or NOT migrate, do as if it never existed as migrating may effectively grant things that were not granted before
- migrate
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.
Digging a bit deeper I found a reference to SecurityManager.can_access_all_databases
(which wraps the all_database_access
permission) in superset/databases/filters.py
, which seems potentially at odds with the model-based perms as in Database.can_write
and Database.can_read
. If we were to deprecate all_database_access
we could transfer it to Database.can_read
in roles where it's specified to try to match the current behavior.
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.
@eschutho mentioned the case where we want to give someone access to all the tables for them to query in SQL Lab, but we don't want them modifying the database configuration. In that case, we'd want them to have all_datasource_access
but not all_database_access
, no?
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.
I think the CRUD on Database is tied to the model / modelview, as opposed to this special term.
I have a spreadsheet of all the perms, and the relevant ones for database CRUD should be these ->
<style type="text/css"></style>
view_menu | permission | perm_type |
---|---|---|
Database | can_export | MODEL_VIEW |
Database | can_read | MODEL_VIEW |
Database | can_write | MODEL_VIEW |
Databases | menu_access | MENU |
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.
We (Airbnb) tend not to use these access controls and thus I don't really have much to add. My only question would be, why this hasn't been an issue in the past as it potentially looks like a fairly egregious oversight from a consistency standpoint—if in fact the proposed behavior is correct.
So, what do we do? Deprecate or fix? |
/testenv up |
@eschutho Ephemeral environment spinning up at http://34.221.9.95:8080. Credentials are |
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 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 |
…dashboards When granting `all_database_access`, one would expect to see all dashboards, 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.
00bf1ac
to
e716e68
Compare
I agree with this change, to my understanding before 2.1, |
Ephemeral environment shutdown and build artifacts deleted. |
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 toall_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:
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.