Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

feat: Allow superadmins to list vfolders as specific user #544

Merged
merged 16 commits into from
Mar 8, 2022

Conversation

SonMinWoo
Copy link
Contributor

@SonMinWoo SonMinWoo commented Feb 21, 2022

resolve: lablup/backend.ai#321
related: lablup/backend.ai-webui#1237

To solve the problem of webui, this is implemented in list_folders api. This feature uses only the user's email to search the folder of the user. Now only superadmin can use this feature, but it will be expandable like public/private folder option.

It request to DB about vfolder informations belong to carried user email. The other process is same to existing folder listing feature.
issue321-2

When superadmin click checkbox and it is checked, webui request to manager this feature. Parameter is added 'user email'.

@SonMinWoo SonMinWoo requested a review from achimnol February 21, 2022 16:18
@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #544 (1c65e95) into main (268110c) will increase coverage by 0.14%.
The diff coverage is 9.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   49.13%   49.27%   +0.14%     
==========================================
  Files          54       54              
  Lines        9031     9207     +176     
==========================================
+ Hits         4437     4537     +100     
- Misses       4594     4670      +76     
Impacted Files Coverage Δ
src/ai/backend/manager/api/utils.py 52.23% <9.37%> (-5.85%) ⬇️
src/ai/backend/manager/registry.py 17.24% <0.00%> (+0.24%) ⬆️
src/ai/backend/manager/models/base.py 53.28% <0.00%> (+0.26%) ⬆️
src/ai/backend/manager/server.py 61.84% <0.00%> (+0.26%) ⬆️
src/ai/backend/manager/config.py 44.44% <0.00%> (+0.40%) ⬆️
src/ai/backend/manager/cli/etcd.py 53.17% <0.00%> (+3.60%) ⬆️
src/ai/backend/manager/models/utils.py 83.64% <0.00%> (+5.31%) ⬆️
src/ai/backend/manager/plugin/error_monitor.py 62.71% <0.00%> (+16.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 268110c...1c65e95. Read the comment docs.

@achimnol achimnol changed the title feat: add vfolder listing feature with userid to superadmin feat: Allow superadmins to list vfolders as specific user Feb 28, 2022
@achimnol achimnol added this to the 22.03 milestone Feb 28, 2022
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep consistency with owner_access_key used in session APIs to delegate the identity, let's rename the new user argument as owner_user_email.

Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Why not used query_accessible_vfolders() with the target user UUID and role?

@achimnol achimnol requested a review from lizable March 5, 2022 11:36
@SonMinWoo
Copy link
Contributor Author

Q: Why not used query_accessible_vfolders() with the target user UUID and role?

A: It is because double query is needed to know the target user UUID and role by only email. I thought this process cause performance degradation.

@achimnol
Copy link
Member

achimnol commented Mar 7, 2022

Q: Why not used query_accessible_vfolders() with the target user UUID and role?

A: It is because double query is needed to know the target user UUID and role by only email. I thought this process cause performance degradation.

That overhead is okay because it only fetches one row from a unique key. The owner_access_key (get_access_key_scopes()) already doing the same. I've rewritten so by myself.

Copy link
Collaborator

@lizable lizable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed minor things to make list_folders() function works fine.

"Only admins can perform operations on behalf of other users.",
)
else:
owner_user_uuid = request['user']['uuid']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this part since there's no user_uuid or user_role field in the request parameter.
Instead, we can use uuid and role field from user field in request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍🏼

@lizable lizable requested a review from achimnol March 7, 2022 08:21
@achimnol
Copy link
Member

achimnol commented Mar 8, 2022

Let's deprecate the "all" option as it would incur severe performance overheads when used on the cloud. I will update the code.

@achimnol achimnol merged commit 61a3f10 into main Mar 8, 2022
@achimnol achimnol deleted the feature/add-list-by-userid branch March 8, 2022 06:40
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delegated session creation feature is not working
3 participants