Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): Added code flow support for OIDC flow. #1220

Merged
merged 17 commits into from
May 25, 2021
Merged

Conversation

xil222
Copy link
Contributor

@xil222 xil222 commented Apr 6, 2021

RELEASE NOTE: Added code flow support for OIDC flow(previously only support idToken flow).
RELEASE NOTE: Defined OAuthResponseType for specifying responseType either idToken or code.
RELEASE NOTE: Defined two new error codes: INVALID_OAUTH_RESPONSETYPE and MISSING_OAUTH_CLIENT_SECRET.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks good. I mostly had nits. There might be couple of edge cases that need better handling and some tests.

src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Show resolved Hide resolved
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Looks pretty good. Just one last suggestion to cleanup the large loop in the implementation.

src/auth/auth-config.ts Outdated Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
src/auth/auth-config.ts Show resolved Hide resolved
src/auth/auth-config.ts Show resolved Hide resolved
src/auth/auth-config.ts Outdated Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Show resolved Hide resolved
test/unit/auth/auth-config.spec.ts Show resolved Hide resolved
Copy link
Contributor

@bojeil-google bojeil-google left a comment

Choose a reason for hiding this comment

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

Change looks good. Just 2 minor issues.

src/auth/auth-config.ts Outdated Show resolved Hide resolved
@@ -3503,12 +3503,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned about legacy customers and would recommend keeping a test without clientSecret and responseType. Can you add new tests instead of modifying the existing ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add tests for createOAuthIdpConfig and updateOAuthIdpConfig

@xil222 xil222 requested a review from egilmorez April 27, 2021 23:32
Copy link
Contributor

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

One optional nit if it's not too big a PITA to regen the reference.

Thanks!

/**
* The interface representing OIDC provider's response object for OAuth
* authorization flow.
* We need either of them to be true, there are two cases:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment could be improved. Suggest:

" * One of the following must be true:

  • If code is set to true, then we are doing code flow.
  • If dToken is set to true, then we are doing ID token flow."

(Assuming that backticks are rendered as code font, and that "ID token flow" is a thing, separate from the literal idToken flag.

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.

6 participants