-
Notifications
You must be signed in to change notification settings - Fork 6
add generic superadmin flag to endpoints #1038
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
|
@bingzhang please add |
|
the logic looks good to me. I see that the PR is in draft state. So I will wait for now. |
|
|
||
| @router.get("/users/me/admin_mode") | ||
| async def get_admin_mode(current_username=Depends(get_current_user)) -> bool: | ||
| async def get_admin_mode( |
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 we split the dependency from the route and put it in dependencies.py?
- Should we rename
superadmintoadminorforce_adminor something else?
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 don't quite understand question 1. can you say more about "split the dependency from the route"?
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 need to rename superadmin to force_admin, since admin has been used in admin=Depends(get_admin)
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 don't think this was introduced in this PR, but fastapi dependencies don't need to be routes. More importantly this route returns just a boolean and not a JSON document, so we could separate the two and make the route return a valid JSON document. I can make the change.
|
@max-zilla @bingzhang can I rename |
I am fine with |
|
Resolved merge conflicts and changed to The frontend has a bug however. The API call is returning all datasets when admin mode is enabled, but for some reason they are being hidden immediately: If I toggle between Datasets and My Datasets tab, i can see Fake Dataset for just a moment before it disappears. Looking into this. Applying Todd's fix from this PR: https://github.com/clowder-framework/clowder2/pull/1104/files |
|
@max-zilla I fixed the codegen error. |
longshuicy
left a comment
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.
The logic looks good and I tested the list all dataset endpoint, which works as expected.
When I do a project-wide search of the admin_mode, I noticed there are one spot missing enable_admin. Could you double check?
class FeedAuthorization:
"""We use class dependency so that we can provide the `permission` parameter to the dependency.
For more info see https://fastapi.tiangolo.com/advanced/advanced-dependencies/.
Regular users can only see their own feeds"""
# def __init__(self, optional_arg: str = None):
# self.optional_arg = optional_arg
async def __call__(
self,
feed_id: str,
current_user: str = Depends(get_current_username),
admin_mode: bool = Depends(get_admin_mode),
admin: bool = Depends(get_admin),

This adds a new API parameter called
force_adminthat enables you to make a call as if admin_mode were enabled even if it isn't (assuming you are admin).Currently on one endpoint:
http://localhost:8000/api/v2/groups?force_admin=TrueIf this is acceptable, we can include anywhere we use the
get_admin_modedependency above it:example endpoint:
http://localhost:8000/api/v2/groups?force_admin=true
will return a list of group data.