FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(auth): Added code flow support for OIDC flow. by xil222 · Pull Request #1220 · firebase/firebase-admin-node · GitHub

/ firebase-admin-node Public

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

Merged
xil222 merged 17 commits into
masterfrom
xinxinxin-codeflow
May 25, 2021
ConversationCommits (17)ChecksFiles changed
Merged

feat(auth): Added code flow support for OIDC flow.#1220
xil222 merged 17 commits into
masterfrom
xinxinxin-codeflow

Conversation

xil222 commented Apr 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

hiranya911 suggested changes Apr 14, 2021
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts Outdated
Comment thread test/unit/auth/auth-config.spec.ts Outdated
Comment thread test/unit/auth/auth-config.spec.ts Outdated
Comment thread test/unit/auth/auth-config.spec.ts
hiranya911 suggested changes Apr 15, 2021
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/auth/auth-config.ts Outdated
Comment thread test/unit/auth/auth-config.spec.ts Outdated
hiranya911 approved these changes Apr 16, 2021
View reviewed changes

hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks. LGTM 👍

lsirac reviewed Apr 16, 2021
View reviewed changes
Comment thread src/auth/auth-config.ts
lsirac reviewed Apr 16, 2021
View reviewed changes
Comment thread src/auth/auth-config.ts Outdated
bojeil-google suggested changes Apr 20, 2021
View reviewed changes
Comment thread src/auth/auth-config.ts Outdated
Comment thread src/auth/auth-config.ts
Comment thread src/auth/auth-config.ts
Comment thread src/auth/auth-config.ts Outdated
Comment thread test/unit/auth/auth-config.spec.ts
Comment thread test/unit/auth/auth-config.spec.ts
bojeil-google suggested changes Apr 23, 2021
View reviewed changes

bojeil-google left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Change looks good. Just 2 minor issues.

Comment thread src/auth/auth-config.ts Outdated
Comment thread test/unit/auth/auth-api-request.spec.ts Outdated
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',

bojeil-google Apr 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

xil222 Apr 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I add tests for createOAuthIdpConfig and updateOAuthIdpConfig

xil222 requested a review from egilmorez April 27, 2021 23:32
egilmorez approved these changes May 11, 2021
View reviewed changes

egilmorez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Thanks!

Comment thread src/auth/index.ts
/**
* The interface representing OIDC provider's response object for OAuth
* authorization flow.
* We need either of them to be true, there are two cases:

egilmorez May 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

This was referenced Jun 20, 2024
Open
Open
Open
Open
davidhin mentioned this pull request Jul 6, 2024
Open
This was referenced Jul 17, 2024
Open
Open
This was referenced Jul 27, 2024
Open
Open
Open
Open
This was referenced Aug 10, 2024
Open
Open
This was referenced Aug 17, 2024
Open
Open
Open
Open
Open
This was referenced Aug 28, 2024
Open
Open
This was referenced Sep 5, 2024
Open
Open
This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
This was referenced Oct 26, 2024
Open
Open
davidhin mentioned this pull request Nov 9, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers
hiranya911 hiranya911 approved these changes
egilmorez egilmorez approved these changes
bojeil-google bojeil-google approved these changes
lsirac lsirac approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

xil222

Labels

release-note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL