fix(x509): check if fiat is enabled before fetching permissions on x5…

…09 login (#875) (#876)
spinnaker · Aug 15, 2019 · a9ee8eb · a9ee8eb
1 parent 0743b2c
commit a9ee8eb
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 7 deletions.
diff --git a/...oovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy b/...oovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy
@@ -23,6 +23,7 @@ import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.fiat.model.UserPermission
 import com.netflix.spinnaker.fiat.shared.FiatClientConfigurationProperties
 import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator
+import com.netflix.spinnaker.fiat.shared.FiatStatus
 import com.netflix.spinnaker.gate.security.AllowedAccountsSupport
 import com.netflix.spinnaker.gate.services.PermissionService
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
@@ -77,6 +78,9 @@ class X509AuthenticationUserDetailsService implements AuthenticationUserDetailsS
   @Autowired
   FiatClientConfigurationProperties fiatClientConfigurationProperties
 
+  @Autowired
+  FiatStatus fiatStatus
+
   @Autowired
   Registry registry
 
@@ -196,11 +200,13 @@ class X509AuthenticationUserDetailsService implements AuthenticationUserDetailsS
       }
     }
 
-    def permission = fiatPermissionEvaluator.getPermission(email)
-    def roleNames = permission?.getRoles()?.collect { it -> it.getName()}
-    log.debug("Extracted roles from fiat permissions for user {}: {}", email, roleNames)
-    if (roleNames) {
-      roles.addAll(roleNames)
+    if (fiatStatus.isEnabled()) {
+      def permission = fiatPermissionEvaluator.getPermission(email)
+      def roleNames = permission?.getRoles()?.collect { it -> it.getName() }
+      log.debug("Extracted roles from fiat permissions for user {}: {}", email, roleNames)
+      if (roleNames) {
+        roles.addAll(roleNames)
+      }
     }
     return roles.unique(/* mutate = */false)
   }

diff --git a/.../com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsServiceSpec.groovy b/.../com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsServiceSpec.groovy
@@ -4,6 +4,7 @@ import com.netflix.spectator.api.NoopRegistry
 import com.netflix.spinnaker.fiat.model.UserPermission
 import com.netflix.spinnaker.fiat.model.resources.Role
 import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator
+import com.netflix.spinnaker.fiat.shared.FiatStatus
 import com.netflix.spinnaker.gate.services.PermissionService
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import spock.lang.Specification
@@ -33,11 +34,14 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     def cert = Mock(X509Certificate)
     def userDetails = new X509AuthenticationUserDetailsService(clock)
     def fiatPermissionEvaluator = Mock(FiatPermissionEvaluator)
+    def fiatStatus = Mock(FiatStatus)
     userDetails.setPermissionService(perms)
     userDetails.setDynamicConfigService(config)
     userDetails.setFiatPermissionEvaluator(fiatPermissionEvaluator)
     userDetails.registry = registry
+    userDetails.setFiatStatus(fiatStatus)
     fiatPermissionEvaluator.getPermission(email) >> view
+    fiatStatus.isEnabled() >> true
 
     when: "initial login"
     userDetails.handleLogin(email, cert)
@@ -76,9 +80,11 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     def cert = Mock(X509Certificate)
     def userDetails = new X509AuthenticationUserDetailsService(clock)
     def fiatPermissionEvaluator = Mock(FiatPermissionEvaluator)
+    def fiatStatus = Mock(FiatStatus)
     userDetails.setPermissionService(perms)
     userDetails.setDynamicConfigService(config)
     userDetails.setFiatPermissionEvaluator(fiatPermissionEvaluator)
+    userDetails.setFiatStatus(fiatStatus)
     userDetails.registry = registry
 
     when:
@@ -114,25 +120,42 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     def rolesExtractor = Mock(X509RolesExtractor)
     def userDetails = new X509AuthenticationUserDetailsService(clock)
     def fiatPermissionEvaluator = Mock(FiatPermissionEvaluator)
+    def fiatStatus = Mock(FiatStatus)
+    def roles
     userDetails.setPermissionService(perms)
     userDetails.setDynamicConfigService(config)
     userDetails.setRolesExtractor(rolesExtractor)
     userDetails.setFiatPermissionEvaluator(fiatPermissionEvaluator)
+    userDetails.setFiatStatus(fiatStatus)
     userDetails.registry = registry
 
     def view = new UserPermission(id: email, roles: [new Role('bish')]).view
-    fiatPermissionEvaluator.getPermission(email) >> view
 
     when:
-    def roles = userDetails.handleLogin(email, cert)
+    fiatStatus.isEnabled() >> true
+    roles = userDetails.handleLogin(email, cert)
 
     then:
     1 * rolesExtractor.fromCertificate(cert) >> ['foo', 'bar']
     1 * config.isEnabled('x509.loginDebounce', _) >> false
     1 * perms.loginWithRoles(email, [email, 'foo', 'bar'])
+    1 * fiatPermissionEvaluator.getPermission(email) >> view
 
     and: 'the roles retrieved from fiatPermissionEvaluator are also added to the list of returned roles'
     roles == [email, 'foo', 'bar', 'bish']
+
+    when:
+    fiatStatus.isEnabled() >> false
+    roles = userDetails.handleLogin(email, cert)
+
+    then:
+    1 * rolesExtractor.fromCertificate(cert) >> ['foo', 'bar']
+    1 * config.isEnabled('x509.loginDebounce', _) >> false
+    1 * perms.loginWithRoles(email, [email, 'foo', 'bar'])
+    0 * fiatPermissionEvaluator.getPermission(email) >> view
+
+    and: 'no roles are retrieved from fiatPermissionEvaluator when fiat is disabled'
+    roles == [email, 'foo', 'bar']
   }