diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 146c7743..9a409989 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -18,10 +18,10 @@ jobs: strategy: matrix: os: ['ubuntu-latest'] - python: ['3.7', '3.8', '3.9'] + python: ['3.8', '3.9'] include: - os: 'macos-latest' - python: '3.7' + python: '3.8' steps: - name: Checkout uses: actions/checkout@v3 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b22e25fb..7df5d2d5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,10 +13,10 @@ jobs: strategy: matrix: os: ['ubuntu-latest'] - python: ['3.7', '3.8', '3.9'] + python: ['3.8', '3.9'] include: - os: 'macos-latest' - python: '3.7' + python: '3.8' env: PYTEST_ADDOPTS: --cov --color=yes diff --git a/cylc/uiserver/handlers.py b/cylc/uiserver/handlers.py index aecef402..43e4c9d6 100644 --- a/cylc/uiserver/handlers.py +++ b/cylc/uiserver/handlers.py @@ -25,6 +25,11 @@ from graphql import get_default_backend from graphql_ws.constants import GRAPHQL_WS from jupyter_server.base.handlers import JupyterHandler +from jupyter_server.auth.identity import ( + User as JPSUser, + IdentityProvider as JPSIdentityProvider, + PasswordIdentityProvider, +) from tornado import web, websocket from tornado.ioloop import IOLoop @@ -61,60 +66,42 @@ def _inner( **kwargs, ): nonlocal fun - user: Union[ - None, # unauthenticated - dict, # hub auth - str, # token auth or anonymous - - ] = handler.get_current_user() - if user is None or user == 'anonymous': - # user is not authenticated - calls should not get this far - # but the extra safety doesn't hurt - # NOTE: Auth tests will hit this line unless mocked authentication - # is provided. - raise web.HTTPError(403, reason='Forbidden') - if not ( - isinstance(user, str) # token authenticated - or ( - isinstance(user, dict) - and _authorise(handler, user['name']) - ) - ): + user: JPSUser = handler.current_user + + if not user or not user.username: + # the user is only truthy if they have authenticated successfully + raise web.HTTPError(403, reason='authorization insufficient') + + if not handler.identity_provider.auth_enabled: + # if authentication is turned off we don't want to work with this raise web.HTTPError(403, reason='authorization insufficient') + + if is_token_authenticated(handler): + # token or password authenticated, the bearer of the token or + # password has full control + pass + + elif not _authorise(handler, user.username): + # other authentication (e.g. JupyterHub auth), check the user has + # read permissions for Cylc + raise web.HTTPError(403, reason='authorization insufficient') + return fun(handler, *args, **kwargs) return _inner -def is_token_authenticated( - handler: JupyterHandler, - user: Union[bytes, dict, str], -) -> bool: - """Returns True if the UIS is running "standalone". +def is_token_authenticated(handler: 'CylcAppHandler') -> bool: + """Returns True if this request is bearer token authenticated. - At present we cannot use handler.is_token_authenticated because it - returns False when the token is cached in a cookie. + E.G. The default single-user token-based authenticated. - https://github.com/jupyter-server/jupyter_server/pull/562 + In these cases the bearer of the token is awarded full privileges. """ - if isinstance(user, bytes): # noqa: SIM114 - # Cookie authentication: - # * The URL token is added to a secure cookie, it can then be - # removed from the URL for subsequent requests, the cookie is - # used in its place. - # * If the token was used token_authenticated is True. - # * If the cookie was used it is False (despite the cookie auth - # being derived from token auth). - # * Due to a bug in jupyter_server the user is returned as bytes - # when cookie auth is used so at present we can use this to - # tell. - # https://github.com/jupyter-server/jupyter_server/pull/562 - # TODO: this hack is obviously not suitable for production! - return True - elif handler.token_authenticated: - # standalone UIS, the bearer of the token is the owner - # (no multi-user functionality so no further auth required) - return True - return False + identity_provider: JPSIdentityProvider = ( + handler.serverapp.identity_provider + ) + return identity_provider.__class__ == PasswordIdentityProvider + # NOTE: not using isinstance to narrow this down to just the one class def _authorise( @@ -136,28 +123,24 @@ def _authorise( return False -def parse_current_user(current_user): - """Standardises and returns the current user.""" - if isinstance(current_user, dict): - # the server is running with authentication services provided - # by a hub - current_user = dict(current_user) # make a copy for safety - return current_user +def get_username(handler: 'CylcAppHandler'): + """Return the username for the authenticated user. + + If the handler is token authenticated, then we return the username of the + account that this server instance is running under. + """ + if is_token_authenticated(handler): + # the bearer of the token has full privileges + return ME else: - # the server is running using a token - # authentication is provided by jupyter server - return { - 'kind': 'user', - 'name': ME, - 'server': socket.gethostname() - } + return handler.current_user.username class CylcAppHandler(JupyterHandler): """Base handler for Cylc endpoints. This handler adds the Cylc authorisation layer which is triggered by - calling CylcAppHandler.get_current_user which is called by + accessing CylcAppHandler.current_user which is called by web.authenticated. When running as a Hub application the make_singleuser_app method patches @@ -171,11 +154,7 @@ class CylcAppHandler(JupyterHandler): def initialize(self, auth): self.auth = auth - - # Without this, there is no xsrf token from the GET which causes a 403 and - # a missing _xsrf argument error on the first POST. - def prepare(self): - _ = self.xsrf_token + super().initialize() @property def hub_users(self): @@ -266,11 +245,11 @@ def set_default_headers(self) -> None: self.set_header("Content-Type", 'application/json') @web.authenticated - @authorised + # @authorised TODO: I can't think why we would want to authorise this def get(self): - user_info = self.get_current_user() - - user_info = parse_current_user(user_info) + user_info = { + 'name': get_username(self) + } # add an entry for the workflow owner # NOTE: when running behind a hub this may be different from the @@ -283,6 +262,7 @@ def get(self): self.auth.get_permitted_operations(user_info['name'])) ] # Pass the gui mode to the ui + # (used for functionality not security) if not os.environ.get("JUPYTERHUB_SINGLEUSER_APP"): user_info['mode'] = 'single user' else: @@ -337,15 +317,9 @@ def context(self): 'graphql_params': self.graphql_params, 'request': self.request, 'resolvers': self.resolvers, - 'current_user': parse_current_user( - self.get_current_user() - ).get('name'), + 'current_user': get_username(self), } - @web.authenticated - def prepare(self): - super().prepare() - @web.authenticated # type: ignore[arg-type] async def execute(self, *args, **kwargs) -> 'ExecutionResult': # Use own backend, and TornadoGraphQLHandler already does validation. @@ -411,9 +385,7 @@ def context(self): return { 'request': self.request, 'resolvers': self.resolvers, - 'current_user': parse_current_user( - self.get_current_user() - ).get('name'), + 'current_user': get_username(self), 'ops_queue': {}, 'sub_statuses': self.sub_statuses } diff --git a/cylc/uiserver/tests/conftest.py b/cylc/uiserver/tests/conftest.py index fc9f7da9..6391b2cc 100644 --- a/cylc/uiserver/tests/conftest.py +++ b/cylc/uiserver/tests/conftest.py @@ -29,6 +29,8 @@ from traitlets.config import Config import zmq +from jupyter_server.auth.identity import User + from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.id import Tokens from cylc.flow.data_messages_pb2 import ( # type: ignore @@ -201,59 +203,86 @@ def authorisation_false(monkeypatch): @pytest.fixture def mock_authentication(monkeypatch: pytest.MonkeyPatch): - - def _mock_authentication(user=None, server=None, none=False): - ret = { - 'name': user or getuser(), - 'server': server or gethostname() - } - if none: - ret = None - monkeypatch.setattr( - 'cylc.uiserver.handlers.parse_current_user', - lambda x: { - 'kind': 'user', - 'name': None, - 'server': 'some_server' - } - ) - - def mock_redirect(*args): - # normally tornado would attempt to redirect us to the login - # page - for testing purposes we will skip this and raise - # a 403 with an explanatory reason - raise HTTPError( - 403, - reason='login redirect replaced by 403 for test purposes' - ) - - monkeypatch.setattr( - 'cylc.uiserver.handlers.CylcAppHandler.redirect', - mock_redirect - ) - + def _mock_authentication(username=None, is_token_authenticated=False): monkeypatch.setattr( - 'cylc.uiserver.handlers.CylcAppHandler.get_current_user', - lambda x: ret + 'cylc.uiserver.handlers.is_token_authenticated', + lambda x: is_token_authenticated, ) + + user = None + if username: + user = User(username) + monkeypatch.setattr( - 'cylc.uiserver.handlers.CylcAppHandler.get_login_url', - lambda x: "http://cylc" + 'cylc.uiserver.handlers.CylcAppHandler.current_user', + user, ) + # monkeypatch.setattr( + # 'cylc.uiserver.handlers.CylcAppHandler.get_login_url', + # lambda x: "http://cylc" + # ) + _mock_authentication() return _mock_authentication +# @pytest.fixture +# def mock_authentication(monkeypatch: pytest.MonkeyPatch): +# +# def _mock_authentication(user=None, server=None, none=False): +# ret = { +# 'name': user or getuser(), +# 'server': server or gethostname() +# } +# if none: +# ret = None +# monkeypatch.setattr( +# 'cylc.uiserver.handlers.parse_current_user', +# lambda x: { +# 'kind': 'user', +# 'name': None, +# 'server': 'some_server' +# } +# ) +# +# def mock_redirect(*args): +# # normally tornado would attempt to redirect us to the login +# # page - for testing purposes we will skip this and raise +# # a 403 with an explanatory reason +# raise HTTPError( +# 403, +# reason='login redirect replaced by 403 for test purposes' +# ) +# +# monkeypatch.setattr( +# 'cylc.uiserver.handlers.CylcAppHandler.redirect', +# mock_redirect +# ) +# +# monkeypatch.setattr( +# 'cylc.uiserver.handlers.CylcAppHandler.current_user', +# ret, +# ) +# monkeypatch.setattr( +# 'cylc.uiserver.handlers.CylcAppHandler.get_login_url', +# lambda x: "http://cylc" +# ) +# +# _mock_authentication() +# +# return _mock_authentication + + @pytest.fixture def mock_authentication_yossarian(mock_authentication): - mock_authentication(user='yossarian') + mock_authentication(username='yossarian') @pytest.fixture def mock_authentication_none(mock_authentication): - mock_authentication(none=True) + mock_authentication(username=None) @pytest.fixture diff --git a/cylc/uiserver/tests/test_auth.py b/cylc/uiserver/tests/test_auth.py index 5cb9e3ea..a1744293 100644 --- a/cylc/uiserver/tests/test_auth.py +++ b/cylc/uiserver/tests/test_auth.py @@ -32,7 +32,7 @@ async def test_cylc_handler(patch_conf_files, jp_fetch): @pytest.mark.integration -@pytest.mark.usefixtures("mock_authentication") +@pytest.mark.usefixtures("mock_authentication_yossarian") @pytest.mark.parametrize( 'endpoint,code,message,body', [ @@ -77,8 +77,8 @@ async def test_authorised_and_authenticated( [ pytest.param( ('cylc', 'graphql'), - 403, - 'login redirect replaced by 403 for test purposes', + 400, + 'Bad Request', None, id='cylc/graphql', ), diff --git a/cylc/uiserver/tests/test_handlers.py b/cylc/uiserver/tests/test_handlers.py index cf79dd05..273bf12c 100644 --- a/cylc/uiserver/tests/test_handlers.py +++ b/cylc/uiserver/tests/test_handlers.py @@ -56,7 +56,7 @@ def _create_handler(self, logged_in=True): handler.get _current_user = lambda: {'name': getuser()} else: - handler.get_current_user = lambda: None + handler.current_user = None return handler @pytest.mark.usefixtures("mock_authentication") diff --git a/setup.cfg b/setup.cfg index 6b3315b1..2fdecbad 100644 --- a/setup.cfg +++ b/setup.cfg @@ -33,7 +33,6 @@ classifiers = License :: OSI Approved :: GNU General Public License v3 (GPLv3) Operating System :: POSIX Programming Language :: Python - Programming Language :: Python :: 3.7 Programming Language :: Python :: 3.8 Programming Language :: Python :: 3.9 Programming Language :: Python :: 3 :: Only @@ -41,7 +40,7 @@ classifiers = [options] packages = find_namespace: -python_requires = >=3.7 +python_requires = >=3.8 include_package_data = True install_requires = # NB: We have cylc-flow at the top to force it to install its transitive @@ -55,7 +54,7 @@ install_requires = graphene graphene-tornado==2.6.* graphql-ws==0.4.4 - jupyter_server>=1.10.2, <2.0 + jupyter_server>=2.0 requests tornado>=6.1.0 # matches jupyter_server value traitlets>=5.2.1 # required for logging_config (5.2.0 had bugs) @@ -95,6 +94,7 @@ tests = flake8-mutable>=1.2.0 flake8-simplify>=0.14.0 flake8>=3.0.0 + jupyter_server[test] mypy>=0.900 pytest-cov>=2.8.0 pytest-tornasync>=0.5.0