-
Notifications
You must be signed in to change notification settings - Fork 78
fix: pass JSON Web Tokens (JWT) as a header rather than as a request parameter #4818
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
base: v3.x.x
Are you sure you want to change the base?
Changes from all commits
7ecd57c
8e8fefa
20c0e3c
b64e4f8
b595bde
e6fd29a
ff38151
60dd373
b801d89
3dedd00
21114a1
25e749e
b504e92
5ac0eea
01272d4
1b683ca
64d1179
cfea024
0496d8c
32530e4
00f0066
5aa251a
ae882a1
9540e6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,10 @@ | |
| import java.net.URI; | ||
|
|
||
| import static io.restassured.RestAssured.given; | ||
| import static org.apache.http.HttpHeaders.AUTHORIZATION; | ||
| import static org.apache.http.HttpStatus.SC_BAD_REQUEST; | ||
| import static org.apache.http.HttpStatus.SC_METHOD_NOT_ALLOWED; | ||
| import static org.apache.http.HttpStatus.SC_OK; | ||
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
@@ -33,6 +36,7 @@ | |
| @Import(ReactiveAuthenticationControllerTests.MockRegisterToApiLayerConfig.class) | ||
| class ReactiveAuthenticationControllerTests extends AcceptanceTestWithMockServices { | ||
|
|
||
| private static final String BEARER = "Bearer "; | ||
| private static final String REFRESH_ENDPOINT = "/gateway/api/v1/auth/refresh"; | ||
| private static final String LOGIN_ENDPOINT = "/gateway/api/v1/auth/login"; | ||
| private static final String AUTH_COOKIE = "apimlAuthenticationToken"; | ||
|
|
@@ -150,11 +154,12 @@ void whenInvalidateJwt_thenRequireCertificateAuthentication() { | |
|
|
||
| @Test | ||
| void whenInvalidate_wrongMethod_thenFail() { | ||
| var token = login(); | ||
|
|
||
| var token = login(); | ||
| given() | ||
| .header(AUTHORIZATION, BEARER + token) | ||
| .when() | ||
| .get(URI.create(basePath + INVALIDATE_JWT_ENDPOINT + "/" + token)) | ||
| .get(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given statement should have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added .header(AUTHORIZATION, "Bearer " + token) |
||
| .then() | ||
| .statusCode(SC_METHOD_NOT_ALLOWED); | ||
| } | ||
|
|
@@ -166,10 +171,57 @@ void whenInvalidateJwt_withCert_thenSuccess() { | |
| given() | ||
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT + "/" + token)) | ||
| .header(AUTHORIZATION, BEARER + token) | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
| .then() | ||
| .statusCode(200); | ||
| .statusCode(SC_OK); | ||
| given() | ||
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .header(AUTHORIZATION, "Bearer " + token) //this will work as the token has its extra space trimmed | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
| .then() | ||
| .statusCode(SC_OK); | ||
| } | ||
|
|
||
| @Test | ||
| void whenNoHeader_withCert_then401() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo in name 401 -> 400 |
||
|
|
||
| given() | ||
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
| .then() | ||
| .statusCode(SC_BAD_REQUEST); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| void whenInvalidHeader_withCert_then400() { | ||
|
|
||
| given() | ||
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .header(AUTHORIZATION, "wibble") | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
| .then() | ||
| .statusCode(SC_BAD_REQUEST); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| void whenEmptyToken_withCert_then400() { | ||
|
|
||
| given() | ||
| .config(SslContext.clientCertApiml) | ||
| .when() | ||
| .header(AUTHORIZATION, BEARER) | ||
| .delete(URI.create(basePath + INVALIDATE_JWT_ENDPOINT)) | ||
| .then() | ||
| .statusCode(SC_BAD_REQUEST); | ||
|
|
||
| } | ||
|
|
||
|
richard-salac marked this conversation as resolved.
|
||
| @TestConfiguration | ||
| public static class MockRegisterToApiLayerConfig { | ||
| @Bean | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ public class AuthController { | |
| private static final ObjectWriter writer = new ObjectMapper().writer(); | ||
|
|
||
| public static final String CONTROLLER_PATH = "/zaas/api/v1/auth"; // NOSONAR: URL is always using / to separate path segments | ||
| public static final String INVALIDATE_PATH = "/invalidate/**"; // NOSONAR | ||
| public static final String INVALIDATE_PATH = "/invalidate"; // NOSONAR | ||
| public static final String DISTRIBUTE_PATH = "/distribute/**"; // NOSONAR | ||
| public static final String PUBLIC_KEYS_PATH = "/keys/public"; // NOSONAR | ||
| public static final String ACCESS_TOKEN_REVOKE = "/access-token/revoke"; // NOSONAR | ||
|
|
@@ -114,12 +114,17 @@ public class AuthController { | |
| @ApiResponse(responseCode = "400", description = "Invalid token"), | ||
| @ApiResponse(responseCode = "503", description = "Authentication service is not available") | ||
| }) | ||
| public void invalidateJwtToken(HttpServletRequest request, HttpServletResponse response) { | ||
| final String endpoint = "/auth/invalidate/"; | ||
| final String uri = request.getRequestURI(); | ||
| final int index = uri.indexOf(endpoint); | ||
| public void invalidateJwtToken(@RequestHeader("Authorization") String authHeader, HttpServletResponse response) { | ||
| if (!authHeader.startsWith("Bearer ")) { | ||
| response.setStatus(SC_BAD_REQUEST); | ||
| return; | ||
| } | ||
| final var jwtToken = authHeader.substring(7).trim(); | ||
| if (jwtToken.isEmpty()) { //it cannot be null, as "Bearer".substring(7) is empty, not null | ||
| response.setStatus(SC_BAD_REQUEST); | ||
| return; | ||
|
Comment on lines
+123
to
+125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rely on @RequestHeader like in modulith? |
||
| } | ||
|
|
||
| final String jwtToken = uri.substring(index + endpoint.length()); | ||
| try { | ||
| final boolean invalidated = authenticationService.invalidateJwtToken(jwtToken, false); | ||
| response.setStatus(invalidated ? SC_OK : SC_SERVICE_UNAVAILABLE); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,9 @@ | |
| import org.springframework.context.ApplicationContext; | ||
| import org.springframework.context.annotation.Scope; | ||
| import org.springframework.context.annotation.ScopedProxyMode; | ||
| import org.springframework.http.HttpEntity; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.http.HttpMethod; | ||
| import org.springframework.security.authentication.BadCredentialsException; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.web.client.HttpClientErrorException; | ||
|
|
@@ -275,11 +277,10 @@ public Boolean invalidateJwtTokenGateway(String jwtToken, boolean distribute, Ap | |
| * Obtain URL to use to invalidate a JWT | ||
| * | ||
| * @param instanceInfo Registration data for the authentication service used | ||
| * @param jwtToken JWT token to invalidate | ||
| * @return | ||
| * @return the URL | ||
| */ | ||
| protected String getInvalidateUrl(InstanceInfo instanceInfo, String jwtToken) { | ||
| return EurekaUtils.getUrl(instanceInfo) + AuthController.CONTROLLER_PATH + "/invalidate/" + jwtToken; | ||
| protected String getInvalidateUrl(InstanceInfo instanceInfo) { | ||
| return EurekaUtils.getUrl(instanceInfo) + AuthController.CONTROLLER_PATH + "/invalidate"; | ||
| } | ||
|
|
||
| private boolean invalidateTokenOnAnotherInstance(String jwtToken, Application application) { | ||
|
|
@@ -295,9 +296,17 @@ private boolean invalidateTokenOnAnotherInstance(String jwtToken, Application ap | |
| continue; | ||
| } | ||
|
|
||
| final String url = getInvalidateUrl(instanceInfo, jwtToken); | ||
| final String url = getInvalidateUrl(instanceInfo); | ||
| try { | ||
| restTemplate.delete(url); | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.set("Authorization", "Bearer " + jwtToken); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's use org.apache.http.HttpHeaders.AUTHORIZATION whenever possible, not only in tests |
||
| HttpEntity<Void> requestEntity = new HttpEntity<>(headers); | ||
| restTemplate.exchange( | ||
| url, | ||
| HttpMethod.DELETE, | ||
| requestEntity, | ||
| Void.class | ||
| ); | ||
| } catch (HttpClientErrorException e) { | ||
| log.debug("Problem invalidating token on another instance url {}", url, e); | ||
| returnValue = Boolean.FALSE; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * This program and the accompanying materials are made available under the terms of the | ||
| * Eclipse Public License v2.0 which accompanies this distribution, and is available at | ||
| * https://www.eclipse.org/legal/epl-v20.html | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Copyright Contributors to the Zowe Project. | ||
| */ | ||
|
|
||
| package org.zowe.apiml.zaas.security.service; | ||
|
|
||
| import com.netflix.appinfo.InstanceInfo; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| class ModulithAuthenticationServiceTest { | ||
| @Test | ||
| void getInvalidateUrl() { | ||
| ModulithAuthenticationService service = new ModulithAuthenticationService(null, null, null, null, null, null, null, null); | ||
| InstanceInfo instanceInfo = mock(InstanceInfo.class); | ||
| when(instanceInfo.getHostName()).thenReturn("localhost"); | ||
| when(instanceInfo.getSecurePort()).thenReturn(443); | ||
| when(instanceInfo.isPortEnabled(InstanceInfo.PortType.SECURE)).thenReturn(true); | ||
| String invalidateUrl = service.getInvalidateUrl(instanceInfo); | ||
| assertEquals("https://localhost:443/gateway/api/v1/auth/invalidate/", invalidateUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the token is missing?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.