This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit f2542273fc176c617baa30df8c1b84cf0fc1f94f Author: Benoit Tellier <[email protected]> AuthorDate: Fri Jun 4 21:23:17 2021 +0700 [PERFORMANCE] Avoid doing JWT parsing twice --- .../james/jmap/http/JWTAuthenticationStrategyTest.java | 11 +++++------ .../apache/james/jmap/jwt/JWTAuthenticationStrategy.java | 8 +++----- .../main/java/org/apache/james/jwt/JwtTokenVerifier.java | 10 ++++++---- .../java/org/apache/james/jwt/JwtTokenVerifierTest.java | 16 ++++++++-------- .../apache/james/webadmin/authentication/JwtFilter.java | 10 ++++------ .../james/webadmin/authentication/JwtFilterTest.java | 8 +++++--- 6 files changed, 31 insertions(+), 32 deletions(-) diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/http/JWTAuthenticationStrategyTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/http/JWTAuthenticationStrategyTest.java index 78b57ad..cf4ea0e 100644 --- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/http/JWTAuthenticationStrategyTest.java +++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/http/JWTAuthenticationStrategyTest.java @@ -24,6 +24,8 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.Optional; + import org.apache.james.core.Username; import org.apache.james.domainlist.api.DomainList; import org.apache.james.jmap.exceptions.UnauthorizedException; @@ -81,8 +83,7 @@ public class JWTAuthenticationStrategyTest { String fakeAuthHeaderWithPrefix = JWTAuthenticationStrategy.AUTHORIZATION_HEADER_PREFIX + validAuthHeader; MailboxSession fakeMailboxSession = mock(MailboxSession.class); - when(stubTokenVerifier.verify(validAuthHeader)).thenReturn(false); - when(stubTokenVerifier.extractLogin(validAuthHeader)).thenReturn(username); + when(stubTokenVerifier.verifyAndExtractLogin(validAuthHeader)).thenReturn(Optional.empty()); when(mockedMailboxManager.createSystemSession(eq(Username.of(username)))) .thenReturn(fakeMailboxSession); @@ -121,8 +122,7 @@ public class JWTAuthenticationStrategyTest { String fakeAuthHeaderWithPrefix = JWTAuthenticationStrategy.AUTHORIZATION_HEADER_PREFIX + validAuthHeader; MailboxSession fakeMailboxSession = mock(MailboxSession.class); - when(stubTokenVerifier.verify(validAuthHeader)).thenReturn(true); - when(stubTokenVerifier.extractLogin(validAuthHeader)).thenReturn(username); + when(stubTokenVerifier.verifyAndExtractLogin(validAuthHeader)).thenReturn(Optional.of(username)); when(mockedMailboxManager.createSystemSession(eq(Username.of(username)))) .thenReturn(fakeMailboxSession); when(mockedHeaders.get(AUTHORIZATION_HEADERS)) @@ -139,8 +139,7 @@ public class JWTAuthenticationStrategyTest { String fakeAuthHeaderWithPrefix = JWTAuthenticationStrategy.AUTHORIZATION_HEADER_PREFIX + validAuthHeader; MailboxSession fakeMailboxSession = mock(MailboxSession.class); - when(stubTokenVerifier.verify(validAuthHeader)).thenReturn(true); - when(stubTokenVerifier.extractLogin(validAuthHeader)).thenReturn(username); + when(stubTokenVerifier.verifyAndExtractLogin(validAuthHeader)).thenReturn(Optional.of(username)); when(mockedMailboxManager.createSystemSession(eq(Username.of(username)))) .thenReturn(fakeMailboxSession); when(mockedHeaders.get(AUTHORIZATION_HEADERS)) diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/jwt/JWTAuthenticationStrategy.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/jwt/JWTAuthenticationStrategy.java index 55352af..06d8c80 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/jwt/JWTAuthenticationStrategy.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/jwt/JWTAuthenticationStrategy.java @@ -63,11 +63,9 @@ public class JWTAuthenticationStrategy implements AuthenticationStrategy { .filter(header -> header.startsWith(AUTHORIZATION_HEADER_PREFIX)) .map(header -> header.substring(AUTHORIZATION_HEADER_PREFIX.length())) .flatMap(userJWTToken -> Mono.fromCallable(() -> { - if (!tokenManager.verify(userJWTToken)) { - throw new UnauthorizedException("Failed Jwt verification"); - } - - Username username = Username.of(tokenManager.extractLogin(userJWTToken)); + Username username = tokenManager.verifyAndExtractLogin(userJWTToken) + .map(Username::of) + .orElseThrow(() -> new UnauthorizedException("Failed Jwt verification")); try { usersRepository.assertValid(username); } catch (UsersRepositoryException e) { diff --git a/server/protocols/jwt/src/main/java/org/apache/james/jwt/JwtTokenVerifier.java b/server/protocols/jwt/src/main/java/org/apache/james/jwt/JwtTokenVerifier.java index f7f833c..6eb71ec 100644 --- a/server/protocols/jwt/src/main/java/org/apache/james/jwt/JwtTokenVerifier.java +++ b/server/protocols/jwt/src/main/java/org/apache/james/jwt/JwtTokenVerifier.java @@ -18,6 +18,8 @@ ****************************************************************/ package org.apache.james.jwt; +import java.util.Optional; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,20 +49,20 @@ public class JwtTokenVerifier { this.pubKeyProvider = pubKeyProvider; } - public boolean verify(String token) { + public Optional<String> verifyAndExtractLogin(String token) { try { String subject = extractLogin(token); if (Strings.isNullOrEmpty(subject)) { throw new MalformedJwtException("'subject' field in token is mandatory"); } - return true; + return Optional.of(subject); } catch (JwtException e) { LOGGER.info("Failed Jwt verification", e); - return false; + return Optional.empty(); } } - public String extractLogin(String token) throws JwtException { + private String extractLogin(String token) throws JwtException { Jws<Claims> jws = parseToken(token); return jws .getBody() diff --git a/server/protocols/jwt/src/test/java/org/apache/james/jwt/JwtTokenVerifierTest.java b/server/protocols/jwt/src/test/java/org/apache/james/jwt/JwtTokenVerifierTest.java index 75b638a..08c854b 100644 --- a/server/protocols/jwt/src/test/java/org/apache/james/jwt/JwtTokenVerifierTest.java +++ b/server/protocols/jwt/src/test/java/org/apache/james/jwt/JwtTokenVerifierTest.java @@ -80,7 +80,7 @@ class JwtTokenVerifierTest { @Test void shouldReturnTrueOnValidSignature() { - assertThat(sut.verify(VALID_TOKEN_WITHOUT_ADMIN)).isTrue(); + assertThat(sut.verifyAndExtractLogin(VALID_TOKEN_WITHOUT_ADMIN)).isPresent(); } @Test @@ -89,7 +89,7 @@ class JwtTokenVerifierTest { "tPL3EZdkeYxw_DV2KimE1U2FvuLHmfR_mimJ5US3JFU4J2Gd94O7rwpSTGN1B9h-_lsTebo4ua4xHsTtmczZ9xa8a_kWKaSkqFjNFa" + "Fp6zcoD6ivCu03SlRqsQzSRHXo6TKbnqOt9D6Y2rNa3C4igSwoS0jUE4BgpXbc0"; - assertThat(sut.verify(invalidToken)).isFalse(); + assertThat(sut.verifyAndExtractLogin(invalidToken)).isEmpty(); } @Test @@ -100,7 +100,7 @@ class JwtTokenVerifierTest { "VPY3q15vWzZH9O9IJzB2KdHRMPxl2luRjzDbh4DLp56NhZuLX_2a9UAlmbV8MQX4Z_04ybhAYrcBfxR3MgJyr0jlxSibqSbXrkXuo-" + "PyybfZCIhK_qXUlO5OS6sO7AQhKZO9p0MQ"; - assertThat(sut.verify(tokenWithNullSubject)).isFalse(); + assertThat(sut.verifyAndExtractLogin(tokenWithNullSubject)).isEmpty(); } @Test @@ -112,22 +112,22 @@ class JwtTokenVerifierTest { "sUSbogmgObA_BimNtUq_Q1p0SGtIYBXmQ"; - assertThat(sut.verify(tokenWithEmptySubject)).isFalse(); + assertThat(sut.verifyAndExtractLogin(tokenWithEmptySubject)).isEmpty(); } @Test void verifyShouldNotAcceptNoneAlgorithm() { - assertThat(sut.verify(TOKEN_NONE_ALGORITHM)).isFalse(); + assertThat(sut.verifyAndExtractLogin(TOKEN_NONE_ALGORITHM)).isEmpty(); } @Test void verifyShouldNotAcceptNoneAlgorithmWithoutSignature() { - assertThat(sut.verify(TOKEN_NONE_ALGORITHM_NO_SIGNATURE)).isFalse(); + assertThat(sut.verifyAndExtractLogin(TOKEN_NONE_ALGORITHM_NO_SIGNATURE)).isEmpty(); } @Test void shouldReturnUserLoginFromValidToken() { - assertThat(sut.extractLogin(VALID_TOKEN_WITHOUT_ADMIN)).isEqualTo("1234567890"); + assertThat(sut.verifyAndExtractLogin(VALID_TOKEN_WITHOUT_ADMIN)).contains("1234567890"); } @Test @@ -153,7 +153,7 @@ class JwtTokenVerifierTest { @Test void extractLoginShouldWorkWithAdminClaim() { - assertThat(sut.extractLogin(VALID_TOKEN_ADMIN_TRUE)).isEqualTo("[email protected]"); + assertThat(sut.verifyAndExtractLogin(VALID_TOKEN_ADMIN_TRUE)).contains("[email protected]"); } @Test diff --git a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/authentication/JwtFilter.java b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/authentication/JwtFilter.java index 972569b..ab61c60 100644 --- a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/authentication/JwtFilter.java +++ b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/authentication/JwtFilter.java @@ -52,10 +52,9 @@ public class JwtFilter implements AuthenticationFilter { .map(value -> value.substring(AUTHORIZATION_HEADER_PREFIX.length())); checkHeaderPresent(bearer); - checkValidSignature(bearer); + String login = retrieveUser(bearer); checkIsAdmin(bearer); - String login = jwtTokenVerifier.extractLogin(bearer.get()); request.attribute(LOGIN, login); } } @@ -66,10 +65,9 @@ public class JwtFilter implements AuthenticationFilter { } } - private void checkValidSignature(Optional<String> bearer) { - if (!jwtTokenVerifier.verify(bearer.get())) { - halt(HttpStatus.UNAUTHORIZED_401, "Invalid Bearer header."); - } + private String retrieveUser(Optional<String> bearer) { + return jwtTokenVerifier.verifyAndExtractLogin(bearer.get()) + .orElseThrow(() -> halt(HttpStatus.UNAUTHORIZED_401, "Invalid Bearer header.")); } private void checkIsAdmin(Optional<String> bearer) { diff --git a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/authentication/JwtFilterTest.java b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/authentication/JwtFilterTest.java index 6931493..765de36 100644 --- a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/authentication/JwtFilterTest.java +++ b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/authentication/JwtFilterTest.java @@ -24,6 +24,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; +import java.util.Optional; + import org.apache.james.jwt.JwtTokenVerifier; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -86,7 +88,7 @@ class JwtFilterTest { Request request = mock(Request.class); when(request.requestMethod()).thenReturn("GET"); when(request.headers(JwtFilter.AUTHORIZATION_HEADER_NAME)).thenReturn("Bearer value"); - when(jwtTokenVerifier.verify("value")).thenReturn(false); + when(jwtTokenVerifier.verifyAndExtractLogin("value")).thenReturn(Optional.empty()); assertThatThrownBy(() -> jwtFilter.handle(request, mock(Response.class))) @@ -100,7 +102,7 @@ class JwtFilterTest { Request request = mock(Request.class); when(request.requestMethod()).thenReturn("GET"); when(request.headers(JwtFilter.AUTHORIZATION_HEADER_NAME)).thenReturn("Bearer value"); - when(jwtTokenVerifier.verify("value")).thenReturn(true); + when(jwtTokenVerifier.verifyAndExtractLogin("value")).thenReturn(Optional.of("value")); when(jwtTokenVerifier.hasAttribute("admin", true, "value")).thenReturn(false); assertThatThrownBy(() -> jwtFilter.handle(request, mock(Response.class))) @@ -114,7 +116,7 @@ class JwtFilterTest { Request request = mock(Request.class); when(request.requestMethod()).thenReturn("GET"); when(request.headers(JwtFilter.AUTHORIZATION_HEADER_NAME)).thenReturn("Bearer value"); - when(jwtTokenVerifier.verify("value")).thenReturn(true); + when(jwtTokenVerifier.verifyAndExtractLogin("value")).thenReturn(Optional.of("value")); when(jwtTokenVerifier.hasAttribute("admin", true, "value")).thenReturn(true); jwtFilter.handle(request, mock(Response.class)); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
