Handle verification of JWT tokens gracefully.

[nk1506]

Description

Expired JWT token should return 401 instead of 5xx.

Fixes # (issue)
#304

Please delete options that are not relevant.

• [*] Bug fix (non-breaking change which fixes an issue)

Checklist:

Please delete options that are not relevant.

• [*] I have performed a self-review of my code
• [*] I have commented my code, particularly in hard-to-understand areas
• [*] My changes generate no new warnings

[dimas-b]

Will this message be reported to the client? Exposing too specific validation messages (like Token Expired, Invalid Claim) may not be desirable from the security perspective.

[dimas-b]

I'd suggest logging the specific reason on the server side and returning a generic failure message to the client. WDYT?

If we want debugging aids, we could add the same fresh UUID to both messages for correlation.

[nk1506]

I think "Token expired" should be good others reasons are not required to throw. Also the other validations looks redundant to me.

 if (decodedJWT.getExpiresAtAsInstant().isBefore(Instant.now())) {
      throw new NotAuthorizedException("Token has expired");
    }

Adding UUID is a good idea.

[dimas-b]

Yeah, anything that can be derived from the public JWT data (e.g. expiry) is ok to mention in the public error message (to help with trouble-shooting).

[collado-mike]

We shouldn't add another UUID. The traceId and spanId are already in the Logging MDC - https://github.com/collado-mike/polaris/blob/8cb6b44bf57dc597dab612d109d3eb534aef5715/polaris-service/src/main/java/org/apache/polaris/service/tracing/TracingFilter.java#L80-L82 .

I think a great idea would be to modify the IcebergExceptionMapper to include the traceId in the error message (or maybe we can subclass the ErrorResponse to have its own traceId field?). This would include the traceId for every error returned so we can always find the log statements that correspond to the error

[eric-maynard]

I've also recently introduced a PolarisExceptionMapper, we could use that

[nk1506]

I have done some modification of token verifying logic. this should do expiry and token status validation.

JWT.require(getAlgorithm()).withClaim(CLAIM_KEY_ACTIVE, true).build();

I have added tests for the same.

I might need help for adding the trace it. Do we have traceId set in thread local ? or I should just assign a randomId with exception and logs.

[nk1506]

Although other fix would have been adding JWTVerificationException at IcebergExceptionMapper as

case JWTVerificationException e -> Response.Status.UNAUTHORIZED.getStatusCode();

But this will start showing all the token validation failed reasons as response entity.

[nk1506]

@eric-maynard / @collado-mike , PTAL. let me know if there are any thoughts.

[eric-maynard]

+1 to adding the traceId to the error message

[dimas-b]

suggestion: add the whole e object (instead of individual e.getCause() and e.getMessage()) to the log call to get its stack trace printed.

[dimas-b]

LGTM 👍 Thanks @nk1506

[nk1506]

@collado-mike @eric-maynard , Please review . There was a flaky test failure with new test Class so I had to move tests to PolarisServiceImplIntegrationTest