fix(error): Test various access denied scenarios and improve readabil…

…ity (#475)
spinnaker · Sep 26, 2019 · 623dae5 · 623dae5
1 parent 1b5a3d2
commit 623dae5
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 5 deletions.
diff --git a/...api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java b/...api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandler.java
@@ -46,10 +46,14 @@ public void handleAccessDeniedException(
 
   private String authorizationFailureMessage(
       FiatPermissionEvaluator.AuthorizationFailure authorizationFailure) {
+    // Make the resource type readable (ie, "service account" instead of "serviceaccount")
+    String resourceType =
+        authorizationFailure.getResourceType().toString().replace("_", " ").toLowerCase();
+
     StringJoiner sj =
         new StringJoiner(" ")
             .add("Access denied to")
-            .add(authorizationFailure.getResourceType().modelClass.getSimpleName().toLowerCase())
+            .add(resourceType)
             .add(authorizationFailure.getResourceName());
 
     if (authorizationFailure.hasAuthorization()) {

diff --git a/...test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy b/...test/groovy/com/netflix/spinnaker/fiat/shared/FiatAccessDeniedExceptionHandlerSpec.groovy
@@ -19,9 +19,12 @@ 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 com.netflix.spinnaker.fiat.model.resources.ServiceAccount
 import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken
+import spock.lang.Shared
 import spock.lang.Subject
 import org.springframework.security.access.AccessDeniedException
+import spock.lang.Unroll
 
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
@@ -33,6 +36,9 @@ class FiatAccessDeniedExceptionHandlerSpec extends FiatSharedSpecification {
     def request = Mock(HttpServletRequest)
     def response = Mock(HttpServletResponse)
 
+    @Shared
+    def authentication = new PreAuthenticatedAuthenticationToken("testUser", null, [])
+
     FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator(
             registry,
             fiatService,
@@ -41,23 +47,48 @@ class FiatAccessDeniedExceptionHandlerSpec extends FiatSharedSpecification {
             FiatPermissionEvaluator.RetryHandler.NOOP
     )
 
-    def "when access denied exception is handled, a descriptive error message is sent in response"() {
+    @Unroll
+    def "when access denied exception is handled accessing application, an appropriate 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([userAuthroizationType] as Set)] as Set)
+        fiatService.getUserPermission("testUser") >> upv
+
+        when:
+        evaluator.hasPermission(authentication, resource, 'APPLICATION', authorizationTypeRequired)
+
+        and:
+        fiatAccessDeniedExceptionHandler.handleAccessDeniedException(new AccessDeniedException("Forbidden"), response, request)
+
+        then:
+        1 * response.sendError(403, "Access denied to application service - required authorization: " + authorizationTypeRequired)
 
+        where:
+        userAuthroizationType | authorizationTypeRequired
+        Authorization.READ    | "WRITE"
+        Authorization.WRITE   | "READ"
+    }
+
+    def "when access denied exception is handled accessing a service account, an appropriate error message is sent in response"() {
+        setup:
+        String resource = "readable"
+        String svcAcct = "svcAcct"
         UserPermission.View upv = new UserPermission.View()
         upv.setApplications([new Application.View().setName(resource)
                                      .setAuthorizations([Authorization.READ] as Set)] as Set)
+        upv.setServiceAccounts([new ServiceAccount.View().setName(svcAcct)
+                                        .setMemberOf(["foo"])] as Set)
         fiatService.getUserPermission("testUser") >> upv
 
         when:
-        evaluator.hasPermission(authentication, resource, 'APPLICATION', 'WRITE')
+        evaluator.hasPermission(authentication, resource, 'SERVICE_ACCOUNT', 'WRITE')
 
         and:
         fiatAccessDeniedExceptionHandler.handleAccessDeniedException(new AccessDeniedException("Forbidden"), response, request)
 
         then:
-        1 * response.sendError(403, "Access denied to application service - required authorization: WRITE")
+        1 * response.sendError(403, "Access denied to service account readable")
     }
 }