-
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: fix hosted domain support. #1000
Conversation
May also fix the example: :) |
So quick thought on this. The hosted domain concept is Google specific, not related to OpenID Connect. There are a ton of features that have been requested that apply to Google and aren't applicable to general oidc providers (e.g. groups support). Why arent we just split this into a unique "google" connector instead of pushing Google features into the oidc connector? |
Created a new issue for this (#1001). Closing out this PR |
But... This feature is still broken... Better remove it then, I believe. :) |
New plan....going to fix this feature for now and then in the next release pull it out into its own Google connector :) |
d39d98d
to
68a8c6e
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.
Minor fix
examples/config-dev.yaml
Outdated
@@ -67,7 +67,7 @@ connectors: | |||
# clientID: $GOOGLE_CLIENT_ID | |||
# clientSecret: $GOOGLE_CLIENT_SECRET | |||
# redirectURI: http://127.0.0.1:5556/dex/callback | |||
# hostedDomain: $GOOGLE_HOSTED_DOMAIN | |||
# hostedDomains: $GOOGLE_HOSTED_DOMAIN |
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.
It's an array. I think it should be:
hostedDomains:
- $GOOGLE_HOSTED_DOMAIN
68a8c6e
to
5e0bf8b
Compare
@ericchiang if this looks ok could you approve the PR? Thanks |
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
@rithujohn191 I was just wondering how soon a release could be cut with this in it? I have a need to restrict logins to a certain gsuite domain. Thanks! |
@alonl This PR does check the hd claim in the id token: https://github.com/coreos/dex/pull/1000/files#diff-dbaa70d509cfdb00b729984f27415946R201 So it would validate for those configured hosted domains. |
Oh, you're right. That's just the issue I reported myself. Sorry for the confusion. I deleted my previous comments to avoid confusing others. |
connector/oidc: fix hosted domain support.
fixes #999