From d28b81f27f512338ca0cc24c7874cf6fd203564d Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 25 Jun 2018 14:46:38 +0200 Subject: [PATCH] Improve exception handling in session managers. We now log the HTTP status text along the HTTP status itself and a potential error message. Client errors (HTTP status 4xx) are logged on WARN level as feedback to the application as client errors aren't considered fatal. We also introduce VaultSessionManagerException and VaultTokenRenewalException to provide exceptions with a specific context. Closes gh-257. Related ticket: gh-203. --- .../LifecycleAwareSessionManager.java | 21 +++++---- .../ReactiveLifecycleAwareSessionManager.java | 42 +++++++++-------- .../VaultSessionManagerException.java | 47 +++++++++++++++++++ .../VaultTokenRenewalException.java | 45 ++++++++++++++++++ .../vault/client/VaultResponses.java | 23 +++++++-- ...LifecycleAwareSessionManagerUnitTests.java | 18 +++++++ ...LifecycleAwareSessionManagerUnitTests.java | 24 ++++++++++ 7 files changed, 187 insertions(+), 33 deletions(-) create mode 100644 spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultSessionManagerException.java create mode 100644 spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultTokenRenewalException.java diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/LifecycleAwareSessionManager.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/LifecycleAwareSessionManager.java index 220d45c1..8c812c7e 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/authentication/LifecycleAwareSessionManager.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/LifecycleAwareSessionManager.java @@ -27,7 +27,6 @@ import org.springframework.http.HttpEntity; import org.springframework.scheduling.TaskScheduler; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; -import org.springframework.vault.VaultException; import org.springframework.vault.client.VaultHttpHeaders; import org.springframework.vault.client.VaultResponses; import org.springframework.vault.support.VaultResponse; @@ -154,8 +153,7 @@ public class LifecycleAwareSessionManager extends LifecycleAwareSessionManagerSu VaultHttpHeaders.from(token)), Map.class); } catch (HttpStatusCodeException e) { - logger.warn(String.format("Cannot revoke VaultToken: %s", - VaultResponses.getError(e.getResponseBodyAsString()))); + logger.warn(format("Cannot revoke VaultToken", e)); } catch (RuntimeException e) { logger.warn("Cannot revoke VaultToken: %s", e); @@ -215,17 +213,18 @@ public class LifecycleAwareSessionManager extends LifecycleAwareSessionManagerSu } catch (HttpStatusCodeException e) { - logger.debug(String.format( - "Cannot renew token, resetting token and performing re-login: %s", - VaultResponses.getError(e.getResponseBodyAsString()))); this.token = Optional.empty(); + String message = "Cannot renew token, resetting token and performing re-login"; + if (e.getStatusCode().is4xxClientError()) { + logger.warn(format(message, e)); return false; } - throw new VaultException(String.format("Cannot renew token: %s", - VaultResponses.getError(e.getResponseBodyAsString()))); + logger.debug(format(message, e)); + + throw new VaultTokenRenewalException(format("Cannot renew token", e), e); } catch (RuntimeException e) { @@ -234,7 +233,7 @@ public class LifecycleAwareSessionManager extends LifecycleAwareSessionManagerSu e.toString())); this.token = Optional.empty(); - throw new VaultException("Cannot renew token", e); + throw new VaultTokenRenewalException("Cannot renew token", e); } } @@ -329,6 +328,10 @@ getRefreshTrigger().nextExecutionTime( (LoginToken) tokenWrapper.getToken())); } + private static String format(String message, HttpStatusCodeException e) { + return String.format("%s: Status %s %s %s", message, e.getStatusCode(), + e.getStatusText(), VaultResponses.getError(e.getResponseBodyAsString())); + } /** * Wraps a {@link VaultToken} and specifies whether the token is revocable on factory diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManager.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManager.java index 2135c7dd..f1cb8537 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManager.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManager.java @@ -34,7 +34,6 @@ import org.springframework.vault.client.VaultResponses; import org.springframework.vault.support.VaultResponse; import org.springframework.vault.support.VaultToken; import org.springframework.web.reactive.function.client.WebClient; -import org.springframework.web.reactive.function.client.WebClientException; import org.springframework.web.reactive.function.client.WebClientResponseException; /** @@ -171,8 +170,13 @@ public class ReactiveLifecycleAwareSessionManager extends return webClient.post().uri("auth/token/revoke-self").headers(httpHeaders -> { httpHeaders.addAll(VaultHttpHeaders.from(token)); - }).retrieve().bodyToMono(String.class) - .onErrorResume(WebClientException.class, e -> { + }).retrieve().bodyToMono(String.class).then() + .onErrorResume(WebClientResponseException.class, e -> { + + logger.warn(format("Could not revoke token", e)); + + return Mono.empty(); + }).onErrorResume(Exception.class, e -> { logger.warn("Could not revoke token", e); @@ -213,25 +217,21 @@ public class ReactiveLifecycleAwareSessionManager extends dropCurrentToken(); + String message = "Cannot renew token, resetting token and performing re-login on next token access"; + if (e.getStatusCode().is4xxClientError()) { - logger.debug(String - .format("Cannot renew token, resetting token and performing re-login on next token access: %s", - VaultResponses.getError(e - .getResponseBodyAsString()))); - + logger.warn(format(message, e)); return EMPTY; } - logger.debug(String - .format("Cannot renew token, resetting token and performing re-login on next token access: %s", - e.toString())); + logger.debug(format(message, e)); - return Mono.error(new VaultException(String.format( - "Cannot renew token: %s", - VaultResponses.getError(e.getResponseBodyAsString())))); + return Mono.error(new VaultTokenRenewalException(format( + "Cannot renew token", e), e)); }) .onErrorMap( + it -> !VaultTokenRenewalException.class.isInstance(it), e -> { dropCurrentToken(); @@ -239,11 +239,11 @@ public class ReactiveLifecycleAwareSessionManager extends .format("Cannot renew token, resetting token and performing re-login on next token access: %s", e.toString())); - return new VaultException("Cannot renew token", e); + return new VaultTokenRenewalException("Cannot renew token", e); }).map(TokenWrapper::getToken); } - private Mono doRenew(TokenWrapper tokenWrapper) { + Mono doRenew(TokenWrapper tokenWrapper) { Mono exchange = webClient .post() @@ -420,12 +420,16 @@ public class ReactiveLifecycleAwareSessionManager extends .onErrorMap( WebClientResponseException.class, e -> { - return new VaultTokenLookupException(String.format( - "Token self-lookup failed: %s %s", e.getStatusCode(), - VaultResponses.getError(e.getResponseBodyAsString()))); + return new VaultTokenLookupException(format( + "Token self-lookup", e), e); }); } + private static String format(String message, WebClientResponseException e) { + return String.format("%s: Status %s %s %s", message, e.getStatusCode(), + e.getStatusText(), VaultResponses.getError(e.getResponseBodyAsString())); + } + /** * Wraps a {@link VaultToken} and specifies whether the token is revocable on factory * shutdown. diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultSessionManagerException.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultSessionManagerException.java new file mode 100644 index 00000000..1998616e --- /dev/null +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultSessionManagerException.java @@ -0,0 +1,47 @@ +/* + * Copyright 2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.vault.authentication; + +import org.springframework.vault.VaultException; + +/** + * Abstract superclass for all exceptions thrown in the session manager implementations + * + * @author Mark Paluch + * @since 2.1 + */ +public abstract class VaultSessionManagerException extends VaultException { + + /** + * Create a {@code VaultSessionManagerException} with the specified detail message. + * + * @param msg the detail message. + */ + public VaultSessionManagerException(String msg) { + super(msg); + } + + /** + * Create a {@code VaultSessionManagerException} with the specified detail message and + * nested exception. + * + * @param msg the detail message. + * @param cause the nested exception. + */ + public VaultSessionManagerException(String msg, Throwable cause) { + super(msg, cause); + } +} diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultTokenRenewalException.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultTokenRenewalException.java new file mode 100644 index 00000000..9586a93c --- /dev/null +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/VaultTokenRenewalException.java @@ -0,0 +1,45 @@ +/* + * Copyright 2018 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.vault.authentication; + +/** + * Exception thrown when a Vault token renewal fails. + * + * @author Mark Paluch + * @since 2.1 + */ +public class VaultTokenRenewalException extends VaultSessionManagerException { + + /** + * Create a {@code VaultTokenRenewalException} with the specified detail message. + * + * @param msg the detail message. + */ + public VaultTokenRenewalException(String msg) { + super(msg); + } + + /** + * Create a {@code VaultTokenRenewalException} with the specified detail message and + * nested exception. + * + * @param msg the detail message. + * @param cause the nested exception. + */ + public VaultTokenRenewalException(String msg, Throwable cause) { + super(msg, cause); + } +} diff --git a/spring-vault-core/src/main/java/org/springframework/vault/client/VaultResponses.java b/spring-vault-core/src/main/java/org/springframework/vault/client/VaultResponses.java index 354663e0..eb8640a7 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/client/VaultResponses.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/client/VaultResponses.java @@ -60,25 +60,26 @@ public abstract class VaultResponses { String message = VaultResponses.getError(e.getResponseBodyAsString()); if (StringUtils.hasText(message)) { - return new VaultException(String.format("Status %s: %s", e.getStatusCode(), - message)); + return new VaultException(String.format("Status %s %s: %s", + e.getStatusCode(), e.getStatusText(), message)); } - return new VaultException(String.format("Status %s", e.getStatusCode())); + return new VaultException(String.format("Status %s %s", e.getStatusCode(), + e.getStatusText())); } /** * Build a {@link VaultException} given {@link HttpStatusCodeException} and request * {@code path}. * @param e must not be {@literal null}. - * @param path + * @param path must not be {@literal null}. * @return the {@link VaultException}. */ public static VaultException buildException(HttpStatusCodeException e, String path) { Assert.notNull(e, "HttpStatusCodeException must not be null"); - return buildException(e.getStatusCode(), path, + return buildException(e.getStatusCode(), e.getStatusText(), path, VaultResponses.getError(e.getResponseBodyAsString())); } @@ -93,6 +94,18 @@ public abstract class VaultResponses { return new VaultException(String.format("Status %s %s", statusCode, path)); } + private static VaultException buildException(HttpStatus statusCode, + String statusText, String path, String message) { + + if (StringUtils.hasText(message)) { + return new VaultException(String.format("Status %s %s %s: %s", statusCode, + statusText, path, message)); + } + + return new VaultException(String.format("Status %s %s %s", statusCode, + statusText, path)); + } + /** * Create a {@link ParameterizedTypeReference} for {@code responseType}. * @param responseType must not be {@literal null}. diff --git a/spring-vault-core/src/test/java/org/springframework/vault/authentication/LifecycleAwareSessionManagerUnitTests.java b/spring-vault-core/src/test/java/org/springframework/vault/authentication/LifecycleAwareSessionManagerUnitTests.java index 746b14cc..513e9a39 100644 --- a/spring-vault-core/src/test/java/org/springframework/vault/authentication/LifecycleAwareSessionManagerUnitTests.java +++ b/spring-vault-core/src/test/java/org/springframework/vault/authentication/LifecycleAwareSessionManagerUnitTests.java @@ -43,6 +43,7 @@ import org.springframework.web.client.ResourceAccessException; import org.springframework.web.client.RestOperations; 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.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -124,6 +125,23 @@ public class LifecycleAwareSessionManagerUnitTests { assertThat(sessionToken).isExactlyInstanceOf(VaultToken.class); } + @Test + public void shouldTranslateExceptionOnTokenRenewal() { + + when(clientAuthentication.login()).thenReturn( + LoginToken.renewable("login".toCharArray(), Duration.ofMinutes(5))); + when(restOperations.postForObject(anyString(), any(HttpEntity.class), any())) + .thenThrow( + new HttpServerErrorException(HttpStatus.INTERNAL_SERVER_ERROR, + "Some server error")); + + sessionManager.getSessionToken(); + assertThatThrownBy(() -> sessionManager.renewToken()) + .isInstanceOf(VaultTokenRenewalException.class) + .hasCauseInstanceOf(HttpServerErrorException.class) + .hasMessageContaining("Cannot renew token: Status 500 Some server error"); + } + @Test public void shouldRevokeLoginTokenOnDestroy() { diff --git a/spring-vault-core/src/test/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManagerUnitTests.java b/spring-vault-core/src/test/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManagerUnitTests.java index 4864c503..4cf28b29 100644 --- a/spring-vault-core/src/test/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManagerUnitTests.java +++ b/spring-vault-core/src/test/java/org/springframework/vault/authentication/ReactiveLifecycleAwareSessionManagerUnitTests.java @@ -154,6 +154,30 @@ public class ReactiveLifecycleAwareSessionManagerUnitTests { }).verifyComplete(); } + @Test + public void tokenRenewalShouldMapException() { + + mockToken(LoginToken.renewable("foo".toCharArray(), Duration.ofMinutes(1))); + + when(responseSpec.bodyToMono((Class) any())).thenReturn( + Mono.error(new WebClientResponseException("Some server error", 500, + "Some server error", null, null, null))); + + sessionManager.getVaultToken().as(StepVerifier::create).expectNextCount(1) + .verifyComplete(); + sessionManager + .renewToken() + .as(StepVerifier::create) + .consumeErrorWith( + exception -> { + assertThat(exception) + .isInstanceOf(VaultTokenRenewalException.class) + .hasCauseInstanceOf(WebClientResponseException.class) + .hasMessageContaining( + "Cannot renew token: Status 500 Some server error"); + }).verify(); + } + @Test public void shouldRevokeLoginTokenOnDestroy() {