-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: enhance okta sso #5424
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?
fix: enhance okta sso #5424
Conversation
|
Skipped: This PR does not target one of your configured branches: ( |
|
@cody-eding is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
| self.jwks_url = f"{self.okta_issuer}/.well-known/jwks.json" | ||
| self.jwks_url = f"{self.okta_issuer}/v1/keys" | ||
|
|
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.
how did it work until now?
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 was unable to get the original implementation working unless I overrode the pre-configured default by setting the OKTA_JWKS_URL environment variable.
The Okta documentation suggests that /keys is the appropriate endpoint.
I checked the three Okta tenants I have access to, and they all seem to use /keys for the jwks_url.
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.
what have you set to make it work, I am struggling with that actually
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.
With my test integration tenant, I set the following in the compose file for both the API and UI with the currently released code:
OKTA_JWKS_URL=https://integrator-XXXXXXX.okta.com/oauth2/default/v1/keys
This assumes your Okta OIDC issuer is something like: https://integrator-XXXXXXX.okta.com/oauth2/default
|
Hey! thanks for opening this PR! We have some problem with the CI/CD. Going to handle it soon and let this PR in :) |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
shahargl
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.
lgtm
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #5424 +/- ##
===========================================
- Coverage 46.39% 30.54% -15.86%
===========================================
Files 176 101 -75
Lines 18412 11669 -6743
===========================================
- Hits 8543 3564 -4979
+ Misses 9869 8105 -1764 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #5423
📑 Description
This pull request enhances the Okta SSO implementation and documentation to more closely match other OAuth providers such as OneLogin. The
okta_authverifier.pyfile has been improved by taking many pieces of the OneLogin implementation.Improvements include:
✅ Checks