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

Fill empty UserIDClaim before assigning it to other values #1920

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

mdreem
Copy link
Contributor

@mdreem mdreem commented Dec 5, 2022

the email claim gets overridden if userIDClaim is not set.

Description

During login I am getting the error:

 Error creating session during OAuth2 callback: neither the id_token nor the profileURL set an email

Motivation and Context

It seems to be related to:
#1623
#1732

The underlying problem seems to be the code that handles Backwards Compatibility for Deprecated UserIDClaim option. it overrides the EmailClaim if they are different. But at that point the UserIDClaim is still empty, if not explicitly set and so it clears EmailClaim. Later in the code it actually is explicitly set, so I just pulled this piece of code up.
As this seems to be a workaround I am unsure about the behavior it should have and potential additional side-effects.

How Has This Been Tested?

I wrote a new test-case.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@JoelSpeed
Copy link
Member

Apologies for the delay in getting to this, but it seems like a good fix, can you add a changelog entry and I'll look to get this merged

@mdreem
Copy link
Contributor Author

mdreem commented Feb 3, 2023

Apologies for the delay in getting to this, but it seems like a good fix, can you add a changelog entry and I'll look to get this merged

Great to hear. I added a changelog entry.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Thanks!

@JoelSpeed JoelSpeed merged commit 13202fd into oauth2-proxy:master Feb 6, 2023
@JoelSpeed JoelSpeed added the bug label Feb 6, 2023
@mdreem mdreem deleted the do-not-remove-emails-claim branch February 6, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants