Sharing messaging #481
Sharing messaging #481
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
jhub_apps/service/routes.py
Outdated
| 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.
| 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.
jhub_apps/service/routes.py
Outdated
| servername=server_name, | ||
| ) | ||
| # Check if user is an admin | ||
| if user.admin: |
There was a problem hiding this comment.
We don't need to do this check, nothing related to permissions needs to be checked by jhub-apps, its the responsibility of JupyterHub, we simply try to start from jupyterhub and based on the response from it, we return in jhub-apps API.
jhub_apps/service/routes.py
Outdated
| remove: bool = False, | ||
| user: User = Depends(get_current_user), | ||
| ): |
There was a problem hiding this comment.
no changes should be made for delete server, we don't plan to let the user delete shared server (not owned by the user)
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?