Skip to content

Commit bda60d9

Browse files
nodecesrinath-ctds
nodece
authored and
srinath-ctds
committed
[fix][broker] Continue using the next provider for authentication if one fails (apache#23797)
Signed-off-by: Zixuan Liu <nodeces@gmail.com> (cherry picked from commit 7619e2f) Signed-off-by: Zixuan Liu <nodeces@gmail.com> (cherry picked from commit 62170e7)
1 parent 89ce9a3 commit bda60d9

File tree

2 files changed

+154
-35
lines changed

2 files changed

+154
-35
lines changed

‎pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderList.java

Lines changed: 26 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public class AuthenticationProviderList implements AuthenticationProvider {
4040

4141
private interface AuthProcessor<T, W> {
4242

43-
T apply(W process) throws AuthenticationException;
43+
T apply(W process) throws Exception;
4444

4545
}
4646

@@ -49,20 +49,29 @@ private enum ErrorCode {
4949
AUTH_REQUIRED,
5050
}
5151

52+
private static AuthenticationException newAuthenticationException(String message, Exception e) {
53+
AuthenticationException authenticationException = new AuthenticationException(message);
54+
if (e != null) {
55+
authenticationException.initCause(e);
56+
}
57+
return authenticationException;
58+
}
59+
5260
static <T, W> T applyAuthProcessor(List<W> processors, AuthProcessor<T, W> authFunc)
5361
throws AuthenticationException {
54-
AuthenticationException authenticationException = null;
62+
Exception authenticationException = null;
5563
String errorCode = ErrorCode.UNKNOWN.name();
5664
for (W ap : processors) {
5765
try {
5866
return authFunc.apply(ap);
59-
} catch (AuthenticationException ae) {
67+
} catch (Exception ae) {
6068
if (log.isDebugEnabled()) {
6169
log.debug("Authentication failed for auth provider " + ap.getClass() + ": ", ae);
6270
}
63-
// Store the exception so we can throw it later instead of a generic one
6471
authenticationException = ae;
65-
errorCode = ap.getClass().getSimpleName() + "-INVALID-AUTH";
72+
if (ae instanceof AuthenticationException) {
73+
errorCode = ap.getClass().getSimpleName() + "-INVALID-AUTH";
74+
}
6675
}
6776
}
6877

@@ -75,7 +84,7 @@ static <T, W> T applyAuthProcessor(List<W> processors, AuthProcessor<T, W> authF
7584
AuthenticationMetrics.authenticateFailure(
7685
AuthenticationProviderList.class.getSimpleName(),
7786
"authentication-provider-list", errorCode);
78-
throw authenticationException;
87+
throw newAuthenticationException("Authentication failed", authenticationException);
7988
}
8089

8190
}
@@ -273,12 +282,12 @@ public AuthenticationState newAuthState(AuthData authData, SocketAddress remoteA
273282
throws AuthenticationException {
274283
final List<AuthenticationState> states = new ArrayList<>(providers.size());
275284

276-
AuthenticationException authenticationException = null;
285+
Exception authenticationException = null;
277286
for (AuthenticationProvider provider : providers) {
278287
try {
279288
AuthenticationState state = provider.newAuthState(authData, remoteAddress, sslSession);
280289
states.add(state);
281-
} catch (AuthenticationException ae) {
290+
} catch (Exception ae) {
282291
if (log.isDebugEnabled()) {
283292
log.debug("Authentication failed for auth provider " + provider.getClass() + ": ", ae);
284293
}
@@ -288,11 +297,8 @@ public AuthenticationState newAuthState(AuthData authData, SocketAddress remoteA
288297
}
289298
if (states.isEmpty()) {
290299
log.debug("Failed to initialize a new auth state from {}", remoteAddress, authenticationException);
291-
if (authenticationException != null) {
292-
throw authenticationException;
293-
} else {
294-
throw new AuthenticationException("Failed to initialize a new auth state from " + remoteAddress);
295-
}
300+
throw newAuthenticationException("Failed to initialize a new auth state from " + remoteAddress,
301+
authenticationException);
296302
} else {
297303
return new AuthenticationListState(states);
298304
}
@@ -302,12 +308,12 @@ public AuthenticationState newAuthState(AuthData authData, SocketAddress remoteA
302308
public AuthenticationState newHttpAuthState(HttpServletRequest request) throws AuthenticationException {
303309
final List<AuthenticationState> states = new ArrayList<>(providers.size());
304310

305-
AuthenticationException authenticationException = null;
311+
Exception authenticationException = null;
306312
for (AuthenticationProvider provider : providers) {
307313
try {
308314
AuthenticationState state = provider.newHttpAuthState(request);
309315
states.add(state);
310-
} catch (AuthenticationException ae) {
316+
} catch (Exception ae) {
311317
if (log.isDebugEnabled()) {
312318
log.debug("Authentication failed for auth provider " + provider.getClass() + ": ", ae);
313319
}
@@ -318,34 +324,20 @@ public AuthenticationState newHttpAuthState(HttpServletRequest request) throws A
318324
if (states.isEmpty()) {
319325
log.debug("Failed to initialize a new http auth state from {}",
320326
request.getRemoteHost(), authenticationException);
321-
if (authenticationException != null) {
322-
throw authenticationException;
323-
} else {
324-
throw new AuthenticationException(
325-
"Failed to initialize a new http auth state from " + request.getRemoteHost());
326-
}
327+
throw newAuthenticationException(
328+
"Failed to initialize a new http auth state from " + request.getRemoteHost(),
329+
authenticationException);
327330
} else {
328331
return new AuthenticationListState(states);
329332
}
330333
}
331334

332335
@Override
333336
public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletResponse response) throws Exception {
334-
Boolean authenticated = applyAuthProcessor(
337+
return applyAuthProcessor(
335338
providers,
336-
provider -> {
337-
try {
338-
return provider.authenticateHttpRequest(request, response);
339-
} catch (Exception e) {
340-
if (e instanceof AuthenticationException) {
341-
throw (AuthenticationException) e;
342-
} else {
343-
throw new AuthenticationException("Failed to authentication http request");
344-
}
345-
}
346-
}
339+
provider -> provider.authenticateHttpRequest(request, response)
347340
);
348-
return authenticated;
349341
}
350342

351343
@Override

‎pulsar-broker-common/src/test/java/org/apache/pulsar/broker/authentication/AuthenticationProviderListTest.java

Lines changed: 128 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import static java.nio.charset.StandardCharsets.UTF_8;
2222
import static org.apache.pulsar.broker.web.AuthenticationFilter.AuthenticatedDataAttributeName;
2323
import static org.apache.pulsar.broker.web.AuthenticationFilter.AuthenticatedRoleAttributeName;
24+
import static org.assertj.core.api.Assertions.assertThat;
25+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2426
import static org.mockito.ArgumentMatchers.eq;
2527
import static org.mockito.ArgumentMatchers.isA;
2628
import static org.mockito.Mockito.mock;
@@ -36,13 +38,17 @@
3638
import java.security.KeyPair;
3739
import java.security.PrivateKey;
3840
import java.util.Date;
41+
import java.util.List;
3942
import java.util.Optional;
4043
import java.util.Properties;
44+
import java.util.concurrent.CompletableFuture;
4145
import java.util.concurrent.TimeUnit;
46+
import javax.naming.AuthenticationException;
4247
import javax.servlet.http.HttpServletRequest;
4348
import org.apache.pulsar.broker.ServiceConfiguration;
4449
import org.apache.pulsar.broker.authentication.utils.AuthTokenUtils;
4550
import org.apache.pulsar.common.api.AuthData;
51+
import org.apache.pulsar.common.util.FutureUtil;
4652
import org.assertj.core.util.Lists;
4753
import org.testng.annotations.AfterMethod;
4854
import org.testng.annotations.BeforeMethod;
@@ -261,4 +267,125 @@ public void testAuthenticateHttpRequest() throws Exception {
261267
verify(requestBB).setAttribute(eq(AuthenticatedDataAttributeName), isA(AuthenticationDataSource.class));
262268
}
263269

264-
}
270+
@Test
271+
public void testAuthenticateWithMultipleProviders() throws Exception {
272+
HttpServletRequest httpRequest = mock(HttpServletRequest.class);
273+
AuthenticationDataSource authenticationDataSource = mock(AuthenticationDataSource.class);
274+
275+
AuthenticationProvider failingProvider = mock(AuthenticationProvider.class);
276+
List<AuthenticationProvider> providers = Lists.newArrayList(
277+
failingProvider
278+
);
279+
try (AuthenticationProvider provider = new AuthenticationProviderList(providers)) {
280+
provider.initialize(new ServiceConfiguration());
281+
RuntimeException authenticateException = new RuntimeException("authenticateException");
282+
283+
when(failingProvider.authenticateAsync(authenticationDataSource))
284+
.thenReturn(FutureUtil.failedFuture(authenticateException));
285+
when(failingProvider.authenticate(authenticationDataSource))
286+
.thenThrow(authenticateException);
287+
assertThat(provider.authenticateAsync(authenticationDataSource))
288+
.failsWithin(3, TimeUnit.SECONDS)
289+
.withThrowableThat().withCause(authenticateException);
290+
assertThatThrownBy(() -> provider.authenticate(authenticationDataSource))
291+
.isInstanceOf(AuthenticationException.class)
292+
.hasCause(authenticateException);
293+
294+
RuntimeException authenticateHttpRequestException = new RuntimeException("authenticateHttpRequestAsync");
295+
when(failingProvider.authenticateHttpRequestAsync(httpRequest, null))
296+
.thenReturn(FutureUtil.failedFuture(authenticateHttpRequestException));
297+
when(failingProvider.authenticateHttpRequest(httpRequest, null))
298+
.thenThrow(authenticateHttpRequestException);
299+
assertThat(provider.authenticateHttpRequestAsync(httpRequest, null))
300+
.failsWithin(3, TimeUnit.SECONDS)
301+
.withThrowableThat()
302+
.havingCause()
303+
.withCause(authenticateHttpRequestException);
304+
assertThatThrownBy(() -> provider.authenticateHttpRequest(httpRequest, null))
305+
.isInstanceOf(AuthenticationException.class)
306+
.hasCause(authenticateHttpRequestException);
307+
308+
RuntimeException newAuthStateException = new RuntimeException("newAuthState");
309+
when(failingProvider.newAuthState(null, null, null))
310+
.thenThrow(newAuthStateException);
311+
assertThatThrownBy(() -> provider.newAuthState(null, null, null))
312+
.isInstanceOf(AuthenticationException.class)
313+
.hasCause(newAuthStateException);
314+
315+
RuntimeException newHttpAuthStateException = new RuntimeException("newHttpAuthState");
316+
when(failingProvider.newHttpAuthState(httpRequest))
317+
.thenThrow(newHttpAuthStateException);
318+
assertThatThrownBy(() -> provider.newHttpAuthState(httpRequest))
319+
.isInstanceOf(AuthenticationException.class)
320+
.hasCause(newHttpAuthStateException);
321+
}
322+
323+
AuthenticationProvider successfulProvider = mock(AuthenticationProvider.class);
324+
providers.add(successfulProvider);
325+
String subject = "test-role";
326+
327+
try (AuthenticationProvider provider = new AuthenticationProviderList(providers)) {
328+
provider.initialize(new ServiceConfiguration());
329+
330+
when(successfulProvider.authenticateAsync(authenticationDataSource))
331+
.thenReturn(CompletableFuture.completedFuture(subject));
332+
when(successfulProvider.authenticate(authenticationDataSource))
333+
.thenReturn(subject);
334+
assertThat(provider.authenticateAsync(authenticationDataSource))
335+
.succeedsWithin(3, TimeUnit.SECONDS)
336+
.matches(subject::equals);
337+
assertThat(provider.authenticate(authenticationDataSource))
338+
.isEqualTo(subject);
339+
340+
when(successfulProvider.authenticateHttpRequestAsync(httpRequest, null))
341+
.thenReturn(CompletableFuture.completedFuture(true));
342+
when(successfulProvider.authenticateHttpRequest(httpRequest, null))
343+
.thenReturn(true);
344+
assertThat(provider.authenticateHttpRequestAsync(httpRequest, null))
345+
.succeedsWithin(3, TimeUnit.SECONDS)
346+
.isEqualTo(true);
347+
assertThat(provider.authenticateHttpRequest(httpRequest, null))
348+
.isEqualTo(true);
349+
350+
AuthenticationState authenticationState = new AuthenticationState() {
351+
@Override
352+
public String getAuthRole() {
353+
return subject;
354+
}
355+
356+
@Override
357+
public AuthData authenticate(AuthData authData) {
358+
return null;
359+
}
360+
361+
@Override
362+
public AuthenticationDataSource getAuthDataSource() {
363+
return null;
364+
}
365+
366+
@Override
367+
public boolean isComplete() {
368+
return false;
369+
}
370+
};
371+
when(successfulProvider.newAuthState(null, null, null))
372+
.thenReturn(authenticationState);
373+
when(successfulProvider.newHttpAuthState(httpRequest)).thenReturn(authenticationState);
374+
verifyAuthenticationStateSuccess(provider.newAuthState(null, null, null), true, subject);
375+
verifyAuthenticationStateSuccess(provider.newAuthState(null, null, null), false, subject);
376+
verifyAuthenticationStateSuccess(provider.newHttpAuthState(httpRequest), true, subject);
377+
verifyAuthenticationStateSuccess(provider.newHttpAuthState(httpRequest), false, subject);
378+
}
379+
}
380+
381+
private void verifyAuthenticationStateSuccess(AuthenticationState authState, boolean isAsync, String expectedRole)
382+
throws Exception {
383+
assertThat(authState).isNotNull();
384+
if (isAsync) {
385+
assertThat(authState.authenticateAsync(null)).succeedsWithin(3, TimeUnit.SECONDS);
386+
} else {
387+
assertThat(authState.authenticate(null)).isNull();
388+
}
389+
assertThat(authState.getAuthRole()).isEqualTo(expectedRole);
390+
}
391+
}

0 commit comments

Comments
 (0)