From aafba23b2d33cb4a193a4febc8c155d3e45434bd Mon Sep 17 00:00:00 2001 From: Cameron Fieber Date: Wed, 17 Apr 2019 00:51:01 -0700 Subject: [PATCH 1/2] fix(dependencies): dependency cleanup * removes lots of cargo-culted dependency declarations * trims dependencies in fiat-core and fiat-api to minimize the impact consuming projects * guava caches -> caffeine --- build.gradle | 7 ++ fiat-api/fiat-api.gradle | 35 ++++------ .../fiat/shared/FiatAuthenticationConfig.java | 27 +------- .../FiatClientConfigurationProperties.java | 5 +- .../fiat/shared/FiatPermissionEvaluator.java | 68 ++++++++++--------- .../spinnaker/fiat/shared/FiatService.java | 2 +- .../spinnaker/fiat/shared/FiatStatus.java | 5 +- .../shared/FiatPermissionEvaluatorSpec.groovy | 50 +++----------- fiat-core/fiat-core.gradle | 16 ++--- .../spinnaker/fiat/model/Authorization.java | 9 ++- .../fiat/model/resources/Account.java | 5 +- .../fiat/model/resources/Application.java | 4 +- .../fiat/model/resources/BuildService.java | 3 +- .../fiat/model/resources/Permissions.java | 30 +++----- .../fiat/model/resources/ResourceType.java | 11 ++- .../spinnaker/fiat/model/resources/Role.java | 10 ++- .../model/resources/PermissionsSpec.groovy | 5 +- .../fiat/model/resources/ResourceSpec.groovy | 2 +- fiat-file/fiat-file.gradle | 2 +- fiat-ldap/fiat-ldap.gradle | 8 +-- fiat-web/fiat-web.gradle | 1 - .../fiat/config/ResourcesConfig.java | 18 +---- gradle/buildViaTravis.sh | 1 - gradle/init-publish.gradle | 2 +- gradle/installViaTravis.sh | 1 - 25 files changed, 117 insertions(+), 210 deletions(-) diff --git a/build.gradle b/build.gradle index f956b288b..64e63bf58 100644 --- a/build.gradle +++ b/build.gradle @@ -47,6 +47,13 @@ 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 -> diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index d02c6d632..2c460b98f 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -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" } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java index 7ec8dd860..045246189 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java @@ -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; @@ -42,7 +38,6 @@ 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; @@ -60,8 +55,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. @@ -110,21 +104,4 @@ protected void configure(HttpSecurity http) throws Exception { .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); - } - } } 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 c0ccbb352..5541562a3 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 @@ -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; 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 704b34ceb..27353ea8e 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 @@ -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; @@ -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; @@ -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) @@ -205,35 +203,41 @@ public UserPermission.View getPermission(String username) { AtomicReference 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); } @@ -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; 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 8defb3668..f7f0e5107 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 @@ -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; diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatStatus.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatStatus.java index f7a31b246..bd6492eab 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatStatus.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatStatus.java @@ -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; @@ -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() { 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 b7d7e364b..5e185bd3a 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 @@ -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 - 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 @@ -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) @@ -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 diff --git a/fiat-core/fiat-core.gradle b/fiat-core/fiat-core.gradle index 73d85659f..584f7785f 100644 --- a/fiat-core/fiat-core.gradle +++ b/fiat-core/fiat-core.gradle @@ -1,17 +1,9 @@ dependencies { - compileOnly "com.squareup.retrofit:retrofit" - compileOnly "com.squareup.okhttp:okhttp" - compileOnly "com.squareup.okhttp:okhttp-urlconnection" - compileOnly "com.squareup.okhttp:okhttp-apache" - compileOnly "com.squareup.retrofit:converter-jackson" - - implementation "org.apache.commons:commons-lang3" + implementation "com.fasterxml.jackson.core:jackson-annotations" + implementation "com.google.code.findbugs:jsr305" implementation "org.slf4j:slf4j-api" - implementation "com.github.ben-manes.caffeine:guava" - testImplementation "commons-io:commons-io" - testImplementation "com.squareup.retrofit:converter-jackson" - testImplementation "org.springframework.boot:spring-boot-autoconfigure" - testImplementation "org.springframework.boot:spring-boot-starter-actuator" + testImplementation "org.springframework.boot:spring-boot" + testImplementation "com.fasterxml.jackson.core:jackson-databind" } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java index 487314da0..08224b875 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java @@ -16,7 +16,14 @@ package com.netflix.spinnaker.fiat.model; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + public enum Authorization { READ, - WRITE + WRITE; + + public static Set ALL = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(values()))); } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java index 4cda6156a..848beecac 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java @@ -17,14 +17,11 @@ package com.netflix.spinnaker.fiat.model.resources; import com.fasterxml.jackson.annotation.JsonIgnore; -import com.google.common.collect.Sets; import com.netflix.spinnaker.fiat.model.Authorization; import lombok.Data; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import java.util.HashSet; import java.util.Set; @Data @@ -51,7 +48,7 @@ public static class View extends BaseView implements Authorizable { public View(Account account, Set userRoles, boolean isAdmin) { this.name = account.name; if (isAdmin) { - this.authorizations = Sets.newHashSet(Authorization.READ, Authorization.WRITE); + this.authorizations = Authorization.ALL; } else { this.authorizations = account.permissions.getAuthorizations(userRoles); } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java index bf48a3893..bb8b9be64 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java @@ -17,12 +17,10 @@ package com.netflix.spinnaker.fiat.model.resources; import com.fasterxml.jackson.annotation.JsonIgnore; -import com.google.common.collect.Sets; import com.netflix.spinnaker.fiat.model.Authorization; import lombok.Data; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; -import lombok.extern.slf4j.Slf4j; import java.util.Set; @@ -49,7 +47,7 @@ public static class View extends BaseView implements Authorizable { public View(Application application, Set userRoles, boolean isAdmin) { this.name = application.name; if (isAdmin) { - this.authorizations = Sets.newHashSet(Authorization.READ, Authorization.WRITE); + this.authorizations = Authorization.ALL; } else { this.authorizations = application.permissions.getAuthorizations(userRoles); } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java index a18227ed7..a2410fa8b 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java @@ -17,7 +17,6 @@ package com.netflix.spinnaker.fiat.model.resources; -import com.google.common.collect.Sets; import com.netflix.spinnaker.fiat.model.Authorization; import lombok.Data; import lombok.EqualsAndHashCode; @@ -50,7 +49,7 @@ public static class View extends Viewable.BaseView implements Authorizable { public View(BuildService buildService, Set userRoles, boolean isAdmin) { this.name = buildService.name; if (isAdmin) { - this.authorizations = Sets.newHashSet(Authorization.READ, Authorization.WRITE); + this.authorizations = Authorization.ALL; } else { this.authorizations = buildService.permissions.getAuthorizations(userRoles); } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java index 42061c8d2..373cf0d34 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java @@ -18,28 +18,18 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonValue; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.netflix.spinnaker.fiat.model.Authorization; import lombok.EqualsAndHashCode; import lombok.ToString; import lombok.val; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; /** * Representation of authorization configuration for a resource. This object is immutable, which * makes it challenging when working with Jackson's - * {@link com.fasterxml.jackson.databind.ObjectMapper} and Spring's {@link ConfigurationProperties}. + * {{ObjectMapper}} and Spring's {{\@ConfigurationProperties}}. * The {@link Builder} is a helper class for the latter use case. */ @ToString @@ -47,7 +37,6 @@ public class Permissions { public static Permissions EMPTY = new Permissions.Builder().build(); - private static Set UNRESTRICTED_AUTH = ImmutableSet.copyOf(Authorization.values()); private final Map> permissions; @@ -95,7 +84,7 @@ public Set getAuthorizations(Set userRoles) { public Set getAuthorizations(List userRoles) { if (!isRestricted()) { - return UNRESTRICTED_AUTH; + return Authorization.ALL; } return this.permissions @@ -106,7 +95,7 @@ public Set getAuthorizations(List userRoles) { .collect(Collectors.toSet()); } - @VisibleForTesting + //VisibleForTesting protected List get(Authorization a) { return permissions.get(a); } @@ -153,15 +142,16 @@ public Builder add(Authorization a, List groups) { } public Permissions build() { - ImmutableMap.Builder> builder = ImmutableMap.builder(); + final Map> perms = new HashMap<>(); this.forEach((auth, groups) -> { - List lowerGroups = groups.stream() + List lowerGroups = Collections.unmodifiableList(groups.stream() .map(String::trim) + .filter(s -> !s.isEmpty()) .map(String::toLowerCase) - .collect(Collectors.toList()); - builder.put(auth, ImmutableList.copyOf(lowerGroups)); + .collect(Collectors.toList())); + perms.put(auth, lowerGroups); }); - return new Permissions(builder.build()); + return new Permissions(Collections.unmodifiableMap(perms)); } } } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java index 09231d043..d0e5ef679 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java @@ -16,8 +16,7 @@ package com.netflix.spinnaker.fiat.model.resources; -import lombok.NonNull; -import org.apache.commons.lang3.StringUtils; +import javax.annotation.Nonnull; public enum ResourceType { ACCOUNT(Account.class), @@ -33,11 +32,9 @@ public enum ResourceType { } // TODO(ttomsu): This is Redis-specific, so it probably shouldn't go here. - public static ResourceType parse(@NonNull String pluralOrKey) { - if (pluralOrKey.contains(":")) { - pluralOrKey = StringUtils.substringAfterLast(pluralOrKey, ":"); - } - String singular = StringUtils.removeEnd(pluralOrKey, "s"); + public static ResourceType parse(@Nonnull String pluralOrKey) { + pluralOrKey = pluralOrKey.substring(pluralOrKey.lastIndexOf(':') + 1); + String singular = pluralOrKey.endsWith("s") ? pluralOrKey.substring(0, pluralOrKey.length() - 1) : pluralOrKey; return ResourceType.valueOf(singular.toUpperCase()); } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java index 2c9da7727..b2b16788f 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java @@ -20,10 +20,8 @@ import lombok.Data; import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; -import org.apache.commons.lang3.StringUtils; -import java.util.Collections; -import java.util.List; +import javax.annotation.Nonnull; import java.util.Set; @Data @@ -48,9 +46,9 @@ public Role(String name) { this.setName(name); } - public Role setName(String name) { - if (StringUtils.isEmpty(name)) { - throw new IllegalArgumentException("name cannot be null or empty"); + public Role setName(@Nonnull String name) { + if (name.isEmpty()) { + throw new IllegalArgumentException("name cannot be empty"); } this.name = name.toLowerCase(); return this; diff --git a/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/PermissionsSpec.groovy b/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/PermissionsSpec.groovy index a258720f6..c0a769133 100644 --- a/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/PermissionsSpec.groovy +++ b/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/PermissionsSpec.groovy @@ -36,7 +36,10 @@ class PermissionsSpec extends Specification { @Autowired TestConfigProps testConfigProps - ObjectMapper mapper = new ObjectMapper().configure(SerializationFeature.INDENT_OUTPUT, true) + ObjectMapper mapper = + new ObjectMapper() + .enable(SerializationFeature.INDENT_OUTPUT) + .enable(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS) String permissionJson = '''{ "READ" : [ "foo" ], diff --git a/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/ResourceSpec.groovy b/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/ResourceSpec.groovy index 59c4c19d6..bf78f8b9e 100644 --- a/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/ResourceSpec.groovy +++ b/fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/ResourceSpec.groovy @@ -45,7 +45,7 @@ class ResourceSpec extends Specification { where: input || e - null || IllegalArgumentException.class + null || NullPointerException.class "" || IllegalArgumentException.class "account:" || IllegalArgumentException.class "account:s" || IllegalArgumentException.class diff --git a/fiat-file/fiat-file.gradle b/fiat-file/fiat-file.gradle index 0eaed648c..925790949 100644 --- a/fiat-file/fiat-file.gradle +++ b/fiat-file/fiat-file.gradle @@ -16,7 +16,7 @@ dependencies { implementation project(":fiat-roles") - implementation project(":fiat-api") + implementation project(":fiat-core") implementation "com.fasterxml.jackson.core:jackson-core" implementation "com.fasterxml.jackson.core:jackson-databind" implementation "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml" diff --git a/fiat-ldap/fiat-ldap.gradle b/fiat-ldap/fiat-ldap.gradle index 5eaf62af7..47aa62cde 100644 --- a/fiat-ldap/fiat-ldap.gradle +++ b/fiat-ldap/fiat-ldap.gradle @@ -16,13 +16,7 @@ dependencies { implementation project(":fiat-roles") - implementation project(":fiat-api") - - compileOnly "com.squareup.retrofit:retrofit" - compileOnly "com.squareup.okhttp:okhttp" - compileOnly "com.squareup.okhttp:okhttp-urlconnection" - compileOnly "com.squareup.okhttp:okhttp-apache" - compileOnly "com.squareup.retrofit:converter-jackson" + implementation project(":fiat-core") implementation "org.apache.commons:commons-lang3" implementation "org.springframework.boot:spring-boot-autoconfigure" diff --git a/fiat-web/fiat-web.gradle b/fiat-web/fiat-web.gradle index 4d1896770..0323bca55 100644 --- a/fiat-web/fiat-web.gradle +++ b/fiat-web/fiat-web.gradle @@ -31,7 +31,6 @@ dependencies { implementation "com.netflix.spinnaker.kork:kork-swagger" implementation project(':fiat-core') - implementation project(':fiat-api') implementation project(':fiat-roles') // Add each included authz provider as a runtime dependency diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java index e1ed84e82..22f96dc74 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java @@ -24,6 +24,7 @@ import com.netflix.spinnaker.fiat.providers.internal.Front50Service; import com.netflix.spinnaker.fiat.providers.internal.IgorApi; import com.netflix.spinnaker.fiat.providers.internal.IgorService; +import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger; import lombok.Setter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -127,21 +128,4 @@ IgorService igorService(IgorApi igorApi) { ProviderHealthTracker providerHealthTracker(ProviderCacheConfig config) { return new ProviderHealthTracker(config.getMaximumStalenessTimeMs()); } - - 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.debug(message); - } - } } diff --git a/gradle/buildViaTravis.sh b/gradle/buildViaTravis.sh index 331e83857..7f755d9ff 100755 --- a/gradle/buildViaTravis.sh +++ b/gradle/buildViaTravis.sh @@ -2,7 +2,6 @@ # This script will build the project. GRADLE="./gradlew -I gradle/init-publish.gradle" -export GRADLE_OPTS="-Xmx1g -Xms1g" if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then echo -e "Build Pull Request #$TRAVIS_PULL_REQUEST => Branch [$TRAVIS_BRANCH]" diff --git a/gradle/init-publish.gradle b/gradle/init-publish.gradle index d379a0165..d646f32c0 100644 --- a/gradle/init-publish.gradle +++ b/gradle/init-publish.gradle @@ -6,7 +6,7 @@ initscript { maven { url "https://plugins.gradle.org/m2/" } } dependencies { - classpath 'com.netflix.spinnaker.gradle:spinnaker-gradle-project:5.2.1' + classpath 'com.netflix.spinnaker.gradle:spinnaker-gradle-project:5.2.2' } } diff --git a/gradle/installViaTravis.sh b/gradle/installViaTravis.sh index f4e7244ea..292344d7d 100755 --- a/gradle/installViaTravis.sh +++ b/gradle/installViaTravis.sh @@ -2,7 +2,6 @@ # This script will build the project. GRADLE="./gradlew -I gradle/init-publish.gradle" -export GRADLE_OPTS="-Xmx1g -Xms1g" if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then echo -e "Assemble Pull Request #$TRAVIS_PULL_REQUEST => Branch [$TRAVIS_BRANCH]" From 95927386a461e71a8bf0e91d5a0167bcde500a23 Mon Sep 17 00:00:00 2001 From: Cameron Fieber Date: Thu, 18 Apr 2019 22:30:19 -0700 Subject: [PATCH 2/2] fix(api): cleanup FiatAuthenticationConfig WebSecurityConfigurerAdapter --- build.gradle | 9 ++++++--- .../fiat/shared/FiatAuthenticationConfig.java | 18 ++++++------------ fiat-google-groups/fiat-google-groups.gradle | 2 +- fiat-roles/fiat-roles.gradle | 2 +- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/build.gradle b/build.gradle index 64e63bf58..f85e8daec 100644 --- a/build.gradle +++ b/build.gradle @@ -15,6 +15,9 @@ */ buildscript { + ext { + korkVersion = "4.1.0-rc.4-springBoot2" + } repositories { jcenter() maven { url "https://spinnaker.bintray.com/gradle" } @@ -58,11 +61,11 @@ allprojects { 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" diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java index 045246189..e40b86262 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java @@ -35,6 +35,7 @@ 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; @@ -85,23 +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); + http.servletApi().and() + .exceptionHandling().and() + .anonymous().and() + .addFilterBefore(new FiatAuthenticationFilter(fiatStatus), AnonymousAuthenticationFilter.class); } } } diff --git a/fiat-google-groups/fiat-google-groups.gradle b/fiat-google-groups/fiat-google-groups.gradle index 1c60bc8ee..3c9254067 100644 --- a/fiat-google-groups/fiat-google-groups.gradle +++ b/fiat-google-groups/fiat-google-groups.gradle @@ -21,5 +21,5 @@ dependencies { implementation "org.springframework.boot:spring-boot-starter-web" implementation "org.apache.commons:commons-lang3" implementation "com.google.api-client:google-api-client" - implementation "com.google.apis:google-api-services-admin-directory:directory_v1-rev105-1.25.0" + implementation "com.google.apis:google-api-services-admin-directory" } diff --git a/fiat-roles/fiat-roles.gradle b/fiat-roles/fiat-roles.gradle index 3f3549c13..0acccbb7e 100644 --- a/fiat-roles/fiat-roles.gradle +++ b/fiat-roles/fiat-roles.gradle @@ -30,7 +30,7 @@ dependencies { implementation "com.netflix.spinnaker.kork:kork-hystrix" implementation "com.netflix.spinnaker.kork:kork-jedis" implementation "redis.clients:jedis" - implementation "com.google.api-client:google-api-client:1.28.0" + implementation "com.google.api-client:google-api-client" testImplementation "com.netflix.spinnaker.kork:kork-jedis-test" testImplementation "org.apache.commons:commons-collections4:4.1"