Skip to content

Support multiple issuers #246 - #288

Merged
lbalmaceda merged 10 commits into
auth0:masterfrom
itdevelopmentapps:master
Mar 11, 2019
Merged

Support multiple issuers #246#288
lbalmaceda merged 10 commits into
auth0:masterfrom
itdevelopmentapps:master

Conversation

@itdevelopmentapps

Copy link
Copy Markdown
Contributor

Added support for multiple issuers.

I wanted to change the previous pull request so I closed it, maybe if you guys can remove that one in favor for this one.

Many thanks.

@lbalmaceda lbalmaceda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the delay. I've left you some comments. Please remember to rebase the branch.

Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java
Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
@lbalmaceda lbalmaceda closed this Jan 28, 2019
@lbalmaceda lbalmaceda reopened this Jan 28, 2019
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated

@lbalmaceda lbalmaceda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion. I've left a few changes I need you to accept before merging this.

Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated
Comment thread lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated
@lbalmaceda

Copy link
Copy Markdown
Contributor

@itdevelopmentapps are you still interesting in merging this PR? Please, don't hesitate to ask any questions.

@itdevelopmentapps

Copy link
Copy Markdown
Contributor Author

@lbalmaceda please advise in what test you would like to see to increase coverage so that we can get this pull request accepted.

@lbalmaceda

Copy link
Copy Markdown
Contributor

By checking the coverage report, there are 2 missing branches.

  • withAudience needs to be called both with a valid audience and a null string value. Then you need to call verify. Tests are only present for a "valid audience value" here.
  • for withIssuer tests are present for "both valid issuer and null string values" here but you need to add another one that verifies against a JWT payload that doesn't have an issuer claim defined.

I hope that's clear enough. It should help make the check green.

@itdevelopmentapps

Copy link
Copy Markdown
Contributor Author

@lbalmaceda we've added the suggested tests.

@lbalmaceda lbalmaceda added this to the v3-Next milestone Mar 11, 2019
@lbalmaceda
lbalmaceda merged commit 6ffa703 into auth0:master Mar 11, 2019
@lbalmaceda

Copy link
Copy Markdown
Contributor

Thank you. Will release before eow.

@itdevelopmentapps

Copy link
Copy Markdown
Contributor Author

Many thanks.

@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.8.0 Mar 14, 2019
@vikeri

vikeri commented Oct 4, 2019

Copy link
Copy Markdown

Using Clojure this was a breaking change, got the following error:

java.lang.ClassCastException: java.lang.String cannot be cast to [Ljava.lang.String;

When trying to call withIssuer.
To fix:
Before

(.withIssuer Verification "example")

After

(.withIssuer Verification (into-array String ["example]))

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