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: allow to specify suggested domain #2

Merged
merged 1 commit into from
Jun 15, 2022
Merged

google: allow to specify suggested domain #2

merged 1 commit into from
Jun 15, 2022

Conversation

bgentry
Copy link

@bgentry bgentry commented Jun 15, 2022

This pulls in thomseddon#293, whose description reads:

As per the Google OIDC docs, the hd parameter may be used to suggest the domain the user may login with:
https://developers.google.com/identity/protocols/oauth2/openid-connect#hd-param

In effect, the Google account chooser only displays accounts with that very domain, which simplifies the process for users with very long account lists.

This will let us restrict our auth wall to only show accounts from the Mux domain, simplifying the auth wall flow if anybody happens to be logged in to other Google accounts.

As per the Google OIDC docs, the `hd` parameter may be used to suggest
the domain the user may login with:
https://developers.google.com/identity/protocols/oauth2/openid-connect#hd-param

In effect, the Google account chooser only displays accounts with that
very domain, which simplifies the process for users with very long
account lists.
@bgentry
Copy link
Author

bgentry commented Jun 15, 2022

@com6056 it doesn't seem I can request a review on this repo but this seems up your alley!

@@ -14,6 +14,7 @@ type Google struct {
ClientSecret string `long:"client-secret" env:"CLIENT_SECRET" description:"Client Secret" json:"-"`
Scope string
Prompt string `long:"prompt" env:"PROMPT" default:"select_account" description:"Space separated list of OpenID prompt options"`
EmailDomain string `long:"email-domain" env:"EMAIL_DOMAIN" description:"Email domain the user is suggested to login with"`
Copy link

Choose a reason for hiding this comment

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

any chance we can just use the same domain as specified with --domain so we don't have to specify it twice? although looks like it would allow multiple domains, maybe we can just grab the first one: https://github.com/muxinc/traefik-forward-auth/blob/master/internal/config.go#L35

Copy link
Author

@bgentry bgentry Jun 15, 2022

Choose a reason for hiding this comment

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

Up to you if you want me to go down that route. I would prefer to just add the 2nd config, because these map to independent parameters in Google's OpenID Connect flow and have slightly different purposes. Also it seems unlikely we will ever need to change the domains we're authenticating against, so that config overhead is pretty low IMO.

Plus if we use a trick like "always grab the first entry from domains" then there's more potential to break this by accidentally specifying the domain list in the "wrong" order.

Copy link

Choose a reason for hiding this comment

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

Makes sense, this approach works for me, thanks!

@bgentry
Copy link
Author

bgentry commented Jun 15, 2022

FYI I'm not an owner here so I can't take this any further than opening the PR :)

@com6056 com6056 merged commit 64913c0 into muxinc:master Jun 15, 2022
@bgentry bgentry deleted the bg-google-hd-param branch June 15, 2022 20:51
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.

3 participants