Repository: dexidp/dex
Pull request #1634 (2020-01-20 21:38:36)

Support username, email and groups claim in OIDC connector
This PR leverage the `PreferredUsername` in `Identity` for OIDC connector
https://github.com/dexidp/dex/blob/aca67b083984982c11942ac505b883c1d75bf6f9/connector/connector.go#L28

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: Thanks for the PR @xtremerui !

What's the reason behind letting the user change the claim for `PreferredUsername`?

xtremerui: 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: Yeah, but `preferred_username` *is* a standard claim.
xtremerui: 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: I think there is a misunderstanding: `preferred_username` is a standard claim. Why do you want to change it? It's standard.
pivotal-jwinters: @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: @sagikazarmark @jwntrs @xtremerui - Can I try to help resolve this issue here? :smile: 

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 https://github.com/dexidp/dex/pull/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. :smile: :+1: 
jwntrs: @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: 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: @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: @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: 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: Thanks @xtremerui !

LGTM

@sagikazarmark Your turn :smile:
sagikazarmark: Please see #1777

Patch:
diff --git a/Documentation/connectors/oidc.md b/Documentation/connectors/oidc.md
index c472e303..e64cd2de 100644
--- a/Documentation/connectors/oidc.md
+++ b/Documentation/connectors/oidc.md
@@ -8,8 +8,6 @@ Prominent examples of OpenID Connect providers include Google Accounts, Salesfor
 
 ## Caveats
 
