-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feat: implement OAuth/OIDC sign on with Authlib #414
Conversation
Now handled under the hood by the Django authentication system.
add corresponding env vars to sample remove unneeded AuthLib model attributes, add name attribute for later client creation using settings
This reverts commit 7c08132.
we need the OIDC server to send the user back with our session cookie
remove unused Django auth bits
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.
This approach looks good to me! I noticed a few things causing errors, so I left suggestions as well as a question.
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.
We are definitely going to want some tests for this functionality; want to do them in this pull request, or create a separate issue?
the values are picked up automatically from settings
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.
Will be nice to have this in since so many things hang off of it. My only hesitation with this being merged is if we are unable to log in due to configuration issues with the authorization server. If we're willing to accept that, let's do it!
Holding on the merge until everyone can get their local environment configured |
Ah, looks like I need to fix the merge conflicts. Will do so. |
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.
Oh lol, I thought it would let you resolve the merge conflict without needing a re-review, but makes sense! Looks good
Closes #375.
Replaces #407 (we're doing this all on the server now).
Replaces #409 (we don't need Django's auth system/user models).
Reviewing this PR / after merge
django.db
:rm django.db
.env
with new variables / values from CDT samplecd .devcontainer
docker compose build --no-cache client
docker compose build --no-cache dev
See more at https://docs.calitp.org/benefits/configuration/oauth