Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to have authentication enabled for all endpoints by default #1392

Merged
merged 20 commits into from
Mar 4, 2024
Merged
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3f65147
Add `allow_unauthenticated_access` traitlet and `@allow_unauthenticated`
krassowski Feb 10, 2024
a9cff16
Tests, correct coroutine handling, add decorators where needed,
krassowski Feb 10, 2024
646739e
Align ordering of test cases, improve wording of comments
krassowski Feb 11, 2024
4cbb504
Implement auth and tests for websockets
krassowski Feb 11, 2024
faee488
Use `allow_unauthenticated` in `FilesRedirectHandler` for now
krassowski Feb 11, 2024
204d29f
Do not touch coroutines
krassowski Feb 11, 2024
dccc423
Add runtime check to ensure handlers have auth decorators
krassowski Feb 12, 2024
8e13727
Do not ever raise for extension endpoints, and only
krassowski Feb 13, 2024
e6a8d9a
Add test for extension handler warning, correct type
krassowski Feb 13, 2024
2882516
Fix test for authentication enforcement for extensions
krassowski Feb 13, 2024
4e1d664
Add test for GET redirect in handler tests
krassowski Feb 13, 2024
cd84175
Add `JUPYTER_SERVER_ALLOW_UNAUTHENTICATED_ACCESS` env variable
krassowski Feb 13, 2024
5f6bc16
Better heuristic for `@tornado.web.authenticated`
krassowski Feb 13, 2024
0facfec
Improve docstring/fix typo
krassowski Feb 13, 2024
90d7044
Improve and test tornado.web.authenticated heuristic
krassowski Feb 13, 2024
54c2ea2
Merge branch 'main' into auth-by-default
krassowski Feb 15, 2024
319a4d1
Call `super.prepare()` in all code paths, test it
krassowski Feb 20, 2024
5e7615d
Add test for identity provider being used
krassowski Feb 20, 2024
4265f4e
Ensure that identity provider is used for auth,
krassowski Feb 20, 2024
c3f5d8f
Update minimum `pytest-jupyter`; we need 0.7 as it
krassowski Feb 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Ensure that identity provider is used for auth,
even in websockets (but only if those inherit from JupyterHandler,
and if they do not fallback to previous implementation and warn).
krassowski committed Feb 20, 2024
commit 4265f4e6df6dcf60164a6bfcf158cdb03910e819
21 changes: 15 additions & 6 deletions jupyter_server/base/handlers.py
Original file line number Diff line number Diff line change
@@ -589,7 +589,7 @@ def check_host(self) -> bool:
)
return allow

async def prepare(self) -> Awaitable[None] | None: # type:ignore[override]
async def prepare(self, *, _redirect_to_login=True) -> Awaitable[None] | None: # type:ignore[override]
Copy link
Contributor

Choose a reason for hiding this comment

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

private keyword arg makes this hard for subclasses, e.g. extensions with websockets, but I suppose they will inherit from our own websocket classes, too?

I wonder if this should be a Handler class attribute, rather than a private argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a check for inheritance from JupyterHandler in the weboscket mixin class:

if not isinstance(self, JupyterHandler):
should_authenticate = not self.settings.get("allow_unauthenticated_access", False)
if "identity_provider" in self.settings and should_authenticate:
warnings.warn(
"WebSocketMixin sub-class does not inherit from JupyterHandler"
" preventing proper authentication using custom identity provider.",
JupyterServerAuthWarning,
stacklevel=2,
)
self._maybe_auth()
return super().prepare(*args, **kwargs)
return super().prepare(*args, **kwargs, _redirect_to_login=False)

A Handler class attribute is certainly a reasonable alternative that I briefly considered. My thinking was along: if you pass _redirect_to_login argument but are inheriting from wrong class you will get an error. If you set a class attribute but are inheriting from a wrong class you would not get anything. The downside of passing argument is that mypy typing might complain downstream.

"""Prepare a response."""
# Set the current Jupyter Handler context variable.
CallContext.set(CallContext.JUPYTER_HANDLER, self)
@@ -636,9 +636,18 @@ async def prepare(self) -> Awaitable[None] | None: # type:ignore[override]
raise HTTPError(403)
method = getattr(self, self.request.method.lower())
if not getattr(method, "__allow_unauthenticated", False):
# reuse `web.authenticated` logic, which redirects to the login
# page on GET and HEAD and otherwise raises 403
return web.authenticated(lambda _: super().prepare)(self)
if _redirect_to_login:
# reuse `web.authenticated` logic, which redirects to the login
# page on GET and HEAD and otherwise raises 403
return web.authenticated(lambda _: super().prepare())(self)
else:
# raise 403 if user is not known without redirecting to login page
user = self.current_user
if user is None:
self.log.warning(
f"Couldn't authenticate {self.__class__.__name__} connection"
)
raise web.HTTPError(403)

return super().prepare()

