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 5295cd484..bbb8e785a 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 @@ -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/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 index 5af199d30..81f90be92 100644 --- 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 @@ -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") } }