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

Support multiple issuers #246 #288

Merged
merged 10 commits into from
Mar 11, 2019
Merged

Support multiple issuers #246 #288

merged 10 commits into from
Mar 11, 2019

Conversation

itdevelopmentapps
Copy link
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.

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 for the PR and sorry for the delay. I've left you some comments. Please remember to rebase the branch.

lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated Show resolved Hide resolved
lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
@lbalmaceda lbalmaceda closed this Jan 28, 2019
@lbalmaceda lbalmaceda reopened this Jan 28, 2019
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.

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

lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
lib/src/main/java/com/auth0/jwt/JWTVerifier.java Outdated Show resolved Hide resolved
lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated Show resolved Hide resolved
lib/src/test/java/com/auth0/jwt/JWTVerifierTest.java Outdated Show resolved Hide resolved
@lbalmaceda
Copy link
Contributor

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

@itdevelopmentapps
Copy link
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
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
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
Contributor

Thank you. Will release before eow.

@itdevelopmentapps
Copy link
Contributor Author

Many thanks.

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

vikeri commented Oct 4, 2019

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