-
-
Notifications
You must be signed in to change notification settings - Fork 34
OIDC Improvement: Auto redirecting #47
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
base: main
Are you sure you want to change the base?
Conversation
This commit introduces OpenID Connect (OIDC) authentication support. Backend changes include new OIDC settings, migration, validation, and endpoints for OIDC login and callback, as well as utility functions for OIDC flows. The frontend adds OIDC login UI, settings management, and connection testing. Both backend and frontend enums, schemas, and services are updated to support OIDC configuration and integration.
Bug in the settings API where it assumes CRONTAB_EXPR is always included when saving settings. When saving only OIDC settings, this fails.
Error checking if OIDC was enabled, stopped the auth method from showing.
- fix an issue in the token exchange where I'm using the session outside its context - fix cookie domain logging and session context in the token exchange - added debug logging
Changed OIDC state cookie 'samesite' from 'strict' to 'lax' and 'secure' to False to facilitate HTTP testing and cross-origin redirects. Added debug print statements to log state and cookies during OIDC callback for easier troubleshooting.
Some notes to assist the user in setting up their OIDC config
Fix the CSS
Clarifies the OIDC provider setup instructions
Fix css as it didn't appear very well.
This commit introduces OpenID Connect (OIDC) authentication support. Backend changes include new OIDC settings, migration, validation, and endpoints for OIDC login and callback, as well as utility functions for OIDC flows. The frontend adds OIDC login UI, settings management, and connection testing. Both backend and frontend enums, schemas, and services are updated to support OIDC configuration and integration.
Bug in the settings API where it assumes CRONTAB_EXPR is always included when saving settings. When saving only OIDC settings, this fails.
Error checking if OIDC was enabled, stopped the auth method from showing.
- fix an issue in the token exchange where I'm using the session outside its context - fix cookie domain logging and session context in the token exchange - added debug logging
Changed OIDC state cookie 'samesite' from 'strict' to 'lax' and 'secure' to False to facilitate HTTP testing and cross-origin redirects. Added debug print statements to log state and cookies during OIDC callback for easier troubleshooting.
Some notes to assist the user in setting up their OIDC config
Fix the CSS
Clarifies the OIDC provider setup instructions
Fix css as it didn't appear very well.
+ removed migrations + remoted translations + removed fake observable result of initiateOidcLogin + removed values of oidc vars in .env.example, edited comments + fixed typing errors in auth_api + fixed errors in auth_core (settings now in config)
+ added auth providers classes + removed /backend/app folder + removed /backend/core/auth_core.py
+ added more universal endpoints + added exception for no auth provider enabled
+ removed docker-compose (was removed in main) + removed env from docker-compose.dev.yml (for dev purposes it is handy to use .env in workspace) + removed oidc settings from frontend enum + removed unused validators and schemas + removed DOCKER_TIMEOUT (was removed in main)
New API method to check if OIDC auto-redirect is enabled and updates the AuthPage to automatically redirect to OIDC
Quenary
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. Thanks for the PR. I looked at it and put forward my proposals.
| public readonly isOidcAutoRedirectEnabled = resource({ | ||
| defaultValue: false, | ||
| loader: () => | ||
| firstValueFrom( | ||
| this.authApiService.isOidcAutoRedirectEnabled().pipe( | ||
| retry(1), | ||
| catchError((error) => { | ||
| this.toastService.error(error); | ||
| return throwError(() => error); | ||
| }), | ||
| ), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the resource pattern isn't justified sirce this value is not used in the template.
It is worth simply subscribe to authApiService method in the constructor.
| @router.get( | ||
| path="/oidc_auto_redirect", | ||
| description="Check if OIDC auto redirect is enabled", | ||
| response_model=bool, | ||
| ) | ||
| async def oidc_auto_redirect() -> bool: | ||
| from backend.config import Config | ||
| return Config.OIDC_AUTO_REDIRECT | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right approach to implement it as separate exclusive method for the OIDC in auth_api.
I think should be in AuthProvider and child classes - something like existing /{provider}/login and /{provider}/callback
For password provider it is always false obviously, but other providers may be added in the future.
| /** | ||
| * Check if OIDC auto redirect is enabled | ||
| * @returns | ||
| */ | ||
| isOidcAutoRedirectEnabled(): Observable<boolean> { | ||
| return this.httpClient.get<boolean>(`${this.basePath}/oidc_auto_redirect`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it's need to be universal request for provider
To aid in streamlining OIDC authentication process, this proposed PR adds
OIDC_AUTO_REDIRECTto the env file. While the default isfalse, changing this totruewill enable a new endpoint automatically redirecting the user to the OIDC provider.This is a relatively small PR with only 5 file changes. Please review and let me know if you have any concerns or questions. For any other features, I will make sure I create an issue to discuss them first - I felt an issue to discuss this feature was not necessary, now that the OIDC support is in place.