-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement JupyterHub 5.x sharing APIs in the hub client #224
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 code looks good to me, I left a few questions.
url = f"/shares/{username}/{servername}" | ||
if share_to_user: | ||
data = {"user": share_to_user} | ||
elif share_to_group: | ||
data = {"group": share_to_group} | ||
else: | ||
raise ValueError("None of share_to_user or share_to_group provided") | ||
share_with = share_to_group or share_to_user | ||
logger.info(f"Sharing {username}/{servername} with {share_with}") | ||
return requests.post(API_URL + url, headers=self._headers(), json=data) |
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.
Do we need some escaping here? My concern is, what if a user names their server ../../another-endpoint
? Maybe it is safe because normalize_server_name
should strip both .
and /
. Maybe all that needs to be done is having a test case to ensure no path traversal.
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.
Maybe it is safe because normalize_server_name should strip both . and /.
Yes, correct. I added tests for normalize_server_name
.
jhub_apps/hub_client/hub_client.py
Outdated
users = hclient.get_users() | ||
user_names = [user["name"] for user in users] | ||
groups = hclient.get_groups() | ||
group_names = [group['name'] for group in groups] | ||
# TODO: Filter users and groups based on what the user has access to share with | ||
# parsed_scopes = parse_scopes(scopes) | ||
return { | ||
"users": user_names, |
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.
Does it makes sense to always filter the current user out as sharing with yourself does not have much utility?
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.
Yes, good catch. Done.
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.
Looks good to me, thank you!
# TODO: Filter users and groups based on what the user has access to share with | ||
# parsed_scopes = parse_scopes(scopes) |
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 see there is now #268 for this 👍
Reference Issues or PRs
Implements #185 and part of #11
On top of: #248
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Documentation
Access-centered content checklist
Text styling
H1
or#
in markdown).Non-text content
Any other comments?