fix(oauth2): Handle oddly-formatted roles userInfo (#1200)

* Update SpinnakerUserInfoTokenServices.groovy * fix(oauth2): Remove handling of curly bracket * Add check for instance type * Add test
spinnaker · May 19, 2020 · f4d43a1 · f4d43a1
1 parent 62ad7ff
commit f4d43a1
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 12 deletions.
diff --git a/...n/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy b/...n/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy
@@ -25,6 +25,7 @@ import com.netflix.spinnaker.gate.services.PermissionService
 import com.netflix.spinnaker.gate.services.internal.Front50Service
 import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.security.User
+import groovy.json.JsonSlurper
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.boot.autoconfigure.security.oauth2.resource.ResourceServerProperties
@@ -237,6 +238,11 @@ class SpinnakerUserInfoTokenServices implements ResourceServerTokenServices {
     }
     def roles = details[userInfoMapping.roles] ?: []
     if (roles instanceof Collection) {
+      // Some providers (Azure AD) return roles in this format: ["[\"role-1\", \"role-2\"]"]
+      if (roles[0] instanceof String && roles[0][0] == '[') {
+        def js = new JsonSlurper()
+        return js.parseText(roles).flatten() as List<String>
+      }
       return roles as List<String>
     }
     if (roles instanceof String) {

diff --git a/...oovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServicesSpec.groovy b/...oovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServicesSpec.groovy
@@ -91,17 +91,20 @@ class SpinnakerUserInfoTokenServicesSpec extends Specification {
     tokenServices.getRoles(details) == expectedRoles
 
     where:
-    rolesValue        || expectedRoles
-    null              || []
-    ""                || []
-    ["foo", "bar"]    || ["foo", "bar"]
-    "foo,bar"         || ["foo", "bar"]
-    "foo bar"         || ["foo", "bar"]
-    "foo"             || ["foo"]
-    "foo   bar"       || ["foo", "bar"]
-    "foo,,,bar"       || ["foo", "bar"]
-    "foo, bar"        || ["foo", "bar"]
-    1                 || []
-    [blergh: "blarg"] || []
+    rolesValue                || expectedRoles
+    null                      || []
+    ""                        || []
+    ["foo", "bar"]            || ["foo", "bar"]
+    "foo,bar"                 || ["foo", "bar"]
+    "foo bar"                 || ["foo", "bar"]
+    "foo"                     || ["foo"]
+    "foo   bar"               || ["foo", "bar"]
+    "foo,,,bar"               || ["foo", "bar"]
+    "foo, bar"                || ["foo", "bar"]
+    ["[]"]                    || []
+    ["[\"foo\"]"]             || ["foo"]
+    ["[\"foo\", \"bar\"]"]    || ["foo", "bar"]
+    1                         || []
+    [blergh: "blarg"]         || []
   }
 }