Skip to content

feat(auth): enables OIDC auth code flow #549

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

Merged
merged 16 commits into from
Dec 14, 2021

Conversation

ScruffyProdigy
Copy link
Contributor

@ScruffyProdigy ScruffyProdigy commented May 3, 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.

@ScruffyProdigy
Copy link
Contributor Author

@lsirac

@ScruffyProdigy ScruffyProdigy changed the title Code flow fix(auth): Enable OIDC auth code flow May 3, 2021
@lsirac lsirac changed the title fix(auth): Enable OIDC auth code flow feat(auth): enables OIDC auth code flow Jun 2, 2021
@lsirac lsirac requested a review from hiranya911 June 2, 2021 21:16
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 mostly good. Needs a few tweaks to the public API, and a couple of readability nits.

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 mostly good. Please fix the errors reported by the linter. Plus couple of minor nits on style.

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.

There seem to be some redundant code mistakenly included in the PR. PTAL.

@@ -198,7 +225,7 @@ def test_update(self, user_mgt_app):
assert len(recorder) == 1
req = recorder[0]
assert req.method == 'PATCH'
mask = ['clientId', 'displayName', 'enabled', 'issuer']
mask = ['clientId', 'clientSecret', 'displayName', 'enabled', 'issuer', 'responseType.code', 'responseType.idToken']
Copy link
Member

Choose a reason for hiding this comment

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

The following lines also seem to be triggering more lint errors.

Running linter on module tests
************* Module tests.test_auth_providers
tests/test_auth_providers.py:124:0: C0301: Line too long (111/100) (line-too-long)
tests/test_auth_providers.py:208:0: C0301: Line too long (111/100) (line-too-long)
tests/test_auth_providers.py:228:0: C0301: Line too long (124/100) (line-too-long)
tests/test_auth_providers.py:253:0: C0301: Line too long (127/100) (line-too-long)

@lahirumaramba
Copy link
Member

Thanks @ScruffyProdigy !
Hi @egilmorez please take a look at the reference docs when you get a chance. Thank you!

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 👍

@lahirumaramba lahirumaramba removed the request for review from egilmorez December 9, 2021 22:50
@@ -528,6 +529,15 @@ def create_oidc_provider_config(
This name is also used as the provider label in the Cloud Console.
enabled: A boolean indicating whether the provider configuration is enabled or disabled
(optional). A user cannot sign in using a disabled provider.
client_secret: A string which sets the client secret for the new provider.
This is required for the code flow.
code_response_type: Sets whether to enable the code response flow for the new provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fixed

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!

@lahirumaramba lahirumaramba merged commit 008b1d8 into firebase:master Dec 14, 2021
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.

5 participants