From 1b5a3d2725a6b3129dc2dd7f4904053b0f8a67d3 Mon Sep 17 00:00:00 2001 From: Chris Smalley Date: Thu, 26 Sep 2019 12:11:19 -0700 Subject: [PATCH] fix(error): Improve access denied error messaging (#474) --- .../FiatAccessDeniedExceptionHandler.java | 35 +++++++++-- .../fiat/shared/FiatPermissionEvaluator.java | 7 +++ ...iatAccessDeniedExceptionHandlerSpec.groovy | 63 +++++++++++++++++++ .../shared/FiatPermissionEvaluatorSpec.groovy | 26 +++----- .../shared/FiatSharedSpecification.groovy | 37 +++++++++++ 5 files changed, 145 insertions(+), 23 deletions(-) create mode 100644 fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy create mode 100644 fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java index 173ec9714..5295cd484 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java @@ -17,8 +17,10 @@ package com.netflix.spinnaker.fiat.shared; import java.io.IOException; +import java.util.StringJoiner; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.springframework.boot.web.servlet.error.DefaultErrorAttributes; import org.springframework.http.HttpStatus; import org.springframework.security.access.AccessDeniedException; import org.springframework.web.bind.annotation.ControllerAdvice; @@ -26,20 +28,41 @@ @ControllerAdvice public class FiatAccessDeniedExceptionHandler { + + private final DefaultErrorAttributes defaultErrorAttributes = new DefaultErrorAttributes(); + @ExceptionHandler(AccessDeniedException.class) public void handleAccessDeniedException( AccessDeniedException e, HttpServletResponse response, HttpServletRequest request) throws IOException { + storeException(request, response, e); String errorMessage = FiatPermissionEvaluator.getAuthorizationFailure() - .map( - a -> - "Access denied to " - + a.getResourceType().modelClass.getSimpleName().toLowerCase() - + " " - + a.getResourceName()) + .map(this::authorizationFailureMessage) .orElse("Access is denied"); response.sendError(HttpStatus.FORBIDDEN.value(), errorMessage); } + + private String authorizationFailureMessage( + FiatPermissionEvaluator.AuthorizationFailure authorizationFailure) { + StringJoiner sj = + new StringJoiner(" ") + .add("Access denied to") + .add(authorizationFailure.getResourceType().modelClass.getSimpleName().toLowerCase()) + .add(authorizationFailure.getResourceName()); + + if (authorizationFailure.hasAuthorization()) { + sj.add("- required authorization:").add(authorizationFailure.getAuthorization().toString()); + } + + return sj.toString(); + } + + private void storeException( + HttpServletRequest request, HttpServletResponse response, Exception ex) { + // store exception as an attribute of HttpServletRequest such that it can be referenced by + // GenericErrorController + defaultErrorAttributes.resolveException(request, response, null, ex); + } } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java index 0621f9624..8dbcd6e05 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java @@ -38,6 +38,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.stream.Collectors; +import javax.annotation.Nonnull; import lombok.extern.slf4j.Slf4j; import lombok.val; import org.apache.commons.lang3.StringUtils; @@ -422,10 +423,16 @@ public Authorization getAuthorization() { return authorization; } + public boolean hasAuthorization() { + return authorization != null; + } + + @Nonnull public ResourceType getResourceType() { return resourceType; } + @Nonnull public String getResourceName() { return resourceName; } diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy new file mode 100644 index 000000000..5af199d30 --- /dev/null +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy @@ -0,0 +1,63 @@ +/* + * Copyright 2019 Netflix, Inc. + * + * 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 com.netflix.spinnaker.fiat.shared + +import com.netflix.spinnaker.fiat.model.Authorization +import com.netflix.spinnaker.fiat.model.UserPermission +import com.netflix.spinnaker.fiat.model.resources.Application +import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken +import spock.lang.Subject +import org.springframework.security.access.AccessDeniedException + +import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse + +class FiatAccessDeniedExceptionHandlerSpec extends FiatSharedSpecification { + @Subject + def fiatAccessDeniedExceptionHandler = new FiatAccessDeniedExceptionHandler() + + def request = Mock(HttpServletRequest) + def response = Mock(HttpServletResponse) + + FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( + registry, + fiatService, + buildConfigurationProperties(), + fiatStatus, + FiatPermissionEvaluator.RetryHandler.NOOP + ) + + def "when access denied exception is handled, a descriptive error message is sent in response"() { + setup: + def authentication = new PreAuthenticatedAuthenticationToken("testUser", null, []) + String resource = "service" + + UserPermission.View upv = new UserPermission.View() + upv.setApplications([new Application.View().setName(resource) + .setAuthorizations([Authorization.READ] as Set)] as Set) + fiatService.getUserPermission("testUser") >> upv + + when: + evaluator.hasPermission(authentication, resource, 'APPLICATION', 'WRITE') + + and: + fiatAccessDeniedExceptionHandler.handleAccessDeniedException(new AccessDeniedException("Forbidden"), response, request) + + then: + 1 * response.sendError(403, "Access denied to application service - required authorization: WRITE") + } +} diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy index cd2ecae3c..9a6f615dd 100644 --- a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy @@ -16,8 +16,6 @@ package com.netflix.spinnaker.fiat.shared -import com.netflix.spectator.api.NoopRegistry -import com.netflix.spectator.api.Registry import com.netflix.spinnaker.fiat.model.Authorization import com.netflix.spinnaker.fiat.model.UserPermission import com.netflix.spinnaker.fiat.model.resources.Application @@ -29,29 +27,23 @@ import com.netflix.spinnaker.security.AuthenticatedRequest import org.slf4j.MDC import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken import spock.lang.Shared -import spock.lang.Specification import spock.lang.Subject import spock.lang.Unroll -class FiatPermissionEvaluatorSpec extends Specification { - FiatService fiatService = Mock(FiatService) - Registry registry = new NoopRegistry() - FiatStatus fiatStatus = Mock(FiatStatus) { - _ * isEnabled() >> { return true } - } - - @Shared - def authentication = new PreAuthenticatedAuthenticationToken("testUser", null, []) +class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { @Subject FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( - registry, - fiatService, - buildConfigurationProperties(), - fiatStatus, - FiatPermissionEvaluator.RetryHandler.NOOP + registry, + fiatService, + buildConfigurationProperties(), + fiatStatus, + FiatPermissionEvaluator.RetryHandler.NOOP ) + @Shared + def authentication = new PreAuthenticatedAuthenticationToken("testUser", null, []) + def cleanup() { MDC.clear() } diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy new file mode 100644 index 000000000..68bbad021 --- /dev/null +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy @@ -0,0 +1,37 @@ +/* + * Copyright 2019 Netflix, Inc. + * + * 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 com.netflix.spinnaker.fiat.shared + +import com.netflix.spectator.api.NoopRegistry +import com.netflix.spectator.api.Registry +import spock.lang.Specification + +abstract class FiatSharedSpecification extends Specification { + FiatService fiatService = Mock(FiatService) + Registry registry = new NoopRegistry() + FiatStatus fiatStatus = Mock(FiatStatus) { + _ * isEnabled() >> { return true } + } + + private static FiatClientConfigurationProperties buildConfigurationProperties() { + FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties() + configurationProperties.enabled = true + configurationProperties.cache.maxEntries = 0 + configurationProperties.cache.expiresAfterWriteSeconds = 0 + return configurationProperties + } +}