feat(fiat): for x509 users, add roles read from fiat on login (#816)

spinnaker · Jun 3, 2019 · 41a9c2e · 41a9c2e
1 parent eb7775e
commit 41a9c2e
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 17 deletions.
diff --git a/...oovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy b/...oovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy
@@ -121,8 +121,9 @@ class X509AuthenticationUserDetailsService implements AuthenticationUserDetailsS
 
     if (requiredRoles) {
       if (!requiredRoles.any { it in roles }) {
-        log.debug("User $email does not have all roles $requiredRoles")
-        throw new BadCredentialsException("User $email does not have all roles $requiredRoles")
+        String errorMessage = "User $email with roles $roles does not have any of the required roles $requiredRoles"
+        log.debug(errorMessage)
+        throw new BadCredentialsException(errorMessage)
       }
     }
 
@@ -195,7 +196,13 @@ class X509AuthenticationUserDetailsService implements AuthenticationUserDetailsS
       }
     }
 
-    return roles
+    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
@@ -2,6 +2,7 @@ package com.netflix.spinnaker.gate.security.x509
 
 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.gate.services.PermissionService
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
@@ -36,40 +37,34 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     userDetails.setDynamicConfigService(config)
     userDetails.setFiatPermissionEvaluator(fiatPermissionEvaluator)
     userDetails.registry = registry
+    fiatPermissionEvaluator.getPermission(email) >> view
 
     when: "initial login"
     userDetails.handleLogin(email, cert)
 
     then: "should call login"
-    1 * fiatPermissionEvaluator.getPermission(email) >> view
     1 * perms.login(email)
-    0 * _
 
     when: "subsequent login during debounce window"
     clock.advanceTime(Duration.ofSeconds(30))
     userDetails.handleLogin(email, cert)
 
     then: "should not call login"
-    1 * fiatPermissionEvaluator.getPermission(email) >> view
-    0 * _
+    0 * perms.login(email)
 
     when: "subsequent login after debounce window"
     clock.advanceTime(Duration.ofMinutes(5))
     userDetails.handleLogin(email, cert)
 
     then: "should call login"
-    1 * fiatPermissionEvaluator.getPermission(email) >> view
     1 * perms.login(email)
-    0 * _
 
     when: "login with no cached permission"
+    fiatPermissionEvaluator.getPermission(email) >> null
     userDetails.handleLogin(email, cert)
 
     then: "should call login"
-    1 * fiatPermissionEvaluator.getPermission(email) >> null
     1 * perms.login(email)
-    0 * _
-
   }
 
   def "should always login if debounceDisabled"() {
@@ -92,15 +87,13 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     then:
     1 * config.isEnabled('x509.loginDebounce', _) >> false
     1 * perms.login(email)
-    0 * _
 
     when:
     userDetails.handleLogin(email, cert)
 
     then:
     1 * config.isEnabled('x509.loginDebounce', _) >> false
     1 * perms.login(email)
-    0 * _
 
     when:
     clock.advanceTime(Duration.ofSeconds(30))
@@ -109,7 +102,6 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     then:
     1 * config.isEnabled('x509.loginDebounce', _) >> false
     1 * perms.login(email)
-    0 * _
   }
 
   def "should loginWithRoles if roleExtractor provided"() {
@@ -128,14 +120,19 @@ class X509AuthenticationUserDetailsServiceSpec extends Specification {
     userDetails.setFiatPermissionEvaluator(fiatPermissionEvaluator)
     userDetails.registry = registry
 
+    def view = new UserPermission(id: email, roles: [new Role('bish')]).view
+    fiatPermissionEvaluator.getPermission(email) >> view
+
     when:
-    userDetails.handleLogin(email, cert)
+    def 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 * _
+
+    and: 'the roles retrieved from fiatPermissionEvaluator are also added to the list of returned roles'
+    roles == [email, 'foo', 'bar', 'bish']
   }