Issue #20 added url constructor to JwkProviderBuilder #22

darthvalinor · 2018-04-06T23:06:55Z

URL constructor added to JwkProviderBuilder, which allows to customize the url to get JWKS. Before, it could only be <domain>/.well-known/jwks.json, now it can be <domain>/sub/path/.well-known/jwks.json
In case url is null IllegalStateException is thrown, so the behavior is similar to the String constructor
Unfortunately, I didn't find a good way to move JwkProviderBuilder.normalizeDomain to UrlJwkProvider keeping the same behavior
Slight fixes for JavaDocs
Please, let me know if more tests required

lbalmaceda · 2018-04-13T15:50:25Z

Thanks for the contribution!
My only strong opinion is that I see no use on having a separate class for handling the construction of the URL from the string domain (that factory class). I'd keep that logic in the original class private method.

Unfortunately, I didn't find a good way to move JwkProviderBuilder.normalizeDomain to UrlJwkProvider keeping the same behavior

Regarding this, the JwkProviderBuilder shouldn't have that normalizeDomain method. That should be part of the URLJwKProvider because it already handles "that type of data". If you can make those 2 changes, I'll then review javadoc or error messages and merge it. 👍

…actory class

darthvalinor · 2018-04-17T22:20:08Z

@lbalmaceda thanks for the review!
I updated the pull-request with your points.

The added class removed. The URL creation returned to URLJwKProvider.urlForDomain as package-private and reused in JwkProviderBuilder(String).
Domain normalization had to be moved to URLJwKProvider which caused a change in behavior: JwkProviderBuilder(String) no longer fails on non-normilized domains. The according test removed. Hopefully, it's acceptable.
Tests and JavaDocs adjusted accordingly.
Also need your opinion on IllegalStateException in JwkProviderBuilder vs IllegalArgumentException in URLJwKProvider.

mhlulani · 2018-04-22T10:23:32Z

When can we have this on Maven Central?

lbalmaceda

Mostly javadoc changes.

lbalmaceda · 2018-04-23T17:58:37Z

src/main/java/com/auth0/jwk/UrlJwkProvider.java

