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-auth-provider] Prefer pass list email over primary email #4237

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

corneliusludmann
Copy link
Contributor

When the deployment has a blockNewUsersPassList, with this change the GitHub auth provider chooses an email address that matches the passlist as Gitpod email address when such a verified email address is given in GitHub. Otherwise, it uses GitHub's default email address (as it was before).

Internal discussion: https://gitpod.slack.com/archives/C01MLB7J24T/p1621343766002600?thread_ts=1619771446.079600&cid=C01MLB7J24T

Would make #4211 obsolete.

@jankeromnes
Copy link
Contributor

jankeromnes commented May 18, 2021

Clever! I like this approach (for when blockNewUsers is used).

Would make #4211 obsolete.

I don't think that's the case, because this Pull Request doesn't address non-Gitpod-employees wanting to try a core-dev deployment (e.g. starting a workspace).

@corneliusludmann corneliusludmann force-pushed the clu/passlist-email-over-primary branch from 692c9b5 to 7c6e764 Compare May 18, 2021 15:06
@AlexTugarev
Copy link
Member

@corneliusludmann, this approach is redefining of what is actually the primary email address on GitHub, while the underlying issue is that you want to test some of the verified email addresses included in user's profile, right? If that's true, then please join a community PR to enable exactly such use cases in general, see: #4115 (comment)
It would be best to include all verified email addresses in the OAuth process, which would make this PR easier to implement.

@corneliusludmann
Copy link
Contributor Author

@AlexTugarev I would really like to see this resolved in general with the linked PR. However, it seems that this change with multiple emails is a big thing that will not be merged soon. I think the change in this PR would solve the problem now and does not harm / has no impact on production.

The issue is now lying around again for 14 days without a solution (and interferes with me and others every day). I would still be in favor of merging this PR (regardless of the other one).

Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM! Would be better to store the emails and not treat a non-primary email as primary though.

@corneliusludmann corneliusludmann force-pushed the clu/passlist-email-over-primary branch from 7c6e764 to 7126b87 Compare June 1, 2021 09:17
@corneliusludmann corneliusludmann merged commit 42ee644 into main Jun 1, 2021
@corneliusludmann corneliusludmann deleted the clu/passlist-email-over-primary branch June 1, 2021 09:27
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.

4 participants