feat(logging): Adds user header propagation to Fiat API, which is the…

…n optionally logged via MDC (#184)
spinnaker · Jun 19, 2017 · 2dfa632 · 2dfa632
1 parent af0040c
commit 2dfa632
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 14 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.87.0'
+    spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.93.0'
   }
 
   def checkLocalVersions = [spinnakerDependenciesVersion: spinnakerDependenciesVersion]

diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle
@@ -20,6 +20,7 @@ dependencies {
   compileOnly spinnaker.dependency("bootWeb")
   compileOnly spinnaker.dependency("frigga")
   compileOnly spinnaker.dependency("korkSecurity")
+  compileOnly spinnaker.dependency("korkWeb")
   compileOnly spinnaker.dependency("lombok")
   compileOnly spinnaker.dependency("okHttp")
   compileOnly spinnaker.dependency("okHttpUrlconnection")

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
@@ -18,6 +18,7 @@
 
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
 import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
 import lombok.val;
@@ -53,16 +54,21 @@ public class FiatAuthenticationConfig {
   @Setter
   private RestAdapter.LogLevel retrofitLogLevel = RestAdapter.LogLevel.BASIC;
 
+  @Autowired
+  SpinnakerRequestInterceptor spinnakerRequestInterceptor;
+
   @Bean
   @ConditionalOnMissingBean(FiatService.class) // Allows for override
   public FiatService fiatService(FiatClientConfigurationProperties fiatConfigurationProperties,
+                                 SpinnakerRequestInterceptor interceptor,
                                  OkClient okClient) {
     // 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()))
+        .setRequestInterceptor(interceptor)
         .setClient(okClient)
         .setConverter(new JacksonConverter(objectMapper))
         .setLogLevel(retrofitLogLevel)

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
@@ -24,6 +24,7 @@
 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;
@@ -40,6 +41,7 @@
 
 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;
@@ -130,16 +132,18 @@ private boolean isAuthorized(String username,
                                String resourceName,
                                Authorization a) {
     try {
-      fiatService.hasAuthorization(username, resourceType.toString(), resourceName, a.toString());
-    } catch (RetrofitError re) {
+      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,
-                                     re.getMessage());
+                                     e.getMessage());
       if (log.isDebugEnabled()) {
-        log.debug(message, re);
+        log.debug(message, e);
       } else {
         log.info(message);
       }
@@ -158,7 +162,7 @@ public UserPermission.View getPermission(String username) {
       AtomicBoolean cacheHit = new AtomicBoolean(true);
       view = permissionsCache.get(username, () -> {
         cacheHit.set(false);
-        return fiatService.getUserPermission(username);
+        return AuthenticatedRequest.propagate(() -> fiatService.getUserPermission(username)).call();
       });
       log.debug("Fiat permission cache hit: " + cacheHit.get());
     } catch (ExecutionException | UncheckedExecutionException ee) {

diff --git a/fiat-web/config/fiat.yml b/fiat-web/config/fiat.yml
@@ -14,9 +14,5 @@ endpoints:
 
 # For options, see com.netflix.spinnaker.fiat.config.FiatServerConfigurationProperties
 
-fiat:
-  permissions:
-    read:
-      - foo
-    write:
-      - bar
+logging:
+  config: classpath:logback-defaults.xml
diff --git a/fiat-web/fiat-web.gradle b/fiat-web/fiat-web.gradle
@@ -32,7 +32,6 @@ dependencies {
   compile project(':fiat-github')
   compile project(':fiat-google-groups')
   compile project(':fiat-ldap')
-  compile project(':fiat-api')
 
   testCompile spinnaker.dependency('korkJedisTest')
   testCompile "org.skyscreamer:jsonassert:1.3.0"

diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatConfig.java
@@ -4,13 +4,17 @@
 import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.fiat.model.resources.Role;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
+import com.netflix.spinnaker.filters.AuthenticatedRequestFilter;
 import com.netflix.spinnaker.kork.web.interceptors.MetricsInterceptor;
+import lombok.val;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
+import org.springframework.boot.context.embedded.FilterRegistrationBean;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.core.Ordered;
 import org.springframework.http.MediaType;
 import org.springframework.web.servlet.config.annotation.ContentNegotiationConfigurer;
 import org.springframework.web.servlet.config.annotation.InterceptorRegistry;
@@ -62,4 +66,15 @@ public List<Role> loadRoles(String userId) {
       }
     };
   }
+
+  /**
+   * This AuthenticatedRequestFilter pulls the email and accounts out of the Spring
+   * security context in order to enabling forwarding them to downstream components.
+   */
+  @Bean
+  FilterRegistrationBean authenticatedRequestFilter() {
+    val frb = new FilterRegistrationBean(new AuthenticatedRequestFilter(true));
+    frb.setOrder(Ordered.LOWEST_PRECEDENCE);
+    return frb;
+  }
 }
diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java
@@ -26,6 +26,8 @@
 import com.netflix.spinnaker.fiat.model.resources.Role;
 import com.netflix.spinnaker.fiat.permissions.PermissionsRepository;
 import io.swagger.annotations.ApiOperation;
+import lombok.extern.slf4j.Slf4j;
+import lombok.val;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -38,6 +40,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 
+@Slf4j
 @RestController
 @RequestMapping("/authorize")
 public class AuthorizeController {
@@ -57,6 +60,7 @@ public Set<UserPermission.View> getAll(HttpServletResponse response) throws IOEx
       return null;
     }
 
+    log.debug("UserPermissions requested for all users");
     return permissionsRepository
         .getAllById()
         .values()
@@ -67,7 +71,9 @@ public Set<UserPermission.View> getAll(HttpServletResponse response) throws IOEx
 
   @RequestMapping(value = "/{userId:.+}", method = RequestMethod.GET)
   public UserPermission.View getUserPermission(@PathVariable String userId) {
-    return permissionsRepository.get(ControllerSupport.convert(userId))
+    val user = ControllerSupport.convert(userId);
+    log.debug("UserPermission requested for " + user);
+    return permissionsRepository.get(user)
                                 .orElseThrow(NotFoundException::new)
                                 .getView();
   }

diff --git a/fiat-web/src/main/resources/logback-defaults.xml b/fiat-web/src/main/resources/logback-defaults.xml
@@ -0,0 +1,14 @@
+<configuration>
+    <conversionRule conversionWord="clr" converterClass="org.springframework.boot.logging.logback.ColorConverter" />
+
+    <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
+        <encoder>
+            <pattern>%clr(%d{yyyy-MM-dd HH:mm:ss.SSS}){faint} %clr(%5p) %clr(${PID:- }){magenta} %clr(---){faint} %clr([%15.15t]){faint} %clr(%-40.40logger{39}){cyan} %clr(:){faint} [%X{X-SPINNAKER-USER}] %m%n
+            </pattern>
+        </encoder>
+    </appender>
+
+    <root level="INFO">
+        <appender-ref ref="STDOUT" />
+    </root>
+</configuration>