feat: new config to filter specific users from dropdown lists #21515
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21515 +/- ##
=======================================
Coverage 66.67% 66.68%
=======================================
Files 1793 1793
Lines 68493 68513 +20
Branches 7275 7275
=======================================
+ Hits 45665 45685 +20
Misses 20966 20966
Partials 1862 1862
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Should this be an attribute for the |
Very good point, an extra attribute for the |
There was a problem hiding this comment.
In general, I'm not a big fan of filtering entities by hard-coded id/names as they are not very scalable. You cannot automate (e.g. syncing account list with an internal access control service) and any change would require code re-deployment.
It seems there is actually a not-very-much-used user_attribute table already exist in Superset that could be used to store such information (whether a user is service account or not). Do you think it'd worth looking into whether we can use this table for filtering instead?
|
|
||
| :return: A list of usernames | ||
| """ | ||
| return [] |
There was a problem hiding this comment.
Should this be a list of ids instead of usernames? Internal filtering should preferably use IDs as they are more determinant.
There was a problem hiding this comment.
username is unique so it has the same level of determinism has id, I can't internally identify service users that are dynamically created by id.
There was a problem hiding this comment.
A username can be changed, but id cannot right? By definition that makes id more deterministic. It may not happen often in this case, but I've seen things broken in other systems because a username was changed.
Wouldn't you be able to get the user id by just going to the users CRUD views after they are created? The user id is in the /users/edit/:userId URL.
There was a problem hiding this comment.
I could, but why make an extra query to fetch an ID?
Service users normally have pre-defined usernames, does not seem common practice to me to identify users on a config key by user IDs. There are lot's of cases of pre-defined usernames for service users or special well known users: postgres, root, rdsadmin, rdsrepladmin, operator, backup, guest, anonymous, QSECOFR. We just assume these names will not change.
| exclude_users = ( | ||
| security_manager.get_exclude_users_from_lists() | ||
| if current_app.config["EXCLUDE_USERS_FROM_LISTS"] is None | ||
| else current_app.config["EXCLUDE_USERS_FROM_LISTS"] |
There was a problem hiding this comment.
I'm still not sure if we need two config values. I understand setting a hard-coded EXCLUDE_USERS_FROM_LISTS is easy but so is setting it in a custom security manager.
Even if we do want to keep EXCLUDE_USERS_FROM_LISTS, you can use it in the default get_exclude_users_from_lists so if users set get_exclude_users_from_lists, EXCLUDE_USERS_FROM_LISTS will be ignored. This prevents them from changing both configs and bite themselves down the road.
There was a problem hiding this comment.
Overriding the method makes it possibly more dynamic and the existing config key makes it easier to discover the functionality itself.
I understand yet another config setting, but relying on configuration hidden behind overriding a method on security manager is harder to discover and forces users to override the security manager (they may have not done it)
There was a problem hiding this comment.
I'm not sure why this needs to be "easy to discover"? If someone really want this feature, they will dig into the docs of config setup then hopefully find some mention of this in the Security Manager section? If we really want to keep this config value for ease of use, then maybe there is no value in adding a security manager solution as it just adds more confusion.
There was a problem hiding this comment.
Also, I think most enterprise users would probably have to have their custom security manager anyway. I understand a config value is easier to set up and would do the work for most people; re-reploying Superset every time a new service account needs to be added also doesn't sound like a big deal since it is likely not frequent. But sooner or later, we'd need to migrate current FAB views for user profiles to React and add proper REST API for users and roles. When the API is added, it'd be only natural to configure things like user types in the Edit User page.
A robust and scalable user layer is critical to Superset's success among enterprise users so it'd be prudent to be more future-proof here. Superset has long suffered from not having a built-in user and access control layer, which IMO had made access control in Superset more complex than it needs to be and hindered the development of features like RBAC. Current overrides on top of FAB are hacky and error-prone. Dynamic string-based permissions also feel unreliable. Not the scope of this PR, but I really hope there would be a discussion on what the user and auth models in Superset should look like in the far future.
I'm not opposed to merging this PR as it since it gets the job done, just thought, in general, this area needs a little bit more love.
There was a problem hiding this comment.
I agree with the majority of your points. We have some initiatives for discussing string based data access permissions on the near future.
dpgaspar
deleted the
danielgaspar/sc-46720/implement-option-on-apache-superset-to-remove
branch
mistercrunch
added
🏷️ bot
SUMMARY
Introduces a new config key named
EXCLUDE_USERS_FROM_LISTSwith a list of usernames to exclude from all UIdropdown user list, owners, created by filters etc.
This is useful for excluding service user's like an initial
adminand/or the thumbnail user.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION