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

xtremerui · 2020-01-20T21:38:36Z

This PR leverage the PreferredUsername in Identity for OIDC connector

Line 28 in aca67b0

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 · 2020-01-21T00:03:16Z

Thanks for the PR @xtremerui !

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

connector/oidc/oidc.go

xtremerui · 2020-01-21T00:35:39Z

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 · 2020-01-21T07:29:20Z

Yeah, but preferred_username is a standard claim.

xtremerui · 2020-01-21T16:21:46Z

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 · 2020-03-17T22:06:22Z

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

jwntrs · 2020-03-19T15:49:27Z

@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 · 2020-07-30T08:42:40Z

@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 · 2020-07-30T13:55:42Z

@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 · 2020-08-04T19:41:03Z

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 · 2020-08-04T19:54:16Z

@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 · 2020-08-05T20:09:45Z

@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 · 2020-08-05T20:13:20Z

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 · 2020-08-06T06:36:21Z

Thanks @xtremerui !

LGTM

@sagikazarmark Your turn 😄

sagikazarmark · 2020-08-07T22:37:56Z

Please see #1777

Signed-off-by: Josh Winters <[email protected]> Co-authored-by: Mark Huang <[email protected]> Signed-off-by: Rui Yang <[email protected]>

Signed-off-by: Rui Yang <[email protected]>

Signed-off-by: Rui Yang <[email protected]> Signed-off-by: Rui Yang <[email protected]>

Signed-off-by: Rui Yang <[email protected]>

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 <[email protected]> Signed-off-by: Rui Yang <[email protected]>

add tests for groups key mapping Signed-off-by: Rui Yang <[email protected]>

xtremerui · 2020-08-11T20:34:53Z

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

mvdkleijn · 2020-09-04T15:58:34Z

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

sagikazarmark · 2020-09-04T16:30:40Z

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

sagikazarmark

Couple nits, but I think it's great!

sagikazarmark · 2020-09-04T16:34:47Z

Documentation/connectors/oidc.md

+    # 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


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.

sagikazarmark · 2020-09-04T16:39:42Z

connector/oidc/oidc.go

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


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.

sagikazarmark · 2020-09-04T16:46:38Z

Documentation/connectors/oidc.md

+      # 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


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

sagikazarmark · 2020-09-07T15:19:09Z

@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 <[email protected]>

xtremerui · 2020-09-08T14:08:01Z

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

sagikazarmark

@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!

sagikazarmark · 2020-09-08T14:48:38Z

connector/oidc/oidc.go

+		// 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"


I guess these shouldn't be here either.

sagikazarmark · 2020-09-08T14:48:58Z

Documentation/connectors/oidc.md

-    # 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
-


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

sagikazarmark · 2020-09-08T14:49:15Z

connector/oidc/oidc.go

+	// Deprecated: use UserIDKey in claimMapping instead
 	UserIDKey string `json:"userIDKey"`

-	// Configurable key which contains the user name claim
+	// Deprecated: use UserNameKey in claimMapping instead


These shouldn't be deprecated.

Signed-off-by: Rui Yang <[email protected]>

xtremerui · 2020-09-08T17:17:29Z

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

sagikazarmark

Thanks a lot @xtremerui

mvdkleijn · 2020-09-08T19:05:15Z

Yay! Done! 😄

@sagikazarmark

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)

@sagikazarmark

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 mentioned this pull request Jan 20, 2020

Sync concourse/dex with upstream Dex concourse/concourse#4983

Open

xtremerui requested a review from JoelSpeed January 20, 2020 21:40

sagikazarmark reviewed Jan 21, 2020

View reviewed changes

connector/oidc/oidc.go Outdated Show resolved Hide resolved

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 force-pushed the pr/oidc-username-key-sync branch from 988553a to 255312e Compare August 4, 2020 19:46

xtremerui force-pushed the pr/oidc-username-key-sync branch from 255312e to 6a3be20 Compare August 6, 2020 01:51

sagikazarmark mentioned this pull request Aug 7, 2020

Unify OIDC provider claim mapping #1777

Closed

Josh Winters and others added 7 commits August 11, 2020 16:26

Make OIDC username key configurable

9a4e0fc

Signed-off-by: Josh Winters <[email protected]> Co-authored-by: Mark Huang <[email protected]> Signed-off-by: Rui Yang <[email protected]>

default to preferred_username claim

d9afb7e

Signed-off-by: Rui Yang <[email protected]>

add tests when preferred username key is not set

4812079

Signed-off-by: Rui Yang <[email protected]>

check if upstream contains preferrend username claim first

52c39fb

Signed-off-by: Rui Yang <[email protected]> Signed-off-by: Rui Yang <[email protected]>

Add parameter configuration to override email claim key

61312e7

Signed-off-by: Rui Yang <[email protected]>

Combine dexidp#1691 and dexidp#1776 to unify OIDC provider claim mapping

41207ba

