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

Dynamic jwks url by discovery document #417 #420

Closed
wants to merge 15 commits into from

Conversation

mabuonomo
Copy link

Hi, this PR is linked to issue #417

@mabuonomo mabuonomo requested a review from a team January 27, 2020 21:27
@mabuonomo mabuonomo changed the title Consider to customize the jwks path #417 Dynamic jwks url from discovery documento #417 Jan 27, 2020
@mabuonomo mabuonomo changed the title Dynamic jwks url from discovery documento #417 Dynamic jwks url from discovery document #417 Jan 27, 2020
@mabuonomo mabuonomo changed the title Dynamic jwks url from discovery document #417 Dynamic jwks url by discovery document #417 Jan 27, 2020
Copy link
Contributor

@joshcanhelp joshcanhelp 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 for the contribution!

I have 2 important changes here. Also, please revert your whitespace changes here, as it makes it difficult to review what has actually changed. Also please use the PR template that is provided.

@mabuonomo
Copy link
Author

mabuonomo commented Jan 28, 2020

@joshcanhelp the whitespaces are added/removed by the command "phpcbf", if you want I can restore them.
NB. Also phpcs has many errors

@joshcanhelp
Copy link
Contributor

joshcanhelp commented Jan 28, 2020

For this project, use composer phpcbf to load our PHPCS config file. This is on master:

~/Sites/php-auth0/auth0 master
❯ composer phpcbf
> "vendor/bin/phpcbf"
................................................ 48 / 48 (100%)

No fixable errors were found

PHPCS definitely needs some work here but that should happen in different PR, not this one.

@mabuonomo
Copy link
Author

For this project, use composer phpcbf to load our PHPCS config file. This is on master:

~/Sites/php-auth0/auth0 master
❯ composer phpcbf
> "vendor/bin/phpcbf"
................................................ 48 / 48 (100%)

No fixable errors were found

PHPCS definitely needs some work here but that should happen in different PR, not this one.

same result on my branch, maybe phpcbf not fix (and check) the spaces.
I've reverted spaces

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

@mabuonomo - Sorry but can you revert all the style and naming changes in this PR? Most of the changes here are noise and renaming requestJwks -> request is a breaking change for anyone extending that class.

I would suggest adding a new class for the discovery doc (aka issuer) as that information can be used in a number of different places, not just for the JWKS.

@joshcanhelp
Copy link
Contributor

@mabuonomo - Just FYI ... we're doing some work with the JWKS fetching in #426 that should address your custom JWKS issue. We'll add discovery at some point but that will probably come with a new Auth0 login helper class.

@joshcanhelp
Copy link
Contributor

Addressed in #426

@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants