-
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
Documentation/github-connector: warn user that GitHub email id should be public. #972
Conversation
Documentation/github-connector.md
Outdated
@@ -8,7 +8,7 @@ When a client redeems a refresh token through dex, dex will re-query GitHub to u | |||
|
|||
## Configuration | |||
|
|||
Register a new application with [GitHub][github-oauth2] ensuring the callback URL is `(dex issuer)/callback`. For example if dex is listening at the non-root path `https://auth.example.com/dex` the callback would be `https://auth.example.com/dex/callback`. | |||
Register a new application with [GitHub][github-oauth2] ensuring the callback URL is `(dex issuer)/callback`. For example if dex is listening at the non-root path `https://auth.example.com/dex` the callback would be `https://auth.example.com/dex/callback`. Please note that a user needs to mark their email id as public in GitHub, so that the ID token that gets returned for this user contains an email id field in the ID token claims. |
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 kinda sounds like you're saying that ID tokens are returned by GitHub.
Also might be worth marking this as a known caveat. e.g. https://github.com/coreos/dex/blob/master/Documentation/oidc-connector.md#caveats or https://github.com/coreos/dex/blob/master/Documentation/saml-connector.md#caveats
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.
Should I just leave at "users should have their email ids as public"?
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.
Yep will create a new caveat section for it. Maybe also add our GitHub orgs problem there.
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 think detail the fact that GitHub's API won't return an email if the end user's email isn't public. However you want to word that, but the current sentence isn't super clear.
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.
👍
Made changes according to the feedback. |
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
Note that you should be able to get non-public emails with user:email scope, but you need to explicitly query |
Documentation/github-connector: warn user that GitHub email id should be public.
Looks like a lot of users are trying the GitHub connector with private email ids.