-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: new config to filter specific users from dropdown lists #21515
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,15 +14,19 @@ | |
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import logging | ||
from typing import Any, cast, Optional | ||
|
||
from flask import current_app | ||
from flask_appbuilder.models.filters import BaseFilter | ||
from flask_babel import lazy_gettext | ||
from sqlalchemy import or_ | ||
from sqlalchemy import and_, or_ | ||
from sqlalchemy.orm import Query | ||
|
||
from superset import security_manager | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class FilterRelatedOwners(BaseFilter): # pylint: disable=too-few-public-methods | ||
|
||
|
@@ -48,3 +52,31 @@ def apply(self, query: Query, value: Optional[Any]) -> Query: | |
user_model.username.ilike(like_value), | ||
) | ||
) | ||
|
||
|
||
class BaseFilterRelatedUsers(BaseFilter): # pylint: disable=too-few-public-methods | ||
|
||
""" | ||
Filter to apply on related users. Will exclude users in EXCLUDE_USERS_FROM_LISTS | ||
Use in the api by adding something like: | ||
``` | ||
filter_rel_fields = { | ||
"owners": [["id", BaseFilterRelatedUsers, lambda: []]], | ||
"created_by": [["id", BaseFilterRelatedUsers, lambda: []]], | ||
} | ||
``` | ||
""" | ||
|
||
name = lazy_gettext("username") | ||
arg_name = "username" | ||
|
||
def apply(self, query: Query, value: Optional[Any]) -> Query: | ||
user_model = security_manager.user_model | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not sure if we need two config values. I understand setting a hard-coded Even if we do want to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overriding the method makes it possibly more dynamic and the existing config key makes it easier to discover the functionality itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the majority of your points. We have some initiatives for discussing string based data access permissions on the near future. |
||
) | ||
query_ = query.filter(and_(user_model.username.not_in(exclude_users))) | ||
return query_ |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
username
is unique so it has the same level of determinism hasid
, I can't internally identify service users that are dynamically created byid
.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.