From 77dbb1c96c5ca15bacadbe98c60fedee6548539f Mon Sep 17 00:00:00 2001 From: Markbeep Date: Fri, 14 Mar 2025 22:01:48 +0100 Subject: [PATCH] logout oidc, more settings info, group claim is optional --- README.md | 2 +- app/internal/auth/authentication.py | 15 +++++- app/internal/auth/oidc_config.py | 6 ++- app/internal/models.py | 6 +++ app/routers/auth.py | 49 +++++++++++++------ app/routers/settings.py | 6 ++- templates/settings_page/security.html | 69 ++++++++++++++++++++------- templates/settings_page/users.html | 15 +++++- 8 files changed, 131 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 7f448eb..0167860 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,7 @@ OIDC allows you to use an external authentication service (Authentik, Keycloak, - client id - client secret -In your auth server settings, make sure you allow for redirecting to `/auth/oidc`. The oidc-login flow will redirect you there after you log in. +In your auth server settings, make sure you allow for redirecting to `/auth/oidc`. The oidc-login flow will redirect you there after you log in. Additionally, the access token expiry time from the authentication server will be used if provided. This might be fairly low by default. Applying settings does not directly invalidate your current session. To test OIDC-settings, press the "log out" button to invalidate your current session. diff --git a/app/internal/auth/authentication.py b/app/internal/auth/authentication.py index fa9608b..73f57a4 100644 --- a/app/internal/auth/authentication.py +++ b/app/internal/auth/authentication.py @@ -1,3 +1,4 @@ +import time from typing import Annotated, Optional from argon2 import PasswordHasher @@ -91,10 +92,12 @@ class ABRAuth: ) -> DetailedUser: login_type = auth_config.get_login_type(session) - if login_type == LoginTypeEnum.forms or login_type == LoginTypeEnum.oidc: + if login_type == LoginTypeEnum.forms: user = await self._get_session_auth(request, session) elif login_type == LoginTypeEnum.none: user = await self._get_none_auth(session) + elif login_type == LoginTypeEnum.oidc: + user = await self._get_oidc_auth(request, session) else: user = await self._get_basic_auth(request, session) @@ -146,6 +149,16 @@ class ABRAuth: return user + async def _get_oidc_auth( + self, + request: Request, + session: Session, + ) -> User: + if exp := request.session.get("exp"): + if exp < time.time(): + raise RequiresLoginException() + return await self._get_session_auth(request, session) + async def _get_none_auth(self, session: Session) -> User: """Treats every request as being root by returning the first admin user""" if self.none_user: diff --git a/app/internal/auth/oidc_config.py b/app/internal/auth/oidc_config.py index 8912dd0..37f261a 100644 --- a/app/internal/auth/oidc_config.py +++ b/app/internal/auth/oidc_config.py @@ -16,6 +16,7 @@ oidcConfigKey = Literal[ "oidc_token_endpoint", "oidc_userinfo_endpoint", "oidc_authorize_endpoint", + "oidc_end_session_endpoint", ] @@ -41,6 +42,9 @@ class oidcConfig(StringConfigCache[oidcConfigKey]): ) self.set(session, "oidc_token_endpoint", data["token_endpoint"]) self.set(session, "oidc_userinfo_endpoint", data["userinfo_endpoint"]) + self.set( + session, "oidc_end_session_endpoint", data["end_session_endpoint"] + ) async def validate( self, session: Session, client_session: ClientSession @@ -72,7 +76,7 @@ class oidcConfig(StringConfigCache[oidcConfigKey]): return "Username claim is not supported by the provider" group_claim = self.get(session, "oidc_group_claim") - if not group_claim or group_claim not in provider_claims: + if group_claim and group_claim not in provider_claims: return "Group claim is not supported by the provider" diff --git a/app/internal/models.py b/app/internal/models.py index 00f69b9..1d4e314 100644 --- a/app/internal/models.py +++ b/app/internal/models.py @@ -25,6 +25,12 @@ class User(BaseModel, table=True): sa_column_kwargs={"server_default": "untrusted"}, ) root: bool = False + + # TODO: Add last_login + # last_login: datetime = Field( + # default_factory=datetime.now, sa_column_kwargs={"server_default": "now()"} + # ) + """ untrusted: Requests need to be manually reviewed trusted: Requests are automatically downloaded if possible diff --git a/app/routers/auth.py b/app/routers/auth.py index 46561cd..a8e9ecf 100644 --- a/app/routers/auth.py +++ b/app/routers/auth.py @@ -1,5 +1,6 @@ import base64 import secrets +import time from typing import Annotated, Optional from urllib.parse import urlencode @@ -79,10 +80,21 @@ async def login( @router.post("/logout") -def logout( - request: Request, user: Annotated[DetailedUser, Depends(get_authenticated_user())] +async def logout( + request: Request, + user: Annotated[DetailedUser, Depends(get_authenticated_user())], + session: Annotated[Session, Depends(get_session)], ): request.session["sub"] = "" + + login_type = auth_config.get_login_type(session) + if login_type == LoginTypeEnum.oidc: + end_session_endpoint = oidc_config.get(session, "oidc_end_session_endpoint") + if end_session_endpoint: + return Response( + status_code=status.HTTP_204_NO_CONTENT, + headers={"HX-Redirect": end_session_endpoint}, + ) return Response( status_code=status.HTTP_204_NO_CONTENT, headers={"HX-Redirect": "/login"} ) @@ -143,8 +155,6 @@ async def login_oidc( raise InvalidOIDCConfiguration("Missing OIDC client secret") if not username_claim: raise InvalidOIDCConfiguration("Missing OIDC username claim") - if not group_claim: - raise InvalidOIDCConfiguration("Missing OIDC group claim") base_url = str(request.base_url).rstrip("/") @@ -162,7 +172,7 @@ async def login_oidc( ) as response: body = await response.json() - access_token = body.get("access_token") + access_token: Optional[str] = body.get("access_token") if not access_token: return Response(status_code=status.HTTP_401_UNAUTHORIZED) @@ -176,9 +186,12 @@ async def login_oidc( if not username: raise InvalidOIDCConfiguration("Missing username claim") - groups: list[str] | str = userinfo.get(group_claim, []) - if isinstance(groups, str): - groups = groups.split(" ") + if group_claim: + groups: list[str] | str = userinfo.get(group_claim, []) + if isinstance(groups, str): + groups = groups.split(" ") + else: + groups = [] user = session.exec(select(User).where(User.username == username)).first() if not user: @@ -190,22 +203,28 @@ async def login_oidc( # Don't overwrite the group if the user is root admin if not user.root: - ensure_group = GroupEnum.untrusted for group in groups: if group.lower() == "admin": - ensure_group = GroupEnum.admin + user.group = GroupEnum.admin break elif group.lower() == "trusted": - ensure_group = GroupEnum.trusted + user.group = GroupEnum.trusted break elif group.lower() == "untrusted": - ensure_group = GroupEnum.untrusted + user.group = GroupEnum.untrusted break - user.group = ensure_group - session.add(user) - session.commit() + + session.add(user) + session.commit() + + expires_in: int = body.get( + "expires_in", + auth_config.get_access_token_expiry_minutes(session) * 60, + ) + expires = int(time.time() + expires_in) request.session["sub"] = username + request.session["exp"] = expires # We can't redirect server side, because that results in an infinite loop. # The session token is never correctly set causing any other endpoint to diff --git a/app/routers/settings.py b/app/routers/settings.py index fabfe89..c78eb76 100644 --- a/app/routers/settings.py +++ b/app/routers/settings.py @@ -92,11 +92,12 @@ def read_users( session: Annotated[Session, Depends(get_session)], ): users = session.exec(select(User)).all() + is_oidc = auth_config.get_login_type(session) == LoginTypeEnum.oidc return template_response( "settings_page/users.html", request, admin_user, - {"page": "users", "users": users}, + {"page": "users", "users": users, "is_oidc": is_oidc}, ) @@ -721,7 +722,8 @@ async def update_security( oidc_config.set(session, "oidc_scope", oidc_scope) if oidc_username_claim: oidc_config.set(session, "oidc_username_claim", oidc_username_claim) - if oidc_group_claim: + + if oidc_group_claim is not None: oidc_config.set(session, "oidc_group_claim", oidc_group_claim) error_message = await oidc_config.validate(session, client_session) diff --git a/templates/settings_page/security.html b/templates/settings_page/security.html index 410e93f..919c385 100644 --- a/templates/settings_page/security.html +++ b/templates/settings_page/security.html @@ -51,7 +51,7 @@ -