-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
connector/oidc: expose oauth2.RegisterBrokenAuthHeaderProvider #860
Conversation
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.
Great works like a charm for me.
Thanks for fixing this so quickly!
@curtisallen do you have any suggestions for setting up okta as an OpenID Connect provider? I've tried with their free tier, but only see options for SAML and their SOO solution. We'd like to test this internally. |
Basically, I've tried following this doc but it looks outdated or not available on the free tier https://support.okta.com/help/Documentation/Knowledge_Article/Using-OpenID-Connect |
@ericchiang yeah absolutely setting up an OIDC app is hidden this guide is really useful https://help.okta.com/en/prev/Content/Topics/Apps/Apps_App_Integration_Wizard.htm#OIDCWizard |
Yeah that's really weird, I don't see those options on my dev account. Blah. |
Oh strange maybe you have a make a new account or something (perhaps it's out of beta) ¯_(ツ)_/¯ |
6ba88d4
to
179d059
Compare
Do you wanna add an example of its usage here: https://github.com/coreos/dex/blob/master/examples/config-dev.yaml#L58 ? |
// avoid having to explicitly set "basicAuthUnsupported" in their config. | ||
// | ||
// Setting the config field always overrides values returned by this function. | ||
func knownBrokenAuthHeaderProvider(issuerURL string) bool { |
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.
IMO implementing a known list of broken providers isn't a good idea. This list will grow and grow over time just like it has in oauth2 violating the open close principle.
I think it's better to rely on clear documentation in this case
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.
conversely, not all IDPs have nice error messages like Okta so things could fail and operators will have no idea why... So I can see both sides of it
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.
If we can detect cases where someone will always have to set the basicAuthUnsupported
flag then I think we should just avoid the overhead and do it automatically. These providers being weird shouldn't cost our users ease of use, and maintaining a list isn't that bad.
@rithujohn191 docs added. |
connector/oidc/oidc.go
Outdated
// Detect Okta. | ||
// | ||
// https://github.com/coreos/dex/issues/859 | ||
for _, host := range []string{"okta.com", "oktapreview.com"} { |
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.
Perhaps this list should be defined separately above?
5465c70
to
d7c450f
Compare
d7c450f
to
f503ff7
Compare
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
connector/oidc: expose oauth2.RegisterBrokenAuthHeaderProvider
closes #859
@curtisallen if you can confirm the fix I'll add docs and such.