-This connector does not support the "groups" claim. Progress for this is tracked in [issue #1065][issue-1065].
-
 When using refresh tokens, changes to the upstream claims aren't propagated to the id_token returned by dex. If a user's email changes, the "email" claim returned by dex won't change unless the user logs in again. Progress for this is tracked in [issue #863][issue-863].
 
 ## Configuration
@@ -75,11 +73,10 @@ connectors:
     # getUserInfo: true
 
     # The set claim is used as user id.
-    # Default: sub
     # Claims list at https://openid.net/specs/openid-connect-core-1_0.html#Claims
-    #
+    # Default: sub
     # userIDKey: nickname
-    
+
     # The set claim is used as user name.
     # Default: name
     # userNameKey: nickname
@@ -88,9 +85,25 @@ connectors:
     # However this is not supported by all OIDC providers, some of them support different
     # value for prompt, like "prompt=login" or "prompt=none"
     # promptType: consent
+
+    # 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.
+    claimMapping:
+      # The set claim is used as preferred username.
+      # Default: preferred_username
+      # preferred_username: other_user_name
+
+      # The set claim is used as email.
+      # Default: email
+      # email: mail
+
+      # The set claim is used as groups.
+      # Default: groups
+      # groups: "cognito:groups"
 ```
 
 [oidc-doc]: openid-connect.md
 [issue-863]: https://github.com/dexidp/dex/issues/863
-[issue-1065]: https://github.com/dexidp/dex/issues/1065
 [azure-ad-v1]: https://github.com/coreos/go-oidc/issues/133
diff --git a/README.md b/README.md
index 412aa84e..432c2fa1 100644
--- a/README.md
+++ b/README.md
@@ -69,7 +69,7 @@ Dex implements the following connectors:
 | [GitHub](Documentation/connectors/github.md) | yes | yes | yes | stable | |
 | [SAML 2.0](Documentation/connectors/saml.md) | no | yes | no | stable |
 | [GitLab](Documentation/connectors/gitlab.md) | yes | yes | yes | beta | |
-| [OpenID Connect](Documentation/connectors/oidc.md) | yes | no ([#1065][issue-1065]) | no | beta | Includes Salesforce, Azure, etc. |
+| [OpenID Connect](Documentation/connectors/oidc.md) | yes | yes | yes | beta | Includes Salesforce, Azure, etc. |
 | [Google](Documentation/connectors/google.md) | yes | yes | yes | alpha | |
 | [LinkedIn](Documentation/connectors/linkedin.md) | yes | no | no | beta | |
 | [Microsoft](Documentation/connectors/microsoft.md) | yes | yes | no | beta | |
diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 675e4b95..b8e543d4 100644
--- a/connector/oidc/oidc.go
+++ b/connector/oidc/oidc.go
@@ -49,14 +49,23 @@ type Config struct {
 	// id tokens
 	GetUserInfo bool `json:"getUserInfo"`
 
-	// Configurable key which contains the user id claim
 	UserIDKey string `json:"userIDKey"`
 
-	// Configurable key which contains the user name claim
 	UserNameKey string `json:"userNameKey"`
 
 	// PromptType will be used fot the prompt parameter (when offline_access, by default prompt=consent)
 	PromptType string `json:"promptType"`
+
+	ClaimMapping struct {
+		// Configurable key which contains the preferred username claims
+		PreferredUsernameKey string `json:"preferred_username"` // defaults to "preferred_username"
+
+		// Configurable key which contains the email claims
+		EmailKey string `json:"email"` // defaults to "email"
+
+		// Configurable key which contains the groups claims
+		GroupsKey string `json:"groups"` // defaults to "groups"
+	} `json:"claimMapping"`
 }
 
 // Domains that don't support basic auth. golang.org/x/oauth2 has an internal
@@ -141,9 +150,12 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e
 		insecureSkipEmailVerified: c.InsecureSkipEmailVerified,
 		insecureEnableGroups:      c.InsecureEnableGroups,
 		getUserInfo:               c.GetUserInfo,
+		promptType:                c.PromptType,
 		userIDKey:                 c.UserIDKey,
 		userNameKey:               c.UserNameKey,
-		promptType:                c.PromptType,
+		preferredUsernameKey:      c.ClaimMapping.PreferredUsernameKey,
+		emailKey:                  c.ClaimMapping.EmailKey,
+		groupsKey:                 c.ClaimMapping.GroupsKey,
 	}, nil
 }
 
@@ -163,9 +175,12 @@ type oidcConnector struct {
 	insecureSkipEmailVerified bool
 	insecureEnableGroups      bool
 	getUserInfo               bool
+	promptType                string
 	userIDKey                 string
 	userNameKey               string
-	promptType                string
+	preferredUsernameKey      string
+	emailKey                  string
+	groupsKey                 string
 }
 
 func (c *oidcConnector) Close() error {
@@ -273,6 +288,11 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		return identity, fmt.Errorf("missing \"%s\" claim", userNameKey)
 	}
 
+	preferredUsername, found := claims["preferred_username"].(string)
+	if !found {
+		preferredUsername, _ = claims[c.preferredUsernameKey].(string)
+	}
+
 	hasEmailScope := false
 	for _, s := range c.oauth2Config.Scopes {
 		if s == "email" {
@@ -281,9 +301,16 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		}
 	}
 
-	email, found := claims["email"].(string)
+	var email string
+	emailKey := "email"
+	email, found = claims[emailKey].(string)
+	if !found && c.emailKey != "" {
+		emailKey = c.emailKey
+		email, found = claims[emailKey].(string)
+	}
+
 	if !found && hasEmailScope {
-		return identity, errors.New("missing \"email\" claim")
+		return identity, fmt.Errorf("missing email claim, not found \"%s\" key", emailKey)
 	}
 
 	emailVerified, found := claims["email_verified"].(bool)
@@ -294,8 +321,28 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 			return identity, errors.New("missing \"email_verified\" claim")
 		}
 	}
-	hostedDomain, _ := claims["hd"].(string)
 
+	var groups []string
+	if c.insecureEnableGroups {
+		groupsKey := "groups"
+		vs, found := claims[groupsKey].([]interface{})
+		if !found {
+			groupsKey = c.groupsKey
+			vs, found = claims[groupsKey].([]interface{})
+		}
+
+		if found {
+			for _, v := range vs {
+				if s, ok := v.(string); ok {
+					groups = append(groups, s)
+				} else {
+					return identity, fmt.Errorf("malformed \"%v\" claim", groupsKey)
+				}
+			}
+		}
+	}
+
+	hostedDomain, _ := claims["hd"].(string)
 	if len(c.hostedDomains) > 0 {
 		found := false
 		for _, domain := range c.hostedDomains {
@@ -320,11 +367,13 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 	}
 
 	identity = connector.Identity{
-		UserID:        idToken.Subject,
-		Username:      name,
-		Email:         email,
-		EmailVerified: emailVerified,
-		ConnectorData: connData,
+		UserID:            idToken.Subject,
+		Username:          name,
+		PreferredUsername: preferredUsername,
+		Email:             email,
+		EmailVerified:     emailVerified,
+		Groups:            groups,
+		ConnectorData:     connData,
 	}
 
 	if c.userIDKey != "" {
@@ -335,18 +384,5 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		identity.UserID = userID
 	}
 
-	if c.insecureEnableGroups {
-		vs, ok := claims["groups"].([]interface{})
-		if ok {
-			for _, v := range vs {
-				if s, ok := v.(string); ok {
-					identity.Groups = append(identity.Groups, s)
-				} else {
-					return identity, errors.New("malformed \"groups\" claim")
-				}
-			}
-		}
-	}
-
 	return identity, nil
 }


Test patch:
diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go
index 52afa158..ae92f70c 100644
--- a/connector/oidc/oidc_test.go
+++ b/connector/oidc/oidc_test.go
@@ -49,10 +49,15 @@ func TestHandleCallback(t *testing.T) {
 		name                      string
 		userIDKey                 string
 		userNameKey               string
+		preferredUsernameKey      string
+		emailKey                  string
+		groupsKey                 string
 		insecureSkipEmailVerified bool
 		scopes                    []string
 		expectUserID              string
 		expectUserName            string
+		expectGroups              []string
+		expectPreferredUsername   string
 		expectedEmailField        string
 		token                     map[string]interface{}
 	}{
@@ -62,14 +67,31 @@ func TestHandleCallback(t *testing.T) {
 			userNameKey:        "", // not configured
 			expectUserID:       "subvalue",
 			expectUserName:     "namevalue",
+			expectGroups:       []string{"group1", "group2"},
 			expectedEmailField: "emailvalue",
 			token: map[string]interface{}{
 				"sub":            "subvalue",
 				"name":           "namevalue",
+				"groups":         []string{"group1", "group2"},
 				"email":          "emailvalue",
 				"email_verified": true,
 			},
 		},
+		{
+			name:               "customEmailClaim",
+			userIDKey:          "", // not configured
+			userNameKey:        "", // not configured
+			emailKey:           "mail",
+			expectUserID:       "subvalue",
+			expectUserName:     "namevalue",
+			expectedEmailField: "emailvalue",
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"mail":           "emailvalue",
+				"email_verified": true,
+			},
+		},
 		{
 			name:                      "email_verified not in claims, configured to be skipped",
 			insecureSkipEmailVerified: true,
@@ -108,6 +130,48 @@ func TestHandleCallback(t *testing.T) {
 				"email_verified": true,
 			},
 		},
+		{
+			name:                    "withPreferredUsernameKey",
+			preferredUsernameKey:    "username_key",
+			expectUserID:            "subvalue",
+			expectUserName:          "namevalue",
+			expectPreferredUsername: "username_value",
+			expectedEmailField:      "emailvalue",
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"username_key":   "username_value",
+				"email":          "emailvalue",
+				"email_verified": true,
+			},
+		},
+		{
+			name:                    "withoutPreferredUsernameKeyAndBackendReturns",
+			expectUserID:            "subvalue",
+			expectUserName:          "namevalue",
+			expectPreferredUsername: "preferredusernamevalue",
+			expectedEmailField:      "emailvalue",
+			token: map[string]interface{}{
+				"sub":                "subvalue",
+				"name":               "namevalue",
+				"preferred_username": "preferredusernamevalue",
+				"email":              "emailvalue",
+				"email_verified":     true,
+			},
+		},
+		{
+			name:                    "withoutPreferredUsernameKeyAndBackendNotReturn",
+			expectUserID:            "subvalue",
+			expectUserName:          "namevalue",
+			expectPreferredUsername: "",
+			expectedEmailField:      "emailvalue",
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"email":          "emailvalue",
+				"email_verified": true,
+			},
+		},
 		{
 			name:                      "emptyEmailScope",
 			expectUserID:              "subvalue",
@@ -135,6 +199,41 @@ func TestHandleCallback(t *testing.T) {
 				"email":     "emailvalue",
 			},
 		},
+		{
+			name:                      "customGroupsKey",
+			groupsKey:                 "cognito:groups",
+			expectUserID:              "subvalue",
+			expectUserName:            "namevalue",
+			expectedEmailField:        "emailvalue",
+			expectGroups:              []string{"group3", "group4"},
+			scopes:                    []string{"groups"},
+			insecureSkipEmailVerified: true,
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"user_name":      "username",
+				"email":          "emailvalue",
+				"cognito:groups": []string{"group3", "group4"},
+			},
+		},
+		{
+			name:                      "customGroupsKeyButGroupsProvided",
+			groupsKey:                 "cognito:groups",
+			expectUserID:              "subvalue",
+			expectUserName:            "namevalue",
+			expectedEmailField:        "emailvalue",
+			expectGroups:              []string{"group1", "group2"},
+			scopes:                    []string{"groups"},
+			insecureSkipEmailVerified: true,
+			token: map[string]interface{}{
+				"sub":            "subvalue",
+				"name":           "namevalue",
+				"user_name":      "username",
+				"email":          "emailvalue",
+				"groups":         []string{"group1", "group2"},
+				"cognito:groups": []string{"group3", "group4"},
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -162,8 +261,12 @@ func TestHandleCallback(t *testing.T) {
 				UserIDKey:                 tc.userIDKey,
 				UserNameKey:               tc.userNameKey,
 				InsecureSkipEmailVerified: tc.insecureSkipEmailVerified,
+				InsecureEnableGroups:      true,
 				BasicAuthUnsupported:      &basicAuth,
 			}
+			config.ClaimMapping.PreferredUsernameKey = tc.preferredUsernameKey
+			config.ClaimMapping.EmailKey = tc.emailKey
+			config.ClaimMapping.GroupsKey = tc.groupsKey
 
 			conn, err := newConnector(config)
 			if err != nil {
@@ -182,8 +285,10 @@ func TestHandleCallback(t *testing.T) {
 
 			expectEquals(t, identity.UserID, tc.expectUserID)
 			expectEquals(t, identity.Username, tc.expectUserName)
+			expectEquals(t, identity.PreferredUsername, tc.expectPreferredUsername)
 			expectEquals(t, identity.Email, tc.expectedEmailField)
 			expectEquals(t, identity.EmailVerified, true)
+			expectEquals(t, identity.Groups, tc.expectGroups)
 		})
 	}
 }