From 47430c045a2f57f1ca4a163117c174ba94b281c4 Mon Sep 17 00:00:00 2001 From: Zixuan Liu Date: Mon, 13 Jan 2025 16:31:06 +0800 Subject: [PATCH] [fix][broker] Continue using the next provider for http authentication if one fails Signed-off-by: Zixuan Liu --- .../authentication/AuthenticationService.java | 22 +-- .../auth/AuthenticationServiceTest.java | 126 ++++++++++++++++++ 2 files changed, 133 insertions(+), 15 deletions(-) diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java index f6eb785d2e479..e2bf4dcc0156d 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java @@ -24,11 +24,10 @@ import java.io.Closeable; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; import javax.naming.AuthenticationException; import javax.servlet.http.HttpServletRequest; @@ -49,7 +48,7 @@ public class AuthenticationService implements Closeable { private static final Logger LOG = LoggerFactory.getLogger(AuthenticationService.class); private final String anonymousUserRole; - private final Map providers = new HashMap<>(); + private final Map providers = new LinkedHashMap<>(); public AuthenticationService(ServiceConfiguration conf) throws PulsarServerException { this(conf, OpenTelemetry.noop()); @@ -60,7 +59,7 @@ public AuthenticationService(ServiceConfiguration conf, OpenTelemetry openTeleme anonymousUserRole = conf.getAnonymousUserRole(); if (conf.isAuthenticationEnabled()) { try { - Map> providerMap = new HashMap<>(); + Map> providerMap = new LinkedHashMap<>(); for (String className : conf.getAuthenticationProviders()) { if (className.isEmpty()) { continue; @@ -131,7 +130,7 @@ public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletRe AuthenticationProvider providerToUse = getAuthProvider(authMethodName); try { return providerToUse.authenticateHttpRequest(request, response); - } catch (AuthenticationException e) { + } catch (Exception e) { if (LOG.isDebugEnabled()) { LOG.debug("Authentication failed for provider " + providerToUse.getAuthMethodName() + " : " + e.getMessage(), e); @@ -142,7 +141,7 @@ public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletRe for (AuthenticationProvider provider : providers.values()) { try { return provider.authenticateHttpRequest(request, response); - } catch (AuthenticationException e) { + } catch (Exception e) { if (LOG.isDebugEnabled()) { LOG.debug("Authentication failed for provider " + provider.getAuthMethodName() + ": " + e.getMessage(), e); @@ -183,25 +182,18 @@ public String authenticateHttpRequest(HttpServletRequest request, Authentication } // Backward compatible, the authData value was null in the previous implementation return providerToUse.authenticateAsync(authData).get(); - } catch (AuthenticationException e) { + } catch (Exception e) { if (LOG.isDebugEnabled()) { LOG.debug("Authentication failed for provider " + providerToUse.getAuthMethodName() + " : " + e.getMessage(), e); } - throw e; - } catch (ExecutionException | InterruptedException e) { - if (LOG.isDebugEnabled()) { - LOG.debug("Authentication failed for provider " + providerToUse.getAuthMethodName() + " : " - + e.getMessage(), e); - } - throw new RuntimeException(e); } } else { for (AuthenticationProvider provider : providers.values()) { try { AuthenticationState authenticationState = provider.newHttpAuthState(request); return provider.authenticateAsync(authenticationState.getAuthDataSource()).get(); - } catch (ExecutionException | InterruptedException | AuthenticationException e) { + } catch (Exception e) { if (LOG.isDebugEnabled()) { LOG.debug("Authentication failed for provider " + provider.getAuthMethodName() + ": " + e.getMessage(), e); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthenticationServiceTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthenticationServiceTest.java index 78ae046b0c8c8..0d5fc4e19d5b3 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthenticationServiceTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthenticationServiceTest.java @@ -20,6 +20,8 @@ import static org.apache.pulsar.broker.web.AuthenticationFilter.AuthenticatedDataAttributeName; import static org.apache.pulsar.broker.web.AuthenticationFilter.AuthenticatedRoleAttributeName; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -29,15 +31,22 @@ import static org.testng.Assert.assertTrue; import com.google.common.collect.Sets; import java.io.IOException; +import java.util.LinkedHashSet; import java.util.Set; +import java.util.concurrent.CompletableFuture; import javax.naming.AuthenticationException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import lombok.Cleanup; import org.apache.pulsar.broker.ServiceConfiguration; +import org.apache.pulsar.broker.authentication.AuthenticationDataCommand; import org.apache.pulsar.broker.authentication.AuthenticationDataHttps; import org.apache.pulsar.broker.authentication.AuthenticationDataSource; import org.apache.pulsar.broker.authentication.AuthenticationProvider; import org.apache.pulsar.broker.authentication.AuthenticationService; +import org.apache.pulsar.broker.authentication.AuthenticationState; +import org.apache.pulsar.broker.web.AuthenticationFilter; +import org.apache.pulsar.common.api.AuthData; import org.testng.Assert; import org.testng.annotations.Test; @@ -166,6 +175,123 @@ public void testAuthenticationHttpRequestResponseWithAnonymousRole() throws Exce service.close(); } + @Test + public void testHttpRequestWithMultipleProviders() throws Exception { + ServiceConfiguration config = new ServiceConfiguration(); + Set providersClassNames = new LinkedHashSet<>(); + providersClassNames.add(MockAuthenticationProviderAlwaysFail.class.getName()); + providersClassNames.add(MockHttpAuthenticationProvider.class.getName()); + config.setAuthenticationProviders(providersClassNames); + config.setAuthenticationEnabled(true); + @Cleanup + AuthenticationService service = new AuthenticationService(config); + + HttpServletRequest request = mock(HttpServletRequest.class); + + when(request.getParameter("role")).thenReturn("success-role1"); + assertTrue(service.authenticateHttpRequest(request, (HttpServletResponse) null)); + + when(request.getParameter("role")).thenReturn(""); + assertThatThrownBy(() -> service.authenticateHttpRequest(request, (HttpServletResponse) null)) + .isInstanceOf(AuthenticationException.class); + + when(request.getParameter("role")).thenReturn("error-role1"); + assertThatThrownBy(() -> service.authenticateHttpRequest(request, (HttpServletResponse) null)) + .isInstanceOf(AuthenticationException.class); + + when(request.getHeader(AuthenticationFilter.PULSAR_AUTH_METHOD_NAME)).thenReturn("http-auth"); + assertThatThrownBy(() -> service.authenticateHttpRequest(request, (HttpServletResponse) null)) + .isInstanceOf(RuntimeException.class); + + HttpServletRequest requestForAuthenticationDataSource = mock(HttpServletRequest.class); + assertThatThrownBy(() -> service.authenticateHttpRequest(requestForAuthenticationDataSource, + (AuthenticationDataSource) null)) + .isInstanceOf(AuthenticationException.class); + + when(requestForAuthenticationDataSource.getParameter("role")).thenReturn("error-role2"); + assertThatThrownBy(() -> service.authenticateHttpRequest(requestForAuthenticationDataSource, + (AuthenticationDataSource) null)) + .isInstanceOf(AuthenticationException.class); + + when(requestForAuthenticationDataSource.getParameter("role")).thenReturn("success-role2"); + assertThat(service.authenticateHttpRequest(requestForAuthenticationDataSource, + (AuthenticationDataSource) null)).isEqualTo("role2"); + } + + public static class MockHttpAuthenticationProvider implements AuthenticationProvider { + @Override + public void close() throws IOException { + } + + @Override + public void initialize(ServiceConfiguration config) throws IOException { + } + + @Override + public String getAuthMethodName() { + return "http-auth"; + } + + private String getRole(HttpServletRequest request) { + String role = request.getParameter("role"); + if (role != null) { + String[] s = role.split("-"); + if (s.length == 2 && s[0].equals("success")) { + return s[1]; + } + } + return null; + } + + @Override + public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletResponse response) { + String role = getRole(request); + if (role != null) { + return true; + } + throw new RuntimeException("test authentication failed"); + } + + @Override + public String authenticate(AuthenticationDataSource authData) throws AuthenticationException { + return authData.getCommandData(); + } + + @Override + public AuthenticationState newHttpAuthState(HttpServletRequest request) throws AuthenticationException { + String role = getRole(request); + if (role != null) { + return new AuthenticationState() { + @Override + public String getAuthRole() throws AuthenticationException { + return role; + } + + @Override + public AuthData authenticate(AuthData authData) throws AuthenticationException { + return null; + } + + @Override + public AuthenticationDataSource getAuthDataSource() { + return new AuthenticationDataCommand(role); + } + + @Override + public boolean isComplete() { + return true; + } + + @Override + public CompletableFuture authenticateAsync(AuthData authData) { + return AuthenticationState.super.authenticateAsync(authData); + } + }; + } + throw new RuntimeException("new http auth failed"); + } + } + public static class MockAuthenticationProvider implements AuthenticationProvider { @Override