add tests for groups key mapping Signed-off-by: Rui Yang <[email protected]>

xtremerui force-pushed the pr/oidc-username-key-sync branch from c48b882 to 41207ba Compare August 11, 2020 20:27

sagikazarmark reviewed Sep 4, 2020

View reviewed changes

bonifaido approved these changes Sep 7, 2020

View reviewed changes

update oidc documentation and email claim err msg

0494993

Signed-off-by: Rui Yang <[email protected]>

sagikazarmark reviewed Sep 8, 2020

View reviewed changes

revert changes for user id and user name

058202d

Signed-off-by: Rui Yang <[email protected]>

sagikazarmark changed the title ~~Support username claim in OIDC connector~~ Support username, email and groups claim in OIDC connector Sep 8, 2020

sagikazarmark approved these changes Sep 8, 2020

View reviewed changes

sagikazarmark merged commit ca0a9e8 into dexidp:master Sep 8, 2020

This was referenced Sep 8, 2020

OIDC: Add parameter configuration to override email claim key #1691

Closed

Add groupsClaimMapping to the OIDC connector #1776

Closed

xtremerui mentioned this pull request Sep 8, 2020

Support customize oidc groups key #1631

Closed

lentzi90 mentioned this pull request Sep 14, 2020

Propagate groups from upstream OIDC provider #1065

Open

xtremerui deleted the pr/oidc-username-key-sync branch September 17, 2020 19:07

sagikazarmark mentioned this pull request Oct 4, 2020

OAuth connector #1630

Merged

lukasmrtvy mentioned this pull request Nov 1, 2020

(question): Pomerium->Dex->Auth0 groups ? #1850

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xtremerui commented Jan 20, 2020 •

edited

Loading

sagikazarmark commented Jan 21, 2020

xtremerui commented Jan 21, 2020

sagikazarmark commented Jan 21, 2020

xtremerui commented Jan 21, 2020

sagikazarmark commented Mar 17, 2020

jwntrs commented Mar 19, 2020

mvdkleijn commented Jul 30, 2020

jwntrs commented Jul 30, 2020

xtremerui commented Aug 4, 2020

sagikazarmark commented Aug 4, 2020

mvdkleijn commented Aug 5, 2020

mvdkleijn commented Aug 5, 2020

mvdkleijn commented Aug 6, 2020

sagikazarmark commented Aug 7, 2020

xtremerui commented Aug 11, 2020

mvdkleijn commented Sep 4, 2020

sagikazarmark commented Sep 4, 2020 •

edited

Loading

sagikazarmark left a comment

sagikazarmark Sep 4, 2020

sagikazarmark Sep 4, 2020

sagikazarmark Sep 4, 2020

sagikazarmark commented Sep 7, 2020

xtremerui commented Sep 8, 2020

sagikazarmark left a comment

sagikazarmark Sep 8, 2020

sagikazarmark Sep 8, 2020

sagikazarmark Sep 8, 2020

xtremerui commented Sep 8, 2020

sagikazarmark left a comment

mvdkleijn commented Sep 8, 2020

-    # 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.

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

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

Conversation

xtremerui commented Jan 20, 2020 • edited Loading

sagikazarmark commented Jan 21, 2020

xtremerui commented Jan 21, 2020

sagikazarmark commented Jan 21, 2020

xtremerui commented Jan 21, 2020

sagikazarmark commented Mar 17, 2020

jwntrs commented Mar 19, 2020

mvdkleijn commented Jul 30, 2020

jwntrs commented Jul 30, 2020

xtremerui commented Aug 4, 2020

sagikazarmark commented Aug 4, 2020

mvdkleijn commented Aug 5, 2020

mvdkleijn commented Aug 5, 2020

mvdkleijn commented Aug 6, 2020

sagikazarmark commented Aug 7, 2020

xtremerui commented Aug 11, 2020

mvdkleijn commented Sep 4, 2020

sagikazarmark commented Sep 4, 2020 • edited Loading

sagikazarmark left a comment

Choose a reason for hiding this comment

sagikazarmark Sep 4, 2020

Choose a reason for hiding this comment

sagikazarmark Sep 4, 2020

Choose a reason for hiding this comment

sagikazarmark Sep 4, 2020

Choose a reason for hiding this comment

sagikazarmark commented Sep 7, 2020

xtremerui commented Sep 8, 2020

sagikazarmark left a comment

Choose a reason for hiding this comment

sagikazarmark Sep 8, 2020

Choose a reason for hiding this comment

sagikazarmark Sep 8, 2020

Choose a reason for hiding this comment

sagikazarmark Sep 8, 2020

Choose a reason for hiding this comment

xtremerui commented Sep 8, 2020

sagikazarmark left a comment

Choose a reason for hiding this comment

mvdkleijn commented Sep 8, 2020

xtremerui commented Jan 20, 2020 •

edited

Loading

sagikazarmark commented Sep 4, 2020 •

edited

Loading