OAuth connector

[xtremerui]

Hi, this implements an oauth connector, which could be used for generic authentication following OAuth2.0 framework.

[mvdkleijn]

Nice! This would be most welcome in my opinion.

@xtremerui Would you mind adding a commit or two to

a) list the generic oauth connector in the table of the main readme; and
b) add documentation entry for those of use who are too lazy (or unable) to read the code (in Documentation/connectors)

I believe that would be helpful to get the PR to be accepted.

@bonifaido @sagikazarmark This PR has stalled basically on a single person's review, I believe it'd be nice to get this in (with above mentions additions) so I'm hoping one of you could take a look since @srenatus doesn't appear to have time?

Disclaimer: I'm in no way involved with either the Concourse or Dex team. I'm just a simple user trying to help the process as it were.

[xtremerui]

I have updated the documents for oauth connector. Please let me know if there is anything I can do. Thx!

[mvdkleijn]

So overall... with the caveat of me having read through the PR on my phone and not having "physically" tested it... 😄

LGTM assuming my comments are addressed.

[mvdkleijn]

Unless I am mistaken, you guys wanted to make sure the preferred_username field is available for users.

However, hardcoding it to be the same as the name field does not seem desirable to me. Apart from that it also doesn't give the backend a chance to pass preferred_username if it tries to and effectively renders preferred_username fairly useless.

My suggestion would be to:

• default it to unset if its unset at the backend
• pass through the value unchanged if the backend does provide the field
• and allow a user to override the above two through a config entry which tells the connector to copy the value of another field to preferred_username

Please see the Atlassian Crowd connector for an example.

Again disclaimer: I'm not a member of Dex, just an occasional contributor trying to help both of you. 😄

[xtremerui]

I think that makes sense. If there is connector that handles preferred_username already. I'd like to keep the behavior the same as much as possible.

[mvdkleijn]

some whitespace issues

[xtremerui]

hmm can't tell where the whitespace is. I took an existing line and just changed the name. It shown no difference.

[mvdkleijn]

Hmm... I'm wondering if I made a booboo here... I believe that preferred_username is part of the OIDC spec and not the OAUTH spec. (whoops) If so, my apologies in advance.

Can we verify this? @sagikazarmark & @xtremerui ? I did not find any reference to preferred_username in the OAUTH 2 specs, only in the OIDC specs.

[xtremerui]

@mvdkleijn I think you are right. Though I can't see how it affects the current implementation in this PR.

Upstream might or might not include preferred_username claim.
If it does, users of oauth connection could leave the key not configured so it will pick up the claim.
If it does not, the preferred_username field in identity could be used for any claim, which is more flexible since oauth connector meant to be a backup plan for auth provider that support Oauth but not OIDC.

[mvdkleijn]

Sounds sensible enough, but I'll leave this decision to one of the Dex mainteiners like @sagikazarmark 😄 I'm juat happy to see there's some movement in these PRs again since they're so close to being merged.

[sagikazarmark]

Thanks @xtremerui

I think it's a great feature.

Found a few nits, can you please take a look at those? Thanks!

[sagikazarmark]

Can we add a claim mapping for these (except userIDKey) similar to the one introduced in #1634?

(userIDKey is not actually an OIDC claim, so it should remain separate)

[xtremerui]

I wonder if userIDKey is not a standard claim (actullay i believe for oauth framework there is no standard claim, compared to OIDC), should we also include it in the claim mapping so user can configure it to match whatever they want from the auth provider?

I think the same case also apply to email and email_verified. Please correct me if there is a doc about standard claims of oauth.

[sagikazarmark]

I think in our case claims are interpreted from the Dex providing OIDC point of view. No, they are not oauth claims, but we map some information from oauth to the OIDC token.

The userIDKey is only used by Dex to determine which field to use as the user identifier for the specific connector. The rest of the keys are "mapped" into an OIDC token as standard claims.

That's how I see it at least, correct me if my thought process is wrong.

[xtremerui]

I have put eveything but userIDKey under ClaimMappings and added an error if user ID is not found since it is critical. EmailKey and EmailVerifiedKey are added so it can handle a response like reddit:

Reddit

https://oauth.reddit.com/api/v1/me
{
    "comment_karma": 0, 
    "created": 1389649907.0, 
    "created_utc": 1389649907.0, 
    "has_mail": false,    
    "has_mod_mail": false, 
    "has_verified_email": null,   <--- emailVerifiedKey
    "id": "1",                               <--- userIDKey
    "is_gold": false, 
    "is_mod": true, 
    "link_karma": 1, 
    "name": "reddit_bot",           <--- preferredUsernameKey
    "over_18": true
}

[sagikazarmark]

I think these default should probably be assigned in the "constructor", no?

[xtremerui]

I put them here to follow what OIDC does in the handlerCallback(). I have no preference here so if you insiit I can move them to the Open() method where it constructs Oauth connection.

dex/connector/oidc/oidc.go

Lines 282 to 310 in 2a28286

	userNameKey := "name"
	if c.userNameKey != "" {
	userNameKey = c.userNameKey
	}
	name, found := claims[userNameKey].(string)
	if !found {
	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" {
	hasEmailScope = true
	break
	}
	}
	var email string
	emailKey := "email"
	email, found = claims[emailKey].(string)
	if !found && c.emailKey != "" {
	emailKey = c.emailKey
	email, found = claims[emailKey].(string)
	}

[panxunying]

I have a similar scene, i want to know why this pr isn't merged now?

[sagikazarmark]

@xtremerui Thanks for the efforts you've put into this. I finally got the time around to give it a proper review. It looks good to me.

I had two small comments. Would you mind taking a look at them? We can finally get this merged after that.

[xtremerui]

Thx for reviewing. I have updated it accordingly. @sagikazarmark

[sagikazarmark]

Thanks @xtremerui