Sharing messaging #481
Open
Sharing messaging #481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
jbouder
reviewed
Sep 16, 2024
jbouder
requested changes
Sep 16, 2024
jbouder
requested changes
Sep 17, 2024
aktech
reviewed
Sep 19, 2024
jhub_apps/service/routes.py
Outdated
Comment on lines
173
to
186
| shared_servers = get_shared_servers(current_hub_user=user) | ||
| if server_name and any(server['name'] == server_name for server in shared_servers): | ||
| # User is trying to start a shared server without permission | ||
| raise HTTPException( | ||
| detail=f"User '{user.name}' does not have permission to start server '{server_name}'", | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| except ValueError as e: | ||
| logger.error(f"Error in shared servers check: {e}") | ||
| raise HTTPException( | ||
| detail=f"Failed to check shared servers: {e}", | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This permissions part needs to be handled on JupyterHub side (as JupyterHub knows what permissions a user have and jhub-apps service doesn't), jhub-apps service is not responsible for this and besides the checks done here is not sufficient to determine if the user has permissions to start the server or not. We actually just need to call hub_client.start_server and based on the response to it, we need to return appropriate response.
aktech
reviewed
Sep 19, 2024
Comment on lines
83
to
85
| assert response.json() == create_server_response | ||
|
|
||
|
|
||
| @patch.object(HubClient, "start_server") | ||
| def test_api_start_server(create_server, client): | ||
| start_server_response = {"user": "jovyan"} | ||
| create_server.return_value = start_server_response | ||
| server_name = "server-name" | ||
| response = client.post( | ||
| f"/server/{server_name}", | ||
| ) | ||
| create_server.assert_called_once_with( | ||
| username=MOCK_USER.name, | ||
| servername=server_name, | ||
| ) | ||
| assert response.status_code == 200 | ||
| assert response.json() == start_server_response | ||
|
|
||
|
|
||
| @patch.object(HubClient, "start_server") | ||
| def test_api_start_server_404(start_server, client): | ||
| start_server_response = None | ||
| start_server.return_value = start_server_response | ||
| server_name = "server-name" | ||
| response = client.post( | ||
| f"/server/{server_name}", | ||
| ) | ||
| start_server.assert_called_once_with( | ||
| username=MOCK_USER.name, | ||
| servername=server_name, | ||
| ) | ||
| assert response.status_code == 404 | ||
| assert response.json() == {"detail": "server 'server-name' not found"} | ||
|
|
||
| assert response.json() == create_server_response | ||
|
|
There was a problem hiding this comment.
We'd need to undo removal of these tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR handle error handling and messaging. When a user with a shared app tries to start/stop an app they will get a message alerting them they don't have permission to perform that action.
Reference Issues or PRs
What does this implement/fix?
Fixes the issues of a user wondering why they can't perform an action, now it give them an issue.
Put a
xin the boxes that applyTesting
Documentation
Access-centered content checklist
Text styling
H1or#in markdown).Non-text content
Any other comments?