feat(web): Support defaulting to __unrestricted_user__ when no perm…

…issions found (#230) This PR handles the case where a user attempts to access a service directly without going through `gate`. It is a common operational requirement to be able to access internal APIs directly. - bump spinnaker-dependencies to suppress 404 stack traces - emit a metric from `getPermission` vs a debug log message
spinnaker · Jun 6, 2018 · 6048e25 · 6048e25
1 parent f5ddbc2
commit 6048e25
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 89 deletions.
diff --git a/build.gradle b/build.gradle
@@ -36,7 +36,7 @@ allprojects {
   apply plugin: 'groovy'
 
   ext {
-    spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.154.3'
+    spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.155.1'
   }
 
   def checkLocalVersions = [spinnakerDependenciesVersion: spinnakerDependenciesVersion]

diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle
@@ -27,6 +27,7 @@ dependencies {
   compileOnly spinnaker.dependency("okHttpApache")
   compileOnly spinnaker.dependency("retrofit")
   compileOnly spinnaker.dependency("retrofitJackson")
+  compileOnly spinnaker.dependency("spectatorApi")
 
   compile spinnaker.dependency("guava")
   compile spinnaker.dependency("springSecurityConfig")
@@ -39,4 +40,5 @@ dependencies {
   testCompile spinnaker.dependency("frigga")
   testCompile spinnaker.dependency("korkSecurity")
   testCompile spinnaker.dependency("bootAutoConfigure")
+  testCompile spinnaker.dependency("spectatorApi")
 }
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
@@ -1,3 +1,4 @@
+
 /*
  * Copyright 2016 Google, Inc.
  *
@@ -19,60 +20,58 @@
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.util.concurrent.UncheckedExecutionException;
-import com.netflix.frigga.Names;
+import com.netflix.spectator.api.Id;
+import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.fiat.model.Authorization;
 import com.netflix.spinnaker.fiat.model.UserPermission;
 import com.netflix.spinnaker.fiat.model.resources.Authorizable;
 import com.netflix.spinnaker.fiat.model.resources.ResourceType;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import com.netflix.spinnaker.security.User;
-import lombok.Setter;
 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;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.stereotype.Component;
-import retrofit.RetrofitError;
 
 import java.io.Serializable;
 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.function.Function;
 
 @Component
 @Slf4j
-public class FiatPermissionEvaluator implements PermissionEvaluator, InitializingBean {
-
-  @Autowired
-  @Setter
-  private FiatService fiatService;
+public class FiatPermissionEvaluator implements PermissionEvaluator {
 
-  @Autowired
-  @Setter
-  private FiatClientConfigurationProperties configProps;
+  private final Registry registry;
+  private final FiatService fiatService;
+  private final FiatClientConfigurationProperties configProps;
 
-  @Value("${services.fiat.enabled:false}")
-  @Setter
-  private String fiatEnabled;
+  private final Cache<String, UserPermission.View> permissionsCache;
 
-  private Cache<String, UserPermission.View> permissionsCache;
+  private final Id getPermissionCounterId;
 
-  @Override
-  public void afterPropertiesSet() throws Exception {
-    permissionsCache = CacheBuilder
+  @Autowired
+  public FiatPermissionEvaluator(Registry registry,
+                                 FiatService fiatService,
+                                 FiatClientConfigurationProperties configProps) {
+    this.registry = registry;
+    this.fiatService = fiatService;
+    this.configProps = configProps;
+
+    this.permissionsCache = CacheBuilder
         .newBuilder()
         .maximumSize(configProps.getCache().getMaxEntries())
         .expireAfterWrite(configProps.getCache().getExpiresAfterWriteSeconds(), TimeUnit.SECONDS)
         .recordStats()
         .build();
+
+    this.getPermissionCounterId = registry.createId("fiat.getPermission");
   }
 
   @Override
@@ -87,12 +86,12 @@ public boolean hasPermission(Authentication authentication,
                                Serializable resourceName,
                                String resourceType,
                                Object authorization) {
-    if (!Boolean.valueOf(fiatEnabled)) {
+    if (!configProps.isEnabled()) {
       return true;
     }
     if (resourceName == null || resourceType == null || authorization == null) {
       log.debug("Permission denied due to null argument. resourceName={}, resourceType={}, " +
-                    "authorization={}", resourceName, resourceType, authorization);
+          "authorization={}", resourceName, resourceType, authorization);
       return false;
     }
 
@@ -104,7 +103,7 @@ public boolean hasPermission(Authentication authentication,
     }
 
     if (r == ResourceType.APPLICATION && StringUtils.isNotEmpty(resourceName.toString())) {
-        resourceName = resourceName.toString();
+      resourceName = resourceName.toString();
     }
 
     UserPermission.View permission = getPermission(getUsername(authentication));
@@ -124,61 +123,48 @@ private String getUsername(Authentication authentication) {
     return username;
   }
 
-  private boolean isAuthorized(String username,
-                               ResourceType resourceType,
-                               String resourceName,
-                               Authorization a) {
-    try {
-      AuthenticatedRequest.propagate(() ->
-        fiatService.hasAuthorization(username, resourceType.toString(), resourceName, a.toString())
-      ).call();
-    } catch (Exception e) {
-      String message = String.format("Fiat authorization failed for user '%s' '%s'-ing '%s' resourceType named '%s'. Cause: %s",
-                                     username,
-                                     a,
-                                     resourceType,
-                                     resourceName,
-                                     e.getMessage());
-      if (log.isDebugEnabled()) {
-        log.debug(message, e);
-      } else {
-        log.info(message);
-      }
-      return false;
-    }
-    return true;
-  }
-
   public UserPermission.View getPermission(String username) {
     UserPermission.View view = null;
     if (StringUtils.isEmpty(username)) {
       return null;
     }
 
+    AtomicBoolean cacheHit = new AtomicBoolean(true);
+    boolean successfulLookup = true;
+
     try {
-      AtomicBoolean cacheHit = new AtomicBoolean(true);
       view = permissionsCache.get(username, () -> {
         cacheHit.set(false);
         return AuthenticatedRequest.propagate(() -> fiatService.getUserPermission(username)).call();
       });
-      log.debug("Fiat permission cache hit: " + cacheHit.get());
     } catch (ExecutionException | UncheckedExecutionException ee) {
-      String message = String.format("Cannot get whole user permission for user %s. Cause: %s",
-                                     username,
-                                     ee.getCause().getMessage());
+      successfulLookup = false;
+
+      String message = String.format(
+          "Cannot get whole user permission for user %s. Cause: %s",
+          username,
+          ee.getCause().getMessage()
+      );
       if (log.isDebugEnabled()) {
         log.debug(message, ee.getCause());
       } else {
         log.info(message);
       }
     }
+
+    registry.counter(
+        getPermissionCounterId
+            .withTag("cached", cacheHit.get())
+            .withTag("success", successfulLookup)
+    ).increment();
+
     return view;
   }
 
   @SuppressWarnings("unused")
   @Deprecated
   public boolean storeWholePermission() {
-    if (!Boolean.valueOf(fiatEnabled)) {
+    if (!configProps.isEnabled()) {
       return true;
     }
 
@@ -209,8 +195,8 @@ private boolean permissionContains(UserPermission.View permission,
         return containsAuth.apply(permission.getApplications());
       case SERVICE_ACCOUNT:
         return permission.getServiceAccounts()
-                         .stream()
-                         .anyMatch(view -> view.getName().equalsIgnoreCase(resourceName));
+            .stream()
+            .anyMatch(view -> view.getName().equalsIgnoreCase(resourceName));
       default:
         return false;
     }

diff --git a/...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.fiat.shared
 
+import com.netflix.spectator.api.NoopRegistry
+import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.fiat.model.Authorization
 import com.netflix.spinnaker.fiat.model.UserPermission
 import com.netflix.spinnaker.fiat.model.resources.Application
@@ -25,27 +27,20 @@ import com.netflix.spinnaker.fiat.model.resources.Role
 import com.netflix.spinnaker.fiat.model.resources.ServiceAccount
 import org.springframework.security.core.Authentication
 import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken
-import spock.lang.Shared
 import spock.lang.Specification
+import spock.lang.Subject
 import spock.lang.Unroll
 
 class FiatPermissionEvaluatorSpec extends Specification {
+  FiatService fiatService = Mock(FiatService)
+  Registry registry = new NoopRegistry();
 
-  @Shared
-  FiatPermissionEvaluator evaluator
-
-  @Shared
-  FiatService fiatService
-
-  def setup() {
-    FiatClientConfigurationProperties configProps = new FiatClientConfigurationProperties()
-    configProps.cache.maxEntries = 0
-    fiatService = Mock(FiatService)
-    evaluator = new FiatPermissionEvaluator(fiatEnabled: true,
-                                            fiatService: fiatService,
-                                            configProps: configProps)
-    evaluator.afterPropertiesSet()
-  }
+  @Subject
+  FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator(
+      registry,
+      fiatService,
+      buildConfigurationProperties()
+  )
 
   @Unroll
   def "should parse application name"() {
@@ -143,4 +138,12 @@ class FiatPermissionEvaluatorSpec extends Specification {
     1 * fiatService.getUserPermission("testUser") >> upv
     hasPermission
   }
+
+  private static FiatClientConfigurationProperties buildConfigurationProperties() {
+    FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties();
+    configurationProperties.enabled = true
+    configurationProperties.cache.maxEntries = 0
+
+    return configurationProperties
+  }
 }
diff --git a/...eb/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java b/...eb/src/main/java/com/netflix/spinnaker/fiat/config/FiatServerConfigurationProperties.java
@@ -28,6 +28,8 @@ public class FiatServerConfigurationProperties {
    */
   private boolean getAllEnabled = false;
 
+  private boolean defaultToUnrestrictedUser = false;
+
   private WriteMode writeMode = new WriteMode();
 
   @Data