OIDC implementation #1701
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import asyncio | ||
| import random | ||
| import secrets | ||
| import logging | ||
|
|
||
| from aiohttp import web | ||
| from aiohttp_security import authorized_userid, forget, remember | ||
|
|
@@ -11,7 +13,7 @@ | |
| from ...db.models.user import User | ||
| from ...mail import send_mail | ||
| from ...state.auth import auth_state | ||
| from .oidc import oidc_auth | ||
|
|
||
| async def is_authed(request): | ||
| user = await get_authorized_user(request) | ||
|
|
@@ -25,6 +27,8 @@ async def is_authed(request): | |
|
|
||
|
|
||
| async def login(request): | ||
| if "local" not in cfg().general.authentication_methods: | ||
| return web.HTTPForbidden(reason="Local authentication is disabled") | ||
| try: | ||
| user = await get_authorized_user(request) | ||
| except web.HTTPUnauthorized: | ||
|
|
@@ -83,6 +87,8 @@ async def logout(request): | |
|
|
||
|
|
||
| async def forgot_password(request): | ||
| if "local" not in cfg().general.authentication_methods: | ||
| return web.HTTPForbidden(reason="Local authentication is disabled") | ||
| data = await request.json() | ||
| email = data["email"] | ||
| user = User.by_email(email) | ||
|
|
@@ -110,6 +116,8 @@ async def forgot_password(request): | |
|
|
||
|
|
||
| async def reset_password(request): | ||
| if "local" not in cfg().general.authentication_methods: | ||
| return web.HTTPForbidden(reason="Local authentication is disabled") | ||
| data = await request.json() | ||
| token = data["token"] | ||
| password = data["password"] | ||
|
|
@@ -123,3 +131,87 @@ async def reset_password(request): | |
| user.save() | ||
|
|
||
| return web.HTTPOk() | ||
|
|
||
| # OIDC Authentication endpoints | ||
|
|
||
| async def oidc_providers(request): | ||
| if "oidc" not in cfg().general.authentication_methods: | ||
| return web.HTTPForbidden(reason="OIDC authentication is disabled") | ||
| providers = oidc_auth.get_providers() | ||
| return web.json_response({"providers": providers}) | ||
|
|
||
| async def oidc_login(request): | ||
| logger = logging.getLogger("PlanarAllyServer") | ||
|
nwhansen marked this conversation as resolved.
|
||
| if "oidc" not in cfg().general.authentication_methods: | ||
| logger.warning(f"OIDC authentication attempted but disabled in config") | ||
| return web.HTTPForbidden(reason="OIDC authentication is disabled") | ||
| try: | ||
| data = await request.json() | ||
| except Exception as e: | ||
| return web.HTTPBadRequest(reason="Invalid request format") | ||
|
|
||
| # Grab the state from the browser's request ensuring we can validate it later | ||
| provider_name = data.get("provider_name") | ||
| if not provider_name: | ||
| return web.HTTPBadRequest(reason="Missing required parameters") | ||
|
|
||
| auth_url = await oidc_auth.get_authorization_url(provider_name) | ||
|
|
||
| if not auth_url: | ||
| return web.HTTPInternalServerError(reason="Failed to initiate OIDC login") | ||
| logger.debug(f"Redirecting to OIDC provider with URL: {auth_url}") | ||
| # Instruct the client to redirect to the auth_url | ||
| return web.json_response({"authorization_url": auth_url}) | ||
|
|
||
| async def oidc_callback(request): | ||
| logger = logging.getLogger("PlanarAllyServer") | ||
| logger.debug("OIDC callback invoked") | ||
| if "oidc" not in cfg().general.authentication_methods: | ||
| logger.warning(f"OIDC authentication attempted but disabled in config") | ||
| return web.HTTPForbidden(reason="OIDC authentication is disabled") | ||
|
|
||
| # Get the provider name from the URL path since we have multiple providers | ||
| # and cannot rely on a single endpoint | ||
| provider_name = request.match_info["provider"] | ||
| code = request.query.get("code") | ||
| if not code or not provider_name: | ||
| logger.error("Missing code or provider_name in OIDC callback request") | ||
| return web.HTTPBadRequest(reason="Missing required parameters") | ||
|
|
||
| try: | ||
| user_info = await oidc_auth.exchange_code_for_token(code, provider_name, request.query.get("state")) | ||
| if not user_info: | ||
| logger.error("Failed to retrieve user info from OIDC provider") | ||
| return web.HTTPUnauthorized(reason="OIDC authentication failed") | ||
|
|
||
|
|
||
| if not user_info.username or not user_info.email: | ||
| logger.error("OIDC user info missing username/email") | ||
| return web.HTTPUnauthorized(reason="OIDC authentication failed") | ||
|
|
||
| user = User.by_name(user_info.username) or User.by_email(user_info.email) | ||
| if user is None: | ||
|
Comment on lines
+192
to
+193
Owner
There was a problem hiding this comment. This is not ideal, it's perfectly possible for a regular user to choose a particular username, and then someone else logging in with an OIDC provider that happens to give out the same username, in which case you'll log in as the existing user. we're going to have to modify the User table so that OIDC users cannot be mistaken for regular users. A simple approach would be to add a new column1, but I think we're better off with more drastic changes. A main User table which would only have the essentials for interacting with PA once authenticated. (e.g. last_login / colour_history / asset links / ...) and separate tables for auth purposes (e.g. UserAuthPassword and UserAuthOidc) which contain a link to the user and any relevant data that is required for the relevant auth flow.2 It would then probably also be wise to introduce a This is a bigger modification, so if you don't feel up for it, I don't mind doing this work myself, and you'll just have to rebase/merge my this PR with my changes. Footnotes
Contributor
Author
There was a problem hiding this comment. That is an excellent point, my perspective was an OIDC provider that I also own, so the conflict wasn't even on my radar. I can try to take a crack at the SQL side of this, although I would have to get up to speed quite quickly on how you manage the sqllite db. I'll fix the issues mentioned here, so at least i can re-base my changes. I like the direction your describing. One item that might need some more work is the "Admin" user concept. Either it would need to be fixed against Local Authentication or it needs to also indicate the auth provider it needs.
Owner
There was a problem hiding this comment. I'll manage the user sql rework and good point on the admin table
Contributor
Author
There was a problem hiding this comment. I started the SQL rework, I won't be offended if you finish it first. I was intimated by the save.py file. However the database management system is starting to make sense. I will create an issue to discuss what I got up to and maybe we can catch any other changes that make sense
Owner
There was a problem hiding this comment. ah feel free to finish it as well, I haven't started on it myself anyway as I had some other migrations to finish first :D Don't hesitate to reach out either here or on discord if you need help/clarifications! The save file is purely for database migration, you have to up the SAVE_VERSION and write a migration in pure SQL. |
||
| # Check if auto-signup is allowed | ||
| if not cfg().general.allow_signups: | ||
| logger.info(f"Auto-signup disabled, rejecting OIDC login for unknown user: {user_info.username}") | ||
| return web.HTTPForbidden(reason="User does not exist and auto-signup is disabled") | ||
| # Auto-register the user | ||
| with db.atomic(): | ||
| # Generate a sufficiently random password, since it won't be used for login | ||
| password = secrets.token_urlsafe(16) | ||
| user = User.create_new(user_info.username, password, user_info.email) | ||
| stats.events.user_created(user.id) | ||
| logger.info(f"Auto-registered new user: {user_info.username}") | ||
| response = web.HTTPOk() | ||
| user.update_last_login() | ||
| await remember(request, response, user_info.username) | ||
| logger.info(f"User {user_info.username} logged in via OIDC") | ||
| # Convert this response into a redirect for the browser | ||
| # we need the the auth system to modify the response headers | ||
| response.headers["Location"] = f"{cfg().general.client_url}" | ||
| response.set_status(302) | ||
| return response | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error during OIDC callback processing: {e}") | ||
| return web.HTTPInternalServerError(reason="An error occurred during OIDC authentication") | ||
Uh oh!
There was an error while loading. Please reload this page.