Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dependencies): dependency cleanup #377

Merged
merged 2 commits into from
Apr 19, 2019
Merged

fix(dependencies): dependency cleanup #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aafba23
fix(dependencies): dependency cleanup
cfieber Apr 17, 2019
9592738
fix(api): cleanup FiatAuthenticationConfig WebSecurityConfigurerAdapter
cfieber Apr 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
*/

buildscript {
ext {
korkVersion = "4.1.0-rc.4-springBoot2"
}
repositories {
jcenter()
maven { url "https://spinnaker.bintray.com/gradle" }
Expand Down Expand Up @@ -47,15 +50,22 @@ allprojects {
jvmArgs '-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=7103'
}
}

//workaround nebula oss default 1.7 opinion..
plugins.withType(JavaBasePlugin) {
JavaPluginConvention convention = project.convention.getPlugin(JavaPluginConvention)
convention.sourceCompatibility = JavaVersion.VERSION_1_8
convention.targetCompatibility = JavaVersion.VERSION_1_8
}
}

subprojects { project ->
dependencies {
implementation platform("com.netflix.spinnaker.kork:kork-bom:4.1.0-rc.3-springBoot2")
implementation platform("com.netflix.spinnaker.kork:kork-bom:$korkVersion")
compileOnly "org.projectlombok:lombok"
annotationProcessor platform("com.netflix.spinnaker.kork:kork-bom:4.1.0-rc.3-springBoot2")
annotationProcessor platform("com.netflix.spinnaker.kork:kork-bom:$korkVersion")
annotationProcessor "org.projectlombok:lombok"
testAnnotationProcessor platform("com.netflix.spinnaker.kork:kork-bom:4.1.0-rc.3-springBoot2")
testAnnotationProcessor platform("com.netflix.spinnaker.kork:kork-bom:$korkVersion")
testAnnotationProcessor "org.projectlombok:lombok"

implementation "org.springframework.boot:spring-boot-properties-migrator"
Expand Down
35 changes: 13 additions & 22 deletions fiat-api/fiat-api.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,22 @@
*/

dependencies {
compile project(":fiat-core")
implementation project(":fiat-core")

compileOnly "org.springframework.boot:spring-boot-starter-web"
compileOnly "com.netflix.frigga:frigga"
compileOnly "com.netflix.spinnaker.kork:kork-security"
compileOnly "com.netflix.spinnaker.kork:kork-web"
compileOnly "com.netflix.spinnaker.kork:kork-core"
compileOnly "com.squareup.okhttp:okhttp"
compileOnly "com.squareup.okhttp:okhttp-urlconnection"
compileOnly "com.squareup.okhttp:okhttp-apache"
compileOnly "com.squareup.retrofit:retrofit"
compileOnly "com.squareup.retrofit:converter-jackson"
compileOnly "com.netflix.spectator:spectator-api"

implementation "com.github.ben-manes.caffeine:guava"
implementation "org.springframework.security:spring-security-config"
implementation "org.springframework:spring-aop"
implementation "org.springframework:spring-web"
implementation "org.springframework.security:spring-security-core"
implementation "org.springframework.security:spring-security-web"
implementation "com.netflix.spectator:spectator-api"
implementation "com.netflix.spinnaker.kork:kork-core"
implementation "com.netflix.spinnaker.kork:kork-security"
implementation "com.netflix.spinnaker.kork:kork-web"
implementation "com.squareup.retrofit:retrofit"
implementation "com.squareup.retrofit:converter-jackson"

compileOnly "javax.servlet:javax.servlet-api"

implementation "com.github.ben-manes.caffeine:caffeine"

testImplementation "com.squareup.retrofit:retrofit"
testImplementation "com.squareup.okhttp:okhttp"
testImplementation "com.netflix.frigga:frigga"
testImplementation "com.netflix.spinnaker.kork:kork-security"
testImplementation "com.netflix.spinnaker.kork:kork-core"
testImplementation "com.netflix.spectator:spectator-api"
testImplementation "org.slf4j:slf4j-api"
testImplementation "org.springframework.boot:spring-boot-autoconfigure"
}
45 changes: 8 additions & 37 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,14 @@

import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.OkHttpClientConfiguration;
import com.netflix.spinnaker.okhttp.OkHttpMetricsInterceptor;
import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
import com.squareup.okhttp.OkHttpClient;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import lombok.val;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.annotation.Bean;
Expand All @@ -39,10 +35,10 @@
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.web.authentication.AnonymousAuthenticationFilter;
import org.springframework.security.web.context.SecurityContextPersistenceFilter;
import retrofit.Endpoints;
import retrofit.RestAdapter;
import retrofit.client.Client;
import retrofit.client.OkClient;
import retrofit.converter.JacksonConverter;

Expand All @@ -60,8 +56,7 @@ public class FiatAuthenticationConfig {

@Bean
@ConditionalOnMissingBean(FiatService.class) // Allows for override
public FiatService fiatService(Registry registry,
FiatClientConfigurationProperties fiatConfigurationProperties,
public FiatService fiatService(FiatClientConfigurationProperties fiatConfigurationProperties,
SpinnakerRequestInterceptor interceptor,
OkHttpClientConfiguration okHttpClientConfiguration) {
// New role providers break deserialization if this is not enabled.
Expand Down Expand Up @@ -91,40 +86,16 @@ private class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdap
private final FiatStatus fiatStatus;

private FiatWebSecurityConfigurerAdapter(FiatStatus fiatStatus) {
super(true);
this.fiatStatus = fiatStatus;
}

@Override
protected void configure(HttpSecurity http) throws Exception {
/*
* Having `FiatAuthenticationFilter` prior to `SecurityContextPersistenceFilter` results in the
* `SecurityContextHolder` being overridden with a null value.
*
* The null value then causes the `AnonymousAuthenticationFilter` to inject an "anonymousUser" which when
* passed over the wire to fiat is promptly rejected.
*
* This behavior is triggered when `management.security.enabled` is `false`.
*/
http
.csrf().disable()
.addFilterAfter(new FiatAuthenticationFilter(fiatStatus), SecurityContextPersistenceFilter.class);
}
}

private static class Slf4jRetrofitLogger implements RestAdapter.Log {
private final Logger logger;

Slf4jRetrofitLogger(Class type) {
this(LoggerFactory.getLogger(type));
}

Slf4jRetrofitLogger(Logger logger) {
this.logger = logger;
}

@Override
public void log(String message) {
logger.info(message);
http.servletApi().and()
.exceptionHandling().and()
.anonymous().and()
.addFilterBefore(new FiatAuthenticationFilter(fiatStatus), AnonymousAuthenticationFilter.class);
}
}
}
5 changes: 2 additions & 3 deletions ...pi/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,15 @@ public RetryConfiguration getRetry() {
}

@Data
class PermissionsCache {
public static class PermissionsCache {
private Integer maxEntries = 1000;

private Integer expiresAfterWriteSeconds = 20;
}

@Data
class RetryConfiguration {
public static class RetryConfiguration {
private DynamicConfigService dynamicConfigService;

private long maxBackoffMillis = 10000;
private long initialBackoffMillis = 500;
private double retryMultiplier = 1.5;
Expand Down
68 changes: 36 additions & 32 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@

package com.netflix.spinnaker.fiat.shared;

import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.netflix.spectator.api.Id;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.fiat.model.Authorization;
Expand All @@ -46,7 +45,6 @@
import java.util.Optional;
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;
import java.util.concurrent.atomic.AtomicReference;
Expand Down Expand Up @@ -130,7 +128,7 @@ private static RetryHandler buildRetryHandler(FiatClientConfigurationProperties
this.fiatStatus = fiatStatus;
this.retryHandler = retryHandler;

this.permissionsCache = CacheBuilder
this.permissionsCache = Caffeine
.newBuilder()
.maximumSize(configProps.getCache().getMaxEntries())
.expireAfterWrite(configProps.getCache().getExpiresAfterWriteSeconds(), TimeUnit.SECONDS)
Expand Down Expand Up @@ -205,35 +203,41 @@ public UserPermission.View getPermission(String username) {
AtomicReference<Throwable> exception = new AtomicReference<>();

try {
view = permissionsCache.get(username, () -> {
view = permissionsCache.get(username, (loadUserName) -> {
cacheHit.set(false);
return AuthenticatedRequest.propagate(() -> {
try {
return retryHandler.retry("getUserPermission for " + username, () -> fiatService.getUserPermission(username));
} catch (Exception e) {
if (!fiatStatus.isLegacyFallbackEnabled()) {
throw e;
try {
return AuthenticatedRequest.propagate(() -> {
try {
return retryHandler.retry("getUserPermission for " + loadUserName, () -> fiatService.getUserPermission(loadUserName));
} catch (Exception e) {
if (!fiatStatus.isLegacyFallbackEnabled()) {
throw e;
}

legacyFallback.set(true);
successfulLookup.set(false);
exception.set(e);

// this fallback permission will be temporarily cached in the permissions cache
return new UserPermission.View(
new UserPermission()
.setId(AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"))
.setAccounts(
Arrays
.stream(AuthenticatedRequest.getSpinnakerAccounts().orElse("").split(","))
.map(a -> new Account().setName(a))
.collect(Collectors.toSet())
)
).setLegacyFallback(true).setAllowAccessToUnknownApplications(true);
}

legacyFallback.set(true);
successfulLookup.set(false);
exception.set(e);

// this fallback permission will be temporarily cached in the permissions cache
return new UserPermission.View(
new UserPermission()
.setId(AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"))
.setAccounts(
Arrays
.stream(AuthenticatedRequest.getSpinnakerAccounts().orElse("").split(","))
.map(a -> new Account().setName(a))
.collect(Collectors.toSet())
)
).setLegacyFallback(true).setAllowAccessToUnknownApplications(true);
}
}).call();
}).call();
} catch (RuntimeException re) {
throw re;
} catch (Exception e) {
throw new RuntimeException(e);
}
});
} catch (ExecutionException | UncheckedExecutionException e) {
} catch (Exception e) {
successfulLookup.set(false);
exception.set(e.getCause() != null ? e.getCause() : e);
}
Expand Down Expand Up @@ -352,7 +356,7 @@ public boolean isAdmin(Authentication authentication) {
return permission != null && permission.isAdmin();
}

public class AuthorizationFailure {
public static class AuthorizationFailure {
private final Authorization authorization;
private final ResourceType resourceType;
private final String resourceName;
Expand Down
2 changes: 1 addition & 1 deletion fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package com.netflix.spinnaker.fiat.shared;

import com.netflix.spinnaker.fiat.model.UserPermission;
import com.squareup.okhttp.Response;
import retrofit.client.Response;
import retrofit.http.Body;
import retrofit.http.DELETE;
import retrofit.http.GET;
Expand Down
5 changes: 3 additions & 2 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.netflix.spinnaker.fiat.shared;

import com.netflix.spectator.api.Registry;
import com.netflix.spectator.api.patterns.PolledMeter;
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -46,8 +47,8 @@ public FiatStatus(Registry registry,
this.enabled = new AtomicBoolean(fiatClientConfigurationProperties.isEnabled());
this.legacyFallbackEnabled = new AtomicBoolean(fiatClientConfigurationProperties.isLegacyFallback());

registry.gauge("fiat.enabled", enabled, value -> enabled.get() ? 1 : 0);
registry.gauge("fiat.legacyFallback.enabled", legacyFallbackEnabled, value -> legacyFallbackEnabled.get() ? 1 : 0);
PolledMeter.using(registry).withName("fiat.enabled").monitorValue(enabled, value -> enabled.get() ? 1 : 0);
PolledMeter.using(registry).withName("fiat.legacyFallback.enabled").monitorValue(legacyFallbackEnabled, value -> legacyFallbackEnabled.get() ? 1 : 0);
}

public boolean isEnabled() {
Expand Down
50 changes: 11 additions & 39 deletions ...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,46 +103,18 @@ class FiatPermissionEvaluatorSpec extends Specification {
.setAuthorizations([Authorization.READ] as Set)] as Set)
upv.setServiceAccounts([new ServiceAccount.View().setName(svcAcct)
.setMemberOf(["foo"])] as Set)
fiatService.getUserPermission("testUser") >> upv
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in here are related to the change to Caffeine cache not being deterministic about the number of items (we have it set to maxItems = 0 for tests, but eviction back down to 0 happens async). The places where it was asserting the number of calls against the mock were brittle but also were not relevant to the tests - they were effectively stub calls anyway so the tests got a bit simpler as a result


when:
def hasPermission = evaluator.hasPermission(authentication,
resource,
'APPLICATION',
'READ')

then:
1 * fiatService.getUserPermission("testUser") >> upv
hasPermission

when:
hasPermission = evaluator.hasPermission(authentication,
resource,
'APPLICATION',
'WRITE') // Missing authorization

then:
1 * fiatService.getUserPermission("testUser") >> upv
!hasPermission
expect:
evaluator.hasPermission(authentication, resource, 'APPLICATION', 'READ')

when:
hasPermission = evaluator.hasPermission(authentication,
resource, // Missing resource
'SERVICE_ACCOUNT',
'WRITE')
// Missing authorization:
!evaluator.hasPermission(authentication, resource, 'APPLICATION', 'WRITE')

then:
1 * fiatService.getUserPermission("testUser") >> upv
!hasPermission
// Missing resource
!evaluator.hasPermission(authentication, resource, 'SERVICE_ACCOUNT', 'WRITE')

when:
hasPermission = evaluator.hasPermission(authentication,
svcAcct,
'SERVICE_ACCOUNT',
'WRITE')

then:
1 * fiatService.getUserPermission("testUser") >> upv
hasPermission
evaluator.hasPermission(authentication, svcAcct, 'SERVICE_ACCOUNT', 'WRITE')
}

@Unroll
Expand Down Expand Up @@ -241,9 +213,9 @@ class FiatPermissionEvaluatorSpec extends Specification {
}

@Unroll
def "should allow an admin to access all resource types"() {
def "should allow an admin to access #resourceType"() {
given:
2 * fiatService.getUserPermission("testUser") >> {
fiatService.getUserPermission("testUser") >> {
return new UserPermission.View()
.setApplications(Collections.emptySet())
.setAdmin(true)
Expand Down Expand Up @@ -275,7 +247,7 @@ class FiatPermissionEvaluatorSpec extends Specification {
}

private static FiatClientConfigurationProperties buildConfigurationProperties() {
FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties();
FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties()
configurationProperties.enabled = true
configurationProperties.cache.maxEntries = 0

Expand Down
Loading