Skip to content

Commit

Permalink
feat(api): Adds shared cache of user permissions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cfieber
Travis Tomsu authored and cfieber committed Mar 29, 2017
1 parent f43f407 commit 15bfbbd
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 49 deletions.
1 change: 1 addition & 0 deletions fiat-api/fiat-api.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
11 changes: 11 additions & 0 deletions ...pi/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
}
86 changes: 52 additions & 34 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<String, UserPermission.View> 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,
Expand All @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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<Set<? extends Authorizable>, Boolean> containsAuth = resources ->
resources
Expand Down
6 changes: 3 additions & 3 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> roles);
Response loginWithRoles(@Path("userId") String userId, @Body Collection<String> roles);

/**
* @param userId The user being logged out
Expand Down
26 changes: 14 additions & 12 deletions ...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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,
Expand All @@ -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:
Expand Down

0 comments on commit 15bfbbd

Please sign in to comment.