diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index 252d96f8d..c3e39876c 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -27,6 +27,7 @@ dependencies { compileOnly spinnaker.dependency("retrofit") compileOnly spinnaker.dependency("retrofitJackson") + compile spinnaker.dependency("guava") compile spinnaker.dependency("springSecurityConfig") compile spinnaker.dependency("springSecurityCore") compile spinnaker.dependency("springSecurityWeb") diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java index 02bfba41b..aae3ebab2 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java @@ -18,6 +18,7 @@ import lombok.Data; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.boot.context.properties.NestedConfigurationProperty; import org.springframework.stereotype.Component; @Data @@ -27,4 +28,14 @@ public class FiatClientConfigurationProperties { private boolean enabled; private String baseUrl; + + @NestedConfigurationProperty + private PermissionsCache cache = new PermissionsCache(); + + @Data + class PermissionsCache { + private Integer maxEntries = 1000; + + private Integer expiresAfterWriteSeconds = 20; + } } 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 9e5a0e232..077b0775d 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 @@ -16,6 +16,8 @@ package com.netflix.spinnaker.fiat.shared; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; import com.netflix.frigga.Names; import com.netflix.spinnaker.fiat.model.Authorization; import com.netflix.spinnaker.fiat.model.UserPermission; @@ -26,6 +28,7 @@ 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; @@ -38,20 +41,38 @@ import java.io.Serializable; import java.util.Set; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; @Component @Slf4j -public class FiatPermissionEvaluator implements PermissionEvaluator { +public class FiatPermissionEvaluator implements PermissionEvaluator, InitializingBean { @Autowired @Setter private FiatService fiatService; + @Autowired + @Setter + private FiatClientConfigurationProperties configProps; + @Value("${services.fiat.enabled:false}") @Setter private String fiatEnabled; + private Cache permissionsCache; + + @Override + public void afterPropertiesSet() throws Exception { + permissionsCache = CacheBuilder.newBuilder() + .maximumSize(configProps.getCache().getMaxEntries()) + .expireAfterWrite(configProps.getCache().getExpiresAfterWriteSeconds(), TimeUnit.SECONDS) + .recordStats() + .build(); + } + @Override public boolean hasPermission(Authentication authentication, Object resource, @@ -73,8 +94,6 @@ public boolean hasPermission(Authentication authentication, return false; } - - String username = getUsername(authentication); ResourceType r = ResourceType.parse(resourceType); Authorization a = null; // Service accounts don't have read/write authorizations. @@ -89,9 +108,8 @@ public boolean hasPermission(Authentication authentication, } } - return isWholePermissionStored(authentication) ? - permissionContains(authentication, resourceName.toString(), r, a) : - isAuthorized(username, r, resourceName.toString(), a); + UserPermission.View permission = getPermission(getUsername(authentication)); + return permissionContains(permission, resourceName.toString(), r, a); } private String getUsername(Authentication authentication) { @@ -130,51 +148,51 @@ private boolean isAuthorized(String username, return true; } - @SuppressWarnings("unused") - public boolean storeWholePermission() { - if (!Boolean.valueOf(fiatEnabled)) { - return true; + public UserPermission.View getPermission(String username) { + UserPermission.View view = null; + if (StringUtils.isEmpty(username)) { + return null; } - String username = getUsername(SecurityContextHolder.getContext().getAuthentication()); - - UserPermission.View view; try { - view = fiatService.getUserPermission(username); - } catch (RetrofitError re) { + AtomicBoolean cacheHit = new AtomicBoolean(true); + view = permissionsCache.get(username, () -> { + cacheHit.set(false); + return fiatService.getUserPermission(username); + }); + log.debug("Fiat permission cache hit: " + cacheHit.get()); + } catch (ExecutionException ee) { String message = String.format("Cannot get whole user permission for user %s. Cause: %s", username, - re.getMessage()); + ee.getCause().getMessage()); if (log.isDebugEnabled()) { - log.debug(message, re); + log.debug(message, ee.getCause()); } else { log.info(message); } - return false; } - - PreAuthenticatedAuthenticationToken auth = new PreAuthenticatedAuthenticationToken(username, - null, - null); - auth.setDetails(view); - - SecurityContext ctx = SecurityContextHolder.createEmptyContext(); - ctx.setAuthentication(auth); - SecurityContextHolder.setContext(ctx); - - return true; + return view; } - private boolean isWholePermissionStored(Authentication authentication) { - return authentication.getDetails() != null && - authentication.getDetails() instanceof UserPermission.View; + @SuppressWarnings("unused") + @Deprecated + public boolean storeWholePermission() { + if (!Boolean.valueOf(fiatEnabled)) { + return true; + } + + val authentication = SecurityContextHolder.getContext().getAuthentication(); + val permission = getPermission(getUsername(authentication)); + return permission != null; } - private boolean permissionContains(Authentication authentication, + private boolean permissionContains(UserPermission.View permission, String resourceName, ResourceType resourceType, Authorization authorization) { - UserPermission.View permission = (UserPermission.View) authentication.getDetails(); + if (permission == null) { + return false; + } Function, Boolean> containsAuth = resources -> resources diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java index 07dc579c3..8defb3668 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java @@ -76,13 +76,13 @@ Response hasAuthorization(@Path("userId") String userId, /** - * Used specifically for SAML assertions that contain the users roles/groups. + * Used specifically for logins that contain the users roles/groups. * @param userId The user being logged in - * @param roles Optional collection of roles from the SAML provider + * @param roles Collection of roles from the identity provider * @return ignored. */ @PUT("/roles/{userId}") - Response loginSAMLUser(@Path("userId") String userId, @Body Collection roles); + Response loginWithRoles(@Path("userId") String userId, @Body Collection roles); /** * @param userId The user being logged out 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 640743999..7c5197334 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,11 +16,11 @@ package com.netflix.spinnaker.fiat.shared +import com.netflix.spinnaker.fiat.model.UserPermission +import com.netflix.spinnaker.fiat.model.resources.Application import com.netflix.spinnaker.fiat.model.resources.ResourceType import org.springframework.security.core.Authentication import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken -import retrofit.RetrofitError -import retrofit.client.Response import spock.lang.Specification import spock.lang.Unroll @@ -29,13 +29,17 @@ class FiatPermissionEvaluatorSpec extends Specification { @Unroll def "should parse application name"() { setup: + FiatClientConfigurationProperties configProps = new FiatClientConfigurationProperties() + configProps.cache.maxEntries = 0 FiatService fiatService = Mock(FiatService) FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator(fiatEnabled: true, - fiatService: fiatService); + fiatService: fiatService, + configProps: configProps) + evaluator.afterPropertiesSet() Authentication authentication = new PreAuthenticatedAuthenticationToken("testUser", null, - new ArrayList<>()); + new ArrayList<>()) when: def result = evaluator.hasPermission(authentication, @@ -44,23 +48,21 @@ class FiatPermissionEvaluatorSpec extends Specification { authorization) then: - 1 * fiatService.hasAuthorization("testUser", resourceType.name(), resourceName, authorization) >> { - throw RetrofitError.httpError( - "/", - new Response("/", 404, "not found", Collections.emptyList(), null), - null, - null) - } + 1 * fiatService.getUserPermission("testUser") >> new UserPermission.View(new UserPermission()) !result when: + result = evaluator.hasPermission(authentication, resource, resourceType.name(), authorization) then: - 1 * fiatService.hasAuthorization("testUser", resourceType.name(), resourceName, authorization) + 1 * fiatService.getUserPermission("testUser") >> new UserPermission.View( + new UserPermission( + applications: [new Application(name: "abc")] + )) result where: