feat(serviceAccounts): Simplifies service accounts. (#172)

Also pulls out provider health into it's own class, and adds a cache to
all resource providers.

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java

@@ -60,6 +60,7 @@ public FiatService fiatService(FiatClientConfigurationProperties fiatConfigurati
     // New role providers break deserialization if this is not enabled.
     val objectMapper = new ObjectMapper();
     objectMapper.enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL);
+    objectMapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
     return new RestAdapter.Builder()
         .setEndpoint(Endpoints.newFixedEndpoint(fiatConfigurationProperties.getBaseUrl()))
         .setClient(okClient)

fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ServiceAccount.java

@@ -22,43 +22,35 @@
 import lombok.EqualsAndHashCode;
 import lombok.NoArgsConstructor;
 import lombok.val;
-import org.apache.commons.lang3.StringUtils;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
 
 @Data
 @EqualsAndHashCode(callSuper = false)
-public class ServiceAccount extends BaseAccessControlled implements Viewable {
+public class ServiceAccount implements Resource, Viewable {
   private final ResourceType resourceType = ResourceType.SERVICE_ACCOUNT;
 
   private String name;
   private List<String> memberOf = new ArrayList<>();
-  private Permissions permissions = Permissions.EMPTY;
 
   public UserPermission toUserPermission() {
     val roles = memberOf.stream()
                         .map(membership -> new Role(membership).setSource(Role.Source.EXTERNAL))
                         .collect(Collectors.toSet());
     return new UserPermission().setId(name).setRoles(roles);
   }
-
-  @JsonIgnore
-  public List<String> getRequiredGroupMembership() {
-    // There is a potential here for a naming collision where service account
-    // "my-svc-account@abc.com" and "my-svc-account@xyz.com" each allow one another's users to use
-    // their service account. In practice, though, I don't think this will be an issue.
-    return Collections.singletonList(StringUtils.substringBefore(name, "@"));
-  }
 
   public ServiceAccount setMemberOf(List<String> membership) {
     if (membership == null) {
       membership = new ArrayList<>();
     }
-    memberOf = membership.stream().map(String::toLowerCase).collect(Collectors.toList());
+    memberOf = membership.stream()
+                         .map(String::trim)
+                         .map(String::toLowerCase)
+                         .collect(Collectors.toList());
     return this;
   }
 
@@ -72,9 +64,11 @@ public View getView(Set<Role> ignored) {
   @NoArgsConstructor
   public static class View extends BaseView {
     private String name;
+    private List<String> memberOf;
 
     public View(ServiceAccount serviceAccount) {
       this.name = serviceAccount.name;
+      this.memberOf = serviceAccount.memberOf;
     }
   }
 }

fiat-core/src/test/groovy/com/netflix/spinnaker/fiat/model/resources/PermissionsSpec.groovy

@@ -72,6 +72,21 @@ class PermissionsSpec extends Specification {
     permissionJson ==  mapper.writeValueAsString(b.build())
   }
 
+  def "can deserialize to builder from serialized Permissions"() {
+    setup:
+    Permissions.Builder b1 = new Permissions.Builder().add(W, "batman").add(R, "robin")
+    Permissions p1 = b1.build()
+
+    when:
+    def serialized = mapper.writeValueAsString(p1)
+    Permissions.Builder b2 = mapper.readValue(serialized, Permissions.Builder)
+    Permissions p2 = b2.build()
+
+    then:
+    p1 == p2
+    b1 == b2
+  }
+
   def "should trim and lower"() {
     when:
     Permissions.Builder b = new Permissions.Builder()

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/ProviderCacheConfig.java

@@ -0,0 +1,27 @@
+/*
+ * Copyright 2017 Google, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.fiat.config;
+
+import lombok.Data;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+@Data
+@ConfigurationProperties("fiat.cache")
+public class ProviderCacheConfig {
+
+  private int expiresAfterWriteSeconds = 20;
+}

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/ResourceProvidersHealthIndicator.java

@@ -16,9 +16,7 @@
 
 package com.netflix.spinnaker.fiat.config;
 
-import com.netflix.spinnaker.fiat.providers.BaseProvider;
-import com.netflix.spinnaker.fiat.providers.ProviderException;
-import com.netflix.spinnaker.fiat.providers.ResourceProvider;
+import com.netflix.spinnaker.fiat.providers.HealthTrackable;
 import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -37,16 +35,16 @@ public class ResourceProvidersHealthIndicator extends AbstractHealthIndicator {
 
   @Autowired
   @Setter
-  List<BaseProvider> providers;
+  List<HealthTrackable> providers;
 
   private AtomicBoolean previousHealthCheckIsUp = new AtomicBoolean(false);
 
   @Override
   protected void doHealthCheck(Health.Builder builder) throws Exception {
     boolean isDown = false;
-    for (BaseProvider provider : providers) {
-      builder.withDetail(provider.getClass().getSimpleName(), provider.getHealthView());
-      isDown = isDown || !provider.isProviderHealthy();
+    for (HealthTrackable provider : providers) {
+      builder.withDetail(provider.getClass().getSimpleName(), provider.getHealthTracker().getHealthView());
+      isDown = isDown || !provider.getHealthTracker().isProviderHealthy();
     }
 
     if (isDown) {

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolver.java

@@ -55,7 +55,7 @@ public class DefaultPermissionsResolver implements PermissionsResolver {
 
   @Autowired
   @Setter
-  private List<ResourceProvider<? extends Resource.AccessControlled>> resourceProviders;
+  private List<ResourceProvider<? extends Resource>> resourceProviders;
 
   @Autowired
   @Setter
@@ -111,9 +111,6 @@ private UserPermission getUserPermission(String userId, Set<Role> roles) {
   public Map<String, UserPermission> resolve(@NonNull Collection<ExternalUser> users) {
     val userToRoles = getAndMergeUserRoles(users);
 
-    // This is the reverse index of each resourceProvider's getAllRestricted() call.
-    AccessControlLists acls = buildAcls();
-
     return userToRoles
         .entrySet()
         .stream()
@@ -123,25 +120,11 @@ public Map<String, UserPermission> resolve(@NonNull Collection<ExternalUser> use
 
           return new UserPermission().setId(username)
                                      .setRoles(userRoles)
-                                     .addResources(acls.canAccess(userRoles));
+                                     .addResources(getResources(userRoles));
         })
         .collect(Collectors.toMap(UserPermission::getId, Function.identity()));
   }
 
-  private AccessControlLists buildAcls() {
-    AccessControlLists acls = new AccessControlLists();
-
-    for (ResourceProvider<? extends Resource.AccessControlled> provider : resourceProviders) {
-      try {
-        provider.getAll().forEach(acls::add);
-      } catch (ProviderException pe) {
-        throw new PermissionResolutionException(pe);
-      }
-    }
-
-    return acls;
-  }
-
   private Map<String, Collection<Role>> getAndMergeUserRoles(@NonNull Collection<ExternalUser> users) {
     List<String> usernames = users.stream()
                                   .map(ExternalUser::getId)
@@ -165,22 +148,16 @@ private Map<String, Collection<Role>> getAndMergeUserRoles(@NonNull Collection<E
     return userToRoles;
   }
 
-  static class AccessControlLists {
-    // This object indexes:
-    // role (group name) -> resources (account, application, etc)
-    Multimap<Role, Resource.AccessControlled> acl = ArrayListMultimap.create();
-
-    void add(Resource.AccessControlled resource) {
-      resource.getPermissions()
-              .allGroups()
-              .forEach(group -> acl.put(new Role(group), resource));
-    }
-
-    Collection<Resource> canAccess(Set<Role> roles) {
-      return roles.stream()
-                  .filter(role -> acl.containsKey(role))
-                  .flatMap(role -> acl.get(role).stream())
-                  .collect(Collectors.toSet());
-    }
+  private Set<Resource> getResources(Set<Role> roles) {
+    return resourceProviders
+        .stream()
+        .flatMap(provider -> {
+          try {
+            return provider.getAllRestricted(roles).stream();
+          } catch (ProviderException pe) {
+            throw new PermissionResolutionException(pe);
+          }
+        })
+        .collect(Collectors.toSet());
   }
 }

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/BaseProvider.java

@@ -16,72 +16,86 @@
 
 package com.netflix.spinnaker.fiat.providers;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import com.netflix.spinnaker.fiat.config.ProviderCacheConfig;
 import com.netflix.spinnaker.fiat.model.resources.Resource;
 import com.netflix.spinnaker.fiat.model.resources.Role;
-import lombok.Data;
+import lombok.Getter;
 import lombok.NonNull;
-import lombok.Setter;
-import lombok.val;
-import org.springframework.beans.factory.annotation.Value;
+import lombok.extern.slf4j.Slf4j;
+import org.springframework.beans.factory.annotation.Autowired;
 
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
-public abstract class BaseProvider<R extends Resource.AccessControlled> implements ResourceProvider<R> {
+@Slf4j
+public abstract class BaseProvider<R extends Resource> implements ResourceProvider<R>, HealthTrackable {
 
-  @Value("${unhealthy.threshold:5}")
-  @Setter
-  private int unhealthyThreshold;
+  private static final Integer CACHE_KEY = 0;
 
-  private AtomicInteger failureCountSinceLastSuccess = new AtomicInteger(-1);
-
-  void failure() {
-    // Increment the failure count only if there has been at least 1 success() call. Otherwise,
-    // leave the default, which is considered unhealthy.
-    if (!failureCountSinceLastSuccess.compareAndSet(-1, -1)) {
-      failureCountSinceLastSuccess.incrementAndGet();
-    }
-  }
-
-  void success() {
-    failureCountSinceLastSuccess.set(0);
-  }
-
-  public boolean isProviderHealthy() {
-    int count = failureCountSinceLastSuccess.get();
-    return 0 <= count && count < unhealthyThreshold;
-  }
-
-  public HealthView getHealthView() {
-    return new HealthView();
-  }
-
-  @Data
-  class HealthView {
-    boolean providerHealthy = BaseProvider.this.isProviderHealthy();
-    int failureCountSinceLastSuccess = BaseProvider.this.failureCountSinceLastSuccess.get();
-  }
+  @Getter
+  private ProviderHealthTracker healthTracker = new ProviderHealthTracker();
 
+  private Cache<Integer, Set<R>> cache = buildCache(20);
 
   @Override
   @SuppressWarnings("unchecked")
-  public Set<R> getAllRestricted(@NonNull Set<Role> roles) throws ProviderException {
-    val groupNames = roles.stream().map(Role::getName).collect(Collectors.toList());
+  public Set<R> getAllRestricted(@NonNull Set<Role> roles)
+      throws ProviderException {
     return (Set<R>) getAll()
         .stream()
+        .filter(resource -> resource instanceof Resource.AccessControlled)
+        .map(resource -> (Resource.AccessControlled) resource)
         .filter(resource -> resource.getPermissions().isRestricted())
         .filter(resource -> resource.getPermissions().isAuthorized(roles))
         .collect(Collectors.toSet());
-
   }
 
   @Override
   @SuppressWarnings("unchecked")
   public Set<R> getAllUnrestricted() throws ProviderException {
     return (Set<R>) getAll()
         .stream()
+        .filter(resource -> resource instanceof Resource.AccessControlled)
+        .map(resource -> (Resource.AccessControlled) resource)
         .filter(resource -> !resource.getPermissions().isRestricted())
         .collect(Collectors.toSet());
   }
+
+  @Override
+  public Set<R> getAll() throws ProviderException {
+    try {
+      return ImmutableSet.copyOf(cache.get(CACHE_KEY, this::loadAll));
+    } catch (ExecutionException | UncheckedExecutionException e) {
+      throw new ProviderException(e.getCause());
+    }
+  }
+
+  @Autowired
+  public void setProviderCacheConfig(ProviderCacheConfig config) {
+    this.cache = buildCache(config.getExpiresAfterWriteSeconds());
+  }
+
+  private Cache<Integer, Set<R>> buildCache(int expireAfterWrite) {
+    return CacheBuilder
+        .newBuilder()
+        .expireAfterWrite(expireAfterWrite, TimeUnit.SECONDS)
+        .maximumSize(1) // Using this cache loader just for the ability to refresh every X seconds.
+        .build();
+  }
+
+  protected abstract Set<R> loadAll() throws ProviderException;
+
+  protected void success() {
+    getHealthTracker().success();
+  }
+
+  protected void failure() {
+    getHealthTracker().failure();
+  }
 }

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/DefaultAccountProvider.java

@@ -36,18 +36,19 @@ public class DefaultAccountProvider extends BaseProvider<Account> implements Res
 
   @Autowired
   public DefaultAccountProvider(ClouddriverService clouddriverService) {
+    super();
     this.clouddriverService = clouddriverService;
   }
 
   @Override
-  public Set<Account> getAll() throws ProviderException {
+  protected Set<Account> loadAll() throws ProviderException {
     try {
       val returnVal = clouddriverService.getAccounts().stream().collect(Collectors.toSet());
       success();
       return returnVal;
-    } catch (RetrofitError re) {
+    } catch (Exception e) {
       failure();
-      throw new ProviderException(re);
+      throw e;
     }
   }
 }