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

Concatenate JWKS path with existing path on domain if present #67

Merged
merged 3 commits into from
Sep 10, 2019

Conversation

ltj
Copy link

@ltj ltj commented Aug 29, 2019

In the current jwks url provider implementation any path on domain will be replaced by the "well known" jwks path. This will likely work for most providers, but some (e.g. AWS Cognito) uses a unique resource path on a shared endpoint to identify the issuer ie. https://cognito-idp.ap-southeast-2.amazonaws.com/ap-southeast-2_example

I'm aware this might have been a deliberate decision given that there is a unit test to verify this behavior. However, I think allowing for a more flexible approach will benefit this library since it's being actively used in other projects such as ktor

Changes

  • Directly concatenate the domain string with the JWKS path and use URI.normalize() to ensure correct behavior regardless of trailing slash is present or not.
  • Added/changed unit tests to reflect new behavior

References

Please include relevant links supporting this change such as a:

  • support ticket
  • community post
  • StackOverflow post
  • support forum thread

Testing

  • This change adds test coverage
  • This change has been tested on the latest version of Java or why not

Checklist

@ltj ltj requested a review from a team August 29, 2019 10:09
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.

We welcome the change! However I think it's better to documented in the javadocs that if a path is included domain string passed, it will be appended the well-known one. Mainly because the existing unit test that you've changed proved the opposite. What do you think?

Also, we rather have explicit imports for readability. Do you mind changing that back?

-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLConnection;
+import java.net.*;

@ltj
Copy link
Author

ltj commented Sep 9, 2019

We welcome the change! However I think it's better to documented in the javadocs that if a path is included domain string passed, it will be appended the well-known one. Mainly because the existing unit test that you've changed proved the opposite. What do you think?

Also, we rather have explicit imports for readability. Do you mind changing that back?

Thanks @lbalmaceda. I concur so just added a commit with explicit imports and updated javadoc to reflect actual behavior

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.

Thanks!

@lbalmaceda lbalmaceda merged commit cbf103e into auth0:master Sep 10, 2019
@lbalmaceda lbalmaceda added this to the v0-Next milestone Sep 10, 2019
@jimmyjames jimmyjames modified the milestones: v0-Next, 0.9.0 Sep 26, 2019
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.

3 participants