Skip to content
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

Google hosted domain support #974

Merged
merged 3 commits into from Jul 7, 2017

Conversation

bnavetta
Copy link
Contributor

See #959 - Google's OIDC implementation supports limiting access to a certain Google Apps / G Suite domain. This lets dex users ensure that only people in their organization can log in, and not just anyone with a Google account.

@bnavetta bnavetta mentioned this pull request Jun 21, 2017
@stijndehaes
Copy link

Tests fail because of a lint suggestion:
/home/travis/gopath/src/github.com/coreos/dex/connector/oidc/oidc.go:147:9: if block ends with a return statement, so drop this else and outdent its block

@@ -33,6 +33,7 @@ type Config struct {

Scopes []string `json:"scopes"` // defaults to "profile" and "email"

HostedDomain string `json:"hostedDomain"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment describing what this field is and highlighting that it is optional

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to support an array of domains in the configuration and then match on "at least one" in HandleCallback?

@rithujohn191
Copy link
Contributor

Please also add an explanation of the new field here. If possible please do include unit tests in oidc tests. You can refer the ldap or saml tests for reference.

@bnavetta
Copy link
Contributor Author

I added a test for LoginURL, but HandleCallback makes a request to the issuer and I couldn't find a way to mock that.

@stijndehaes
Copy link

Is there anything I can do to help this get merged? I am really in need of this functionality.

Copy link
Contributor

@rithujohn191 rithujohn191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RoguePanda thank you. LGTM

@rithujohn191 rithujohn191 merged commit a5d218f into dexidp:master Jul 7, 2017
Copy link

@alonl alonl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it and I'm afraid there's a bug.

if len(c.hostedDomains) > 0 {
found := false
for _, domain := range c.hostedDomains {
if claims.HostedDomain != domain {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be if claims.HostedDomain == domain ?
@RoguePanda @rithujohn191

Copy link

@alonl alonl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it and I'm afraid there's a bug (2).

@@ -33,6 +33,9 @@ type Config struct {

Scopes []string `json:"scopes"` // defaults to "profile" and "email"

// Optional list of whitelisted domains when using Google
// If this field is nonempty, only users from a listed domain will be allowed to log in
HostedDomains []string `json:"hostedDomain"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be json:"hostedDomains" (domains)

@@ -61,6 +61,7 @@ connectors:
# clientID: $GOOGLE_CLIENT_ID
# clientSecret: $GOOGLE_CLIENT_SECRET
# redirectURI: http://127.0.0.1:5556/dex/callback
# hostedDomain: $GOOGLE_HOSTED_DOMAIN
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this doesn't match the code. If I understand it correctly, it should be:

hostedDomains:
  - $GOOGLE_HOSTED_DOMAIN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants