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

fix(oidc): parse right claims for GitHub authentication #1720

Closed
wants to merge 1 commit into from

Conversation

tboerger
Copy link
Collaborator

Before this change the authentication via GitHub always errored out within getProfileNameFromEmail because the rune list had a zero length, after my investigation I have been able to pin it down to a lacking email address fetched from the GitHub provider.

Since there are various claims like the username, email and fullname which aren't part of the generated claims I have added a switch to properly read all relevant information from the right claims within the GitHub provider.

This commit is still lacking proper error display on the login page, but this should be added on another commit in general.

@tboerger
Copy link
Collaborator Author

I have resolved the conflicts from the other merged PR.

Before this change the authentication via GitHub always errored out within getProfileNameFromEmail because the rune list had a zero length, after my investigation I have been able to pin it down to a lacking email address fetched from the GitHub provider.

Since there are various claims like the username, email and fullname which aren't part of the generated claims I have added a switch to properly read all relevant information from the right claims within the GitHub provider.

This commit is still lacking proper error display on the login page, but this should be added on another commit in general.
@fiftin
Copy link
Collaborator

fiftin commented Jan 31, 2024

Hi @tboerger ,
I figured out this problem. Problem not in the code. GitHub do not return email address of the user until he/she don't add them to the public emails.

@fiftin
Copy link
Collaborator

fiftin commented Jan 31, 2024

@tboerger
Copy link
Collaborator Author

But at least for me it was not only missing the email but also the username and fullname, that's why I have started this PR.

But since my email doesn't get populated I guess I got to enable it within github.

@fiftin
Copy link
Collaborator

fiftin commented Feb 1, 2024

Closing the PR because I fixed issue with auth via GitHub (when email inavailable): 989be6d

@fiftin fiftin closed this Feb 1, 2024
@fiftin
Copy link
Collaborator

fiftin commented Feb 1, 2024

Thanks for your PR because I get userInfo.Claims() from it :)

@tboerger tboerger deleted the github-oidc branch February 1, 2024 09:27
@tboerger
Copy link
Collaborator Author

tboerger commented Feb 1, 2024

Great that this had fixed the GitHub authentication, but are these values expected if I register on Semaphore Cloud?

Screenshot from 2024-02-01 10-35-16

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

2 participants