Skip to content
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

Sharing messaging #481

Merged
merged 20 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion jhub_apps/hub_client/hub_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from concurrent.futures import ThreadPoolExecutor
from functools import wraps


import structlog
import os
import re
Expand All @@ -17,7 +18,6 @@

logger = structlog.get_logger(__name__)


def requires_user_token(func):
"""Decorator to apply to methods of HubClient to create user token before
the method call and revoke them after the method call finishes.
Expand All @@ -36,6 +36,7 @@ def wrapper(self, *args, **kwargs):
return wrapper



class HubClient:
def __init__(self, username=None):
self.username = username
Expand Down
54 changes: 47 additions & 7 deletions jhub_apps/service/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,27 +160,67 @@ async def create_server(
@router.post("/server/")
@router.post("/server/{server_name}")
async def start_server(
server_name=None,
server_name: str = None,
user: User = Depends(get_current_user),
):
"""Start an already existing server."""
logger.info("Starting server", server_name=server_name, user=user.name)
logger.info(f"Starting server with name '{server_name}' for user '{user.name}'")

hub_client = HubClient(username=user.name)

# Check if the server is shared and whether the user has permission to start it
try:
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,
)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

try:
logger.info(f"Attempting to start server '{server_name}' for user '{user.name}'")
response = hub_client.start_server(
username=user.name,
servername=server_name,
)

# Log the actual response from the JupyterHub API
logger.info(f"Received response from JupyterHub API: {response}")

except requests.exceptions.HTTPError as e:
raise HTTPException(
detail=f"Probably server '{server_name}' is already running: {e}",
status_code=status.HTTP_400_BAD_REQUEST,
)
logger.error(f"HTTPError occurred while starting server '{server_name}': {e}")

if e.response.status_code == 403:
raise HTTPException(
detail=f"User '{user.name}' does not have permission to start server '{server_name}'",
status_code=status.HTTP_403_FORBIDDEN,
)
elif e.response.status_code == 500:
raise HTTPException(
detail="Internal server error occurred while trying to start the server.",
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
)
else:
raise HTTPException(
detail=f"Unexpected error occurred while starting server '{server_name}': {e}",
status_code=status.HTTP_400_BAD_REQUEST,
)

if response is None:
logger.error(f"Server '{server_name}' not found for user '{user.name}'")
raise HTTPException(
detail=f"server '{server_name}' not found",
detail=f"Server '{server_name}' not found",
status_code=status.HTTP_404_NOT_FOUND,
)

logger.info(f"Successfully started server '{server_name}' for user '{user.name}'")
return response


Expand Down
49 changes: 41 additions & 8 deletions jhub_apps/service/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import base64
import itertools

import json
import structlog
import os

Expand Down Expand Up @@ -150,21 +150,54 @@ def get_theme(config):


def get_shared_servers(current_hub_user):
# Check if current_hub_user is a dictionary or an object
if isinstance(current_hub_user, dict):
username = current_hub_user.get('name')
else:
username = getattr(current_hub_user, 'name', None)

if not username:
raise ValueError("User name is missing in current_hub_user")

# Initialize hub client for the user
hub_client_user = HubClient(username=username)

# Filter servers shared with the user
hub_client_service = HubClient()
all_users_servers = list(itertools.chain.from_iterable([
list(user['servers'].values()) for user in hub_client_service.get_users()
]))

try:
users = hub_client_service.get_users()
# Check if users is a string, if so, parse it as JSON
if isinstance(users, str):
users = json.loads(users)

# Ensure that users is a list of dictionaries
if not isinstance(users, list):
raise TypeError("Expected a list of users from hub_client_service.get_users()")

# Iterate over users to get their servers
all_users_servers = list(itertools.chain.from_iterable([
list(user.get('servers', {}).values()) for user in users if isinstance(user, dict)
]))

except (json.JSONDecodeError, TypeError, KeyError) as e:
print(f"Error while fetching or parsing user servers: {e}")
return []

# Filter out default JupyterLab server (name="")
user_servers_without_default_jlab = list(filter(lambda server: server["name"] != "", all_users_servers))
hub_client_user = HubClient(username=current_hub_user['name'])

# Get shared servers for the current user
shared_servers = hub_client_user.get_shared_servers()
shared_server_names = {
shared_server["server"]["name"] for shared_server in shared_servers
# remove shared apps by current user
if shared_server["server"]["user"]["name"] != current_hub_user['name']
# Remove shared apps by current user
if shared_server["server"]["user"]["name"] != username
}

shared_servers_rich = [
server for server in user_servers_without_default_jlab
if server["name"] in shared_server_names
]
return shared_servers_rich

return shared_servers_rich
48 changes: 24 additions & 24 deletions jhub_apps/static/js/index.js

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions jhub_apps/tests/tests_e2e/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ def create_app(
logger.info("Creating App")
page.get_by_role("button", name="Deploy App").click()
logger.info("Fill App display Name")
page.get_by_label("*Name").click()
page.get_by_label("*Name").fill(app_name)
page.get_by_label("Name").click()
page.get_by_label("Name").fill(app_name)
logger.info("Select Framework")
page.locator("id=framework").click()
page.get_by_role("option", name="Panel").click()
Expand Down
41 changes: 5 additions & 36 deletions jhub_apps/tests/tests_unit/test_api.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import io
import json
import logging
from unittest.mock import patch

import pytest
Expand All @@ -9,6 +10,8 @@
from jhub_apps.spawner.types import FRAMEWORKS
from jhub_apps.tests.common.constants import MOCK_USER

logging.basicConfig(level=logging.INFO)
logger = logging.getLogger(__name__)

def mock_user_options():
user_options = {
Expand All @@ -34,7 +37,6 @@ def mock_user_options():
}
return user_options


@patch.object(HubClient, "get_user")
def test_api_get_server(get_user, client):
server_data = {"panel-app": {}}
Expand All @@ -58,7 +60,6 @@ def test_api_get_server_not_found(get_user, client):
'detail': "server 'panel-app-not-found' not found",
}


@patch.object(HubClient, "create_server")
def test_api_create_server(create_server, client):
from jhub_apps.service.models import UserOptions
Expand All @@ -80,40 +81,8 @@ def test_api_create_server(create_server, client):
user_options=final_user_options,
)
assert response.status_code == 200
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

Copy link
Member

Choose a reason for hiding this comment

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

We'd need to undo removal of these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.


@pytest.mark.parametrize("name,remove", [
('delete', True,),
Expand Down
118 changes: 118 additions & 0 deletions ui/src/components/app-card/app-card.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -578,4 +578,122 @@ describe('AppCard', () => {
);
expect(getByTestId('LockRoundedIcon')).toBeInTheDocument();
});
test('renders context menu with start, stop, edit, and delete actions', async () => {
const { getByTestId, getByText } = render(
<RecoilRoot>
<QueryClientProvider client={queryClient}>
<AppCard
id="1"
title="Test App"
username="Developer"
framework="Some Framework"
url="/some-url"
serverStatus="Ready"
isShared={false}
app={{ id: '1', name: 'Test App', framework: 'Some Framework', description: 'Test App 1',
url: '/user/test/test-app-1/',
thumbnail: '',
username: 'test',
ready: true,
public: false,
shared: false,
last_activity: new Date(),
pending: false,
stopped: false,
status: 'false' }}
/>
</QueryClientProvider>
</RecoilRoot>,
);

// Open context menu first
const contextMenuButton = getByTestId('context-menu-button-card-menu-1');
act(() => {
contextMenuButton.click();
});

const startMenuItem = await waitFor(() => getByText('Start'));
const stopMenuItem = getByText('Stop');
const editMenuItem = getByText('Edit');
const deleteMenuItem = getByText('Delete');

expect(startMenuItem).toBeInTheDocument();
expect(stopMenuItem).toBeInTheDocument();
expect(editMenuItem).toBeInTheDocument();
expect(deleteMenuItem).toBeInTheDocument();
});


test('disables stop action if app is not running', async () => {
const { getByTestId, getByText } = render(
<RecoilRoot>
<QueryClientProvider client={queryClient}>
<AppCard
id="1"
title="Test App"
username="Developer"
framework="Some Framework"
url="/some-url"
serverStatus="Pending" // App is not running
isShared={false}
app={{ id: '1', name: 'Test App', framework: 'Some Framework',
description: 'Test App 1',
url: '/user/test/test-app-1/',
thumbnail: '',
username: 'test',
ready: true,
public: false,
shared: false,
last_activity: new Date(),
pending: true,
stopped: false,
status: 'false' }}
/>
</QueryClientProvider>
</RecoilRoot>,
);

// Open context menu first
const contextMenuButton = getByTestId('context-menu-button-card-menu-1');
act(() => {
contextMenuButton.click();
});

const stopMenuItem = await waitFor(() => getByText('Stop'));
expect(stopMenuItem).toBeInTheDocument();
expect(stopMenuItem).toHaveAttribute('aria-disabled', 'true');
});


test('disables edit and delete for shared apps', async () => {
const { getByTestId, getByText } = render(
<RecoilRoot>
<QueryClientProvider client={queryClient}>
<AppCard
id="1"
title="Shared App"
username="Other User"
framework="Some Framework"
url="/some-url"
serverStatus="Ready"
isShared={true} // App is shared
/>
</QueryClientProvider>
</RecoilRoot>,
);

// Open context menu first
const contextMenuButton = getByTestId('context-menu-button-card-menu-1');
act(() => {
contextMenuButton.click();
});

const editMenuItem = await waitFor(() => getByText('Edit'));
const deleteMenuItem = getByText('Delete');

expect(editMenuItem).toHaveAttribute('aria-disabled', 'true');
expect(deleteMenuItem).toHaveAttribute('aria-disabled', 'true');
});


});
Loading
Loading