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

github: Use noreply email if public email not set. #2617

Open
2 tasks done
wlynch opened this issue Aug 3, 2022 · 2 comments · May be fixed by #2618
Open
2 tasks done

github: Use noreply email if public email not set. #2617

wlynch opened this issue Aug 3, 2022 · 2 comments · May be fixed by #2618

Comments

@wlynch
Copy link

wlynch commented Aug 3, 2022

Preflight Checklist

  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Problem Description

@leighmcculloch pointed out in sigstore/gitsign#65 that when using the github connector, if a user does not have a public email associated to their account, Dex will query for the user's primary email and use that in the returned token.

if u.Email == "" {
var err error
if u.Email, err = c.userEmail(ctx, client); err != nil {
return u, err
}
}

If the user selected their email to be private on GitHub, this might be unexpected for their email to be present in the token that Dex returns back.

Proposed Solution

GitHub has a feature for commit emails that allows users to set a noreply email that uniquely identifies them in commit messages:

{id}+{login}@users.noreply.github.com

https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address#about-commit-email-addresses

This could be used in place of querying for a private email to uniquely identify a user, since Dex already has this information with the initial user fetch:

type user struct {
Name string `json:"name"`
Login string `json:"login"`
ID int `json:"id"`
Email string `json:"email"`
}

Alternatives Considered

No response

Additional Information

This looks like a fairly straightforward change - happy to handle it if this is something we want to move forward with. Let me know if there's anything else to consider!

@sagikazarmark
Copy link
Member

sagikazarmark commented Aug 3, 2022

Thanks for opening this issue @wlynch !

At the very least, this behavior should be configurable, because email address might be used for matching various login methods in existing systems.

While I understand the reasoning, Dex uses the email address for account mgmt, not for notifications and that's what setting an email to private means in GitHub.

That being said, I see no harm in making this configurabl (keeping the current behavior as the default setting)

@wlynch
Copy link
Author

wlynch commented Aug 3, 2022

👍 Sounds good to me! I'll take a pass at this.

@wlynch wlynch linked a pull request Aug 3, 2022 that will close this issue
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 a pull request may close this issue.

2 participants