fix: pass JSON Web Tokens (JWT) as a header rather than as a request parameter#4818
fix: pass JSON Web Tokens (JWT) as a header rather than as a request parameter#4818iansergeant42 wants to merge 24 commits into
Conversation
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Co-authored-by: Andrew Jandacek <andrew.jandacek@broadcom.com> Signed-off-by: Andrew Jandacek <andrew.jandacek@broadcom.com>
Co-authored-by: Andrew Jandacek <andrew.jandacek@broadcom.com> Signed-off-by: iansergeant42 <129340641+iansergeant42@users.noreply.github.com>
Signed-off-by: iansergeant42 <129340641+iansergeant42@users.noreply.github.com>
…/CompoundAuthProvider.java Signed-off-by: iansergeant42 <129340641+iansergeant42@users.noreply.github.com>
…/CompoundAuthProvider.java Signed-off-by: iansergeant42 <129340641+iansergeant42@users.noreply.github.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
…com/zowe/api-layer into reboot/jwt-tokens-passed-in-headers
| @DeleteMapping(path = "/invalidate/{token}") | ||
| public Mono<ResponseEntity<Void>> invalidateJwtToken(@PathVariable String token) { | ||
| @DeleteMapping(path = "/invalidate") | ||
| public Mono<ResponseEntity<Void>> invalidateJwtToken(ServerHttpRequest request) { |
There was a problem hiding this comment.
You can use annotation @RequestHeader("Authorization")
There was a problem hiding this comment.
Done, and it simplifies the code dramatically.
| given() | ||
| .when() | ||
| .get(URI.create(basePath + INVALIDATE_JWT_ENDPOINT + "/" + token)) | ||
| .get(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) |
There was a problem hiding this comment.
given statement should have .header(AUTHORIZATION, "Baerer " + token)
There was a problem hiding this comment.
Added .header(AUTHORIZATION, "Bearer " + token)
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT + "/" + token)) | ||
| .header("Authorization", "Bearer " + token) |
There was a problem hiding this comment.
please, use instead org.springframework.http.HttpHeaders#AUTHORIZATION, probably not onlyhere
| .header("Authorization", "Bearer " + token) | |
| .header(AUTHORIZATION, "Bearer " + token) |
| var invalidated = authenticationService.invalidateJwtTokenGateway(token, false, app); | ||
| final var authHeader = request.getHeaders(). getFirst("Authorization"); | ||
| if (authHeader == null || !authHeader.startsWith("Bearer ")) { | ||
| return Mono.just(ResponseEntity.status(SC_UNAUTHORIZED).build()); |
There was a problem hiding this comment.
The request is auhteticated by x509 and the token is just payload. If it is missing, it id 400 instead of 401.
There was a problem hiding this comment.
Changed to SC_BAD_REQUEST
| return Mono.just(ResponseEntity.status(SC_UNAUTHORIZED).build()); | ||
| } | ||
| final var jwtToken = authHeader.substring(7); //Starts with "Bearer " | ||
| var invalidated = authenticationService.invalidateJwtTokenGateway(jwtToken, false, app); |
There was a problem hiding this comment.
What if the token is missing?
There was a problem hiding this comment.
I've added an isEmpty() check. The token cannot be null at this stage as "Bearer ".substring(7) is empty. I'll add a test for this.
EDIT - use of @RequestHeader("Authorization") means that you see SC_BAD_REQUEST automatically. There's a test for this.
| final var authHeader = request.getHeader("Authorization"); | ||
| if (authHeader == null || !authHeader.startsWith("Bearer ")) { | ||
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
| return; | ||
| } | ||
| final var jwtToken = authHeader.substring(7); |
There was a problem hiding this comment.
same comments as for the reactive alternative
There was a problem hiding this comment.
Largely the same fixes applied.
|
forgotten, needs an update: |
|
@pavel-jares-bcm Currently, there is a discrepancy in modulith that defines |
This code is valid for the cases when infinispan is not in use. It means in HA when caching storage is one of inMemory, redis, or VSAM. So we should avoid calling this endpoint just in case modulith and infinispan are active. |
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
Signed-off-by: Ian Sergeant <ian.sergeant@broadcom.com>
|
| } | ||
|
|
||
| @Test | ||
| void whenNoHeader_withCert_then401() { |
There was a problem hiding this comment.
typo in name 401 -> 400
| if (jwtToken.isEmpty()) { //it cannot be null, as "Bearer".substring(7) is empty, not null | ||
| response.setStatus(SC_BAD_REQUEST); | ||
| return; |
There was a problem hiding this comment.
Can we rely on @RequestHeader like in modulith?
| try { | ||
| restTemplate.delete(url); | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.set("Authorization", "Bearer " + jwtToken); |
There was a problem hiding this comment.
let's use org.apache.http.HttpHeaders.AUTHORIZATION whenever possible, not only in tests



Description
Prior to this change, JWT are passed between components as request parameters. This change moves their passing to the Authorization header.
Linked to # (issue)
Part of the # (epic)
Type of change
Please delete options that are not relevant.
Checklist:
For more details about how should the code look like read the Contributing guideline