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

Multiple authorized issuer #85

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

antzo
Copy link
Contributor

@antzo antzo commented Nov 28, 2019

Changes

  • Auth0Service now can handle multiple authorized_issuer

This is a non-breaking change that allows the original string definition for jwt_auth.authorized_issuer to continue working.

Testing

[x] All existing tests complete without errors

@antzo antzo requested a review from a team November 28, 2019 18:19
@lbalmaceda lbalmaceda requested review from joshcanhelp and removed request for a team November 29, 2019 19:10
@joshcanhelp
Copy link
Contributor

@antzo - Thank you for the PR here! Can you explain what your use case is for having multiple allowed ID token issuers?

@antzo
Copy link
Contributor Author

antzo commented Dec 3, 2019

@joshcanhelp Of course! In my use case I have two applications:

a) iOS client that is consuming an API with example.domain.com token issuer
b) Android client which is also consuming an API with eu.domain.com token issuer

In this case I have to maintain the compatibility with both app's till the b) app changes to the new token issuer.

Seems like the auth0-php library is implementing authorized_iss as array for the RS256 so I thought can be a good addition to the bundle, btw thanks for create it.

@antzo
Copy link
Contributor Author

antzo commented Dec 3, 2019

Let me know if you want me to add some unit tests but I think that is enough with the current suite

@joshcanhelp
Copy link
Contributor

@antzo - Thank you for the details here, that's helpful.

The next major version of the PHP SDK will actually remove the ability to set a custom issuer, single or multiple. But that change was made because we're improving our compliance with the OIDC specifications, which is centered around ID tokens, not access tokens. Unfortunately, those two concepts are conflated in the SDK, making for situations like this.

I'm going to approve this for now since this bundle will need to do what you're asking for here and it's pinned at the major level for the SDK.

@joshcanhelp
Copy link
Contributor

❯ snyk test

Testing /Users/joshcanhelp/Sites/jwt-auth-bundle...

Organization:      auth0-sdks
Package manager:   composer
Target file:       composer.lock
Open source:       no
Project path:      /Users/josh-cunningham/Sites/jwt-auth-bundle
Licenses:          enabled

✓ Tested 35 dependencies for known issues, no vulnerable paths found.

@joshcanhelp joshcanhelp merged commit 4e453d7 into auth0:master Dec 5, 2019
@joshcanhelp joshcanhelp modified the milestones: 3.2.1, 3.3.0 Dec 5, 2019
@darthf1
Copy link
Contributor

darthf1 commented Dec 6, 2019

@joshcanhelp @antzo this PR introduced a BC.

!!
!!  In BaseNode.php line 511:
!!
!!    A dynamic value is not compatible with a "Symfony\Component\Config\Definiti
!!    on\PrototypedArrayNode" node type at path "jwt_auth.authorized_issuer".
!!
!!
!!
jwt_auth:
  domain: "%env(resolve:AUTH0_DOMAIN)%"
  authorized_issuer: "%env(resolve:AUTH0_AUTHORIZED_ISSUER)%"
  api_identifier: "%env(resolve:AUTH0_AUDIENCE)%"

had to be changed to:

jwt_auth:
  domain: "%env(resolve:AUTH0_DOMAIN)%"
  authorized_issuer:
    - "%env(resolve:AUTH0_AUTHORIZED_ISSUER)%"
  api_identifier: "%env(resolve:AUTH0_AUDIENCE)%"

@joshcanhelp
Copy link
Contributor

@darthf1 - What was the value of authorized_issuer for you? I believe the intent here was to handle both strings and arrays.

@antzo
Copy link
Contributor Author

antzo commented Dec 9, 2019

@darthf1 You are right. I just send a PR to fix the problem

#89

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 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.

3 participants