From 28464f9c4751847e6a0f08fa36ce4500f6434cf1 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Mon, 27 Feb 2023 23:59:56 +0100 Subject: [PATCH 01/26] WIP --- ...add_from_config_column_to_the_services_.py | 29 +++++++++++++++++++ jupyterhub/apihandlers/services.py | 6 ++++ jupyterhub/app.py | 2 +- jupyterhub/orm.py | 3 ++ 4 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py new file mode 100644 index 00000000..31127621 --- /dev/null +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -0,0 +1,29 @@ +"""Add from_config column to the services table + +Revision ID: 3c2384c5aae1 +Revises: 0eee8c825d24 +Create Date: 2023-02-27 16:22:26.196231 +""" + +# revision identifiers, used by Alembic. +revision = '3c2384c5aae1' +down_revision = '0eee8c825d24' +branch_labels = None +depends_on = None + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + engine = op.get_bind().engine + tables = sa.inspect(engine).get_table_names() + if 'services' in tables: + op.add_column( + 'services', + sa.Column('from_config', sa.Boolean, nullable=False, default=True), + ) + op.execute('UPDATE services SET from_config = true') + +def downgrade(): + op.drop_column('services', sa.Column('from_config')) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 43e69aa7..9de4e8b4 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -28,6 +28,12 @@ class ServiceAPIHandler(APIHandler): service = self.services[service_name] self.write(json.dumps(self.service_model(service))) + @needs_scope('read:services', 'read:services:name', 'read:roles:services') + def post(self, service_name): + model = self.get_json_body() + print('############', model) + self.write('ok') + default_handlers = [ (r"/api/services", ServiceListAPIHandler), diff --git a/jupyterhub/app.py b/jupyterhub/app.py index eab6ef60..22c3eab2 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2371,7 +2371,7 @@ class JupyterHub(Application): orm_service = orm.Service.find(self.db, name=name) if orm_service is None: # not found, create a new one - orm_service = orm.Service(name=name) + orm_service = orm.Service(name=name, from_config=True) self.db.add(orm_service) if spec.get('admin', False): self.log.warning( diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index c0fb80ba..17553c29 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -416,6 +416,9 @@ class Service(Base): ondelete='SET NULL', ), ) + + from_config = Column(Boolean, default=True) + oauth_client = relationship( 'OAuthClient', back_populates="service", From e515a4b8206e3302eb83d23830ed09031903e236 Mon Sep 17 00:00:00 2001 From: Duc Trung LE Date: Tue, 28 Feb 2023 18:51:29 +0100 Subject: [PATCH 02/26] Add service post handler --- jupyterhub/apihandlers/base.py | 27 ++++ jupyterhub/apihandlers/services.py | 22 ++- jupyterhub/app.py | 225 ++++++++++++++++------------- jupyterhub/handlers/base.py | 13 ++ 4 files changed, 183 insertions(+), 104 deletions(-) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index f174f0dd..aff0f914 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -2,6 +2,7 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import json +from typing import Union import warnings from functools import lru_cache from http.client import responses @@ -395,6 +396,23 @@ class APIHandler(BaseHandler): _group_model_types = {'name': str, 'users': list, 'roles': list} + _service_model_types = { + 'name': str, + 'admin': bool, + 'url': str, + 'oauth_client_allowed_scopes': list, + 'api_token': str, + 'info': dict, + 'display': bool, + 'oauth_no_confirm': bool, + 'command': list, + 'cwd': str, + 'environment': dict, + 'user': str, + 'oauth_client_id': str, + 'oauth_redirect_uri': str + } + def _check_model(self, model, model_types, name): """Check a model provided by a REST API request @@ -433,6 +451,15 @@ class APIHandler(BaseHandler): 400, ("group names must be str, not %r", type(groupname)) ) + def _check_service_model(self, model): + """Check a request-provided service model from a REST API""" + self._check_model(model, self._service_model_types, 'service') + service_name = model.get('name') + if not isinstance(service_name, str): + raise web.HTTPError( + 400, ("Service name must be str, not %r", type(service_name)) + ) + def get_api_pagination(self): default_limit = self.settings["api_page_default_limit"] max_limit = self.settings["api_page_max_limit"] diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 9de4e8b4..0186c031 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -8,6 +8,7 @@ import json from ..scopes import Scope, needs_scope from .base import APIHandler +from tornado import web class ServiceListAPIHandler(APIHandler): @@ -29,10 +30,23 @@ class ServiceAPIHandler(APIHandler): self.write(json.dumps(self.service_model(service))) @needs_scope('read:services', 'read:services:name', 'read:roles:services') - def post(self, service_name): - model = self.get_json_body() - print('############', model) - self.write('ok') + def post(self, service_name: str): + data = self.get_json_body() + service = self.find_service(service_name) + + if service is not None: + raise web.HTTPError(409, f"Service {service_name} already exists") + + if not data or not isinstance(data, dict): + raise web.HTTPError(400, "Invalid service data") + + data['name'] = service_name + self._check_service_model(data) + self.service_from_spec(data) + self.db.commit() + service = self.find_service(service_name) + self.write(json.dumps(self.service_model(service))) + self.set_status(201) default_handlers = [ diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 22c3eab2..1e64536a 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -21,6 +21,7 @@ from functools import partial from getpass import getuser from operator import itemgetter from textwrap import dedent +from typing import Optional from urllib.parse import unquote, urlparse, urlunparse if sys.version_info[:2] < (3, 3): @@ -2354,6 +2355,129 @@ class JupyterHub(Application): ) pc.start() + def _service_from_spec( + self, + spec: Dict, + from_config=True, + domain: Optional[str] = None, + host: Optional[str] = None, + ) -> None: + if domain is None: + if self.domain: + domain = 'services.' + self.domain + else: + domain = '' + + if host is None: + if self.domain: + parsed = urlparse(self.subdomain_host) + host = f'{parsed.scheme}://services.{parsed.netloc}' + else: + host = '' + + if 'name' not in spec: + raise ValueError('service spec must have a name: %r' % spec) + + name = spec['name'] + # get/create orm + orm_service = orm.Service.find(self.db, name=name) + print('########### found', name, orm_service) + if orm_service is None: + # not found, create a new one + orm_service = orm.Service(name=name, from_config=from_config) + print('########### adding', name, orm_service) + self.db.add(orm_service) + if spec.get('admin', False): + self.log.warning( + f"Service {name} sets `admin: True`, which is deprecated in JupyterHub 2.0." + " You can assign now assign roles via `JupyterHub.load_roles` configuration." + " If you specify services in the admin role configuration, " + "the Service admin flag will be ignored." + ) + roles.update_roles(self.db, entity=orm_service, roles=['admin']) + orm_service.admin = spec.get('admin', False) + print('commiting db', name) + self.db.commit() + service = Service( + parent=self, + app=self, + base_url=self.base_url, + db=self.db, + orm=orm_service, + roles=orm_service.roles, + domain=domain, + host=host, + hub=self.hub, + ) + + traits = service.traits(input=True) + for key, value in spec.items(): + if key not in traits: + raise AttributeError("No such service field: %s" % key) + setattr(service, key, value) + + if service.api_token: + self.service_tokens[service.api_token] = service.name + elif service.managed: + # generate new token + # TODO: revoke old tokens? + service.api_token = service.orm.new_api_token(note="generated at startup") + + if service.url: + parsed = urlparse(service.url) + if parsed.port is not None: + port = parsed.port + elif parsed.scheme == 'http': + port = 80 + elif parsed.scheme == 'https': + port = 443 + server = service.orm.server = orm.Server( + proto=parsed.scheme, + ip=parsed.hostname, + port=port, + cookie_name=service.oauth_client_id, + base_url=service.prefix, + ) + self.db.add(server) + + else: + service.orm.server = None + + if service.oauth_available: + allowed_scopes = set() + if service.oauth_client_allowed_scopes: + allowed_scopes.update(service.oauth_client_allowed_scopes) + if service.oauth_roles: + if not allowed_scopes: + # DEPRECATED? It's still convenient and valid, + # e.g. 'admin' + allowed_roles = list( + self.db.query(orm.Role).filter( + orm.Role.name.in_(service.oauth_roles) + ) + ) + allowed_scopes.update(roles.roles_to_scopes(allowed_roles)) + else: + self.log.warning( + f"Ignoring oauth_roles for {service.name}: {service.oauth_roles}," + f" using oauth_client_allowed_scopes={allowed_scopes}." + ) + oauth_client = self.oauth_provider.add_client( + client_id=service.oauth_client_id, + client_secret=service.api_token, + redirect_uri=service.oauth_redirect_uri, + description="JupyterHub service %s" % service.name, + ) + service.orm.oauth_client = oauth_client + # add access-scopes, derived from OAuthClient itself + allowed_scopes.update(scopes.access_scopes(oauth_client)) + oauth_client.allowed_scopes = sorted(allowed_scopes) + else: + if service.oauth_client: + self.db.delete(service.oauth_client) + + self._service_map[name] = service + def init_services(self): self._service_map.clear() if self.domain: @@ -2364,106 +2488,7 @@ class JupyterHub(Application): domain = host = '' for spec in self.services: - if 'name' not in spec: - raise ValueError('service spec must have a name: %r' % spec) - name = spec['name'] - # get/create orm - orm_service = orm.Service.find(self.db, name=name) - if orm_service is None: - # not found, create a new one - orm_service = orm.Service(name=name, from_config=True) - self.db.add(orm_service) - if spec.get('admin', False): - self.log.warning( - f"Service {name} sets `admin: True`, which is deprecated in JupyterHub 2.0." - " You can assign now assign roles via `JupyterHub.load_roles` configuration." - " If you specify services in the admin role configuration, " - "the Service admin flag will be ignored." - ) - roles.update_roles(self.db, entity=orm_service, roles=['admin']) - orm_service.admin = spec.get('admin', False) - self.db.commit() - service = Service( - parent=self, - app=self, - base_url=self.base_url, - db=self.db, - orm=orm_service, - roles=orm_service.roles, - domain=domain, - host=host, - hub=self.hub, - ) - - traits = service.traits(input=True) - for key, value in spec.items(): - if key not in traits: - raise AttributeError("No such service field: %s" % key) - setattr(service, key, value) - - if service.api_token: - self.service_tokens[service.api_token] = service.name - elif service.managed: - # generate new token - # TODO: revoke old tokens? - service.api_token = service.orm.new_api_token( - note="generated at startup" - ) - - if service.url: - parsed = urlparse(service.url) - if parsed.port is not None: - port = parsed.port - elif parsed.scheme == 'http': - port = 80 - elif parsed.scheme == 'https': - port = 443 - server = service.orm.server = orm.Server( - proto=parsed.scheme, - ip=parsed.hostname, - port=port, - cookie_name=service.oauth_client_id, - base_url=service.prefix, - ) - self.db.add(server) - - else: - service.orm.server = None - - if service.oauth_available: - allowed_scopes = set() - if service.oauth_client_allowed_scopes: - allowed_scopes.update(service.oauth_client_allowed_scopes) - if service.oauth_roles: - if not allowed_scopes: - # DEPRECATED? It's still convenient and valid, - # e.g. 'admin' - allowed_roles = list( - self.db.query(orm.Role).filter( - orm.Role.name.in_(service.oauth_roles) - ) - ) - allowed_scopes.update(roles.roles_to_scopes(allowed_roles)) - else: - self.log.warning( - f"Ignoring oauth_roles for {service.name}: {service.oauth_roles}," - f" using oauth_client_allowed_scopes={allowed_scopes}." - ) - oauth_client = self.oauth_provider.add_client( - client_id=service.oauth_client_id, - client_secret=service.api_token, - redirect_uri=service.oauth_redirect_uri, - description="JupyterHub service %s" % service.name, - ) - service.orm.oauth_client = oauth_client - # add access-scopes, derived from OAuthClient itself - allowed_scopes.update(scopes.access_scopes(oauth_client)) - oauth_client.allowed_scopes = sorted(allowed_scopes) - else: - if service.oauth_client: - self.db.delete(service.oauth_client) - - self._service_map[name] = service + self._service_from_spec(spec, from_config=True, domain=domain, host=host) # delete services from db not in service config: for service in self.db.query(orm.Service): diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 5a052978..69216906 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -8,6 +8,7 @@ import math import random import re import time +from typing import Optional import uuid import warnings from datetime import datetime, timedelta @@ -1307,6 +1308,18 @@ class BaseHandler(RequestHandler): ns.update(self.settings['template_vars']) return ns + def service_from_spec(self, spec) -> None: + """Create service from api request""" + + self.app._service_from_spec(spec, from_config=False) + + def find_service(self, name: str) -> Optional[orm.Service]: + """Get a service by name + return None if no such service + """ + orm_service = orm.Service.find(db=self.db, name=name) + return orm_service + def get_accessible_services(self, user): accessible_services = [] if user is None: From 7dd4e4516ff34dc92a24e633d0e4749bb031656c Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Wed, 1 Mar 2023 14:22:05 +0100 Subject: [PATCH 03/26] Add scope --- docs/source/_static/rest-api.yml | 3 +++ ...3c2384c5aae1_add_from_config_column_to_the_services_.py | 3 ++- jupyterhub/apihandlers/base.py | 5 ++--- jupyterhub/apihandlers/services.py | 7 ++++--- jupyterhub/app.py | 3 --- jupyterhub/handlers/base.py | 2 +- jupyterhub/roles.py | 1 + jupyterhub/scopes.py | 6 +++++- 8 files changed, 18 insertions(+), 12 deletions(-) diff --git a/docs/source/_static/rest-api.yml b/docs/source/_static/rest-api.yml index b611f870..48e49d49 100644 --- a/docs/source/_static/rest-api.yml +++ b/docs/source/_static/rest-api.yml @@ -1498,6 +1498,9 @@ components: read:groups: Read group models. read:groups:name: Read group names. delete:groups: Delete groups. + admin:services: + Create, read, update, delete services, not including services + defined from config files. list:services: List services, including at least their names. read:services: Read service models. read:services:name: Read service names. diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py index 31127621..4e5fe450 100644 --- a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -11,8 +11,8 @@ down_revision = '0eee8c825d24' branch_labels = None depends_on = None -from alembic import op import sqlalchemy as sa +from alembic import op def upgrade(): @@ -25,5 +25,6 @@ def upgrade(): ) op.execute('UPDATE services SET from_config = true') + def downgrade(): op.drop_column('services', sa.Column('from_config')) diff --git a/jupyterhub/apihandlers/base.py b/jupyterhub/apihandlers/base.py index aff0f914..f8bbd569 100644 --- a/jupyterhub/apihandlers/base.py +++ b/jupyterhub/apihandlers/base.py @@ -2,7 +2,6 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import json -from typing import Union import warnings from functools import lru_cache from http.client import responses @@ -410,7 +409,7 @@ class APIHandler(BaseHandler): 'environment': dict, 'user': str, 'oauth_client_id': str, - 'oauth_redirect_uri': str + 'oauth_redirect_uri': str, } def _check_model(self, model, model_types, name): @@ -454,7 +453,7 @@ class APIHandler(BaseHandler): def _check_service_model(self, model): """Check a request-provided service model from a REST API""" self._check_model(model, self._service_model_types, 'service') - service_name = model.get('name') + service_name = model.get('name') if not isinstance(service_name, str): raise web.HTTPError( 400, ("Service name must be str, not %r", type(service_name)) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 0186c031..85c4ff95 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -6,9 +6,10 @@ Currently GET-only, no actions can be taken to modify services. # Distributed under the terms of the Modified BSD License. import json +from tornado import web + from ..scopes import Scope, needs_scope from .base import APIHandler -from tornado import web class ServiceListAPIHandler(APIHandler): @@ -29,7 +30,7 @@ class ServiceAPIHandler(APIHandler): service = self.services[service_name] self.write(json.dumps(self.service_model(service))) - @needs_scope('read:services', 'read:services:name', 'read:roles:services') + @needs_scope('admin:services') def post(self, service_name: str): data = self.get_json_body() service = self.find_service(service_name) @@ -39,7 +40,7 @@ class ServiceAPIHandler(APIHandler): if not data or not isinstance(data, dict): raise web.HTTPError(400, "Invalid service data") - + data['name'] = service_name self._check_service_model(data) self.service_from_spec(data) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 1e64536a..34217580 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2381,11 +2381,9 @@ class JupyterHub(Application): name = spec['name'] # get/create orm orm_service = orm.Service.find(self.db, name=name) - print('########### found', name, orm_service) if orm_service is None: # not found, create a new one orm_service = orm.Service(name=name, from_config=from_config) - print('########### adding', name, orm_service) self.db.add(orm_service) if spec.get('admin', False): self.log.warning( @@ -2396,7 +2394,6 @@ class JupyterHub(Application): ) roles.update_roles(self.db, entity=orm_service, roles=['admin']) orm_service.admin = spec.get('admin', False) - print('commiting db', name) self.db.commit() service = Service( parent=self, diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 69216906..4bf7fcac 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -8,11 +8,11 @@ import math import random import re import time -from typing import Optional import uuid import warnings from datetime import datetime, timedelta from http.client import responses +from typing import Optional from urllib.parse import parse_qs, parse_qsl, urlencode, urlparse, urlunparse from jinja2 import TemplateNotFound diff --git a/jupyterhub/roles.py b/jupyterhub/roles.py index a53e5a53..3ae2d008 100644 --- a/jupyterhub/roles.py +++ b/jupyterhub/roles.py @@ -34,6 +34,7 @@ def get_default_roles(): 'admin-ui', 'admin:users', 'admin:servers', + 'admin:services', 'tokens', 'admin:groups', 'list:services', diff --git a/jupyterhub/scopes.py b/jupyterhub/scopes.py index 00170133..d7b91f16 100644 --- a/jupyterhub/scopes.py +++ b/jupyterhub/scopes.py @@ -123,6 +123,10 @@ scope_definitions = { 'delete:groups': { 'description': "Delete groups.", }, + 'admin:services': { + 'description': 'Create, read, update, delete services, not including services defined from config files.', + 'subscopes': ['list:services', 'read:services', 'read:roles:services'], + }, 'list:services': { 'description': 'List services, including at least their names.', 'subscopes': ['read:services:name'], @@ -435,7 +439,7 @@ def _expand_self_scope(username): @lru_cache(maxsize=65535) def _expand_scope(scope): - """Returns a scope and all all subscopes + """Returns a scope and all subscopes Arguments: scope (str): the scope to expand From 1314eca8ec3a35511480b4b47d45c655c06e9c72 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Thu, 2 Mar 2023 23:26:59 +0100 Subject: [PATCH 04/26] Create service from orm --- jupyterhub/apihandlers/services.py | 75 ++++++++++++++++++++++++++++-- jupyterhub/app.py | 56 +++++++++++++++++++--- jupyterhub/handlers/base.py | 13 ------ jupyterhub/oauth/provider.py | 9 ++++ jupyterhub/services/service.py | 5 ++ 5 files changed, 133 insertions(+), 25 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 85c4ff95..68b5d388 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -5,10 +5,13 @@ Currently GET-only, no actions can be taken to modify services. # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import json +from typing import Optional from tornado import web +from .. import orm from ..scopes import Scope, needs_scope +from ..services.service import Service from .base import APIHandler @@ -31,7 +34,7 @@ class ServiceAPIHandler(APIHandler): self.write(json.dumps(self.service_model(service))) @needs_scope('admin:services') - def post(self, service_name: str): + async def post(self, service_name: str): data = self.get_json_body() service = self.find_service(service_name) @@ -43,12 +46,74 @@ class ServiceAPIHandler(APIHandler): data['name'] = service_name self._check_service_model(data) - self.service_from_spec(data) - self.db.commit() - service = self.find_service(service_name) - self.write(json.dumps(self.service_model(service))) + new_service = self.service_from_spec(data) + + if new_service is None: + raise web.HTTPError(400, f'Failed to create service {service_name}') + if new_service.api_token: + # Add api token to database + await self.app._add_tokens( + {new_service.api_token: new_service.name}, kind='service' + ) + elif new_service.managed: + new_service.api_token = new_service.orm.new_api_token( + note='generated at runtime' + ) + + self.write(json.dumps(self.service_model(new_service))) self.set_status(201) + @needs_scope('admin:services') + async def delete(self, service_name: str): + service = self.find_service(service_name) + + if service is None: + raise web.HTTPError(404, f"Service {service_name} does not exists") + + if service.from_config: + raise web.HTTPError( + 405, f"Service {service_name} is not modifiable at runtime" + ) + + if service.managed: + await service.stop() + + if service.oauth_client: + self.oauth_provider.remove_client(service.oauth_client_id) + + orm_service = orm.Service.find(db=self.db, name=service_name) + if orm_service is not None: + if service.api_token is not None: + # Remove api token from database + orm_token = ( + self.db.query(orm.APIToken) + .filter_by(service_id=orm_service.id) + .first() + ) + if orm_token is not None: + self.db.delete(orm_token) + + self.services.pop(service_name) + self.db.delete(orm_service) + + self.db.commit() + self.set_status(200) + + def service_from_spec(self, spec) -> Optional[Service]: + """Create service from api request""" + service = self.app._service_from_spec(spec, from_config=False) + self.db.commit() + return service + + def find_service(self, name: str) -> Optional[Service]: + """Get a service by name + return None if no such service + """ + orm_service = orm.Service.find(db=self.db, name=name) + if orm_service is not None: + service = self.services.get(name) + return service + default_handlers = [ (r"/api/services", ServiceListAPIHandler), diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 34217580..7aa044b7 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2292,7 +2292,7 @@ class JupyterHub(Application): if not self.authenticator.validate_username(name): raise ValueError("Token user name %r is not valid" % name) if kind == 'service': - if not any(service["name"] == name for service in self.services): + if not any(service_name == name for service_name in self._service_map): self.log.warning( f"service {name} not in services, creating implicitly. It is recommended to register services using services list." ) @@ -2355,13 +2355,43 @@ class JupyterHub(Application): ) pc.start() + def _service_from_orm( + self, + orm_service: orm.Service, + domain: str, + host: str, + ): + name = orm_service.name + service = Service( + parent=self, + app=self, + base_url=self.base_url, + db=self.db, + orm=orm_service, + roles=orm_service.roles, + domain=domain, + host=host, + hub=self.hub, + ) + traits = service.traits(input=True) + for key in traits: + value = getattr(orm_service, key, None) + if value is not None: + setattr(service, key, value) + + if orm_service.oauth_client is not None: + service.oauth_redirect_uri = orm_service.oauth_client.redirect_uri + self._service_map[name] = service + + return service + def _service_from_spec( self, spec: Dict, from_config=True, domain: Optional[str] = None, host: Optional[str] = None, - ) -> None: + ) -> Optional[Service]: if domain is None: if self.domain: domain = 'services.' + self.domain @@ -2393,7 +2423,13 @@ class JupyterHub(Application): "the Service admin flag will be ignored." ) roles.update_roles(self.db, entity=orm_service, roles=['admin']) + else: + # Do nothing if the config file tries to modify a API-base service + # or vice versa. + if orm_service.from_config != from_config: + return orm_service.admin = spec.get('admin', False) + self.db.commit() service = Service( parent=self, @@ -2475,6 +2511,8 @@ class JupyterHub(Application): self._service_map[name] = service + return service + def init_services(self): self._service_map.clear() if self.domain: @@ -2487,10 +2525,15 @@ class JupyterHub(Application): for spec in self.services: self._service_from_spec(spec, from_config=True, domain=domain, host=host) - # delete services from db not in service config: - for service in self.db.query(orm.Service): - if service.name not in self._service_map: - self.db.delete(service) + for service_orm in self.db.query(orm.Service): + if service_orm.from_config: + # delete config-based services from db + # that are not in current config file: + if service_orm.name not in self._service_map: + self.db.delete(service_orm) + else: + self._service_from_orm(service_orm, domain, host) + self.db.commit() async def check_services_health(self): @@ -2674,7 +2717,6 @@ class JupyterHub(Application): for user in self.users.values(): for spawner in user.spawners.values(): oauth_client_ids.add(spawner.oauth_client_id) - for i, oauth_client in enumerate(self.db.query(orm.OAuthClient)): if oauth_client.identifier not in oauth_client_ids: self.log.warning("Deleting OAuth client %s", oauth_client.identifier) diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 4bf7fcac..5a052978 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -12,7 +12,6 @@ import uuid import warnings from datetime import datetime, timedelta from http.client import responses -from typing import Optional from urllib.parse import parse_qs, parse_qsl, urlencode, urlparse, urlunparse from jinja2 import TemplateNotFound @@ -1308,18 +1307,6 @@ class BaseHandler(RequestHandler): ns.update(self.settings['template_vars']) return ns - def service_from_spec(self, spec) -> None: - """Create service from api request""" - - self.app._service_from_spec(spec, from_config=False) - - def find_service(self, name: str) -> Optional[orm.Service]: - """Get a service by name - return None if no such service - """ - orm_service = orm.Service.find(db=self.db, name=name) - return orm_service - def get_accessible_services(self, user): accessible_services = [] if user is None: diff --git a/jupyterhub/oauth/provider.py b/jupyterhub/oauth/provider.py index 9f45a07e..8ff4a97f 100644 --- a/jupyterhub/oauth/provider.py +++ b/jupyterhub/oauth/provider.py @@ -668,6 +668,15 @@ class JupyterHubOAuthServer(WebApplicationServer): self.db.commit() return orm_client + def remove_client(self, client_id): + """Remove a client by its id if it is existed.""" + orm_client = ( + self.db.query(orm.OAuthClient).filter_by(identifier=client_id).one_or_none() + ) + if orm_client is not None: + self.db.delete(orm_client) + self.db.commit() + def fetch_by_client_id(self, client_id): """Find a client by its id""" client = self.db.query(orm.OAuthClient).filter_by(identifier=client_id).first() diff --git a/jupyterhub/services/service.py b/jupyterhub/services/service.py index 2f6f8a27..3d551b17 100644 --- a/jupyterhub/services/service.py +++ b/jupyterhub/services/service.py @@ -371,6 +371,11 @@ class Service(LoggingConfigurable): else: return self.server.base_url + @property + def from_config(self): + """Is the service defined from config file?""" + return self.orm.from_config + def __repr__(self): return "<{cls}(name={name}{managed})>".format( cls=self.__class__.__name__, From e7defa6e1237a3f69830d4a56f7fb0144c7f4aaa Mon Sep 17 00:00:00 2001 From: Duc Trung LE Date: Sun, 5 Mar 2023 23:05:19 +0100 Subject: [PATCH 05/26] Update api handler --- jupyterhub/apihandlers/services.py | 57 ++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 68b5d388..90e7a62d 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -5,7 +5,7 @@ Currently GET-only, no actions can be taken to modify services. # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import json -from typing import Optional +from typing import Optional, Tuple from tornado import web @@ -36,7 +36,7 @@ class ServiceAPIHandler(APIHandler): @needs_scope('admin:services') async def post(self, service_name: str): data = self.get_json_body() - service = self.find_service(service_name) + service, _ = self.find_service(service_name) if service is not None: raise web.HTTPError(409, f"Service {service_name} already exists") @@ -65,7 +65,7 @@ class ServiceAPIHandler(APIHandler): @needs_scope('admin:services') async def delete(self, service_name: str): - service = self.find_service(service_name) + service, orm_service = self.find_service(service_name) if service is None: raise web.HTTPError(404, f"Service {service_name} does not exists") @@ -74,30 +74,46 @@ class ServiceAPIHandler(APIHandler): raise web.HTTPError( 405, f"Service {service_name} is not modifiable at runtime" ) + try: + await self.remove_service(service, orm_service) + self.services.pop(service_name) + except Exception: + msg = f"Failed to remove service {service_name}" + self.log.error(msg, exc_info=True) + raise web.HTTPError(400, msg) + + self.set_status(200) + async def remove_service(self, service: Service, orm_service: orm.Service) -> None: + if service.managed: await service.stop() if service.oauth_client: self.oauth_provider.remove_client(service.oauth_client_id) - orm_service = orm.Service.find(db=self.db, name=service_name) - if orm_service is not None: - if service.api_token is not None: - # Remove api token from database - orm_token = ( - self.db.query(orm.APIToken) - .filter_by(service_id=orm_service.id) - .first() - ) - if orm_token is not None: - self.db.delete(orm_token) + if service.api_token is not None: + # Remove api token from database + orm_token = ( + self.db.query(orm.APIToken) + .filter_by(service_id=orm_service.id) + .first() + ) + if orm_token is not None: + self.db.delete(orm_token) - self.services.pop(service_name) - self.db.delete(orm_service) + if orm_service._server_id is not None: + orm_server = ( + self.db.query(orm.Server) + .filter_by(id=orm_service._server_id) + .first() + ) + if orm_server is not None: + self.db.delete(orm_server) + self.db.delete(orm_service) self.db.commit() - self.set_status(200) + def service_from_spec(self, spec) -> Optional[Service]: """Create service from api request""" @@ -105,15 +121,18 @@ class ServiceAPIHandler(APIHandler): self.db.commit() return service - def find_service(self, name: str) -> Optional[Service]: + def find_service( + self, name: str + ) -> Tuple[Optional[Service], Optional[orm.Service]]: """Get a service by name return None if no such service """ orm_service = orm.Service.find(db=self.db, name=name) if orm_service is not None: service = self.services.get(name) - return service + return service, orm_service + return None, None default_handlers = [ (r"/api/services", ServiceListAPIHandler), From 71fbe5e29deae61ce0b888fe9b5962c9c706d48f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 5 Mar 2023 22:06:19 +0000 Subject: [PATCH 06/26] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- jupyterhub/apihandlers/services.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 90e7a62d..26461b7d 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -81,11 +81,10 @@ class ServiceAPIHandler(APIHandler): msg = f"Failed to remove service {service_name}" self.log.error(msg, exc_info=True) raise web.HTTPError(400, msg) - + self.set_status(200) async def remove_service(self, service: Service, orm_service: orm.Service) -> None: - if service.managed: await service.stop() @@ -95,25 +94,20 @@ class ServiceAPIHandler(APIHandler): if service.api_token is not None: # Remove api token from database orm_token = ( - self.db.query(orm.APIToken) - .filter_by(service_id=orm_service.id) - .first() + self.db.query(orm.APIToken).filter_by(service_id=orm_service.id).first() ) if orm_token is not None: self.db.delete(orm_token) if orm_service._server_id is not None: orm_server = ( - self.db.query(orm.Server) - .filter_by(id=orm_service._server_id) - .first() + self.db.query(orm.Server).filter_by(id=orm_service._server_id).first() ) if orm_server is not None: self.db.delete(orm_server) self.db.delete(orm_service) self.db.commit() - def service_from_spec(self, spec) -> Optional[Service]: """Create service from api request""" @@ -134,6 +128,7 @@ class ServiceAPIHandler(APIHandler): return None, None + default_handlers = [ (r"/api/services", ServiceListAPIHandler), (r"/api/services/([^/]+)", ServiceAPIHandler), From 5bb4b70ab119397706eddd0cb0977824e86979a1 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Mon, 6 Mar 2023 13:00:05 +0100 Subject: [PATCH 07/26] Handle add/remove services --- ...add_from_config_column_to_the_services_.py | 2 +- jupyterhub/apihandlers/services.py | 62 ++++++-- jupyterhub/app.py | 143 ++++++++++++------ 3 files changed, 140 insertions(+), 67 deletions(-) diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py index 4e5fe450..eb5a9f95 100644 --- a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -21,7 +21,7 @@ def upgrade(): if 'services' in tables: op.add_column( 'services', - sa.Column('from_config', sa.Boolean, nullable=False, default=True), + sa.Column('from_config', sa.Boolean, default=True), ) op.execute('UPDATE services SET from_config = true') diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 26461b7d..15f28a57 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -33,6 +33,41 @@ class ServiceAPIHandler(APIHandler): service = self.services[service_name] self.write(json.dumps(self.service_model(service))) + async def add_service(self, spec: dict) -> Service: + """Add a new service and related objects to the database + + Args: + spec (dict): The service specification + + Raises: + web.HTTPError: Raise if the service is not created + + Returns: + Service: Returns the service instance. + + """ + + self._check_service_model(spec) + service_name = spec["name"] + new_service = self.service_from_spec(spec) + + if new_service is None: + raise web.HTTPError(400, f"Failed to create service {service_name}") + if new_service.api_token: + # Add api token to database + await self.app._add_tokens( + {new_service.api_token: new_service.name}, kind='service' + ) + elif new_service.managed: + new_service.api_token = new_service.orm.new_api_token( + note='generated at runtime' + ) + if new_service.managed or new_service.url: + self.app.start_service(service_name, new_service) + self.app.toggle_service_health_check() + + return new_service + @needs_scope('admin:services') async def post(self, service_name: str): data = self.get_json_body() @@ -45,21 +80,7 @@ class ServiceAPIHandler(APIHandler): raise web.HTTPError(400, "Invalid service data") data['name'] = service_name - self._check_service_model(data) - new_service = self.service_from_spec(data) - - if new_service is None: - raise web.HTTPError(400, f'Failed to create service {service_name}') - if new_service.api_token: - # Add api token to database - await self.app._add_tokens( - {new_service.api_token: new_service.name}, kind='service' - ) - elif new_service.managed: - new_service.api_token = new_service.orm.new_api_token( - note='generated at runtime' - ) - + new_service = self.add_service(data) self.write(json.dumps(self.service_model(new_service))) self.set_status(201) @@ -85,8 +106,17 @@ class ServiceAPIHandler(APIHandler): self.set_status(200) async def remove_service(self, service: Service, orm_service: orm.Service) -> None: + """Remove a service and all related objects from the database. + + Args: + service (Service): the service object to be removed + + orm_service (orm.Service): The `orm.Service` object linked + with `service` + """ if service.managed: await service.stop() + self.app.toggle_service_health_check() if service.oauth_client: self.oauth_provider.remove_client(service.oauth_client_id) @@ -111,7 +141,7 @@ class ServiceAPIHandler(APIHandler): def service_from_spec(self, spec) -> Optional[Service]: """Create service from api request""" - service = self.app._service_from_spec(spec, from_config=False) + service = self.app.service_from_spec(spec, from_config=False) self.db.commit() return service diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 7aa044b7..586f6e28 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2355,7 +2355,7 @@ class JupyterHub(Application): ) pc.start() - def _service_from_orm( + def service_from_orm( self, orm_service: orm.Service, domain: str, @@ -2385,7 +2385,7 @@ class JupyterHub(Application): return service - def _service_from_spec( + def service_from_spec( self, spec: Dict, from_config=True, @@ -2523,7 +2523,7 @@ class JupyterHub(Application): domain = host = '' for spec in self.services: - self._service_from_spec(spec, from_config=True, domain=domain, host=host) + self.service_from_spec(spec, from_config=True, domain=domain, host=host) for service_orm in self.db.query(orm.Service): if service_orm.from_config: @@ -2532,7 +2532,7 @@ class JupyterHub(Application): if service_orm.name not in self._service_map: self.db.delete(service_orm) else: - self._service_from_orm(service_orm, domain, host) + self.service_from_orm(service_orm, domain, host) self.db.commit() @@ -3158,6 +3158,86 @@ class JupyterHub(Application): await self.proxy.check_routes(self.users, self._service_map, routes) + async def start_service( + self, + service_name: str, + service: Service, + ssl_context: Optional[ssl.SSLContext] = None, + ) -> bool: + """Start a managed service or poll for external service + + Args: + service_name (str): Name of the service. + service (Service): The service object. + + Returns: + boolean: Returns `True` if the service is started successfully, + returns `False` otherwise. + """ + if ssl_context is None: + ssl_context = make_ssl_context( + self.internal_ssl_key, + self.internal_ssl_cert, + cafile=self.internal_ssl_ca, + purpose=ssl.Purpose.CLIENT_AUTH, + ) + + msg = f'{service_name} at {service.url}' if service.url else service_name + if service.managed: + self.log.info("Starting managed service %s", msg) + try: + await service.start() + except Exception as e: + self.log.critical( + "Failed to start service %s", service_name, exc_info=True + ) + return False + else: + self.log.info("Adding external service %s", msg) + + if service.url: + tries = 10 if service.managed else 1 + for i in range(tries): + try: + await Server.from_orm(service.orm.server).wait_up( + http=True, timeout=1, ssl_context=ssl_context + ) + except AnyTimeoutError: + if service.managed: + status = await service.spawner.poll() + if status is not None: + self.log.error( + "Service %s exited with status %s", + service_name, + status, + ) + break + else: + break + else: + self.log.error( + "Cannot connect to %s service %s at %s. Is it running?", + service.kind, + service_name, + service.url, + ) + return True + + def toggle_service_health_check(self) -> None: + if self.service_check_interval and any( + s.url for s in self._service_map.values() + ): + if self._check_services_health_callback is None: + self._check_services_health_callback = PeriodicCallback( + self.check_services_health, 1e3 * self.service_check_interval + ) + self._check_services_health_callback.start() + else: + # Services requiring health check are removed, stop + # the periodic callback. + if self._check_services_health_callback is not None: + self._check_services_health_callback.stop() + async def start(self): """Start the whole thing""" self.io_loop = loop = IOLoop.current() @@ -3243,55 +3323,18 @@ class JupyterHub(Application): # start the service(s) for service_name, service in self._service_map.items(): - msg = f'{service_name} at {service.url}' if service.url else service_name - if service.managed: - self.log.info("Starting managed service %s", msg) - try: - await service.start() - except Exception as e: - self.log.critical( - "Failed to start service %s", service_name, exc_info=True - ) - self.exit(1) - else: - self.log.info("Adding external service %s", msg) - - if service.url: - tries = 10 if service.managed else 1 - for i in range(tries): - try: - await Server.from_orm(service.orm.server).wait_up( - http=True, timeout=1, ssl_context=ssl_context - ) - except AnyTimeoutError: - if service.managed: - status = await service.spawner.poll() - if status is not None: - self.log.error( - "Service %s exited with status %s", - service_name, - status, - ) - break - else: - break - else: - self.log.error( - "Cannot connect to %s service %s at %s. Is it running?", - service.kind, - service_name, - service.url, - ) + service_status = await self._start_service( + service_name, service, ssl_context + ) + if not service_status: + # Stop the application if a service failed to start. + self.exit(1) await self.proxy.check_routes(self.users, self._service_map) - if self.service_check_interval and any( - s.url for s in self._service_map.values() - ): - pc = PeriodicCallback( - self.check_services_health, 1e3 * self.service_check_interval - ) - pc.start() + # Check services health + self._check_services_health_callback = None + self.toggle_service_health_check() if self.last_activity_interval: pc = PeriodicCallback( From d251b705e83bd4dad258db49c0169834a927d900 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Mon, 6 Mar 2023 23:15:29 +0100 Subject: [PATCH 08/26] [WIP] Update old revisions to support new table --- ...add_from_config_column_to_the_services_.py | 2 +- .../versions/651f5419b74d_api_token_scopes.py | 27 ++++++++++++++++--- jupyterhub/orm.py | 2 +- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py index eb5a9f95..437ddcb7 100644 --- a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -21,7 +21,7 @@ def upgrade(): if 'services' in tables: op.add_column( 'services', - sa.Column('from_config', sa.Boolean, default=True), + sa.Column('from_config', sa.Boolean, nullable=True, default=True), ) op.execute('UPDATE services SET from_config = true') diff --git a/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py b/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py index 483a91c5..31aede31 100644 --- a/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py +++ b/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py @@ -13,16 +13,35 @@ depends_on = None import sqlalchemy as sa from alembic import op -from sqlalchemy import Column, ForeignKey, Table +from sqlalchemy import Column, ForeignKey, Table, text from sqlalchemy.orm import relationship from sqlalchemy.orm.session import Session -from jupyterhub import orm, roles, scopes +from jupyterhub import orm, roles + + +def access_scopes(oauth_client: orm.OAuthClient, db: Session): + """Return scope(s) required to access an oauth client + This is a clone of `scopes.access_scopes` without using + the `orm.Service` + """ + scopes = set() + if oauth_client.identifier == "jupyterhub": + return frozenset() + spawner = oauth_client.spawner + if spawner: + scopes.add(f"access:servers!server={spawner.user.name}/{spawner.name}") + else: + statement = f"SELECT * FROM services WHERE oauth_client_id = '{oauth_client.identifier}'" + service = db.execute(text(statement)).fetchall() + if len(service) > 0: + scopes.add(f"access:services!service={service.name}") + + return frozenset(scopes) def upgrade(): c = op.get_bind() - tables = sa.inspect(c.engine).get_table_names() # oauth codes are short lived, no need to upgrade them @@ -100,7 +119,7 @@ def upgrade(): db = Session(bind=c) for oauth_client in db.query(orm.OAuthClient): allowed_scopes = set(roles.roles_to_scopes(oauth_client.allowed_roles)) - allowed_scopes.update(scopes.access_scopes(oauth_client)) + allowed_scopes.update(access_scopes(oauth_client, db)) oauth_client.allowed_scopes = sorted(allowed_scopes) db.commit() # drop token-role relationship diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index 17553c29..b591555d 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -417,7 +417,7 @@ class Service(Base): ), ) - from_config = Column(Boolean, default=True) + from_config = Column(Boolean, default=True, nullable=True) oauth_client = relationship( 'OAuthClient', From 95781880c55778c501d8b2d254a3744fd2cd1446 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Wed, 8 Mar 2023 17:24:36 +0100 Subject: [PATCH 09/26] Update service table schema --- ...add_from_config_column_to_the_services_.py | 21 ++++++++ jupyterhub/apihandlers/services.py | 2 +- jupyterhub/app.py | 48 +++++++++++++++++-- jupyterhub/orm.py | 20 ++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py index 437ddcb7..558818b0 100644 --- a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -14,6 +14,20 @@ depends_on = None import sqlalchemy as sa from alembic import op +from jupyterhub.orm import JSONDict, JSONList + +COL_DATA = [ + {'name': 'url', 'type': sa.Unicode(length=2047)}, + {'name': 'oauth_client_allowed_scopes', 'type': JSONDict()}, + {'name': 'info', 'type': JSONDict()}, + {'name': 'display', 'type': sa.Boolean}, + {'name': 'oauth_no_confirm', 'type': sa.Boolean}, + {'name': 'command', 'type': JSONList()}, + {'name': 'cwd', 'type': sa.Unicode(length=2047)}, + {'name': 'environment', 'type': JSONDict()}, + {'name': 'user', 'type': sa.Unicode(255)}, +] + def upgrade(): engine = op.get_bind().engine @@ -24,7 +38,14 @@ def upgrade(): sa.Column('from_config', sa.Boolean, nullable=True, default=True), ) op.execute('UPDATE services SET from_config = true') + for item in COL_DATA: + op.add_column( + 'services', + sa.Column(item['name'], item['type'], nullable=True), + ) def downgrade(): op.drop_column('services', sa.Column('from_config')) + for item in COL_DATA: + op.drop_column('services', sa.Column(item['name'])) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 15f28a57..7257fac8 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -80,7 +80,7 @@ class ServiceAPIHandler(APIHandler): raise web.HTTPError(400, "Invalid service data") data['name'] = service_name - new_service = self.add_service(data) + new_service = await self.add_service(data) self.write(json.dumps(self.service_model(new_service))) self.set_status(201) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 586f6e28..45c8cc3a 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -3238,6 +3238,48 @@ class JupyterHub(Application): if self._check_services_health_callback is not None: self._check_services_health_callback.stop() + async def _start_service(self, service_name, service, ssl_context): + for service_name, service in self._service_map.items(): + msg = f'{service_name} at {service.url}' if service.url else service_name + if service.managed: + self.log.info("Starting managed service %s", msg) + try: + await service.start() + except Exception as e: + self.log.critical( + "Failed to start service %s", service_name, exc_info=True + ) + self.exit(1) + else: + self.log.info("Adding external service %s", msg) + + if service.url: + tries = 10 if service.managed else 1 + for i in range(tries): + try: + await Server.from_orm(service.orm.server).wait_up( + http=True, timeout=1, ssl_context=ssl_context + ) + except AnyTimeoutError: + if service.managed: + status = await service.spawner.poll() + if status is not None: + self.log.error( + "Service %s exited with status %s", + service_name, + status, + ) + break + else: + break + else: + self.log.error( + "Cannot connect to %s service %s at %s. Is it running?", + service.kind, + service_name, + service.url, + ) + async def start(self): """Start the whole thing""" self.io_loop = loop = IOLoop.current() @@ -3326,9 +3368,9 @@ class JupyterHub(Application): service_status = await self._start_service( service_name, service, ssl_context ) - if not service_status: - # Stop the application if a service failed to start. - self.exit(1) + # if not service_status: + # # Stop the application if a service failed to start. + # self.exit(1) await self.proxy.check_routes(self.users, self._service_map) diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index b591555d..a469a4d4 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -394,6 +394,26 @@ class Service(Base): 'Role', secondary='service_role_map', back_populates='services' ) + url = Column(Unicode(2047), nullable=True) + + oauth_client_allowed_scopes = Column( + JSONList, nullable=True, default=[] + ) # List of string + + info = Column(JSONDict, nullable=True, default={}) # Dict + + display = Column(Boolean, default=True, nullable=True) + + oauth_no_confirm = Column(Boolean, default=False, nullable=True) + + command = Column(JSONList, nullable=True, default=[]) # List of string + + cwd = Column(Unicode, nullable=True) + + environment = Column(JSONDict, nullable=True, default={}) # Dict + + user = Column(Unicode, nullable=True) + api_tokens = relationship( "APIToken", back_populates="service", cascade="all, delete-orphan" ) From bf565ece3be099c92db8ee56cffbf2b5edda368b Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Thu, 9 Mar 2023 13:15:53 +0100 Subject: [PATCH 10/26] Update service table schema --- ...add_from_config_column_to_the_services_.py | 2 +- .../versions/651f5419b74d_api_token_scopes.py | 8 ++- jupyterhub/apihandlers/services.py | 18 +++++- jupyterhub/app.py | 62 +++++-------------- jupyterhub/orm.py | 59 ++++++++++++++---- 5 files changed, 85 insertions(+), 64 deletions(-) diff --git a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py index 558818b0..26b832a3 100644 --- a/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py +++ b/jupyterhub/alembic/versions/3c2384c5aae1_add_from_config_column_to_the_services_.py @@ -35,7 +35,7 @@ def upgrade(): if 'services' in tables: op.add_column( 'services', - sa.Column('from_config', sa.Boolean, nullable=True, default=True), + sa.Column('from_config', sa.Boolean, default=True), ) op.execute('UPDATE services SET from_config = true') for item in COL_DATA: diff --git a/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py b/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py index 31aede31..6be31b5f 100644 --- a/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py +++ b/jupyterhub/alembic/versions/651f5419b74d_api_token_scopes.py @@ -32,10 +32,12 @@ def access_scopes(oauth_client: orm.OAuthClient, db: Session): if spawner: scopes.add(f"access:servers!server={spawner.user.name}/{spawner.name}") else: - statement = f"SELECT * FROM services WHERE oauth_client_id = '{oauth_client.identifier}'" - service = db.execute(text(statement)).fetchall() + statement = "SELECT * FROM services WHERE oauth_client_id = :identifier" + service = db.execute( + text(statement), {"identifier": oauth_client.identifier} + ).fetchall() if len(service) > 0: - scopes.add(f"access:services!service={service.name}") + scopes.add(f"access:services!service={service[0].name}") return frozenset(scopes) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 7257fac8..82db46bc 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -63,9 +63,21 @@ class ServiceAPIHandler(APIHandler): note='generated at runtime' ) if new_service.managed or new_service.url: - self.app.start_service(service_name, new_service) + service_status = self.app.start_service(service_name, new_service) + if not service_status: + self.log.error( + 'Failed to start service %s', + service_name, + exc_info=True, + ) self.app.toggle_service_health_check() + if new_service.oauth_no_confirm: + oauth_no_confirm_list = self.settings.get('oauth_no_confirm_list') + msg = f"Allowing service {new_service.name} to complete OAuth without confirmation on an authorization web page" + self.log.warning(msg) + oauth_no_confirm_list.add(new_service.oauth_client_id) + return new_service @needs_scope('admin:services') @@ -136,6 +148,10 @@ class ServiceAPIHandler(APIHandler): if orm_server is not None: self.db.delete(orm_server) + if service.oauth_no_confirm: + oauth_no_confirm_list = self.settings.get('oauth_no_confirm_list') + oauth_no_confirm_list.remove(service.oauth_client_id) + self.db.delete(orm_service) self.db.commit() diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 45c8cc3a..b61686ca 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2375,7 +2375,7 @@ class JupyterHub(Application): ) traits = service.traits(input=True) for key in traits: - value = getattr(orm_service, key, None) + value = orm_service.get_column(key) if value is not None: setattr(service, key, value) @@ -2448,6 +2448,7 @@ class JupyterHub(Application): if key not in traits: raise AttributeError("No such service field: %s" % key) setattr(service, key, value) + orm_service.update_column(key, value) if service.api_token: self.service_tokens[service.api_token] = service.name @@ -3238,48 +3239,6 @@ class JupyterHub(Application): if self._check_services_health_callback is not None: self._check_services_health_callback.stop() - async def _start_service(self, service_name, service, ssl_context): - for service_name, service in self._service_map.items(): - msg = f'{service_name} at {service.url}' if service.url else service_name - if service.managed: - self.log.info("Starting managed service %s", msg) - try: - await service.start() - except Exception as e: - self.log.critical( - "Failed to start service %s", service_name, exc_info=True - ) - self.exit(1) - else: - self.log.info("Adding external service %s", msg) - - if service.url: - tries = 10 if service.managed else 1 - for i in range(tries): - try: - await Server.from_orm(service.orm.server).wait_up( - http=True, timeout=1, ssl_context=ssl_context - ) - except AnyTimeoutError: - if service.managed: - status = await service.spawner.poll() - if status is not None: - self.log.error( - "Service %s exited with status %s", - service_name, - status, - ) - break - else: - break - else: - self.log.error( - "Cannot connect to %s service %s at %s. Is it running?", - service.kind, - service_name, - service.url, - ) - async def start(self): """Start the whole thing""" self.io_loop = loop = IOLoop.current() @@ -3365,12 +3324,21 @@ class JupyterHub(Application): # start the service(s) for service_name, service in self._service_map.items(): - service_status = await self._start_service( + service_status = await self.start_service( service_name, service, ssl_context ) - # if not service_status: - # # Stop the application if a service failed to start. - # self.exit(1) + if not service_status: + if service.from_config: + # Stop the application if a config-based service failed to start. + self.exit(1) + else: + # Only warn for database-based service, so that admin can connect + # to hub to remove the service. + self.log.error( + "Failed to start database-based service %s", + service_name, + exc_info=True, + ) await self.proxy.check_routes(self.users, self._service_map) diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index a469a4d4..563f847e 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -396,23 +396,23 @@ class Service(Base): url = Column(Unicode(2047), nullable=True) - oauth_client_allowed_scopes = Column( - JSONList, nullable=True, default=[] - ) # List of string + oauth_client_allowed_scopes = Column(JSONList, nullable=True) - info = Column(JSONDict, nullable=True, default={}) # Dict + info = Column(JSONDict, nullable=True) - display = Column(Boolean, default=True, nullable=True) + display = Column(Boolean, nullable=True) - oauth_no_confirm = Column(Boolean, default=False, nullable=True) + oauth_no_confirm = Column(Boolean, nullable=True) - command = Column(JSONList, nullable=True, default=[]) # List of string + command = Column(JSONList, nullable=True) - cwd = Column(Unicode, nullable=True) + cwd = Column(Unicode(4095), nullable=True) - environment = Column(JSONDict, nullable=True, default={}) # Dict + environment = Column(JSONDict, nullable=True) - user = Column(Unicode, nullable=True) + user = Column(Unicode(255), nullable=True) + + from_config = Column(Boolean, default=True) api_tokens = relationship( "APIToken", back_populates="service", cascade="all, delete-orphan" @@ -437,8 +437,6 @@ class Service(Base): ), ) - from_config = Column(Boolean, default=True, nullable=True) - oauth_client = relationship( 'OAuthClient', back_populates="service", @@ -460,6 +458,43 @@ class Service(Base): """ return db.query(cls).filter(cls.name == name).first() + def update_column(self, column_name: str, value: any) -> bool: + """_summary_ + + Args: + column_name (str): _description_ + value (any): _description_ + + Returns: + bool: _description_ + """ + if ( + hasattr(self, column_name) + and not getattr(Service, column_name).foreign_keys + ): + setattr(self, column_name, value) + return True + else: + return False + + def get_column(self, column_name: str): + """_summary_ + + Args: + column_name (str): _description_ + value (any): _description_ + + Returns: + bool: _description_ + """ + if ( + hasattr(self, column_name) + and not getattr(Service, column_name).foreign_keys + ): + return getattr(self, column_name) + else: + return None + class Expiring: """Mixin for expiring entries From bdcf697fe9fa56f77abf1e278fa5722e643db3fe Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Thu, 9 Mar 2023 22:58:13 +0100 Subject: [PATCH 11/26] Add tests --- ci/init-db.sh | 2 +- jupyterhub/apihandlers/services.py | 10 +- jupyterhub/orm.py | 49 ++++++---- jupyterhub/tests/test_api.py | 152 ++++++++++++++++++++++++++++- jupyterhub/tests/test_db.py | 4 +- jupyterhub/tests/test_orm.py | 27 +++++ jupyterhub/tests/test_roles.py | 10 ++ 7 files changed, 232 insertions(+), 22 deletions(-) diff --git a/ci/init-db.sh b/ci/init-db.sh index a26c30ab..0d3d3223 100755 --- a/ci/init-db.sh +++ b/ci/init-db.sh @@ -21,7 +21,7 @@ fi # Configure a set of databases in the database server for upgrade tests # this list must be in sync with versions in test_db.py:test_upgrade set -x -for SUFFIX in '' _upgrade_110 _upgrade_122 _upgrade_130 _upgrade_150 _upgrade_211; do +for SUFFIX in '' _upgrade_110 _upgrade_122 _upgrade_130 _upgrade_150 _upgrade_211 _upgrade_311; do $SQL_CLIENT "DROP DATABASE jupyterhub${SUFFIX};" 2>/dev/null || true $SQL_CLIENT "CREATE DATABASE jupyterhub${SUFFIX} ${EXTRA_CREATE_DATABASE_ARGS:-};" done diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 82db46bc..ce835102 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -49,10 +49,16 @@ class ServiceAPIHandler(APIHandler): self._check_service_model(spec) service_name = spec["name"] - new_service = self.service_from_spec(spec) + try: + new_service = self.service_from_spec(spec) + except Exception: + msg = f"Failed to create service {service_name}" + self.log.error(msg, exc_info=True) + raise web.HTTPError(400, msg) if new_service is None: raise web.HTTPError(400, f"Failed to create service {service_name}") + if new_service.api_token: # Add api token to database await self.app._add_tokens( @@ -150,7 +156,7 @@ class ServiceAPIHandler(APIHandler): if service.oauth_no_confirm: oauth_no_confirm_list = self.settings.get('oauth_no_confirm_list') - oauth_no_confirm_list.remove(service.oauth_client_id) + oauth_no_confirm_list.discard(service.oauth_client_id) self.db.delete(orm_service) self.db.commit() diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index 563f847e..43d2d077 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -458,39 +458,54 @@ class Service(Base): """ return db.query(cls).filter(cls.name == name).first() - def update_column(self, column_name: str, value: any) -> bool: - """_summary_ + def _check_data_only_column(self, column_name: str) -> bool: + """Check if a column is not a ForeignKey or in a + relationship. Args: - column_name (str): _description_ - value (any): _description_ + column_name (str): ame of the column Returns: - bool: _description_ + bool: returns `True` if the column is data-only, + returns `False` otherwise. """ - if ( - hasattr(self, column_name) - and not getattr(Service, column_name).foreign_keys - ): + if hasattr(self, column_name): + column = getattr(Service, column_name) + if hasattr(column, 'foreign_keys'): + return len(column.foreign_keys) == 0 + else: + return False + else: + return False + + def update_column(self, column_name: str, value: any) -> bool: + """Update the value of a non-ForeignKey column. + + Args: + column_name (str): Name of the column + value (any): New value + + Returns: + bool: returns `True` if the column is updated, + returns `False` otherwise. + """ + if self._check_data_only_column(column_name): setattr(self, column_name, value) return True else: return False def get_column(self, column_name: str): - """_summary_ + """Get non-ForeignKey, non-relationship column Args: - column_name (str): _description_ - value (any): _description_ + column_name (str): Name of the column Returns: - bool: _description_ + The value of requested column, None if it is + a foreign key or it does not exist. """ - if ( - hasattr(self, column_name) - and not getattr(Service, column_name).foreign_keys - ): + if self._check_data_only_column(column_name): return getattr(self, column_name) else: return None diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index b7002e79..724ad89b 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -8,12 +8,13 @@ from datetime import datetime, timedelta from unittest import mock from urllib.parse import quote, urlparse +import pytest from pytest import fixture, mark from tornado.httputil import url_concat import jupyterhub -from .. import orm +from .. import orm, roles from ..apihandlers.base import PAGINATION_MEDIA_TYPE from ..objects import Server from ..utils import url_path_join as ujoin @@ -2091,6 +2092,155 @@ async def test_get_service(app, mockservice_url): assert r.status_code == 403 +@pytest.fixture +def service_data(): + return { + "oauth_client_id": "service-oauth-client-from-api", + "api_token": "api_token-from-api", + "oauth_redirect_uri": "http://127.0.0.1:5555/oauth_callback-from-api", + "oauth_no_confirm": True, + "info": {'foo': 'bar'}, + } + + +@pytest.fixture +def service_admin_user(app): + user_name = 'admin_services' + service_role = { + 'name': 'admin-services-role', + 'description': '', + 'users': [user_name], + 'scopes': ['admin:services'], + } + roles.create_role(app.db, service_role) + user = add_user(app.db, name=user_name) + roles.update_roles(app.db, user, roles=['admin-services-role']) + return user + + +@mark.services +async def test_create_service(app, service_admin_user, service_data): + db = app.db + service_name = 'service-from-api' + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(service_data), + method='post', + ) + + r.raise_for_status() + assert r.status_code == 201 + assert r.json()['name'] == service_name + orm_service = orm.Service.find(db, service_name) + assert orm_service is not None + + oath_client = ( + db.query(orm.OAuthClient) + .filter_by(identifier=service_data['oauth_client_id']) + .first() + ) + assert oath_client.redirect_uri == service_data['oauth_redirect_uri'] + + assert service_name in app._service_map + assert ( + app._service_map[service_name].oauth_no_confirm + == service_data['oauth_no_confirm'] + ) + + +@mark.services +async def test_create_service_no_role(app, service_data): + db = app.db + service_name = 'service-from-api' + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, 'user'), + data=json.dumps(service_data), + method='post', + ) + + assert r.status_code == 403 + + +@mark.services +async def test_create_service_conflict(app, service_admin_user, service_data): + db = app.db + service_name = 'service-from-config' + app.services = [{'name': service_name}] + app.init_services() + + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(service_data), + method='post', + ) + + assert r.status_code == 409 + + +@mark.services +async def test_create_service_duplication(app, service_admin_user, service_data): + db = app.db + service_name = 'service-from-api' + + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(service_data), + method='post', + ) + assert r.status_code == 409 + + +@mark.services +async def test_remove_service(app, service_admin_user, service_data): + db = app.db + service_name = 'service-from-api' + + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + method='delete', + ) + assert r.status_code == 200 + + orm_service = orm.Service.find(db, service_name) + assert orm_service is None + + oath_client = ( + db.query(orm.OAuthClient) + .filter_by(identifier=service_data['oauth_client_id']) + .first() + ) + assert oath_client is None + + assert service_name not in app._service_map + + +@mark.services +async def test_remove_service_from_config(app, service_admin_user): + db = app.db + service_name = 'service-from-config' + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + method='delete', + ) + assert r.status_code == 405 + assert ( + r.json()['message'] + == 'Service service-from-config is not modifiable at runtime' + ) + + async def test_root_api(app): base_url = app.hub.url url = ujoin(base_url, 'api') diff --git a/jupyterhub/tests/test_db.py b/jupyterhub/tests/test_db.py index f8c379bc..d9941ea1 100644 --- a/jupyterhub/tests/test_db.py +++ b/jupyterhub/tests/test_db.py @@ -44,7 +44,9 @@ def generate_old_db(env_dir, hub_version, db_url): # changes to this version list must also be reflected # in ci/init-db.sh -@pytest.mark.parametrize('hub_version', ["1.1.0", "1.2.2", "1.3.0", "1.5.0", "2.1.1"]) +@pytest.mark.parametrize( + 'hub_version', ['1.1.0', '1.2.2', '1.3.0', '1.5.0', '2.1.1', '3.1.1'] +) async def test_upgrade(tmpdir, hub_version): db_url = os.getenv('JUPYTERHUB_TEST_DB_URL') if db_url: diff --git a/jupyterhub/tests/test_orm.py b/jupyterhub/tests/test_orm.py index 74deff3b..0d00c5ea 100644 --- a/jupyterhub/tests/test_orm.py +++ b/jupyterhub/tests/test_orm.py @@ -145,6 +145,33 @@ def test_token_expiry(db): assert orm_token not in user.api_tokens +def test_service_check_data_only_column(db): + orm_service = orm.Service(name='check_data_only_column', from_config=True) + db.add(orm_service) + db.commit() + assert orm_service._check_data_only_column('from_config') + assert not orm_service._check_data_only_column('server') + assert not orm_service._check_data_only_column('oauth_client_id') + + +def test_service_get_column(db): + orm_service = orm.Service(name='test_service_get_column', from_config=True) + db.add(orm_service) + db.commit() + orm_service.server = orm.Server() + assert orm_service.get_column('from_config') + assert orm_service.get_column('server') is None + + +def test_service_update_column(db): + orm_service = orm.Service(name='test_service_set_column', from_config=True) + db.add(orm_service) + db.commit() + + assert orm_service.update_column('from_config', False) + assert not orm_service.update_column('server', orm.Server()) + + def test_service_tokens(db): service = orm.Service(name='secret') db.add(service) diff --git a/jupyterhub/tests/test_roles.py b/jupyterhub/tests/test_roles.py index 7bd90d63..f5e905fc 100644 --- a/jupyterhub/tests/test_roles.py +++ b/jupyterhub/tests/test_roles.py @@ -236,6 +236,16 @@ def test_orm_roles_delete_cascade(db): ['tokens!group=hobbits'], {'tokens!group=hobbits', 'read:tokens!group=hobbits'}, ), + ( + ['admin:services'], + { + 'read:roles:services', + 'read:services:name', + 'admin:services', + 'list:services', + 'read:services', + }, + ), ], ) def test_get_expanded_scopes(db, scopes, expected_scopes): From 5870bedb3e7b484941fa77e04e8d54cd3edb87a1 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Fri, 10 Mar 2023 13:33:24 +0100 Subject: [PATCH 12/26] More tests --- jupyterhub/app.py | 3 + jupyterhub/tests/conftest.py | 11 ++++ jupyterhub/tests/test_api.py | 27 +-------- jupyterhub/tests/test_app.py | 107 ++++++++++++++++++++++++++++++++++- jupyterhub/tests/test_orm.py | 85 ++++++++++++++++++++++++++-- 5 files changed, 204 insertions(+), 29 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index b61686ca..acb6ae84 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2381,6 +2381,9 @@ class JupyterHub(Application): if orm_service.oauth_client is not None: service.oauth_redirect_uri = orm_service.oauth_client.redirect_uri + service.oauth_client_id = orm_service.oauth_client.identifier + service.oauth_redirect_uri = orm_service.oauth_client.redirect_uri + self._service_map[name] = service return service diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index e9b2260e..9930fbf6 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -534,3 +534,14 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config): queries = db_counts[nodeid] if queries: terminalreporter.line(f"{queries:<6} {nodeid}") + +@fixture +def service_data(): + """Data used to create service at runtime""" + return { + "oauth_client_id": "service-oauth-client-from-api", + "api_token": "api_token-from-api", + "oauth_redirect_uri": "http://127.0.0.1:5555/oauth_callback-from-api", + "oauth_no_confirm": True, + "info": {'foo': 'bar'}, + } diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 724ad89b..00ef6112 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -14,7 +14,7 @@ from tornado.httputil import url_concat import jupyterhub -from .. import orm, roles +from .. import orm from ..apihandlers.base import PAGINATION_MEDIA_TYPE from ..objects import Server from ..utils import url_path_join as ujoin @@ -2093,29 +2093,8 @@ async def test_get_service(app, mockservice_url): @pytest.fixture -def service_data(): - return { - "oauth_client_id": "service-oauth-client-from-api", - "api_token": "api_token-from-api", - "oauth_redirect_uri": "http://127.0.0.1:5555/oauth_callback-from-api", - "oauth_no_confirm": True, - "info": {'foo': 'bar'}, - } - - -@pytest.fixture -def service_admin_user(app): - user_name = 'admin_services' - service_role = { - 'name': 'admin-services-role', - 'description': '', - 'users': [user_name], - 'scopes': ['admin:services'], - } - roles.create_role(app.db, service_role) - user = add_user(app.db, name=user_name) - roles.update_roles(app.db, user, roles=['admin-services-role']) - return user +def service_admin_user(create_user_with_scopes): + return create_user_with_scopes('admin:services') @mark.services diff --git a/jupyterhub/tests/test_app.py b/jupyterhub/tests/test_app.py index f1967dce..d691f2d7 100644 --- a/jupyterhub/tests/test_app.py +++ b/jupyterhub/tests/test_app.py @@ -15,10 +15,11 @@ import pytest import traitlets from traitlets.config import Config -from .. import orm +from .. import orm, roles from ..app import COOKIE_SECRET_BYTES, JupyterHub from .mocking import MockHub from .test_api import add_user +from .utils import api_request, auth_header def test_help_all(): @@ -473,3 +474,107 @@ async def test_user_creation(tmpdir, request): "in-group", "in-role", } + +@pytest.fixture(scope='module') +def db_temp_path(tmp_path_factory): + fn = tmp_path_factory.mktemp("db") / "jupyterhub.sqlite" + return fn + + +async def test_add_service_at_runtime(request, db_temp_path, service_data): + if not os.getenv('JUPYTERHUB_TEST_DB_URL'): + p = patch.dict( + os.environ, + {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)}, + ) + p.start() + request.addfinalizer(p.stop) + + kwargs = {"test_clean_db": False} + ssl_enabled = getattr(request.module, "ssl_enabled", False) + if ssl_enabled: + kwargs['internal_certs_location'] = db_temp_path.parents[0] + app = MockHub(**kwargs) + + def end(): + app.log.handlers = [] + MockHub.clear_instance() + try: + app.stop() + except Exception as e: + print("Error stopping Hub: %s" % e, file=sys.stderr) + + request.addfinalizer(end) + + await app.initialize([]) + await app.start() + db = app.db + + user_name = 'admin_services' + service_role = { + 'name': 'admin-services-role', + 'description': '', + 'users': [user_name], + 'scopes': ['admin:services'], + } + roles.create_role(app.db, service_role) + user = add_user(app.db, name=user_name) + roles.update_roles(app.db, user, roles=['admin-services-role']) + + service_name = 'service-from-api' + + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, user_name), + data=json.dumps(service_data), + method='post', + ) + assert r.status_code == 201 + assert r.json()['name'] == service_name + oauth_client = ( + app.db.query(orm.OAuthClient) + .filter_by(identifier=service_data['oauth_client_id']) + .first() + ) + assert oauth_client.redirect_uri == service_data['oauth_redirect_uri'] + + +async def test_recreate_service_from_database(request, db_temp_path, service_data): + if not os.getenv('JUPYTERHUB_TEST_DB_URL'): + p = patch.dict( + os.environ, + {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)}, + ) + p.start() + request.addfinalizer(p.stop) + kwargs = {"test_clean_db": False} + + ssl_enabled = getattr(request.module, "ssl_enabled", False) + if ssl_enabled: + kwargs['internal_certs_location'] = db_temp_path.parents[0] + app = MockHub(**kwargs) + + def end(): + app.log.handlers = [] + MockHub.clear_instance() + try: + app.stop() + except Exception as e: + print("Error stopping Hub: %s" % e, file=sys.stderr) + + request.addfinalizer(end) + + await app.initialize([]) + await app.start() + + assert 'service-from-api' in app._service_map + assert ( + service_data['oauth_client_id'] in app.tornado_settings['oauth_no_confirm_list'] + ) + oauth_client = ( + app.db.query(orm.OAuthClient) + .filter_by(identifier=service_data['oauth_client_id']) + .first() + ) + assert oauth_client.redirect_uri == service_data['oauth_redirect_uri'] diff --git a/jupyterhub/tests/test_orm.py b/jupyterhub/tests/test_orm.py index 0d00c5ea..afa35999 100644 --- a/jupyterhub/tests/test_orm.py +++ b/jupyterhub/tests/test_orm.py @@ -145,13 +145,90 @@ def test_token_expiry(db): assert orm_token not in user.api_tokens -def test_service_check_data_only_column(db): +@pytest.mark.parametrize( + 'column, expected', + [ + ( + 'name', + True, + ), + ( + 'admin', + True, + ), + ( + 'url', + True, + ), + ( + 'oauth_client_allowed_scopes', + True, + ), + ( + 'info', + True, + ), + ( + 'display', + True, + ), + ( + 'oauth_no_confirm', + True, + ), + ( + 'command', + True, + ), + ( + 'cwd', + True, + ), + ( + 'environment', + True, + ), + ( + 'user', + True, + ), + ( + 'from_config', + True, + ), + ( + 'api_tokens', + False, + ), + ( + '_server_id', + False, + ), + ( + 'server', + False, + ), + ( + 'pid', + True, + ), + ( + 'oauth_client_id', + False, + ), + ( + 'oauth_client', + False, + ), + ], +) +def test_service_check_data_only_column(db, column, expected): orm_service = orm.Service(name='check_data_only_column', from_config=True) db.add(orm_service) db.commit() - assert orm_service._check_data_only_column('from_config') - assert not orm_service._check_data_only_column('server') - assert not orm_service._check_data_only_column('oauth_client_id') + assert orm_service._check_data_only_column(column) == expected + db.delete(orm_service) + db.commit() def test_service_get_column(db): From 8cef59bdd7bffa67e92c192603024797269e2737 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Fri, 10 Mar 2023 23:37:09 +0100 Subject: [PATCH 13/26] Add documentation --- docs/source/howto/rest.md | 1 + docs/source/reference/services.md | 41 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/docs/source/howto/rest.md b/docs/source/howto/rest.md index 886acdc5..503b4ba2 100644 --- a/docs/source/howto/rest.md +++ b/docs/source/howto/rest.md @@ -24,6 +24,7 @@ such as: - Checking which users are active - Adding or removing users +- Adding or removing services - Stopping or starting single user notebook servers - Authenticating services - Communicating with an individual Jupyter server's REST API diff --git a/docs/source/reference/services.md b/docs/source/reference/services.md index 81ffaf1d..c6e2d5ef 100644 --- a/docs/source/reference/services.md +++ b/docs/source/reference/services.md @@ -174,6 +174,47 @@ c.JupyterHub.services = [ In this case, the `url` field will be passed along to the Service as `JUPYTERHUB_SERVICE_URL`. +## Adding or removing services at runtime + +Both Hub-Managed services and Externally-Managed services can be added at runtime by using the JupyterHub’s REST API. + +### Add a new service + +To add a new service, send a POST request to this endpoint + +``` +POST /hub/api/services/:servicename +``` + +**Required scope: `admin:services`** + +**Payload**: The payload should contain the definition of the service to be created. The endpoint supports the same properties as services defined in the config file. + +**Possible responses** + +- `201 Created`: The service and related objects are created (and started in case of a Hub-managed one) successfully. +- `400 Bad Request`: The payload is invalid or JupyterHub can not create the service. +- `409 Conflict`: The service with the same name already exist. + +### Remove a new service + +To remove a new service, send a DELETE request to this endpoint + +``` +DELETE /hub/api/services/:servicename +``` + +**Required scope: `admin:services`** + +**Payload**: `None` + +**Possible responses** + +- `200 OK`: The service and related objects are removed (and stopped in case of a Hub-managed one) successfully. +- `400 Bad Request`: JupyterHub can not remove the service. +- `404 Not Found`: The requested service does not exist. +- `405 Not Allowed`: The requested service is created from the config file, it can not be removed at runtime. + ## Writing your own Services When writing your own services, you have a few decisions to make (in addition From 9a0d00fd69a8bbdf8c474e75c72d516770808945 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Mon, 13 Mar 2023 09:40:17 +0100 Subject: [PATCH 14/26] Typo --- docs/source/reference/services.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/source/reference/services.md b/docs/source/reference/services.md index c6e2d5ef..b707d604 100644 --- a/docs/source/reference/services.md +++ b/docs/source/reference/services.md @@ -176,7 +176,7 @@ In this case, the `url` field will be passed along to the Service as ## Adding or removing services at runtime -Both Hub-Managed services and Externally-Managed services can be added at runtime by using the JupyterHub’s REST API. +Both Hub-Managed services and Externally-Managed services can be added at runtime by using JupyterHub’s REST API. ### Add a new service @@ -194,11 +194,11 @@ POST /hub/api/services/:servicename - `201 Created`: The service and related objects are created (and started in case of a Hub-managed one) successfully. - `400 Bad Request`: The payload is invalid or JupyterHub can not create the service. -- `409 Conflict`: The service with the same name already exist. +- `409 Conflict`: The service with the same name already exists. -### Remove a new service +### Remove an existing service -To remove a new service, send a DELETE request to this endpoint +To remove an existing service, send a DELETE request to this endpoint ``` DELETE /hub/api/services/:servicename From 33e6c0de231a27abae5b77c2afbf152bd3270c38 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Wed, 15 Mar 2023 09:45:21 +0100 Subject: [PATCH 15/26] Add docstring --- jupyterhub/app.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index acb6ae84..78a20142 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2360,7 +2360,19 @@ class JupyterHub(Application): orm_service: orm.Service, domain: str, host: str, - ): + ) -> Service: + """Create the service instance and related objects from + ORM data. + + Args: + orm_service (orm.Service): The `orm.Service` object + domain (str): The domain of Hub session + host (str): The host of Hub session + + Returns: + Service: the created service + """ + name = orm_service.name service = Service( parent=self, @@ -2395,6 +2407,21 @@ class JupyterHub(Application): domain: Optional[str] = None, host: Optional[str] = None, ) -> Optional[Service]: + """Create the service instance and related objects from + config data. + + Args: + spec (Dict): The spec of service, defined in the config file. + from_config (bool, optional): `True` if the service will be created + from the config file, `False` if it is created from REST API. + Defaults to `True`. + domain (Optional[str]): The domain of Hub session. Defaults to None. + host (Optional[str]): The host of Hub session. Defaults to None. + + Returns: + Optional[Service]: The created service + """ + if domain is None: if self.domain: domain = 'services.' + self.domain @@ -3228,6 +3255,8 @@ class JupyterHub(Application): return True def toggle_service_health_check(self) -> None: + """Start or stop the service health check callback.""" + if self.service_check_interval and any( s.url for s in self._service_map.values() ): From 9ef5978515865709aa296ff857034f59c86e28e6 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Mon, 15 May 2023 17:41:51 +0200 Subject: [PATCH 16/26] Apply reviewer's suggesstions --- jupyterhub/apihandlers/services.py | 2 +- jupyterhub/app.py | 30 ++++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index ce835102..e0eabd0f 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -107,7 +107,7 @@ class ServiceAPIHandler(APIHandler): service, orm_service = self.find_service(service_name) if service is None: - raise web.HTTPError(404, f"Service {service_name} does not exists") + raise web.HTTPError(404, f"Service {service_name} does not exist") if service.from_config: raise web.HTTPError( diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 78a20142..3603851d 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2457,6 +2457,14 @@ class JupyterHub(Application): # Do nothing if the config file tries to modify a API-base service # or vice versa. if orm_service.from_config != from_config: + if from_config: + self.log.error( + f"The service {name} from the config file is trying to modify a runtime-created service with the same name" + ) + else: + self.log.error( + f"The runtime-created service {name} is trying to modify a config-based service with the same name" + ) return orm_service.admin = spec.get('admin', False) @@ -2489,21 +2497,27 @@ class JupyterHub(Application): if service.url: parsed = urlparse(service.url) + port = None if parsed.port is not None: port = parsed.port elif parsed.scheme == 'http': port = 80 elif parsed.scheme == 'https': port = 443 - server = service.orm.server = orm.Server( - proto=parsed.scheme, - ip=parsed.hostname, - port=port, - cookie_name=service.oauth_client_id, - base_url=service.prefix, - ) - self.db.add(server) + if port is not None: + server = service.orm.server = orm.Server( + proto=parsed.scheme, + ip=parsed.hostname, + port=port, + cookie_name=service.oauth_client_id, + base_url=service.prefix, + ) + self.db.add(server) + else: + self.log.error( + f"Can not detect server port for service named {name}, the server is ignored!" + ) else: service.orm.server = None From 2823c125528d4f085f4d0f0eeabe21c645757b90 Mon Sep 17 00:00:00 2001 From: Duc Trung LE Date: Tue, 16 May 2023 12:02:55 +0200 Subject: [PATCH 17/26] Prevent creating managed servicesat runtime --- docs/source/reference/services.md | 4 ++-- jupyterhub/apihandlers/services.py | 18 +++++++++++------- jupyterhub/tests/test_api.py | 21 +++++++++++++++++++++ 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/source/reference/services.md b/docs/source/reference/services.md index b707d604..f3d4ed38 100644 --- a/docs/source/reference/services.md +++ b/docs/source/reference/services.md @@ -176,7 +176,7 @@ In this case, the `url` field will be passed along to the Service as ## Adding or removing services at runtime -Both Hub-Managed services and Externally-Managed services can be added at runtime by using JupyterHub’s REST API. +Only externally-managed services can be added at runtime by using JupyterHub’s REST API. ### Add a new service @@ -188,7 +188,7 @@ POST /hub/api/services/:servicename **Required scope: `admin:services`** -**Payload**: The payload should contain the definition of the service to be created. The endpoint supports the same properties as services defined in the config file. +**Payload**: The payload should contain the definition of the service to be created. The endpoint supports the same properties as externally-managed services defined in the config file. **Possible responses** diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index e0eabd0f..b4018071 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -49,6 +49,11 @@ class ServiceAPIHandler(APIHandler): self._check_service_model(spec) service_name = spec["name"] + managed = bool(spec.get('command')) + if managed: + msg = f"Can not create managed service {service_name} at runtime" + self.log.error(msg, exc_info=True) + raise web.HTTPError(400, msg) try: new_service = self.service_from_spec(spec) except Exception: @@ -64,12 +69,9 @@ class ServiceAPIHandler(APIHandler): await self.app._add_tokens( {new_service.api_token: new_service.name}, kind='service' ) - elif new_service.managed: - new_service.api_token = new_service.orm.new_api_token( - note='generated at runtime' - ) - if new_service.managed or new_service.url: - service_status = self.app.start_service(service_name, new_service) + if new_service.url: + # Start polling for external service + service_status = await self.app.start_service(service_name, new_service) if not service_status: self.log.error( 'Failed to start service %s', @@ -116,6 +118,8 @@ class ServiceAPIHandler(APIHandler): try: await self.remove_service(service, orm_service) self.services.pop(service_name) + if service.url: + self.app.toggle_service_health_check() except Exception: msg = f"Failed to remove service {service_name}" self.log.error(msg, exc_info=True) @@ -178,7 +182,7 @@ class ServiceAPIHandler(APIHandler): service = self.services.get(name) return service, orm_service - return None, None + return (None, None) default_handlers = [ diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 00ef6112..4b15588f 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -4,6 +4,7 @@ import json import re import sys import uuid +from copy import deepcopy from datetime import datetime, timedelta from unittest import mock from urllib.parse import quote, urlparse @@ -2177,6 +2178,26 @@ async def test_create_service_duplication(app, service_admin_user, service_data) assert r.status_code == 409 +@mark.services +async def test_create_managed_service(app, service_admin_user, service_data): + db = app.db + service_name = 'managed-service-from-api' + managed_service_data = deepcopy(service_data) + managed_service_data['command'] = ['foo'] + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(managed_service_data), + method='post', + ) + + assert r.status_code == 400 + assert 'Can not create managed service' in r.json()['message'] + orm_service = orm.Service.find(db, service_name) + assert orm_service is None + + @mark.services async def test_remove_service(app, service_admin_user, service_data): db = app.db From 9034de28f90884d280aaa8fe9b22b934341b7195 Mon Sep 17 00:00:00 2001 From: Duc Trung Le Date: Fri, 19 May 2023 16:54:07 +0200 Subject: [PATCH 18/26] Lmiting runtime services to externally managed services only --- jupyterhub/apihandlers/services.py | 24 ++++++++- jupyterhub/tests/test_api.py | 84 ++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index b4018071..8867f79f 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -10,7 +10,8 @@ from typing import Optional, Tuple from tornado import web from .. import orm -from ..scopes import Scope, needs_scope +from ..roles import get_default_roles +from ..scopes import Scope, _check_token_scopes, needs_scope from ..services.service import Service from .base import APIHandler @@ -33,6 +34,25 @@ class ServiceAPIHandler(APIHandler): service = self.services[service_name] self.write(json.dumps(self.service_model(service))) + def _check_service_scopes(self, spec: dict): + user = self.current_user + requested_scopes = [] + if spec.get('admin'): + default_roles = get_default_roles() + admin_scopes = [ + role['scopes'] for role in default_roles if role['name'] == 'admin' + ] + requested_scopes.extend(admin_scopes[0]) + + requested_client_scope = spec.get('oauth_client_allowed_scopes') + if requested_client_scope is not None: + requested_scopes.extend(requested_client_scope) + if len(requested_scopes) > 0: + try: + _check_token_scopes(requested_scopes, user, None) + except ValueError as e: + raise web.HTTPError(400, str(e)) + async def add_service(self, spec: dict) -> Service: """Add a new service and related objects to the database @@ -48,6 +68,8 @@ class ServiceAPIHandler(APIHandler): """ self._check_service_model(spec) + self._check_service_scopes(spec) + service_name = spec["name"] managed = bool(spec.get('command')) if managed: diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 4b15588f..633bfeb9 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -2198,6 +2198,90 @@ async def test_create_managed_service(app, service_admin_user, service_data): assert orm_service is None +@mark.services +async def test_create_admin_service(app, admin_user, service_data): + db = app.db + service_name = 'admin-service-from-api' + managed_service_data = deepcopy(service_data) + managed_service_data['admin'] = True + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, admin_user.name), + data=json.dumps(managed_service_data), + method='post', + ) + + assert r.status_code == 201 + orm_service = orm.Service.find(db, service_name) + assert orm_service is not None + + +@mark.services +async def test_create_admin_service_without_admin_right( + app, service_admin_user, service_data +): + db = app.db + service_name = 'managed-service-from-api' + managed_service_data = deepcopy(service_data) + managed_service_data['admin'] = True + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(managed_service_data), + method='post', + ) + + assert r.status_code == 400 + assert 'Not assigning requested scopes' in r.json()['message'] + orm_service = orm.Service.find(db, service_name) + assert orm_service is None + + +@mark.services +async def test_create_service_with_scope(app, create_user_with_scopes, service_data): + db = app.db + service_name = 'service-with-scope' + managed_service_data = deepcopy(service_data) + managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"] + managed_service_data['oauth_client_id'] = "service-client-with-scope" + user_with_scope = create_user_with_scopes('admin:services', 'admin:users') + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, user_with_scope.name), + data=json.dumps(managed_service_data), + method='post', + ) + + assert r.status_code == 201 + orm_service = orm.Service.find(db, service_name) + assert orm_service is not None + + +@mark.services +async def test_create_service_without_requested_scope( + app, service_admin_user, service_data +): + db = app.db + service_name = 'service-without-requested-scope' + managed_service_data = deepcopy(service_data) + managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"] + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(managed_service_data), + method='post', + ) + + assert r.status_code == 400 + assert 'Not assigning requested scopes' in r.json()['message'] + orm_service = orm.Service.find(db, service_name) + assert orm_service is None + + @mark.services async def test_remove_service(app, service_admin_user, service_data): db = app.db From aa754a1a2cabc1934d3acd3acf6a4cd8f34e0990 Mon Sep 17 00:00:00 2001 From: Duc Trung LE Date: Tue, 4 Jul 2023 15:13:40 +0200 Subject: [PATCH 19/26] Apply suggestions --- jupyterhub/app.py | 15 ++++++++------- jupyterhub/oauth/provider.py | 3 +++ jupyterhub/orm.py | 1 + 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 3603851d..7f9b141c 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2486,6 +2486,7 @@ class JupyterHub(Application): if key not in traits: raise AttributeError("No such service field: %s" % key) setattr(service, key, value) + print('###### updateing', key, value) orm_service.update_column(key, value) if service.api_token: @@ -3256,9 +3257,9 @@ class JupyterHub(Application): service_name, status, ) - break + return False else: - break + return True else: self.log.error( "Cannot connect to %s service %s at %s. Is it running?", @@ -3266,6 +3267,7 @@ class JupyterHub(Application): service_name, service.url, ) + return False return True def toggle_service_health_check(self) -> None: @@ -3284,6 +3286,7 @@ class JupyterHub(Application): # the periodic callback. if self._check_services_health_callback is not None: self._check_services_health_callback.stop() + self._check_services_health_callback = None async def start(self): """Start the whole thing""" @@ -3370,10 +3373,8 @@ class JupyterHub(Application): # start the service(s) for service_name, service in self._service_map.items(): - service_status = await self.start_service( - service_name, service, ssl_context - ) - if not service_status: + service_ready = await self.start_service(service_name, service, ssl_context) + if not service_ready: if service.from_config: # Stop the application if a config-based service failed to start. self.exit(1) @@ -3381,7 +3382,7 @@ class JupyterHub(Application): # Only warn for database-based service, so that admin can connect # to hub to remove the service. self.log.error( - "Failed to start database-based service %s", + "Failed to reach externally managed service %s", service_name, exc_info=True, ) diff --git a/jupyterhub/oauth/provider.py b/jupyterhub/oauth/provider.py index 8ff4a97f..4a7533ac 100644 --- a/jupyterhub/oauth/provider.py +++ b/jupyterhub/oauth/provider.py @@ -676,6 +676,9 @@ class JupyterHubOAuthServer(WebApplicationServer): if orm_client is not None: self.db.delete(orm_client) self.db.commit() + app_log.info("Removed client %s", client_id) + else: + app_log.warning("No such client %s", client_id) def fetch_by_client_id(self, client_id): """Find a client by its id""" diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index 43d2d077..bb962a07 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -490,6 +490,7 @@ class Service(Base): returns `False` otherwise. """ if self._check_data_only_column(column_name): + print('NOT ForeignKey', column_name) setattr(self, column_name, value) return True else: From 81885d5c61b9a53c0c8302bb04f0ae400128d4b4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 4 Jul 2023 13:14:32 +0000 Subject: [PATCH 20/26] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- jupyterhub/app.py | 1 - jupyterhub/orm.py | 1 - jupyterhub/tests/conftest.py | 1 + jupyterhub/tests/test_app.py | 1 + 4 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 7f9b141c..d717714e 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2486,7 +2486,6 @@ class JupyterHub(Application): if key not in traits: raise AttributeError("No such service field: %s" % key) setattr(service, key, value) - print('###### updateing', key, value) orm_service.update_column(key, value) if service.api_token: diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index bb962a07..43d2d077 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -490,7 +490,6 @@ class Service(Base): returns `False` otherwise. """ if self._check_data_only_column(column_name): - print('NOT ForeignKey', column_name) setattr(self, column_name, value) return True else: diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 9930fbf6..6e7b6423 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -535,6 +535,7 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config): if queries: terminalreporter.line(f"{queries:<6} {nodeid}") + @fixture def service_data(): """Data used to create service at runtime""" diff --git a/jupyterhub/tests/test_app.py b/jupyterhub/tests/test_app.py index d691f2d7..ea0130bd 100644 --- a/jupyterhub/tests/test_app.py +++ b/jupyterhub/tests/test_app.py @@ -475,6 +475,7 @@ async def test_user_creation(tmpdir, request): "in-role", } + @pytest.fixture(scope='module') def db_temp_path(tmp_path_factory): fn = tmp_path_factory.mktemp("db") / "jupyterhub.sqlite" From 051729448c1c533903c40bde5b56b39e5740b9d7 Mon Sep 17 00:00:00 2001 From: Min RK Date: Mon, 7 Aug 2023 14:03:18 +0200 Subject: [PATCH 21/26] remove toggle_service_health_check leave service check always running, since it doesn't cost anything to call an empty function once a minute --- jupyterhub/apihandlers/services.py | 4 ---- jupyterhub/app.py | 25 ++++++------------------- 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 8867f79f..9d6b66a5 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -100,7 +100,6 @@ class ServiceAPIHandler(APIHandler): service_name, exc_info=True, ) - self.app.toggle_service_health_check() if new_service.oauth_no_confirm: oauth_no_confirm_list = self.settings.get('oauth_no_confirm_list') @@ -140,8 +139,6 @@ class ServiceAPIHandler(APIHandler): try: await self.remove_service(service, orm_service) self.services.pop(service_name) - if service.url: - self.app.toggle_service_health_check() except Exception: msg = f"Failed to remove service {service_name}" self.log.error(msg, exc_info=True) @@ -160,7 +157,6 @@ class ServiceAPIHandler(APIHandler): """ if service.managed: await service.stop() - self.app.toggle_service_health_check() if service.oauth_client: self.oauth_provider.remove_client(service.oauth_client_id) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index d717714e..65d059b3 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2585,6 +2585,7 @@ class JupyterHub(Application): """Check connectivity of all services""" for name, service in self._service_map.items(): if not service.url: + # no URL to check, nothing to do continue try: await Server.from_orm(service.orm.server).wait_up(timeout=1, http=True) @@ -3269,24 +3270,6 @@ class JupyterHub(Application): return False return True - def toggle_service_health_check(self) -> None: - """Start or stop the service health check callback.""" - - if self.service_check_interval and any( - s.url for s in self._service_map.values() - ): - if self._check_services_health_callback is None: - self._check_services_health_callback = PeriodicCallback( - self.check_services_health, 1e3 * self.service_check_interval - ) - self._check_services_health_callback.start() - else: - # Services requiring health check are removed, stop - # the periodic callback. - if self._check_services_health_callback is not None: - self._check_services_health_callback.stop() - self._check_services_health_callback = None - async def start(self): """Start the whole thing""" self.io_loop = loop = IOLoop.current() @@ -3390,7 +3373,11 @@ class JupyterHub(Application): # Check services health self._check_services_health_callback = None - self.toggle_service_health_check() + if self.service_check_interval: + self._check_services_health_callback = PeriodicCallback( + self.check_services_health, 1e3 * self.service_check_interval + ) + self._check_services_health_callback.start() if self.last_activity_interval: pc = PeriodicCallback( From 3c0fab744928b0f1b03c397765f6836bd4e3f5dc Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 8 Aug 2023 13:12:43 +0200 Subject: [PATCH 22/26] remove redundant domain, host args from service methods --- jupyterhub/app.py | 44 +++++++++++++++----------------------------- 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 65d059b3..8f14adc0 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2358,21 +2358,24 @@ class JupyterHub(Application): def service_from_orm( self, orm_service: orm.Service, - domain: str, - host: str, ) -> Service: """Create the service instance and related objects from ORM data. Args: orm_service (orm.Service): The `orm.Service` object - domain (str): The domain of Hub session - host (str): The host of Hub session Returns: Service: the created service """ + if self.domain: + domain = 'services.' + self.domain + parsed = urlparse(self.subdomain_host) + host = f'{parsed.scheme}://services.{parsed.netloc}' + else: + domain = host = '' + name = orm_service.name service = Service( parent=self, @@ -2404,8 +2407,6 @@ class JupyterHub(Application): self, spec: Dict, from_config=True, - domain: Optional[str] = None, - host: Optional[str] = None, ) -> Optional[Service]: """Create the service instance and related objects from config data. @@ -2415,25 +2416,17 @@ class JupyterHub(Application): from_config (bool, optional): `True` if the service will be created from the config file, `False` if it is created from REST API. Defaults to `True`. - domain (Optional[str]): The domain of Hub session. Defaults to None. - host (Optional[str]): The host of Hub session. Defaults to None. Returns: Optional[Service]: The created service """ - if domain is None: - if self.domain: - domain = 'services.' + self.domain - else: - domain = '' - - if host is None: - if self.domain: - parsed = urlparse(self.subdomain_host) - host = f'{parsed.scheme}://services.{parsed.netloc}' - else: - host = '' + if self.domain: + domain = 'services.' + self.domain + parsed = urlparse(self.subdomain_host) + host = f'{parsed.scheme}://services.{parsed.netloc}' + else: + domain = host = '' if 'name' not in spec: raise ValueError('service spec must have a name: %r' % spec) @@ -2560,15 +2553,8 @@ class JupyterHub(Application): def init_services(self): self._service_map.clear() - if self.domain: - domain = 'services.' + self.domain - parsed = urlparse(self.subdomain_host) - host = f'{parsed.scheme}://services.{parsed.netloc}' - else: - domain = host = '' - for spec in self.services: - self.service_from_spec(spec, from_config=True, domain=domain, host=host) + self.service_from_spec(spec, from_config=True) for service_orm in self.db.query(orm.Service): if service_orm.from_config: @@ -2577,7 +2563,7 @@ class JupyterHub(Application): if service_orm.name not in self._service_map: self.db.delete(service_orm) else: - self.service_from_orm(service_orm, domain, host) + self.service_from_orm(service_orm) self.db.commit() From d9154681ebdd903e0f7d22a9879e5844dc000409 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 8 Aug 2023 13:13:23 +0200 Subject: [PATCH 23/26] service.url must always be http[s] no chance for undefined port --- jupyterhub/app.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 8f14adc0..23cbbce7 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2490,6 +2490,11 @@ class JupyterHub(Application): if service.url: parsed = urlparse(service.url) + if parsed.scheme not in {"http", "https"}: + raise ValueError( + f"Unsupported scheme in URL for service {name}: {service.url}. Must be http[s]" + ) + port = None if parsed.port is not None: port = parsed.port @@ -2498,19 +2503,14 @@ class JupyterHub(Application): elif parsed.scheme == 'https': port = 443 - if port is not None: - server = service.orm.server = orm.Server( - proto=parsed.scheme, - ip=parsed.hostname, - port=port, - cookie_name=service.oauth_client_id, - base_url=service.prefix, - ) - self.db.add(server) - else: - self.log.error( - f"Can not detect server port for service named {name}, the server is ignored!" - ) + server = service.orm.server = orm.Server( + proto=parsed.scheme, + ip=parsed.hostname, + port=port, + cookie_name=service.oauth_client_id, + base_url=service.prefix, + ) + self.db.add(server) else: service.orm.server = None From 45102b248b1d028b4fdfbd6b98d8508d1a94b848 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 8 Aug 2023 14:52:27 +0200 Subject: [PATCH 24/26] store what fields get persisted in trait metadata rather than checking columns in the db makes things more explicit --- jupyterhub/app.py | 20 ++++--- jupyterhub/orm.py | 52 ----------------- jupyterhub/services/service.py | 13 +++-- jupyterhub/tests/test_orm.py | 104 --------------------------------- 4 files changed, 21 insertions(+), 168 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 23cbbce7..4e53ed8a 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -2389,13 +2389,14 @@ class JupyterHub(Application): hub=self.hub, ) traits = service.traits(input=True) - for key in traits: - value = orm_service.get_column(key) - if value is not None: - setattr(service, key, value) + for key, trait in traits.items(): + if not trait.metadata.get("in_db", True): + continue + orm_value = getattr(orm_service, key) + if orm_value is not None: + setattr(service, key, orm_value) if orm_service.oauth_client is not None: - service.oauth_redirect_uri = orm_service.oauth_client.redirect_uri service.oauth_client_id = orm_service.oauth_client.identifier service.oauth_redirect_uri = orm_service.oauth_client.redirect_uri @@ -2476,10 +2477,15 @@ class JupyterHub(Application): traits = service.traits(input=True) for key, value in spec.items(): - if key not in traits: + trait = traits.get(key) + if trait is None: raise AttributeError("No such service field: %s" % key) setattr(service, key, value) - orm_service.update_column(key, value) + # also set the value on the orm object + # unless it's marked as not in the db + # (e.g. on the oauth object) + if trait.metadata.get("in_db", True): + setattr(orm_service, key, value) if service.api_token: self.service_tokens[service.api_token] = service.name diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index 43d2d077..71cc2f76 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -458,58 +458,6 @@ class Service(Base): """ return db.query(cls).filter(cls.name == name).first() - def _check_data_only_column(self, column_name: str) -> bool: - """Check if a column is not a ForeignKey or in a - relationship. - - Args: - column_name (str): ame of the column - - Returns: - bool: returns `True` if the column is data-only, - returns `False` otherwise. - """ - if hasattr(self, column_name): - column = getattr(Service, column_name) - if hasattr(column, 'foreign_keys'): - return len(column.foreign_keys) == 0 - else: - return False - else: - return False - - def update_column(self, column_name: str, value: any) -> bool: - """Update the value of a non-ForeignKey column. - - Args: - column_name (str): Name of the column - value (any): New value - - Returns: - bool: returns `True` if the column is updated, - returns `False` otherwise. - """ - if self._check_data_only_column(column_name): - setattr(self, column_name, value) - return True - else: - return False - - def get_column(self, column_name: str): - """Get non-ForeignKey, non-relationship column - - Args: - column_name (str): Name of the column - - Returns: - The value of requested column, None if it is - a foreign key or it does not exist. - """ - if self._check_data_only_column(column_name): - return getattr(self, column_name) - else: - return None - class Expiring: """Mixin for expiring entries diff --git a/jupyterhub/services/service.py b/jupyterhub/services/service.py index 3d551b17..24c3f37c 100644 --- a/jupyterhub/services/service.py +++ b/jupyterhub/services/service.py @@ -180,9 +180,12 @@ class Service(LoggingConfigurable): - user: str The name of a system user to become. If unspecified, run as the same user as the Hub. + """ - # inputs: + # traits tagged with `input=True` are accepted as input from configuration / API + # input traits are also persisted to the db UNLESS they are also tagged with `in_db=False` + name = Unicode( help="""The name of the service. @@ -205,7 +208,7 @@ class Service(LoggingConfigurable): DEPRECATED in 3.0: use oauth_client_allowed_scopes """ - ).tag(input=True) + ).tag(input=True, in_db=False) oauth_client_allowed_scopes = List( help="""OAuth allowed scopes. @@ -225,7 +228,7 @@ class Service(LoggingConfigurable): If unspecified, an API token will be generated for managed services. """ - ).tag(input=True) + ).tag(input=True, in_db=False) info = Dict( help="""Provide a place to include miscellaneous information about the service, @@ -310,7 +313,7 @@ class Service(LoggingConfigurable): You shouldn't generally need to change this. Default: `service-` """ - ).tag(input=True) + ).tag(input=True, in_db=False) @default('oauth_client_id') def _default_client_id(self): @@ -331,7 +334,7 @@ class Service(LoggingConfigurable): You shouldn't generally need to change this. Default: `/services/:name/oauth_callback` """ - ).tag(input=True) + ).tag(input=True, in_db=False) @default('oauth_redirect_uri') def _default_redirect_uri(self): diff --git a/jupyterhub/tests/test_orm.py b/jupyterhub/tests/test_orm.py index afa35999..74deff3b 100644 --- a/jupyterhub/tests/test_orm.py +++ b/jupyterhub/tests/test_orm.py @@ -145,110 +145,6 @@ def test_token_expiry(db): assert orm_token not in user.api_tokens -@pytest.mark.parametrize( - 'column, expected', - [ - ( - 'name', - True, - ), - ( - 'admin', - True, - ), - ( - 'url', - True, - ), - ( - 'oauth_client_allowed_scopes', - True, - ), - ( - 'info', - True, - ), - ( - 'display', - True, - ), - ( - 'oauth_no_confirm', - True, - ), - ( - 'command', - True, - ), - ( - 'cwd', - True, - ), - ( - 'environment', - True, - ), - ( - 'user', - True, - ), - ( - 'from_config', - True, - ), - ( - 'api_tokens', - False, - ), - ( - '_server_id', - False, - ), - ( - 'server', - False, - ), - ( - 'pid', - True, - ), - ( - 'oauth_client_id', - False, - ), - ( - 'oauth_client', - False, - ), - ], -) -def test_service_check_data_only_column(db, column, expected): - orm_service = orm.Service(name='check_data_only_column', from_config=True) - db.add(orm_service) - db.commit() - assert orm_service._check_data_only_column(column) == expected - db.delete(orm_service) - db.commit() - - -def test_service_get_column(db): - orm_service = orm.Service(name='test_service_get_column', from_config=True) - db.add(orm_service) - db.commit() - orm_service.server = orm.Server() - assert orm_service.get_column('from_config') - assert orm_service.get_column('server') is None - - -def test_service_update_column(db): - orm_service = orm.Service(name='test_service_set_column', from_config=True) - db.add(orm_service) - db.commit() - - assert orm_service.update_column('from_config', False) - assert not orm_service.update_column('server', orm.Server()) - - def test_service_tokens(db): service = orm.Service(name='secret') db.add(service) From da144c98ce73c3852585a8a4452826965aa3ae32 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 8 Aug 2023 14:53:36 +0200 Subject: [PATCH 25/26] neaten service test management - cleanup services after each test - more fixtures for services --- jupyterhub/tests/conftest.py | 51 +++++++---- jupyterhub/tests/test_api.py | 69 +++++++++------ jupyterhub/tests/test_app.py | 166 +++++++++++++---------------------- 3 files changed, 137 insertions(+), 149 deletions(-) diff --git a/jupyterhub/tests/conftest.py b/jupyterhub/tests/conftest.py index 6e7b6423..98ec9fb8 100644 --- a/jupyterhub/tests/conftest.py +++ b/jupyterhub/tests/conftest.py @@ -167,6 +167,8 @@ async def cleanup_after(request, io_loop): app = MockHub.instance() if app.db_file.closed: return + + # cleanup users for uid, user in list(app.users.items()): for name, spawner in list(user.spawners.items()): if spawner.active: @@ -181,6 +183,16 @@ async def cleanup_after(request, io_loop): # delete groups for group in app.db.query(orm.Group): app.db.delete(group) + + # clear services + for name, service in app._service_map.items(): + if service.managed: + service.stop() + for orm_service in app.db.query(orm.Service): + if orm_service.oauth_client: + app.oauth_provider.remove_client(orm_service.oauth_client_id) + app.db.delete(orm_service) + app._service_map.clear() app.db.commit() @@ -262,10 +274,7 @@ class MockServiceSpawner(jupyterhub.services.service._ServiceSpawner): poll_interval = 1 -_mock_service_counter = 0 - - -async def _mockservice(request, app, external=False, url=False): +async def _mockservice(request, app, name, external=False, url=False): """ Add a service to the application @@ -281,9 +290,6 @@ async def _mockservice(request, app, external=False, url=False): If True, register the service at a URL (as opposed to headless, API-only). """ - global _mock_service_counter - _mock_service_counter += 1 - name = 'mock-service-%i' % _mock_service_counter spec = {'name': name, 'command': mockservice_cmd, 'admin': True} if url: if app.internal_ssl: @@ -330,22 +336,33 @@ async def _mockservice(request, app, external=False, url=False): return service +_service_name_counter = 0 + + @fixture -async def mockservice(request, app): +def service_name(): + global _service_name_counter + _service_name_counter += 1 + name = f'test-service-{_service_name_counter}' + return name + + +@fixture +async def mockservice(request, app, service_name): """Mock a service with no external service url""" - yield await _mockservice(request, app, url=False) + yield await _mockservice(request, app, name=service_name, url=False) @fixture -async def mockservice_external(request, app): +async def mockservice_external(request, app, service_name): """Mock an externally managed service (don't start anything)""" - yield await _mockservice(request, app, external=True, url=False) + yield await _mockservice(request, app, name=service_name, external=True, url=False) @fixture -async def mockservice_url(request, app): +async def mockservice_url(request, app, service_name): """Mock a service with its own url to test external services""" - yield await _mockservice(request, app, url=True) + yield await _mockservice(request, app, name=service_name, url=True) @fixture @@ -537,12 +554,14 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config): @fixture -def service_data(): +def service_data(service_name): """Data used to create service at runtime""" return { - "oauth_client_id": "service-oauth-client-from-api", - "api_token": "api_token-from-api", + "name": service_name, + "oauth_client_id": f"service-{service_name}", + "api_token": f"api_token-{service_name}", "oauth_redirect_uri": "http://127.0.0.1:5555/oauth_callback-from-api", "oauth_no_confirm": True, + "oauth_client_allowed_scopes": ["inherit"], "info": {'foo': 'bar'}, } diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 633bfeb9..6567b488 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -2099,9 +2099,8 @@ def service_admin_user(create_user_with_scopes): @mark.services -async def test_create_service(app, service_admin_user, service_data): +async def test_create_service(app, service_admin_user, service_name, service_data): db = app.db - service_name = 'service-from-api' r = await api_request( app, f'services/{service_name}', @@ -2131,9 +2130,8 @@ async def test_create_service(app, service_admin_user, service_data): @mark.services -async def test_create_service_no_role(app, service_data): +async def test_create_service_no_role(app, service_name, service_data): db = app.db - service_name = 'service-from-api' r = await api_request( app, f'services/{service_name}', @@ -2146,9 +2144,10 @@ async def test_create_service_no_role(app, service_data): @mark.services -async def test_create_service_conflict(app, service_admin_user, service_data): +async def test_create_service_conflict( + app, service_admin_user, mockservice, service_data, service_name +): db = app.db - service_name = 'service-from-config' app.services = [{'name': service_name}] app.init_services() @@ -2164,9 +2163,19 @@ async def test_create_service_conflict(app, service_admin_user, service_data): @mark.services -async def test_create_service_duplication(app, service_admin_user, service_data): +async def test_create_service_duplication( + app, service_admin_user, service_name, service_data +): db = app.db - service_name = 'service-from-api' + + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(service_data), + method='post', + ) + assert r.status_code == 201 r = await api_request( app, @@ -2179,9 +2188,10 @@ async def test_create_service_duplication(app, service_admin_user, service_data) @mark.services -async def test_create_managed_service(app, service_admin_user, service_data): +async def test_create_managed_service( + app, service_admin_user, service_name, service_data +): db = app.db - service_name = 'managed-service-from-api' managed_service_data = deepcopy(service_data) managed_service_data['command'] = ['foo'] r = await api_request( @@ -2199,9 +2209,8 @@ async def test_create_managed_service(app, service_admin_user, service_data): @mark.services -async def test_create_admin_service(app, admin_user, service_data): +async def test_create_admin_service(app, admin_user, service_name, service_data): db = app.db - service_name = 'admin-service-from-api' managed_service_data = deepcopy(service_data) managed_service_data['admin'] = True r = await api_request( @@ -2219,10 +2228,9 @@ async def test_create_admin_service(app, admin_user, service_data): @mark.services async def test_create_admin_service_without_admin_right( - app, service_admin_user, service_data + app, service_admin_user, service_data, service_name ): db = app.db - service_name = 'managed-service-from-api' managed_service_data = deepcopy(service_data) managed_service_data['admin'] = True r = await api_request( @@ -2240,9 +2248,10 @@ async def test_create_admin_service_without_admin_right( @mark.services -async def test_create_service_with_scope(app, create_user_with_scopes, service_data): +async def test_create_service_with_scope( + app, create_user_with_scopes, service_name, service_data +): db = app.db - service_name = 'service-with-scope' managed_service_data = deepcopy(service_data) managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"] managed_service_data['oauth_client_id'] = "service-client-with-scope" @@ -2262,10 +2271,12 @@ async def test_create_service_with_scope(app, create_user_with_scopes, service_d @mark.services async def test_create_service_without_requested_scope( - app, service_admin_user, service_data + app, + service_admin_user, + service_data, + service_name, ): db = app.db - service_name = 'service-without-requested-scope' managed_service_data = deepcopy(service_data) managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"] r = await api_request( @@ -2283,9 +2294,16 @@ async def test_create_service_without_requested_scope( @mark.services -async def test_remove_service(app, service_admin_user, service_data): +async def test_remove_service(app, service_admin_user, service_name, service_data): db = app.db - service_name = 'service-from-api' + r = await api_request( + app, + f'services/{service_name}', + headers=auth_header(db, service_admin_user.name), + data=json.dumps(service_data), + method='post', + ) + assert r.status_code == 201 r = await api_request( app, @@ -2309,9 +2327,9 @@ async def test_remove_service(app, service_admin_user, service_data): @mark.services -async def test_remove_service_from_config(app, service_admin_user): +async def test_remove_service_from_config(app, service_admin_user, mockservice): db = app.db - service_name = 'service-from-config' + service_name = mockservice.name r = await api_request( app, f'services/{service_name}', @@ -2319,15 +2337,10 @@ async def test_remove_service_from_config(app, service_admin_user): method='delete', ) assert r.status_code == 405 - assert ( - r.json()['message'] - == 'Service service-from-config is not modifiable at runtime' - ) + assert r.json()['message'] == f'Service {service_name} is not modifiable at runtime' async def test_root_api(app): - base_url = app.hub.url - url = ujoin(base_url, 'api') kwargs = {} if app.internal_ssl: kwargs['cert'] = (app.internal_ssl_cert, app.internal_ssl_key) diff --git a/jupyterhub/tests/test_app.py b/jupyterhub/tests/test_app.py index ea0130bd..4bc4316f 100644 --- a/jupyterhub/tests/test_app.py +++ b/jupyterhub/tests/test_app.py @@ -15,11 +15,10 @@ import pytest import traitlets from traitlets.config import Config -from .. import orm, roles +from .. import orm from ..app import COOKIE_SECRET_BYTES, JupyterHub from .mocking import MockHub from .test_api import add_user -from .utils import api_request, auth_header def test_help_all(): @@ -221,7 +220,7 @@ def test_cookie_secret_env(tmpdir, request): assert not os.path.exists(hub.cookie_secret_file) -def test_cookie_secret_string_(): +def test_cookie_secret_string(): cfg = Config() cfg.JupyterHub.cookie_secret = "not hex" @@ -271,18 +270,41 @@ async def test_load_groups(tmpdir, request): ) -async def test_resume_spawners(tmpdir, request): - if not os.getenv('JUPYTERHUB_TEST_DB_URL'): - p = patch.dict( - os.environ, - { - 'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' - % tmpdir.join('jupyterhub.sqlite') - }, - ) - p.start() - request.addfinalizer(p.stop) +@pytest.fixture +def persist_db(tmpdir): + """ensure db will persist (overrides default sqlite://:memory:)""" + if os.getenv('JUPYTERHUB_TEST_DB_URL'): + # already using a db, no need + yield + return + with patch.dict( + os.environ, + {'JUPYTERHUB_TEST_DB_URL': f"sqlite:///{tmpdir.join('jupyterhub.sqlite')}"}, + ): + yield + +@pytest.fixture +def new_hub(request, tmpdir, persist_db): + """Fixture to launch a new hub for testing""" + + async def new_hub(): + kwargs = {} + ssl_enabled = getattr(request.module, "ssl_enabled", False) + if ssl_enabled: + kwargs['internal_certs_location'] = str(tmpdir) + app = MockHub(test_clean_db=False, **kwargs) + app.config.ConfigurableHTTPProxy.should_start = False + app.config.ConfigurableHTTPProxy.auth_token = 'unused' + request.addfinalizer(app.stop) + await app.initialize([]) + + return app + + return new_hub + + +async def test_resume_spawners(tmpdir, request, new_hub): async def new_hub(): kwargs = {} ssl_enabled = getattr(request.module, "ssl_enabled", False) @@ -476,100 +498,26 @@ async def test_user_creation(tmpdir, request): } -@pytest.fixture(scope='module') -def db_temp_path(tmp_path_factory): - fn = tmp_path_factory.mktemp("db") / "jupyterhub.sqlite" - return fn +async def test_recreate_service_from_database( + request, new_hub, service_name, service_data +): + # create a hub and add a service (not from config) + app = await new_hub() + app.service_from_spec(service_data, from_config=False) + app.stop() + # new hub, should load service from db + app = await new_hub() + assert service_name in app._service_map -async def test_add_service_at_runtime(request, db_temp_path, service_data): - if not os.getenv('JUPYTERHUB_TEST_DB_URL'): - p = patch.dict( - os.environ, - {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)}, - ) - p.start() - request.addfinalizer(p.stop) + # verify keys + service = app._service_map[service_name] + for key, value in service_data.items(): + if key in {'api_token'}: + # skip some keys + continue + assert getattr(service, key) == value - kwargs = {"test_clean_db": False} - ssl_enabled = getattr(request.module, "ssl_enabled", False) - if ssl_enabled: - kwargs['internal_certs_location'] = db_temp_path.parents[0] - app = MockHub(**kwargs) - - def end(): - app.log.handlers = [] - MockHub.clear_instance() - try: - app.stop() - except Exception as e: - print("Error stopping Hub: %s" % e, file=sys.stderr) - - request.addfinalizer(end) - - await app.initialize([]) - await app.start() - db = app.db - - user_name = 'admin_services' - service_role = { - 'name': 'admin-services-role', - 'description': '', - 'users': [user_name], - 'scopes': ['admin:services'], - } - roles.create_role(app.db, service_role) - user = add_user(app.db, name=user_name) - roles.update_roles(app.db, user, roles=['admin-services-role']) - - service_name = 'service-from-api' - - r = await api_request( - app, - f'services/{service_name}', - headers=auth_header(db, user_name), - data=json.dumps(service_data), - method='post', - ) - assert r.status_code == 201 - assert r.json()['name'] == service_name - oauth_client = ( - app.db.query(orm.OAuthClient) - .filter_by(identifier=service_data['oauth_client_id']) - .first() - ) - assert oauth_client.redirect_uri == service_data['oauth_redirect_uri'] - - -async def test_recreate_service_from_database(request, db_temp_path, service_data): - if not os.getenv('JUPYTERHUB_TEST_DB_URL'): - p = patch.dict( - os.environ, - {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)}, - ) - p.start() - request.addfinalizer(p.stop) - kwargs = {"test_clean_db": False} - - ssl_enabled = getattr(request.module, "ssl_enabled", False) - if ssl_enabled: - kwargs['internal_certs_location'] = db_temp_path.parents[0] - app = MockHub(**kwargs) - - def end(): - app.log.handlers = [] - MockHub.clear_instance() - try: - app.stop() - except Exception as e: - print("Error stopping Hub: %s" % e, file=sys.stderr) - - request.addfinalizer(end) - - await app.initialize([]) - await app.start() - - assert 'service-from-api' in app._service_map assert ( service_data['oauth_client_id'] in app.tornado_settings['oauth_no_confirm_list'] ) @@ -579,3 +527,11 @@ async def test_recreate_service_from_database(request, db_temp_path, service_dat .first() ) assert oauth_client.redirect_uri == service_data['oauth_redirect_uri'] + + # delete service from db, start one more + app.db.delete(service.orm) + app.db.commit() + + # start one more, service should be gone + app = await new_hub() + assert service_name not in app._service_map From 7dbb4ce1ff1a3952964a916960fbc97eb219d67d Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 8 Aug 2023 15:03:13 +0200 Subject: [PATCH 26/26] check 404 error when services don't exist --- jupyterhub/apihandlers/services.py | 10 ++-------- jupyterhub/tests/test_api.py | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/jupyterhub/apihandlers/services.py b/jupyterhub/apihandlers/services.py index 9d6b66a5..b9d23d84 100644 --- a/jupyterhub/apihandlers/services.py +++ b/jupyterhub/apihandlers/services.py @@ -31,6 +31,8 @@ class ServiceListAPIHandler(APIHandler): class ServiceAPIHandler(APIHandler): @needs_scope('read:services', 'read:services:name', 'read:roles:services') def get(self, service_name): + if service_name not in self.services: + raise web.HTTPError(404, f"No such service: {service_name}") service = self.services[service_name] self.write(json.dumps(self.service_model(service))) @@ -161,14 +163,6 @@ class ServiceAPIHandler(APIHandler): if service.oauth_client: self.oauth_provider.remove_client(service.oauth_client_id) - if service.api_token is not None: - # Remove api token from database - orm_token = ( - self.db.query(orm.APIToken).filter_by(service_id=orm_service.id).first() - ) - if orm_token is not None: - self.db.delete(orm_token) - if orm_service._server_id is not None: orm_server = ( self.db.query(orm.Server).filter_by(id=orm_service._server_id).first() diff --git a/jupyterhub/tests/test_api.py b/jupyterhub/tests/test_api.py index 6567b488..3a01dc7a 100644 --- a/jupyterhub/tests/test_api.py +++ b/jupyterhub/tests/test_api.py @@ -2064,7 +2064,7 @@ async def test_get_services(app, mockservice_url): async def test_get_service(app, mockservice_url): mockservice = mockservice_url db = app.db - r = await api_request(app, 'services/%s' % mockservice.name) + r = await api_request(app, f"services/{mockservice.name}") r.raise_for_status() assert r.status_code == 200 @@ -2083,15 +2083,19 @@ async def test_get_service(app, mockservice_url): } r = await api_request( app, - 'services/%s' % mockservice.name, + f"services/{mockservice.name}", headers={'Authorization': 'token %s' % mockservice.api_token}, ) r.raise_for_status() + r = await api_request( - app, 'services/%s' % mockservice.name, headers=auth_header(db, 'user') + app, f"services/{mockservice.name}", headers=auth_header(db, 'user') ) assert r.status_code == 403 + r = await api_request(app, "services/nosuchservice") + assert r.status_code == 404 + @pytest.fixture def service_admin_user(create_user_with_scopes): @@ -2294,7 +2298,7 @@ async def test_create_service_without_requested_scope( @mark.services -async def test_remove_service(app, service_admin_user, service_name, service_data): +async def test_delete_service(app, service_admin_user, service_name, service_data): db = app.db r = await api_request( app, @@ -2325,9 +2329,12 @@ async def test_remove_service(app, service_admin_user, service_name, service_dat assert service_name not in app._service_map + r = await api_request(app, f"services/{service_name}", method="delete") + assert r.status_code == 404 + @mark.services -async def test_remove_service_from_config(app, service_admin_user, mockservice): +async def test_delete_service_from_config(app, service_admin_user, mockservice): db = app.db service_name = mockservice.name r = await api_request(