Add OIDC implementation with authentication and configuration support#1663
Add OIDC implementation with authentication and configuration support#1663JatinVasman wants to merge 4 commits intoKruptein:devfrom
Conversation
|
I don't have the time yet for a full review, but just wanted to say that there is nothing wrong with using the It's not the intention to obfuscate the code with complexer logic just to avoid using "http" ;) |
Kruptein
left a comment
Kruptein
left a comment
There was a problem hiding this comment.
I still need to review the actual oidc code, but there is a lot of noise from other changes that are not related.
I'm assuming something went wrong with the base branch you come from, as a bunch of things that were recently changed in PA are reverted in this PR.
Additionally there are some infra things for docker and env variables, that I would rather keep out of this PR.
| "login": { | ||
| "register": "Register", | ||
| "login": "Login", | ||
| "or": "OR", |
There was a problem hiding this comment.
This already exists as common.ok
|
|
||
| import { router } from "."; | ||
|
|
||
| // Admin |
| "rtoml==0.12.0", | ||
| "typing-extensions==4.15.0", | ||
| "watchdog==6.0.0", | ||
| "requests>=2.32.5", |
There was a problem hiding this comment.
This is not sorted, but also seemingly not actually used?
We have aiohttp for network requests anyway.
| user: Union[User, None] = User.by_name(identity) if identity else None | ||
| if user is None: | ||
| return False | ||
|
|
||
| if permission == "lobby.read": | ||
| if user.role != Constants.role.DM: | ||
| return user.can_play_game(context["room"]) | ||
| return True | ||
| elif permission == "dm": | ||
| return user.role == Constants.role.DM | ||
| elif permission == "token.read": | ||
| return user.role == Constants.role.DM | ||
| return False |
There was a problem hiding this comment.
This seems to use some custom permission logic you have for your personal server I'm assuming.
These permissions don't exist, so just reset this to the original behaviour please.
|
|
||
| if username_pass_should_be_disabled: | ||
| data = data.replace(b'name="PA-username-pass" content="true"', b'name="PA-username-pass" content="false"') | ||
| elif not config.general.username_password_enabled and not oidc_fully_configured: |
There was a problem hiding this comment.
This is now checked twice, both in the condition on line 39 and here
| pass # Meta tag stays as content="true" | ||
|
|
||
| # MAIL CONFIGURATION | ||
| if config.mail is None or not config.mail.enabled or not config.mail.host or not config.mail.port or not config.mail.default_from_address: |
There was a problem hiding this comment.
host, port and the default email cannot be empty if config.mail exists so checking them is unnecessary
| config.oidc.client_id and | ||
| config.oidc.client_secret and | ||
| bool(config.oidc.domain or config.oidc.authorize_url) # Either domain for discovery OR direct URLs |
There was a problem hiding this comment.
These should just become mandatory in the config. If you specify an oidc config, it should contain all the data required.
For the domain vs authorize_url we probably should ideally check that in a sort of post-validation step in the config manager itself, but I'm ok with keeping it here for now
|
|
||
| async def root(request, admin_api=False): | ||
| template = "admin-index.html" if admin_api else "index.html" | ||
| with open(FILE_DIR / "templates" / template, "rb") as f: | ||
| data = __replace_config_data(f.read()) | ||
| return web.Response(body=data, content_type="text/html") | ||
|
|
||
|
|
||
| async def root_dev(request): | ||
| port = 8080 | ||
| async def root_dev(request, admin_api=False): | ||
| port = 8081 if admin_api else 8080 |
| # ADMIN ROUTES | ||
|
|
||
| api_app.router.add_post(f"{subpath}/notifications", notifications.create) | ||
| api_app.router.add_get(f"{subpath}/notifications", notifications.collect) | ||
| api_app.router.add_delete(f"{subpath}/notifications/{{uuid}}", notifications.delete) | ||
| api_app.router.add_get(f"{subpath}/users", admin_users.collect) | ||
| api_app.router.add_post(f"{subpath}/users/reset", admin_users.reset) | ||
| api_app.router.add_post(f"{subpath}/users/remove", admin_users.remove) | ||
| api_app.router.add_get(f"{subpath}/campaigns", campaigns.collect) | ||
|
|
||
| admin_app.router.add_static(f"{subpath}/static", STATIC_DIR) | ||
| admin_app.add_subapp("/api/", api_app) | ||
|
|
||
| TAIL_REGEX = "/{tail:(?!api).*}" | ||
| if "dev" in sys.argv: | ||
| main_app.router.add_route("*", TAIL_REGEX, root_dev) | ||
| admin_app.router.add_route("*", TAIL_REGEX, partial(root_dev, admin_api=True)) | ||
| else: | ||
| main_app.router.add_route("*", TAIL_REGEX, root) | ||
| admin_app.router.add_route("*", TAIL_REGEX, partial(root, admin_api=True)) |
|
Hey @JatinVasman just checking in to see if you're still up for working on this :) I hope I didn't scare you :D I'm still interested in expanding the login options. |
|
I'm going to close this in favour of #1701, which is based on your changes (and credits you in the changelog). |
2 participants
Implements OpenID Connect authentication allowing users to login with external providers.
Features:
OIDC authentication flow with multiple provider support
Account linking via email for existing users
Configurable via environment variables
Added login UI with provider buttons
Changes:
Backend: OIDC auth module, API endpoints, configuration
Frontend: Updated login page with OIDC buttons and callback handling
Config: Environment template and Docker compose setup
No breaking changes - OIDC is optional and works alongside existing auth.