Skip to content

Conversation

@ScruffyProdigy
Copy link
Contributor

@ScruffyProdigy ScruffyProdigy commented Nov 15, 2021

Provides an option for developers to specify the OAuth response type for
their OIDC provider (either one of these can be set:):

id_token
code (if set, must also set the client secret)

RELEASE NOTE: Added support for configuring the authorization code flow for OIDC providers.

Provides an option for developers to specify the OAuth response type for 
their OIDC provider (either one of these can be set:):

id_token
code (if set, must also set the client secret)
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thanks, Ryan! Added a few comments.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you!
LGTM with minor comments. Let's get the reference docs reviewed as well.

if val, ok := config.params.Get(idTokenResponseTypeKey); ok && val.(bool) {
return nil, "", errors.New("Only one response type may be chosen")
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a } else if {? and same below.

}
}

if val, ok := config.params.Get(codeResponseTypeKey); ok && !val.(bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Can codeResponseTypeKey ever be undefined? if not, I think you can use else if here.

@lahirumaramba lahirumaramba requested review from kevinthecheung and removed request for egilmorez December 9, 2021 22:51
Copy link

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants