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

Support username, email and groups claim in OIDC connector #1634

Merged
merged 9 commits into from
Sep 8, 2020

Conversation

xtremerui
Copy link

@xtremerui xtremerui commented Jan 20, 2020

This PR leverage the PreferredUsername in Identity for OIDC connector

PreferredUsername string

User can config PreferredUsernameKey to lookup a custom claim that has user handle. Noted that it is not the same as name or preferred_username standard claims.

Updated: implementes #1777

@sagikazarmark
Copy link
Member

Thanks for the PR @xtremerui !

What's the reason behind letting the user change the claim for PreferredUsername?

@xtremerui
Copy link
Author

Thx for reviewing @sagikazarmark !

What's the reason behind letting the user change the claim for PreferredUsername?

Since in OIDC spec username is not a standard claim (so the claim key is customizable i.e. not always be username), making it configurable in OIDC config allows more flexibility.

@sagikazarmark
Copy link
Member

Yeah, but preferred_username is a standard claim.

@xtremerui
Copy link
Author

Sure, I think that is reasonable. The PR is updated accordingly.

From our end as long as there is a way for OIDC connector to return both name and username(or whatever it is) claim we are totally fine. Thx!

@sagikazarmark
Copy link
Member

I think there is a misunderstanding: preferred_username is a standard claim. Why do you want to change it? It's standard.

@jwntrs
Copy link
Contributor

jwntrs commented Mar 19, 2020

@sagikazarmark the upstream provider might not support the "preferred_username" claim (dex itself only added support recently). So while I agree that the default should be "preferred_username", having it configureable is still valuable.

@mvdkleijn
Copy link
Contributor

@sagikazarmark @jwntrs @xtremerui - Can I try to help resolve this issue here? 😄

The problem is not that the name of the preferred_username field should be configurable, that's just a possible solution. The problem is that not all backends implement / provide the preferred_username field. (feel free to correct me at any time if needed!)

I can understand fully why the Dex maintainers would have an issue with changing the name of the preferred_username field as a potential solution to the above mentioned problem.

However, I propose that there is an alternative solution that keeps the name of the preferred_username field intact but still allows the end user to configure the connector so that the preferred_username field is set in Dex's response.

In fact, this is basically what I did in my (already merged) PR at #1684 for the Atlassian Crowd connector.

By default it would not supply a preferred_username response if none was set by the backend, however, the user could configure the connector to fill the preferred_username field with the value of another field that is supplied by the backend.

It'd be nice if we could drag this PR out of the current stalemate. 😄 👍

@jwntrs
Copy link
Contributor

jwntrs commented Jul 30, 2020

@mvdkleijn yup I totally agree with your assessment. I had to re-check the code, because I thought this was how it was implemented, but it looks like the original behaviour changed in that second commit. I've stepped away from the project using dex, but maybe @xtremerui could update this PR.

@xtremerui xtremerui force-pushed the pr/oidc-username-key-sync branch 2 times, most recently from 1635d22 to f7be9db Compare August 4, 2020 19:31
@xtremerui
Copy link
Author

I have added tests for this @mvdkleijn . Thank you for the suggestion.

By default it would not supply a preferred_username response if none was set by the backend, however, the user could configure the connector to fill the preferred_username field with the value of another field that is supplied by the backend.

@sagikazarmark
Copy link
Member

@mvdkleijn thanks for the suggestion. I think it makes sense.

So in short: if there is a preferred_username coming from the upstream use that value, otherwise let the user make it configurable and use the value of another claim. Is that right?

I think that sounds acceptable, so if @xtremerui updates the PR accordingly, I'd be happy to review it again.

@mvdkleijn
Copy link
Contributor

@sagikazarmark That is 100% correct.

I see that @xtremerui made most of the changes... the claim key's to use instead of preferred_username can now be configured.

However, the connector is not checking if preferred_username is set by upstream or not.

In pseudocode, I'd expect something like

if upstream['preferred_username'] is nil then
  preferred_username = upstream[config.preferred_username_key]
else
  preferred_username = upstream['preferred_username']

if @xtremerui wouldn't mind making that change, I think we can basically wrap this up.

