Skip to content

Adding oidc email scope check#1610

Merged
bonifaido merged 1 commit into
dexidp:masterfrom
deckhouse:oidc-email-scope-check
Jan 6, 2020
Merged

Adding oidc email scope check#1610
bonifaido merged 1 commit into
dexidp:masterfrom
deckhouse:oidc-email-scope-check

Conversation

@nabokihms
Copy link
Copy Markdown
Member

This helps to avoid "no email claim" error if the email scope was not specified.

Signed-off-by: m.nabokikh maksim.nabokikh@flant.com

Closes #1598

@nabokihms nabokihms force-pushed the oidc-email-scope-check branch from 32038d8 to 45bc3be Compare December 27, 2019 22:04
Copy link
Copy Markdown
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.

LGTM, only a few suggestions

Comment thread connector/oidc/oidc.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check is not necessary because of default values.

Comment thread connector/oidc/oidc.go Outdated
Comment thread connector/oidc/oidc.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would revert this, because it doesn't change behavior at all.

@sagikazarmark
Copy link
Copy Markdown
Member

Further thinking about this: email claim will only be included in the returned token if email scope is requested. Given this wasn't the behavior before, one could interpret this as a breaking change. I'm not sure what the standard says about returning non-requested fields, but if nothing, then we should probably continue returning those fields in case the underlying provider returns them.

@nabokihms nabokihms force-pushed the oidc-email-scope-check branch from 7f049f0 to cbed29a Compare December 28, 2019 07:04
@nabokihms
Copy link
Copy Markdown
Member Author

Yep, it has sense. I will change the behavior according to your thoughts.

@nabokihms nabokihms force-pushed the oidc-email-scope-check branch 3 times, most recently from b03ed83 to c4ebee2 Compare December 28, 2019 08:19
Copy link
Copy Markdown
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.

A few more nits

Comment thread connector/oidc/oidc.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about:

	hasEmailScope := false

	for _, s := range c.oauth2Config.Scopes {
		if s == "email" {
			hasEmailScope = true

			break
		}
	}

Feels a bit cleaner and more readable

Comment thread connector/oidc/oidc.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could merge these two ifs. The default value of email is an empty string because of the type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, you are right. Fixed.

This helps to avoid "no email claim" error if email scope was not specified.

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@nabokihms nabokihms force-pushed the oidc-email-scope-check branch from c4ebee2 to 383c2fe Compare December 28, 2019 11:28
Copy link
Copy Markdown
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 @nabokihms

Copy link
Copy Markdown
Member

@bonifaido bonifaido left a comment

Choose a reason for hiding this comment

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

LGTM!

@bonifaido bonifaido merged commit 3cbba11 into dexidp:master Jan 6, 2020
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.

Failed to authenticate: missing "email" claim

3 participants