-     * @param domain domain where to look for the jwks.json file
+     * Creates a provider that loads from the given domain's well-known directory.
+     * <br>If the protocol (http or https) is not provided then https is used by default
+     * (some.domain -> "https://" + some.domain + {@value #WELL_KNOWN_JWKS_PATH}).


use {DOMAIN} instead of "some.domain". You can also say something like "for example, when -domain- is -acme.com- the jwks url that will be used is -https://acme.com/.well-known/jwks.json"

don't mention the constant. Use "/.well-known/jwks.json" instead, since the constant is package private anyway and users won't see it.

the JavaDoc fixed and alligned with JwkProviderBuilder(String)

lbalmaceda · 2018-04-23T18:21:10Z

src/test/java/com/auth0/jwk/JwkProviderBuilderTest.java

-import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.notNullValue;
+import static com.auth0.jwk.UrlJwkProvider.WELL_KNOWN_JWKS_PATH;
+import static org.hamcrest.Matchers.*;


Probably better to import the required ones only

Agree, safer, fixed

lbalmaceda · 2018-04-23T18:30:00Z

src/test/java/com/auth0/jwk/UrlJwkProviderTest.java

@@ -21,35 +21,24 @@
    public ExpectedException expectedException = ExpectedException.none();

    @Before
-    public void setUp() throws Exception {
+    public void setUp() {


This whole "before" method is no longer being used generically. It's only used in shouldReturnSingleJwkById. Feel free to remove this and the provider field.

Made some cleanup

lbalmaceda · 2018-04-23T18:40:40Z

src/main/java/com/auth0/jwk/UrlJwkProvider.java

 /**
 * Jwk provider that loads them from a {@link URL}
 */
 @SuppressWarnings("WeakerAccess")
 public class UrlJwkProvider implements JwkProvider {
+
+    static final String WELL_KNOWN_JWKS_PATH = "/.well-known/jwks.json";


please add a comment above

//visible for testing

Used @VisibleForTesting from Guava instead

lbalmaceda · 2018-04-23T18:48:43Z

src/main/java/com/auth0/jwk/UrlJwkProvider.java

        }

        try {
            final URL url = new URL(domain);
-            return new URL(url, "/.well-known/jwks.json");
+            return new URL(url, WELL_KNOWN_JWKS_PATH);
        } catch (MalformedURLException e) {
            throw new IllegalArgumentException("Invalid jwks uri", e);


This line hasn't been tested. Telling by the docs it will throw "if no protocol is specified, or an unknown protocol is found, or spec is null."

being "protocol" part of the URL (first param) and

being "spec" the well-known constant, which is never null.

So protocol could be any of http, https, ftp, file, and jar. We are adding https if the domain doesn't begin with http, so those 2 protocols are covered. And because we're enforcing any of those 2, this should be a valid value too. I don't see how to trick our logic in a test to make it fail in this line. If you happen to find something, feel free to add it. But don't worry if you can't that's OK anyway and we should catch it here. (which again, should never be called).

covered with a test with "httptest://..."

lbalmaceda · 2018-04-23T18:49:41Z

src/main/java/com/auth0/jwk/JwkProviderBuilder.java

@@ -35,6 +38,26 @@ public JwkProviderBuilder(String domain) {
        this.bucket = new BucketImpl(10, 1, TimeUnit.MINUTES);
    }

+    /**
+     * Creates a new Builder with a domain where to look for the jwks.
+     * It can be a url link 'https://samples.auth0.com' or just a domain 'samples.auth0.com'.


You should mention that this method appends the default jwks path to the given string domain

lbalmaceda · 2018-04-23T20:22:24Z

src/test/java/com/auth0/jwk/UrlJwkProviderTest.java

@@ -122,4 +121,10 @@ public void shouldUseOnlyDomain() {
        String actualJwksUrl = new UrlJwkProvider(domainWithSubPath).url.toString();
        assertThat(actualJwksUrl, equalTo("https://" + domain + WELL_KNOWN_JWKS_PATH));
    }
+
+    @Test(expected = IllegalArgumentException.class)


Please use the other syntax so we have everything the same.

expectedException.expect(IllegalArgumentException.class);

lbalmaceda · 2018-04-23T20:22:44Z

@cocojoe I really like this to be merged. Can I have your blessing?

cocojoe · 2018-04-23T20:24:15Z

@lbalmaceda If you're happy, I'm happy.

darthvalinor · 2018-04-23T20:30:17Z

I accidently made a comment fix and then reverted it. In order to keep the history cleaner, I can rebase. What do you think?

lbalmaceda · 2018-04-23T20:43:15Z

As you want, I can also "squash and merge" and keep 1 single commit for the entire PR. No worries. I'll merge this one and try to release by tomorrow.

darthvalinor · 2018-04-23T20:52:37Z

Removed those two

added url constructor to JwkProviderBuilder

15121b2

darthvalinor mentioned this pull request Apr 6, 2018

Feature Request: Full URLs in Builder Constructor #20

Closed

UrlJwkProvider(String) doesn't fail on invalid domains, removed url f…

74de690

…actory class

lbalmaceda self-assigned this Apr 20, 2018

lbalmaceda suggested changes Apr 23, 2018

View reviewed changes

darthvalinor added 2 commits April 23, 2018 23:00

JavaDocs improved

2e7e2fe

added test for invalid protocol case; some test cleanup

c4a5f97

lbalmaceda reviewed Apr 23, 2018

View reviewed changes

expectedException as rule

d07af0c

darthvalinor force-pushed the master branch from 76fc7de to d07af0c Compare April 23, 2018 20:50

lbalmaceda approved these changes Apr 23, 2018

View reviewed changes

lbalmaceda merged commit f6a209b into auth0:master Apr 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #20 added url constructor to JwkProviderBuilder #22

Issue #20 added url constructor to JwkProviderBuilder #22

darthvalinor commented Apr 6, 2018 •

edited

lbalmaceda commented Apr 13, 2018 •

edited

darthvalinor commented Apr 17, 2018 •

edited

mhlulani commented Apr 22, 2018

lbalmaceda left a comment

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018 •

edited

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda Apr 23, 2018

darthvalinor Apr 23, 2018

lbalmaceda commented Apr 23, 2018

cocojoe commented Apr 23, 2018

darthvalinor commented Apr 23, 2018

lbalmaceda commented Apr 23, 2018

darthvalinor commented Apr 23, 2018

Issue #20 added url constructor to JwkProviderBuilder #22

Issue #20 added url constructor to JwkProviderBuilder #22

Conversation

darthvalinor commented Apr 6, 2018 • edited

lbalmaceda commented Apr 13, 2018 • edited

darthvalinor commented Apr 17, 2018 • edited

mhlulani commented Apr 22, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthvalinor Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Apr 23, 2018

cocojoe commented Apr 23, 2018

darthvalinor commented Apr 23, 2018

lbalmaceda commented Apr 23, 2018

darthvalinor commented Apr 23, 2018

darthvalinor commented Apr 6, 2018 •

edited

lbalmaceda commented Apr 13, 2018 •

edited

darthvalinor commented Apr 17, 2018 •

edited

darthvalinor Apr 23, 2018 •

edited