Repository navigation
test: cover null claim names, zero leeway and null audience in JWTVerifier - #814
Open
renanmpimentel wants to merge 1 commit into
Open
renanmpimentel wants to merge 1 commit into
renanmpimentel wants to merge 1 commit into
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Test-only change in
JWTVerifierTest. No production code, public API or behaviour is changed. It adds three tests for documentedVerificationcontracts that the current suite doesn't check:shouldThrowOnNullCustomClaimNameForEveryValueType:withClaim/withArrayClaimare documented to throwIllegalArgumentExceptionwhen the claim name isnull. Only thewithClaim(String, String)overload (andwithClaimPresence) is tested today. This test covers theBoolean,Integer,Long,Double,InstantandBiPredicateoverloads ofwithClaim, and theString...,Integer...andLong...overloads ofwithArrayClaim.shouldAcceptZeroLeeway:acceptLeeway,acceptExpiresAt,acceptNotBeforeandacceptIssuedAtare documented to throw only when the leeway is negative. Negative values are tested, but0never is.shouldThrowWhenExpectedNullAudienceButTokenHasAudience: mirrors the existingwithAnyOfAudiencecase (shouldThrowWhenExpectedEmptyList) forwithAudience((String[]) null). A token that has anaudclaim must be rejected withIncorrectClaimException.Each of these regressions currently leaves the whole suite green:
JWTVerifierassertNonNull(name)fromwithClaim(String, Boolean)Integer,Long,Double,Instant,BiPredicatewithArrayClaim(String...),(Integer...),(Long...)assertPositive:leeway < 0→leeway <= 0(rejects 0)withAudience:verifyNull(claim, value)→value == null(accepts anyaudwhennullis expected)Each regression was applied on its own against the unmodified suite, then against the new one.
References
No issue. I found these while experimenting with Supertest, a tool for evaluating test effectiveness using mutation testing and test-harness mutilation. With PIT 1.15.8 on
JWTVerifier*, killed mutants go from 155 to 165 of 171. The remaining 6 are equivalent, e.g.addMandatoryClaimCheckslambdas whose callee only returnstrueor throws.Testing
./gradlew assemble apiDiff check jacocoTestReport --continue(same as CI): BUILD SUCCESSFUL. Tests ran on Java 8, 11, 17 and 21 with 0 failures, and checkstyle and apiDiff pass.The before/after table above was produced by applying each regression in isolation and running
./gradlew :java-jwt:test.This change adds test coverage
This change has been tested on the latest version of Java or why not
Checklist