Skip to content

Fix uninitialized user claim #1873

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 7, 2022
Merged

Fix uninitialized user claim #1873

merged 4 commits into from
Nov 7, 2022

Conversation

babs
Copy link
Contributor

@babs babs commented Oct 31, 2022 •
edited
Loading

Description

Some providers doesn't initialize data with setProviderDefaults function (keycloak-oidc for example), therefore UserClaim is never initialized with the default value (sub) and stay an empty string. This leads to an empty user.

Should close #1874

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.

@babs
Fix uninitialized user claim
679970e 
Some providers doesn't initialize data with setProviderDefaults function
(keycloak-oidc for example), therefore UserClaim is never initialized
with the default value and stay as an empty string.
This result in an empty user.
@babs babs requested a review from a team as a code owner October 31, 2022 18:56
@babs babs mentioned this pull request Oct 31, 2022
Closed
JoelSpeed
JoelSpeed reviewed Nov 1, 2022
View reviewed changes
providers/provider_data.go
if p.Scope == "" {
p.Scope = defaults.scope
}

Copy link
Member

Choose a reason for hiding this comment

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

Why not make sure every provider is using setProviderDefaults? Does every provider definitely use buildSessionFromClaims?

Moving it to buildSessionFromClaims means the check is done on every authentication request rather than just in set up, so there's an efficiency hit too

@babs
Copy link
Contributor Author

babs commented Nov 3, 2022

Updated with setProviderDefaults approach:

	p.setProviderDefaults(providerDefaults{
		name:        keycloakOIDCProviderName,
	})

@bradyfontenot
Copy link

bradyfontenot commented Nov 3, 2022 •
edited
Loading

would like to add that we also ran into this issue recently w the generic oidc provider. reverted to 7.2.0 to fix it which was mentioned another issue here:
#1681 (comment)

it was broken starting at 7.2.1 (edited version. typo before)

@babs
Copy link
Contributor Author

babs commented Nov 3, 2022 •
edited
Loading

Just for the sake of testing live, could you test the v7.4.0-babs3 release on my fork before going back to 7.2.0 ?
For security concerns, the build has been performed by github action, so everything is traceable.

JoelSpeed
JoelSpeed previously approved these changes Nov 7, 2022
View reviewed changes
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.

LGTM, thanks @babs

@JoelSpeed JoelSpeed merged commit fd2807c into oauth2-proxy:master Nov 7, 2022
@josemolinai-bcs josemolinai-bcs mentioned this pull request Dec 21, 2022
Closed
babs added a commit to babs/oauth2-proxy that referenced this pull request Oct 7, 2023
@babs
Add CHANGELOG.md entry for oauth2-proxy#1873
15c53c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoelSpeed JoelSpeed JoelSpeed approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some providers has uninitialized user claim

3 participants

@babs @bradyfontenot @JoelSpeed