fix(error): Improve access denied error messaging (#474)

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java

@@ -17,29 +17,52 @@
 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;
 import org.springframework.web.bind.annotation.ExceptionHandler;
 
 @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);
+  }
 }

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;
     }

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")
+    }
+}

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()
   }

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
+    }
+}