(apologies for any typos, I'm writing from my phone)

@mvdkleijn
Copy link
Contributor

By the way @xtremerui ... all my comments here with regards to your use of preferred_username also apply to your oauth PR #1630 I believe

@mvdkleijn
Copy link
Contributor

Thanks @xtremerui !

LGTM

@sagikazarmark Your turn 😄

@sagikazarmark
Copy link
Member

Please see #1777

Josh Winters and others added 7 commits August 11, 2020 16:26
Signed-off-by: Josh Winters <jwinters@pivotal.io>
Co-authored-by: Mark Huang <mhuang@pivotal.io>
Signed-off-by: Rui Yang <ruiya@vmware.com>
Signed-off-by: Rui Yang <ruiya@vmware.com>
Signed-off-by: Rui Yang <ruiya@vmware.com>
Signed-off-by: Rui Yang <ryang@pivotal.io>
Signed-off-by: Rui Yang <ruiya@vmware.com>
Signed-off-by: Rui Yang <ruiya@vmware.com>
The groupsClaimMapping setting allows one to specify which claim to pull
group information from the OIDC provider.  Previously it assumed group
information was always in the "groups" claim, but that isn't the case
for many OIDC providers (such as AWS Cognito using the "cognito:groups"
claim instead)

Signed-off-by: Scott Lemmon <slemmon@aurora.tech>
Signed-off-by: Rui Yang <ruiya@vmware.com>
add tests for groups key mapping

Signed-off-by: Rui Yang <ruiya@vmware.com>
@xtremerui
Copy link
Author

@sagikazarmark the PR is updated following the discussion in #1777 .

@mvdkleijn
Copy link
Contributor

@sagikazarmark Is anything still required for this apart from a review and merge?

@sagikazarmark
Copy link
Member

sagikazarmark commented Sep 4, 2020

@mvdkleijn See #1777 Sorry, missed that it's actually done.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Couple nits, but I think it's great!

Comment on lines 81 to 83
# Some providers return no standard claim that is different to
# claims list at https://openid.net/specs/openid-connect-core-1_0.html#Claims
# Use claimMapping to specify custom claim names
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Some providers return no standard claim that is different to
# claims list at https://openid.net/specs/openid-connect-core-1_0.html#Claims
# Use claimMapping to specify custom claim names
# Some providers return non-standard claims (eg. mail).
# Use claimMapping to map those claims to standard claims:
# https://openid.net/specs/openid-connect-core-1_0.html#Claims
# claimMapping can only map a non-standard claim to a standard one if it's not returned in the id_token.

if !found && hasEmailScope {
return identity, errors.New("missing \"email\" claim")
return identity, fmt.Errorf("missing \"%s\" claim", emailKey)
Copy link
Member

Choose a reason for hiding this comment

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

This error message won't tell the user that it's related to the email scope being set. Also, the original issue is still true: the email claim is missing as well.

I would probably leave email here and maybe mention the alias/mapping key as well.

Comment on lines 85 to 91
# The set claim is used as user id.
# Default: sub
# user_id: nickname

# The set claim is used as user name.
# Default: name
# user_name: nickname
Copy link
Member

Choose a reason for hiding this comment

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

UserID and Username are not actually claims, so I'm not sure we should include them here.

@sagikazarmark
Copy link
Member

@xtremerui can you take a look at the requested changes? I think it's time that we get this merged, it only needs a few final touches.

Signed-off-by: Rui Yang <ruiya@vmware.com>
@xtremerui
Copy link
Author

@sagikazarmark done! Thx for waiting, it was long weekend last week and I took days off as well.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

@xtremerui thanks a lot. I think you missed a few things though (or I wasn't entirely clear): I would keep the user id and username keys as they are, since they are not claims at all. We can always move them under claim mapping later if we think it's the right place, but for now it just seems logically wrong.

Thanks!

Comment on lines 62 to 66
// Configurable key which contains the user id claim
UserIDKey string `json:"user_id"` // defaults to "sub"

// Configurable key which contains the username claim
UserNameKey string `json:"user_name"` // defaults to "name"
Copy link
Member

Choose a reason for hiding this comment

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

I guess these shouldn't be here either.

Comment on lines 77 to 86
# The set claim is used as user id.
# Default: sub
# Claims list at https://openid.net/specs/openid-connect-core-1_0.html#Claims
#
# userIDKey: nickname

# The set claim is used as user name.
# Default: name
# userNameKey: nickname

Copy link
Member

Choose a reason for hiding this comment

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

These should be added back as these are valid options, but not claims.

Comment on lines 52 to 55
// Deprecated: use UserIDKey in claimMapping instead
UserIDKey string `json:"userIDKey"`

// Configurable key which contains the user name claim
// Deprecated: use UserNameKey in claimMapping instead
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be deprecated.

Signed-off-by: Rui Yang <ruiya@vmware.com>
@xtremerui
Copy link
Author

That makes sense. It will make this PR more clean. I have reverted the changes for user id and user name.

@sagikazarmark sagikazarmark changed the title Support username claim in OIDC connector Support username, email and groups claim in OIDC connector Sep 8, 2020
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks a lot @xtremerui

@mvdkleijn
Copy link
Contributor

Yay! Done! 😄

@xtremerui xtremerui deleted the pr/oidc-username-key-sync branch September 17, 2020 19:07
@sagikazarmark sagikazarmark mentioned this pull request Oct 4, 2020
xtremerui pushed a commit to concourse/dex that referenced this pull request Oct 5, 2020
The official docker release for this release can be pulled from

    dexidp/dex:v2.25.0

**Features:**

- Move the API package to a separate module (dexidp#1741, @sagikazarmark)
- OAuth2 Device Authorization Grant (dexidp#1706, @justin-slowik)
- Support username, email and groups claim in OIDC connector (dexidp#1634, @xtremerui)

**Bugfixes:**

- Add offline_access scope in microsoft connector, if required (dexidp#1441, @jimmythedog)
- Allow the google connector to work without a service account (dexidp#1720, @candlerb)

**Minor changes:**

- Remove vendor (finally) (dexidp#1745, @sagikazarmark)
- Fix the LDAP example (dexidp#1762, @heidemn-faro)
- Relocate the example app (dexidp#1764, @sagikazarmark)
xtremerui pushed a commit to concourse/dex that referenced this pull request Nov 5, 2020
The official docker release for this release can be pulled from

    dexidp/dex:v2.25.0

**Features:**

- Move the API package to a separate module (dexidp#1741, @sagikazarmark)
- OAuth2 Device Authorization Grant (dexidp#1706, @justin-slowik)
- Support username, email and groups claim in OIDC connector (dexidp#1634, @xtremerui)

**Bugfixes:**

- Add offline_access scope in microsoft connector, if required (dexidp#1441, @jimmythedog)
- Allow the google connector to work without a service account (dexidp#1720, @candlerb)

**Minor changes:**

- Remove vendor (finally) (dexidp#1745, @sagikazarmark)
- Fix the LDAP example (dexidp#1762, @heidemn-faro)
- Relocate the example app (dexidp#1764, @sagikazarmark)
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.

7 participants