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

Deal with list of issuers in JwtAuthenticationProvider #30

Merged
merged 4 commits into from Jun 18, 2020

Conversation

coperator
Copy link
Contributor

Changes

When we wanted to switch to a custom domain, we had the requirement that the original auth0 subdomain still works for legacy applications.
Thus, I refactored this Spring Security integration, so that it can deal with several issuers.
Since auth0/java-jwt#288 is merged in Java JWT, it is straightforward to do this.
One holding limitation is that all issuers need to have the same public key in case of RS256 and the same secret in case of HS256. Nevertheless, this is the case for Auth0 custom domains of the same tenant.
I want to stress here, that multiple tenants are not supported. We only cover the case of several issuers domains of one Auth0 tenant.

I decided to leave as much of the existing code as possible in order to guarantee backward compatibility.
The respective functions can now be called with a list of issuers or, as before, with one issuer.
I added tests to cover the cases of several issuers.

Testing

I tested the change with our API and several issuers.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • [ X ] This change has been tested on the latest version of the platform/language or why not

Checklist

@coperator coperator requested a review from a team June 24, 2019 11:55
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution.

First I'd like to understand your use case. You said:

When we wanted to switch to a custom domain, we had the requirement that the original auth0 subdomain still works for legacy applications.

If I understood correctly, you had your auth0 domain e.g. "sample.auth0.com" being the issuer of the tokens. When you migrated to a custom domain, e.g. "sample.com", the tokens are still being signed with the "auth0 domain". That looks just right, that's how auth0 behaves.

I guess you encountered verification issues because you've set up an application using this SDK passing your "custom domain" and the server is still signing with the "auth0 domain". Is that right? I'll wait on your response to continue, but I've already reviewed the PR so feel free to ignore those comments for now 👍

@coperator
Copy link
Contributor Author

Hey, thanks for your review! The points you made are valid. Originally, I tried to modify as little as possible of the original code and to not introduce any breaking changes, but this results in code duplication.

Unfortunately, I am currently very busy with other stuff. I try to find time, to address your comments in the code.

Regarding your question:

I guess you encountered verification issues because you've set up an application using this SDK passing your "custom domain" and the server is still signing with the "auth0 domain"
Yes, we encountered verificiation issues and were using this SDK. However, the signing is not the problem, but the content of the access token Auth0 issues.

During the transitional period, our legacy clients use the old domain "sample.auth0.com" and get an access token with issuer: sample.auth0.com, while updated clients use the custom domain "sample.com" and the issued access token accordingly contains issuer: sample.com. Both RS256 access tokens are signed with the same credentials and can be verified with https://sample.auth0.com/.well-known/jwks.json or https://sample.com/.well-known/jwks.json, since both jwks are the same.
During verification, Java JWT checks, if the issuer, which is stated in the token, is according to the configuration. Since auth0/java-jwt#288 it is possible to configure a list of allowed issuers.
So, the only missing thing in our setup was, that this SDK needs to be able to pass a list of allowed issuers instead of a single issuer to Java JWT. This merge request enables this.
I hope I could clarify things and answer your question.

@coperator
Copy link
Contributor Author

I refactored the code according to your suggestions. Additionally, I added a hint to the README.md

@coperator
Copy link
Contributor Author

coperator commented Aug 29, 2019

@lbalmaceda Are you planning to merge this? Is anything unclear? Is there anything left which I could do / improve?

@lbalmaceda
Copy link
Contributor

@coperator sorry for the delay on the review. Now that I see that issuers[0] I'm not entirely convinced this is a good API. We probably shouldn't be picking the "jwk issuer" like that, maybe an extra param is better. Let me discuss this internally with the rest of the team and I'll come back to you next week.

@skjolber
Copy link

For true multi-tenant support, each tenant needs an individual list of audiences.

@stale
Copy link

stale bot commented Jan 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

@stale stale bot added the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@neshanjo
Copy link

neshanjo commented Jan 6, 2020

Please do not close this issue. This is still something that we need.

@stale stale bot removed the closed:stale Issue or PR has not seen activity recently label Jan 6, 2020
@perlauvaas
Copy link

Yes, this would really make the transition to custom domain easier. I am hoping for a merge in the near future.

@stale
Copy link

stale bot commented Apr 15, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

@stale stale bot added the closed:stale Issue or PR has not seen activity recently label Apr 15, 2020
@neshanjo
Copy link

This is still something that we need for migrating from auth0 domain to custom domain.

@stale stale bot removed the closed:stale Issue or PR has not seen activity recently label Apr 17, 2020
@lbalmaceda
Copy link
Contributor

👋@neshanjo Just a heads up that we're also tracking this internally. At first, for evaluating the changes and impact.

@jimmyjames
Copy link
Contributor

jimmyjames commented Jun 4, 2020

@coperator, @neshanjo, @perlauvaas - just to confirm the use case this PR targets: This change is specific to the case when migrating to a new/custom domain within the same tenant, to support applications receiving access tokens issued both by the old and new domain. Is that correct?

EDIT:
If the above assumption is correct, then the only change I'd like to see is to clarify in the JavaDocs the assumption for multiple issuer support (still requires the issuers to be in the same tenant). Given the age of this PR (our bad), it's something we can follow-up on in a separate PR if needed 🙇

@jimmyjames jimmyjames requested a review from a team June 4, 2020 17:51
@jimmyjames jimmyjames requested review from jimmyjames and removed request for a team June 18, 2020 14:11
@jimmyjames jimmyjames merged commit 4bc371a into auth0:master Jun 18, 2020
@jimmyjames jimmyjames added this to the v1-Next milestone Jun 18, 2020
@jimmyjames jimmyjames modified the milestones: v1-Next, 1.4.1 Jun 18, 2020
@rhanton
Copy link

rhanton commented Jul 20, 2020

This is cool, we had this issue in-house as well and wrote our own customized AuthenticationProvider for it. We'll have to see if we can remove that now.

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.

None yet

7 participants