-
Notifications
You must be signed in to change notification settings - Fork 38
services: permissions: Use search_all config for admin user search #143
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
Conversation
@@ -38,7 +38,11 @@ class UsersPermissionPolicy(BasePermissionPolicy): | |||
IfPublicUser(then_=[AnyUser()], else_=[Self()]), | |||
SystemProcess(), | |||
] | |||
can_search = [AuthenticatedUser(), SystemProcess()] | |||
can_search = [ |
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 change is probably not needed: the Self()
user is already authenticated, so the user will be able to perform the search
. Then, on each hit, the can_read
permission check kicks in, filtering the results.
Why not creating a new can_read_all
below, and set it to:
can_read_all = [UserManager, SystemProcess()]
Then, in the func search_all
, use the can_read_all
instead.
@@ -91,6 +91,7 @@ def search_all( | |||
params=params, | |||
search_preference=search_preference, | |||
search_opts=self.config.search_all, | |||
permission_action="search_all", |
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.
permission_action="search_all", | |
permission_action="read_all", |
@@ -62,7 +62,7 @@ | |||
), | |||
"username": dict( | |||
title=_("Username"), | |||
fields=["username", "-created"], | |||
fields=["username.keyword", "-created"], |
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.
Since, we've changed this field in v3 from keyword to analyzed text, we need to use the new keyword subfield here.
@@ -48,6 +48,7 @@ class UsersPermissionPolicy(BasePermissionPolicy): | |||
SystemProcess(), | |||
] | |||
can_read_details = [UserManager, Self(), SystemProcess()] | |||
can_read_all = [UserManager, SystemProcess()] |
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.
UserManager
role covers administration-moderation
role. Should we also add support for administration-access
role? It should be Administration()
, but I can be wrong.
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.
Isn't administration-access
role for just having rights for accessing the administration panel? The administration-moderation
role is for user management.
Fixes: inveniosoftware/invenio-app-rdm#2797