From 30100ea61aeefdaec7f8128f2428ed8d40c70941 Mon Sep 17 00:00:00 2001 From: david-leifker <114954101+david-leifker@users.noreply.github.com> Date: Mon, 1 May 2023 13:18:05 -0500 Subject: [PATCH] logging(auth): adding optional logging to authentication exceptions (#7929) (cherry picked from commit b71baac0b82af5b87c7f5c3c0931281eff3a6d2d) --- .../filter/AuthenticationFilter.java | 6 +++++- .../authenticator/AuthenticatorChain.java | 21 +++++++++++++------ .../authenticator/AuthenticatorChainTest.java | 6 +++--- .../src/main/resources/application.yml | 3 +++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java b/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java index 1e875aeb65b31b..e15918a8131580 100644 --- a/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java +++ b/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java @@ -44,6 +44,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Value; import org.springframework.web.context.support.SpringBeanAutowiringSupport; import static com.datahub.authentication.AuthenticationConstants.*; @@ -67,6 +68,9 @@ public class AuthenticationFilter implements Filter { @Named("dataHubTokenService") private StatefulTokenService _tokenService; + @Value("#{new Boolean('${authentication.logAuthenticatorExceptions}')}") + private boolean _logAuthenticatorExceptions; + private AuthenticatorChain authenticatorChain; @Override @@ -81,7 +85,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha AuthenticationRequest context = buildAuthContext((HttpServletRequest) request); Authentication authentication = null; try { - authentication = this.authenticatorChain.authenticate(context); + authentication = this.authenticatorChain.authenticate(context, _logAuthenticatorExceptions); } catch (AuthenticationException e) { // For AuthenticationExpiredExceptions, terminate and provide that feedback to the user log.debug("Failed to authenticate request. Received an AuthenticationExpiredException from authenticator chain.", diff --git a/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/AuthenticatorChain.java b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/AuthenticatorChain.java index 5d9c63d49c00d8..e72225e6ee9900 100644 --- a/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/AuthenticatorChain.java +++ b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/AuthenticatorChain.java @@ -12,6 +12,8 @@ import java.util.ArrayList; import java.util.List; import java.util.Objects; +import java.util.stream.Collectors; + import lombok.extern.slf4j.Slf4j; @@ -43,10 +45,10 @@ public void register(@Nonnull final Authenticator authenticator) { * Returns null if {@link Authentication} cannot be resolved for the incoming request. */ @Nullable - public Authentication authenticate(@Nonnull final AuthenticationRequest context) throws AuthenticationException { + public Authentication authenticate(@Nonnull final AuthenticationRequest context, boolean logExceptions) throws AuthenticationException { Objects.requireNonNull(context); ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); - List> authenticationFailures = new ArrayList<>(); + List> authenticationFailures = new ArrayList<>(); for (final Authenticator authenticator : this.authenticators) { try { log.debug(String.format("Executing Authenticator with class name %s", authenticator.getClass().getCanonicalName())); @@ -67,17 +69,24 @@ public Authentication authenticate(@Nonnull final AuthenticationRequest context) throw e; } catch (Exception e) { // Log as a normal error otherwise. - authenticationFailures.add(new Pair<>(authenticator.getClass().getCanonicalName(), e.getMessage())); log.debug(String.format( - "Caught exception while attempting to authenticate request using Authenticator %s", - authenticator.getClass().getCanonicalName()), e); + "Caught exception while attempting to authenticate request using Authenticator %s", + authenticator.getClass().getCanonicalName()), e); + authenticationFailures.add(new Pair<>(authenticator.getClass().getCanonicalName(), e)); } finally { Thread.currentThread().setContextClassLoader(contextClassLoader); } } // No authentication resolved. Return null. if (!authenticationFailures.isEmpty()) { - log.warn("Authentication chain failed to resolve a valid authentication. Errors: {}", authenticationFailures); + List> shortMessage = authenticationFailures.stream() + .peek(p -> { + if (logExceptions) { + log.error("Error during {} authentication: ", p.getFirst(), p.getSecond()); + } + }) + .map(p -> Pair.of(p.getFirst(), p.getSecond().getMessage())).collect(Collectors.toList()); + log.warn("Authentication chain failed to resolve a valid authentication. Errors: {}", shortMessage); } return null; } diff --git a/metadata-service/auth-impl/src/test/java/com/datahub/authentication/authenticator/AuthenticatorChainTest.java b/metadata-service/auth-impl/src/test/java/com/datahub/authentication/authenticator/AuthenticatorChainTest.java index df97fc6aae8e56..2e25493133b43d 100644 --- a/metadata-service/auth-impl/src/test/java/com/datahub/authentication/authenticator/AuthenticatorChainTest.java +++ b/metadata-service/auth-impl/src/test/java/com/datahub/authentication/authenticator/AuthenticatorChainTest.java @@ -31,7 +31,7 @@ public void testAuthenticateSuccess() throws Exception { // Verify that the mock authentication is returned on Authenticate. final AuthenticationRequest mockContext = Mockito.mock(AuthenticationRequest.class); - Authentication result = authenticatorChain.authenticate(mockContext); + Authentication result = authenticatorChain.authenticate(mockContext, false); // Verify that the authentication matches the mock returned by authenticator1 assertSame(result, mockAuthentication); @@ -53,7 +53,7 @@ public void testAuthenticateFailure() throws Exception { // Verify that the mock authentication is returned on Authenticate. final AuthenticationRequest mockContext = Mockito.mock(AuthenticationRequest.class); - Authentication result = authenticatorChain.authenticate(mockContext); + Authentication result = authenticatorChain.authenticate(mockContext, false); // If the authenticator throws, verify that null is returned to indicate failure to authenticate. assertNull(result); @@ -71,6 +71,6 @@ public void testAuthenticateThrows() throws Exception { // Verify that the mock authentication is returned on Authenticate. final AuthenticationRequest mockContext = Mockito.mock(AuthenticationRequest.class); - assertThrows(AuthenticationExpiredException.class, () -> authenticatorChain.authenticate(mockContext)); + assertThrows(AuthenticationExpiredException.class, () -> authenticatorChain.authenticate(mockContext, false)); } } diff --git a/metadata-service/factories/src/main/resources/application.yml b/metadata-service/factories/src/main/resources/application.yml index 735115a0d09e06..5e3915aac8658c 100644 --- a/metadata-service/factories/src/main/resources/application.yml +++ b/metadata-service/factories/src/main/resources/application.yml @@ -12,6 +12,9 @@ authentication: signingKey: ${DATAHUB_TOKEN_SERVICE_SIGNING_KEY:WnEdIeTG/VVCLQqGwC/BAkqyY0k+H8NEAtWGejrBI94=} salt: ${DATAHUB_TOKEN_SERVICE_SALT:ohDVbJBvHHVJh9S/UA4BYF9COuNnqqVhr9MLKEGXk1O=} + # Normally failures are only warnings, enable this to throw them. + logAuthenticatorExceptions: ${METADATA_SERVICE_AUTHENTICATOR_EXCEPTIONS_ENABLED:false} + # Required separately from the DataHubTokenAuthenticator as this is used by the AuthServiceController to authorize token generation # at user login time. systemClientId: ${DATAHUB_SYSTEM_CLIENT_ID:__datahub_system}