diff --git a/build.gradle b/build.gradle index aa4321d86..17abf16c5 100644 --- a/build.gradle +++ b/build.gradle @@ -36,7 +36,7 @@ allprojects { apply plugin: 'groovy' ext { - spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.154.3' + spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.155.1' } def checkLocalVersions = [spinnakerDependenciesVersion: spinnakerDependenciesVersion] diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index 7be288644..405897da0 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -27,6 +27,7 @@ dependencies { compileOnly spinnaker.dependency("okHttpApache") compileOnly spinnaker.dependency("retrofit") compileOnly spinnaker.dependency("retrofitJackson") + compileOnly spinnaker.dependency("spectatorApi") compile spinnaker.dependency("guava") compile spinnaker.dependency("springSecurityConfig") @@ -39,4 +40,5 @@ dependencies { testCompile spinnaker.dependency("frigga") testCompile spinnaker.dependency("korkSecurity") testCompile spinnaker.dependency("bootAutoConfigure") + testCompile spinnaker.dependency("spectatorApi") } 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 1db56115a..8b4eb10a0 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 @@ -1,3 +1,4 @@ + /* * Copyright 2016 Google, Inc. * @@ -19,29 +20,25 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.util.concurrent.UncheckedExecutionException; -import com.netflix.frigga.Names; +import com.netflix.spectator.api.Id; +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.Authorizable; import com.netflix.spinnaker.fiat.model.resources.ResourceType; import com.netflix.spinnaker.security.AuthenticatedRequest; import com.netflix.spinnaker.security.User; -import lombok.Setter; import lombok.extern.slf4j.Slf4j; import lombok.val; import org.apache.commons.lang3.StringUtils; -import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.security.access.PermissionEvaluator; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; -import retrofit.RetrofitError; import java.io.Serializable; import java.util.Set; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -49,30 +46,32 @@ @Component @Slf4j -public class FiatPermissionEvaluator implements PermissionEvaluator, InitializingBean { - - @Autowired - @Setter - private FiatService fiatService; +public class FiatPermissionEvaluator implements PermissionEvaluator { - @Autowired - @Setter - private FiatClientConfigurationProperties configProps; + private final Registry registry; + private final FiatService fiatService; + private final FiatClientConfigurationProperties configProps; - @Value("${services.fiat.enabled:false}") - @Setter - private String fiatEnabled; + private final Cache permissionsCache; - private Cache permissionsCache; + private final Id getPermissionCounterId; - @Override - public void afterPropertiesSet() throws Exception { - permissionsCache = CacheBuilder + @Autowired + public FiatPermissionEvaluator(Registry registry, + FiatService fiatService, + FiatClientConfigurationProperties configProps) { + this.registry = registry; + this.fiatService = fiatService; + this.configProps = configProps; + + this.permissionsCache = CacheBuilder .newBuilder() .maximumSize(configProps.getCache().getMaxEntries()) .expireAfterWrite(configProps.getCache().getExpiresAfterWriteSeconds(), TimeUnit.SECONDS) .recordStats() .build(); + + this.getPermissionCounterId = registry.createId("fiat.getPermission"); } @Override @@ -87,12 +86,12 @@ public boolean hasPermission(Authentication authentication, Serializable resourceName, String resourceType, Object authorization) { - if (!Boolean.valueOf(fiatEnabled)) { + if (!configProps.isEnabled()) { return true; } if (resourceName == null || resourceType == null || authorization == null) { log.debug("Permission denied due to null argument. resourceName={}, resourceType={}, " + - "authorization={}", resourceName, resourceType, authorization); + "authorization={}", resourceName, resourceType, authorization); return false; } @@ -104,7 +103,7 @@ public boolean hasPermission(Authentication authentication, } if (r == ResourceType.APPLICATION && StringUtils.isNotEmpty(resourceName.toString())) { - resourceName = resourceName.toString(); + resourceName = resourceName.toString(); } UserPermission.View permission = getPermission(getUsername(authentication)); @@ -124,61 +123,48 @@ private String getUsername(Authentication authentication) { return username; } - private boolean isAuthorized(String username, - ResourceType resourceType, - String resourceName, - Authorization a) { - try { - AuthenticatedRequest.propagate(() -> - fiatService.hasAuthorization(username, resourceType.toString(), resourceName, a.toString()) - ).call(); - } catch (Exception e) { - String message = String.format("Fiat authorization failed for user '%s' '%s'-ing '%s' resourceType named '%s'. Cause: %s", - username, - a, - resourceType, - resourceName, - e.getMessage()); - if (log.isDebugEnabled()) { - log.debug(message, e); - } else { - log.info(message); - } - return false; - } - return true; - } - public UserPermission.View getPermission(String username) { UserPermission.View view = null; if (StringUtils.isEmpty(username)) { return null; } + AtomicBoolean cacheHit = new AtomicBoolean(true); + boolean successfulLookup = true; + try { - AtomicBoolean cacheHit = new AtomicBoolean(true); view = permissionsCache.get(username, () -> { cacheHit.set(false); return AuthenticatedRequest.propagate(() -> fiatService.getUserPermission(username)).call(); }); - log.debug("Fiat permission cache hit: " + cacheHit.get()); } catch (ExecutionException | UncheckedExecutionException ee) { - String message = String.format("Cannot get whole user permission for user %s. Cause: %s", - username, - ee.getCause().getMessage()); + successfulLookup = false; + + String message = String.format( + "Cannot get whole user permission for user %s. Cause: %s", + username, + ee.getCause().getMessage() + ); if (log.isDebugEnabled()) { log.debug(message, ee.getCause()); } else { log.info(message); } } + + registry.counter( + getPermissionCounterId + .withTag("cached", cacheHit.get()) + .withTag("success", successfulLookup) + ).increment(); + return view; } @SuppressWarnings("unused") @Deprecated public boolean storeWholePermission() { - if (!Boolean.valueOf(fiatEnabled)) { + if (!configProps.isEnabled()) { return true; } @@ -209,8 +195,8 @@ private boolean permissionContains(UserPermission.View permission, return containsAuth.apply(permission.getApplications()); case SERVICE_ACCOUNT: return permission.getServiceAccounts() - .stream() - .anyMatch(view -> view.getName().equalsIgnoreCase(resourceName)); + .stream() + .anyMatch(view -> view.getName().equalsIgnoreCase(resourceName)); default: return false; } 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 0bd18ddf7..bfa51f7ae 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,6 +16,8 @@ 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 @@ -25,27 +27,20 @@ import com.netflix.spinnaker.fiat.model.resources.Role import com.netflix.spinnaker.fiat.model.resources.ServiceAccount import org.springframework.security.core.Authentication 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(); - @Shared - FiatPermissionEvaluator evaluator - - @Shared - FiatService fiatService - - def setup() { - FiatClientConfigurationProperties configProps = new FiatClientConfigurationProperties() - configProps.cache.maxEntries = 0 - fiatService = Mock(FiatService) - evaluator = new FiatPermissionEvaluator(fiatEnabled: true, - fiatService: fiatService, - configProps: configProps) - evaluator.afterPropertiesSet() - } + @Subject + FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( + registry, + fiatService, + buildConfigurationProperties() + ) @Unroll def "should parse application name"() { @@ -143,4 +138,12 @@ class FiatPermissionEvaluatorSpec extends Specification { 1 * fiatService.getUserPermission("testUser") >> upv hasPermission } + + private static FiatClientConfigurationProperties buildConfigurationProperties() { + FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties(); + configurationProperties.enabled = true + configurationProperties.cache.maxEntries = 0 + + return configurationProperties + } } diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java index 2aa8081b7..b0bf6c7f7 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java @@ -28,6 +28,8 @@ public class FiatServerConfigurationProperties { */ private boolean getAllEnabled = false; + private boolean defaultToUnrestrictedUser = false; + private WriteMode writeMode = new WriteMode(); @Data diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java index cdaf221a1..f57081001 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java @@ -17,6 +17,7 @@ package com.netflix.spinnaker.fiat.controllers; import com.netflix.spinnaker.fiat.config.FiatServerConfigurationProperties; +import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig; import com.netflix.spinnaker.fiat.model.Authorization; import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.fiat.model.resources.Account; @@ -25,6 +26,7 @@ import com.netflix.spinnaker.fiat.model.resources.ServiceAccount; import com.netflix.spinnaker.fiat.model.resources.Role; import com.netflix.spinnaker.fiat.permissions.PermissionsRepository; +import com.netflix.spinnaker.security.AuthenticatedRequest; import io.swagger.annotations.ApiOperation; import lombok.extern.slf4j.Slf4j; import lombok.val; @@ -37,6 +39,7 @@ import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.util.HashSet; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -45,11 +48,16 @@ @RequestMapping("/authorize") public class AuthorizeController { - @Autowired - private PermissionsRepository permissionsRepository; + private final PermissionsRepository permissionsRepository; + private final FiatServerConfigurationProperties configProps; @Autowired - FiatServerConfigurationProperties configProps; + public AuthorizeController(PermissionsRepository permissionsRepository, + FiatServerConfigurationProperties configProps) { + this.permissionsRepository = permissionsRepository; + this.configProps = configProps; + } + @ApiOperation(value = "Used mostly for testing. Not really any real value to the rest of " + "the system. Disabled by default.") @@ -71,16 +79,15 @@ public Set getAll(HttpServletResponse response) throws IOEx @RequestMapping(value = "/{userId:.+}", method = RequestMethod.GET) public UserPermission.View getUserPermission(@PathVariable String userId) { - val user = ControllerSupport.convert(userId); - log.debug("UserPermission requested for " + user); - return permissionsRepository.get(user) + log.debug("UserPermission requested for " + userId); + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView(); } @RequestMapping(value = "/{userId:.+}/accounts", method = RequestMethod.GET) public Set getUserAccounts(@PathVariable String userId) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getAccounts() @@ -90,7 +97,7 @@ public Set getUserAccounts(@PathVariable String userId) { @RequestMapping(value = "/{userId:.+}/roles", method = RequestMethod.GET) public Set getUserRoles(@PathVariable String userId) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getRoles() @@ -100,7 +107,7 @@ public Set getUserRoles(@PathVariable String userId) { @RequestMapping(value = "/{userId:.+}/accounts/{accountName:.+}", method = RequestMethod.GET) public Account.View getUserAccount(@PathVariable String userId, @PathVariable String accountName) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getAccounts() @@ -112,7 +119,7 @@ public Account.View getUserAccount(@PathVariable String userId, @PathVariable St @RequestMapping(value = "/{userId:.+}/applications", method = RequestMethod.GET) public Set getUserApplications(@PathVariable String userId) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getApplications() @@ -122,7 +129,7 @@ public Set getUserApplications(@PathVariable String userId) { @RequestMapping(value = "/{userId:.+}/applications/{applicationName:.+}", method = RequestMethod.GET) public Application.View getUserApplication(@PathVariable String userId, @PathVariable String applicationName) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getApplications() @@ -134,7 +141,7 @@ public Application.View getUserApplication(@PathVariable String userId, @PathVar @RequestMapping(value = "/{userId:.+}/serviceAccounts", method = RequestMethod.GET) public Set getServiceAccounts(@PathVariable String userId) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getServiceAccounts() @@ -145,7 +152,7 @@ public Set getServiceAccounts(@PathVariable String userId) @RequestMapping(value = "/{userId:.+}/serviceAccounts/{serviceAccountName:.+}", method = RequestMethod.GET) public ServiceAccount.View getServiceAccount(@PathVariable String userId, @PathVariable String serviceAccountName) { - return permissionsRepository.get(ControllerSupport.convert(userId)) + return getUserPermissionOrDefault(userId) .orElseThrow(NotFoundException::new) .getView() .getServiceAccounts() @@ -191,4 +198,31 @@ public void getUserAuthorization(@PathVariable String userId, response.setStatus(HttpServletResponse.SC_NOT_FOUND); } + + private Optional getUserPermissionOrDefault(String userId) { + String authenticatedUserId = AuthenticatedRequest.getSpinnakerUser().orElse(null); + + UserPermission userPermission = permissionsRepository.get( + ControllerSupport.convert(userId) + ).orElse(null); + + if (userPermission != null || !configProps.isDefaultToUnrestrictedUser()) { + return Optional.ofNullable(userPermission); + } + + if (userId.equalsIgnoreCase(authenticatedUserId)) { + /* + * User does not have any stored permissions, likely a request that has not transited gate. + * + * If the request is for the permissions of the currently authenticated user, default to those of the + * unrestricted user. + */ + userPermission = permissionsRepository + .get(UnrestrictedResourceConfig.UNRESTRICTED_USERNAME) + .map(u -> u.setId(authenticatedUserId)) + .orElse(null); + } + + return Optional.ofNullable(userPermission); + } } diff --git a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy index fb10a5355..a82052d2b 100644 --- a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy +++ b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy @@ -27,12 +27,14 @@ import com.netflix.spinnaker.fiat.permissions.PermissionsRepository import com.netflix.spinnaker.fiat.providers.internal.ClouddriverService import com.netflix.spinnaker.fiat.providers.internal.Front50Service import com.netflix.spinnaker.kork.jedis.EmbeddedRedis +import org.slf4j.MDC import org.springframework.beans.factory.annotation.Autowired import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.setup.MockMvcBuilders import org.springframework.web.context.WebApplicationContext import spock.lang.AutoCleanup import spock.lang.Specification +import spock.lang.Unroll import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content @@ -144,7 +146,8 @@ class AuthorizeControllerSpec extends Specification { def "should get user from repo"() { setup: PermissionsRepository repository = Mock(PermissionsRepository) - AuthorizeController controller = new AuthorizeController(permissionsRepository: repository) + AuthorizeController controller = new AuthorizeController(repository, fiatServerConfigurationProperties) + def foo = new UserPermission().setId("foo@batman.com") when: @@ -165,7 +168,8 @@ class AuthorizeControllerSpec extends Specification { def "should get user's accounts from repo"() { setup: PermissionsRepository repository = Mock(PermissionsRepository) - AuthorizeController controller = new AuthorizeController(permissionsRepository: repository) + AuthorizeController controller = new AuthorizeController(repository, fiatServerConfigurationProperties) + def bar = new Account().setName("bar") def foo = new UserPermission().setId("foo").setAccounts([bar] as Set) @@ -234,4 +238,34 @@ class AuthorizeControllerSpec extends Specification { .andExpect(status().isOk()) .andExpect(content().json(expected)) } + + @Unroll + def "should fallback to unrestricted user if no session available"() { + given: + def authorizeController = new AuthorizeController( + permissionsRepository, + new FiatServerConfigurationProperties(defaultToUnrestrictedUser: defaultToUnrestrictedUser) + ) + permissionsRepository.put(unrestrictedUser) + + when: + Optional optionalUserPermission + + try { + MDC.put("X-SPINNAKER-USER", authenticatedUser) + optionalUserPermission = authorizeController.getUserPermissionOrDefault(targetUser) + } finally { + MDC.remove("X-SPINNAKER-USER") + } + + then: + optionalUserPermission.orElse(null) == (shouldReturnUnrestrictedUser ? unrestrictedUser.setId(targetUser) : null) + + where: + authenticatedUser | targetUser | defaultToUnrestrictedUser || shouldReturnUnrestrictedUser + "existing_user" | "user_does_not_exist" | false || false + "existing_user" | "user_does_not_exist" | true || false + "user_has_no_session" | "user_has_no_session" | false || false + "user_has_no_session" | "user_has_no_session" | true || true + } }