@@ -736,7 +745,7 @@ def write_error(self, status_code: int, **kwargs: Any) -> None:
class APIHandler(JupyterHandler):
"""Base class for API handlers"""

async def prepare(self) -> None:
async def prepare(self) -> None: # type:ignore[override]
"""Prepare an API response."""
await super().prepare()
Copy link
Contributor

Choose a reason for hiding this comment

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

Something for later - API handlers should probably 403, not redirect. Now that we have logic for this.

if not self.check_origin():
@@ -848,7 +857,7 @@ def options(self, *args: Any, **kwargs: Any) -> None:
class Template404(JupyterHandler):
"""Render our 404 template"""

async def prepare(self) -> None:
async def prepare(self) -> None: # type:ignore[override]
"""Prepare a 404 response."""
await super().prepare()
raise web.HTTPError(404)
23 changes: 20 additions & 3 deletions jupyter_server/base/websocket.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
"""Base websocket classes."""
import re
import warnings
from typing import Optional, no_type_check
from urllib.parse import urlparse

from tornado import ioloop, web
from tornado.iostream import IOStream

from jupyter_server.base.handlers import JupyterHandler
from jupyter_server.utils import JupyterServerAuthWarning

# ping interval for keeping websockets alive (30 seconds)
WS_PING_INTERVAL = 30000

@@ -84,7 +88,10 @@ def clear_cookie(self, *args, **kwargs):

@no_type_check
def _maybe_auth(self):
"""Verify authentication if required"""
"""Verify authentication if required.

Only used when the websocket class does not inherit from JupyterHandler.
"""
if not self.settings.get("allow_unauthenticated_access", False):
if not self.request.method:
raise web.HTTPError(403)
@@ -100,8 +107,18 @@ def _maybe_auth(self):
@no_type_check
def prepare(self, *args, **kwargs):
"""Handle a get request."""
self._maybe_auth()
return super().prepare(*args, **kwargs)
if not isinstance(self, JupyterHandler):
should_authenticate = not self.settings.get("allow_unauthenticated_access", False)
if "identity_provider" in self.settings and should_authenticate:
warnings.warn(
"WebSocketMixin sub-class does not inherit from JupyterHandler"
" preventing proper authentication using custom identity provider.",
JupyterServerAuthWarning,
stacklevel=2,
)
self._maybe_auth()
return super().prepare(*args, **kwargs)
return super().prepare(*args, **kwargs, _redirect_to_login=False)

@no_type_check
def open(self, *args, **kwargs):
48 changes: 43 additions & 5 deletions tests/base/test_websocket.py
Original file line number Diff line number Diff line change
@@ -11,9 +11,10 @@

from jupyter_server.auth import IdentityProvider, User
from jupyter_server.auth.decorator import allow_unauthenticated
from jupyter_server.base.handlers import JupyterHandler
from jupyter_server.base.websocket import WebSocketMixin
from jupyter_server.serverapp import ServerApp
from jupyter_server.utils import url_path_join
from jupyter_server.utils import JupyterServerAuthWarning, url_path_join


class MockHandler(WebSocketMixin, WebSocketHandler):
@@ -66,11 +67,15 @@ async def test_ping_client_timeout(mixin):
mixin.write_message("hello")


class NoAuthRulesWebsocketHandler(MockHandler):
class MockJupyterHandler(MockHandler, JupyterHandler):
pass


class PermissiveWebsocketHandler(MockHandler):
class NoAuthRulesWebsocketHandler(MockJupyterHandler):
pass


class PermissiveWebsocketHandler(MockJupyterHandler):
@allow_unauthenticated
def get(self, *args, **kwargs) -> None:
return super().get(*args, **kwargs)
@@ -148,5 +153,38 @@ def fetch():
iidp = IndiscriminateIdentityProvider()
# should allow access with the user set be the identity provider
with patch.dict(jp_serverapp.web_app.settings, {"identity_provider": iidp}):
res = await fetch()
assert res.code == 200
ws = await fetch()
ws.close()


class PermissivePlainWebsocketHandler(MockHandler):
# note: inherits from MockHandler not MockJupyterHandler
@allow_unauthenticated
def get(self, *args, **kwargs) -> None:
return super().get(*args, **kwargs)


@pytest.mark.parametrize(
"jp_server_config",
[
{
"ServerApp": {
"allow_unauthenticated_access": False,
"identity_provider": IndiscriminateIdentityProvider(),
}
}
],
)
async def test_websocket_auth_warns_mixin_lacks_jupyter_handler(jp_serverapp, jp_ws_fetch):
app: ServerApp = jp_serverapp
app.web_app.add_handlers(
".*$",
[(url_path_join(app.base_url, "permissive"), PermissivePlainWebsocketHandler)],
)

with pytest.warns(
JupyterServerAuthWarning,
match="WebSocketMixin sub-class does not inherit from JupyterHandler",
):
ws = await jp_ws_fetch("permissive", headers={"Authorization": ""})
ws.close()