From 6e2c4d8357deb1c32349691665dfc7719396da47 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 20 May 2021 13:27:42 +0200 Subject: [PATCH 1/4] handle async functions in check_db_locks check_db_locks checks for db lock state after the end of a function, but wasn't properly waiting when it wrapped an async function, meaning it would run the check while the async function was still outstanding, causing possible spurious failures --- jupyterhub/tests/utils.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/jupyterhub/tests/utils.py b/jupyterhub/tests/utils.py index 73ddcf51..fb51bc3b 100644 --- a/jupyterhub/tests/utils.py +++ b/jupyterhub/tests/utils.py @@ -1,4 +1,5 @@ import asyncio +import inspect import os from concurrent.futures import ThreadPoolExecutor @@ -80,14 +81,23 @@ def check_db_locks(func): """ def new_func(app, *args, **kwargs): - retval = func(app, *args, **kwargs) + maybe_future = func(app, *args, **kwargs) - temp_session = app.session_factory() - temp_session.execute('CREATE TABLE dummy (foo INT)') - temp_session.execute('DROP TABLE dummy') - temp_session.close() + def _check(_=None): + with app.session_factory() as temp_session: + temp_session.execute('CREATE TABLE dummy (foo INT)') + temp_session.execute('DROP TABLE dummy') - return retval + async def await_then_check(): + result = await maybe_future + _check() + return result + + if inspect.isawaitable(maybe_future): + return await_then_check() + else: + _check() + return maybe_future return new_func From af6884bb7d15b149c4b9696e0cf70aabc3c27dc1 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 20 May 2021 13:33:02 +0200 Subject: [PATCH 2/4] oldest suppported sqlalchemy doesn't have session context managers --- jupyterhub/tests/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jupyterhub/tests/utils.py b/jupyterhub/tests/utils.py index fb51bc3b..d300c84a 100644 --- a/jupyterhub/tests/utils.py +++ b/jupyterhub/tests/utils.py @@ -84,9 +84,12 @@ def check_db_locks(func): maybe_future = func(app, *args, **kwargs) def _check(_=None): - with app.session_factory() as temp_session: + temp_session = app.session_factory() + try: temp_session.execute('CREATE TABLE dummy (foo INT)') temp_session.execute('DROP TABLE dummy') + finally: + temp_session.close() async def await_then_check(): result = await maybe_future From 02619b687f58f2427d61cdbccd40edebf6708368 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 20 May 2021 13:48:37 +0200 Subject: [PATCH 3/4] cleanup after failure to create token due to permisison errors have to delete tokens explicitly if we fail to finish creating them --- jupyterhub/orm.py | 24 ++++++++++++++++++------ jupyterhub/roles.py | 2 +- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index e78fcc02..02529f1a 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -3,6 +3,7 @@ # Distributed under the terms of the Modified BSD License. import enum import json +import warnings from base64 import decodebytes from base64 import encodebytes from datetime import datetime @@ -674,18 +675,29 @@ class APIToken(Hashed, Base): orm_token.service = service if expires_in is not None: orm_token.expires_at = cls.now() + timedelta(seconds=expires_in) + db.add(orm_token) - # load default roles if they haven't been initiated - # correct to have this here? otherwise some tests fail token_role = Role.find(db, 'token') if not token_role: + # FIXME: remove this. + # Creating a token before the db has roles defined should raise an error. + # PR #3460 should let us fix it by ensuring default roles are define + + warnings.warn( + "Token created before default roles!", RuntimeWarning, stacklevel=2 + ) default_roles = get_default_roles() for role in default_roles: create_role(db, role) - if roles is not None: - update_roles(db, entity=orm_token, roles=roles) - else: - assign_default_roles(db, entity=orm_token) + try: + if roles is not None: + update_roles(db, entity=orm_token, roles=roles) + else: + assign_default_roles(db, entity=orm_token) + except Exception: + db.delete(orm_token) + db.commit() + raise db.commit() return token diff --git a/jupyterhub/roles.py b/jupyterhub/roles.py index d6ade30c..dbd2e75a 100644 --- a/jupyterhub/roles.py +++ b/jupyterhub/roles.py @@ -435,11 +435,11 @@ def assign_default_roles(db, entity): """Assigns the default roles to an entity: users and services get 'user' role, or admin role if they have admin flag Tokens get 'token' role""" - default_token_role = orm.Role.find(db, 'token') if isinstance(entity, orm.Group): pass elif isinstance(entity, orm.APIToken): app_log.debug('Assigning default roles to tokens') + default_token_role = orm.Role.find(db, 'token') if not entity.roles and (entity.user or entity.service) is not None: default_token_role.tokens.append(entity) app_log.info('Added role %s to token %s', default_token_role.name, entity) From 478ae8a744272758035ae9e62d47cb871054d888 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 21 May 2021 15:36:14 +0200 Subject: [PATCH 4/4] typo in comment Co-authored-by: Ivana --- jupyterhub/orm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jupyterhub/orm.py b/jupyterhub/orm.py index 02529f1a..b16926ac 100644 --- a/jupyterhub/orm.py +++ b/jupyterhub/orm.py @@ -681,7 +681,7 @@ class APIToken(Hashed, Base): if not token_role: # FIXME: remove this. # Creating a token before the db has roles defined should raise an error. - # PR #3460 should let us fix it by ensuring default roles are define + # PR #3460 should let us fix it by ensuring default roles are defined warnings.warn( "Token created before default roles!", RuntimeWarning, stacklevel=2