Add DefaultJwtParser functionality to parse JWSs with empty body. #540
Conversation
There was a problem hiding this comment.
@siebenfarben thanks very much for the PR and starting a discussion. I've made some comments - let me know your thoughts.
…. Set payloadRequired true for each require*() method in JwtParser and JwtParserBuilder.
|
Hi @lhazlewood |
|
Great stuff @siebenfarben ! |
| @@ -293,7 +303,7 @@ public Jwt parse(String jwt) throws ExpiredJwtException, MalformedJwtException, | |||
| base64UrlEncodedDigest = sb.toString(); | |||
| } | |||
|
|
|||
| if (base64UrlEncodedPayload == null) { | |||
| if (base64UrlEncodedPayload == null && payloadRequired) { | |||
There was a problem hiding this comment.
I was thinking about triggering exceptions due to payloadRequired() late last night and how an exception might manifest for the user.
Let's say someone configures requireSubject("foo") and then there isn't a payload. The exception they'll see is a generic MalformedJwtException with a message "JWT string '<whatever>' is missing a body/payload", and doesn't explain at all why the JWT is required to have a payload.
To me, this seems to be undesirable. If you think about it, in practice, almost no one really cares that there is a payload or not. What they really care about is if something in the payload exists and/or matches an expectation. And we already have support for enforcing and communicating those things via the other require* methods in the class.
(Also FWIW, I don't think MalformedJwtException is correct here - the JWT isn't malformed - it's just that expectation/validation requirements have failed, so I think we'd have to throw another exception - probably UnsupportedJwtException, and that's if we throw at all...)
Which brings me to my next point: do we need this check at all?
What if we didn't have a payloadRequired check at all and just let the empty string default as done on line 330 in this PR below? The other require* methods would still fail with a meaningful/descriptive exception as is more desirable.
Would there be any problems? Would any of the existing many tests in the project fail? That would tell us a lot.
There was a problem hiding this comment.
Good point. 'Malformed' is certainly not the correct wording in that case. It's more an unmet requirement the callers asked for themselves.
All I can say from during the implementation - no tests fail if this check just goes away. The only thing that needs to be added would be the isEmpty() check for the payload below.
There was a problem hiding this comment.
Ok - in this case, I don't think we need the concept of payloadRequired at all and just keep the isEmpty() check.
There was a problem hiding this comment.
@siebenfarben thanks for going through this process with us. I think we can wrap this up now based on the comments below. After your changes, I'll review once more and then I think we'll be able to merge. Thanks!
| @@ -293,7 +303,7 @@ public Jwt parse(String jwt) throws ExpiredJwtException, MalformedJwtException, | |||
| base64UrlEncodedDigest = sb.toString(); | |||
| } | |||
|
|
|||
| if (base64UrlEncodedPayload == null) { | |||
| if (base64UrlEncodedPayload == null && payloadRequired) { | |||
There was a problem hiding this comment.
Ok - in this case, I don't think we need the concept of payloadRequired at all and just keep the isEmpty() check.
|
Hi @lhazlewood, please have a look at the updated request. :) Small heads-up: I will from now on be on vacation without my laptop. Happy holidays! Best, @siebenfarben |
|
I'm back - happy new year! |
|
Hi @siebenfarben - apologies for the long delay! This was lost in the cracks 😞 . Merging now! |
Context
We are currently implementing RFC 8555 which requires requests authenticated through JWS object payloads.
It suggests using signed POST-AS-GET requests with empty JWS bodies for authenticating GET requests. When using jjwt library for the JWS handling we found that empty bodies are disallowed quite explicitly in
DefaultJwtParserwhen parsing JWSs.Solution
We added flag
allowEmptyBody, alongside JwtParserBuilder fields and methods, to the DefaultJwtParser. This way the empty body check can still be used if required and allowing them is an intentional call.To successfully parse empty-body JWS objects the parsed payload is accepted in a null-safe way (DefaultJWTParser lines 300 & 324). When proactively checking for and mapping json in line 335 we added a simple empty check for the payload.