Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
  • Loading branch information
minrk and manics authored Apr 2, 2024
1 parent 1feb356 commit 3e61e46
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 12 deletions.
16 changes: 6 additions & 10 deletions docs/source/reference/authenticators.md
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ authorization
It is perfectly fine in the simplest cases for `Authenticator.authenticate` to be responsible for authentication _and_ authorization,
in which case `authenticate` may return `None` if the user is not authorized.

However, Authenticators also have have two methods {meth}`~.Authenticator.check_allowed` and {meth}`~.Authenticator.check_blocked_users`, which are called after successful authentication to further check if the user is allowed.
However, Authenticators also have two methods, {meth}`~.Authenticator.check_allowed` and {meth}`~.Authenticator.check_blocked_users`, which are called after successful authentication to further check if the user is allowed.

If `check_blocked_users()` returns False, authorization stops and the user is not allowed.

Expand All @@ -227,7 +227,7 @@ which is a change from pre-5.0, where `allow_all` was implicitly True if `allowe
### Overriding `check_allowed`

:::{versionchanged} 5.0
`check_allowed()` is **not called** is `allow_all` is True.
`check_allowed()` is **not called** if `allow_all` is True.
:::

:::{versionchanged} 5.0
Expand All @@ -242,14 +242,10 @@ The base implementation of {meth}`~.Authenticator.check_allowed` checks:
- else return False

:::{versionchanged} 5.0
Prior to 5.0, the check was
Prior to 5.0, this would also return True if `allowed_users` was empty.

```python
if (not allowed_users) or username in allowed_users:
```

but the implicit `not allowed_users` has been replaced by explicit `allow_all`, which is checked _before_ calling `check_allowed`.
`check_allowed` **is not called** if `allow_all` is True.
For clarity, this is no longer the case. A new `allow_all` property (default False) has been added which is checked _before_ calling `check_allowed`.
If `allow_all` is True, this takes priority over `check_allowed`, which will be ignored.

If your Authenticator subclass similarly returns True when no allow config is defined,
this is fully backward compatible for your users, but means `allow_all = False` has no real effect.
Expand Down Expand Up @@ -349,7 +345,7 @@ So the logical expression for a user being authorized should look like:
#### Custom error messages

Any of these authentication and authorization methods may
Any of these authentication and authorization methods may raise a `web.HTTPError` Exception

```python
from tornado import web
Expand Down
4 changes: 2 additions & 2 deletions jupyterhub/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def _deprecated_db(self):
False,
help="""Is there any allow config?
Used to show a warning if it looks like nobody can access the HUb,
Used to show a warning if it looks like nobody can access the Hub,
which can happen when upgrading to JupyterHub 5,
now that `allow_all` defaults to False.
Expand Down Expand Up @@ -199,7 +199,7 @@ def check_allow_config(self):
Allow every user who can successfully authenticate to access JupyterHub.
False by default, which means for most Authenticators,
_some_ allow-related configuration is required to allow any users to log in.
_some_ allow-related configuration is required to allow users to log in.
Authenticator subclasses may override the default with e.g.::
Expand Down

0 comments on commit 3e61e46

Please sign in to comment.