modified according to rick's comment

CHANGELOG.md

@@ -22,7 +22,10 @@ Current
 
 ### Fixed:
 
-
+- [Fix and refactor role based filter to allow CORS](https://github.com/yahoo/fili/pull/99)
+    * Fix `RoleBasedAuthFilter` to bypass `OPTIONS` request for CORS
+    * Discovered a bug where `user_roles` is declared but unset still reads as a list with empty string (included a temporary fix by deleting the variable declaration)
+    * Refactored `RoleBasedAuthFilter` and `RoleBasedAuthFilterSpec` for better testing
 
 ### Known Issues:
 

fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilter.java

@@ -62,7 +62,8 @@ public void filter(ContainerRequestContext containerRequestContext) throws IOExc
     }
 
     /**
-     * Request authorization logic.
+     * Request authorization logic, bypassing OPTIONS request and authorize if user is in authorized role or
+     * authorized role is not specified.
      *
      * @param containerRequestContext containing the request context information
      *
@@ -72,6 +73,30 @@ private boolean isAuthorized(ContainerRequestContext containerRequestContext) {
         return isBypassMethod(containerRequestContext) || isUserInRole(getAllowedUserRoles(), containerRequestContext);
     }
 
+    /**
+     * Returns true to bypass the filter if the request is a Http OPTIONS method to allow CORS by default.
+     *
+     * @param containerRequestContext Request context containing request method information
+     *
+     * @return true to bypass the filter, and false otherwise
+     */
+    protected boolean isBypassMethod(ContainerRequestContext containerRequestContext) {
+        return containerRequestContext.getMethod().equals(HttpMethod.OPTIONS);
+    }
+
+    /**
+     * Checks if an user role belongs to the list of allowed roles.
+     *
+     * @param allowedUserRoles List of allowed roles
+     * @param containerRequestContext Request context contains the user role information
+     *
+     * @return boolean based on the user role
+     */
+    protected boolean isUserInRole (List<String> allowedUserRoles, ContainerRequestContext containerRequestContext) {
+        SecurityContext securityContext = containerRequestContext.getSecurityContext();
+        return allowedUserRoles.isEmpty() ? true : allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);
+    }
+
     /**
      * Abort the given request. Re-direct not needed as the user is already assumed to be authenticated.
      *
@@ -113,28 +138,4 @@ private List<String> getAllowedUserRoles() {
             return Collections.emptyList();
         }
     }
-
-    /**
-     * Checks if an user role belongs to the list of allowed roles.
-     *
-     * @param allowedUserRoles List of allowed roles
-     * @param containerRequestContext Request context contains the user role information
-     *
-     * @return boolean based on the user role
-     */
-    public boolean isUserInRole (List<String> allowedUserRoles, ContainerRequestContext containerRequestContext) {
-        SecurityContext securityContext = containerRequestContext.getSecurityContext();
-        return allowedUserRoles.isEmpty() ? true : allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);
-    }
-
-    /**\
-     *  By default, returns true to bypass the filter if the request is a Http OPTIONS method to allow CORS.
-     *
-     * @param containerRequestContext Request context containing request method information
-     *
-     * @return true to bypass the filter, and false otherwise
-     */
-    private boolean isBypassMethod(ContainerRequestContext containerRequestContext) {
-        return containerRequestContext.getMethod().equals(HttpMethod.OPTIONS);
-    }
 }

fili-core/src/main/resources/moduleConfig.properties

@@ -104,6 +104,9 @@ bard__updated_metadata_collection_names_enabled = false
 # change the predefined order of Durations, Threads, Preface at the start and of Epilogue at the end of the log line.
 # bard__requestlog_loginfo_order = BardQueryInfo,DataRequest,DimensionRequest,MetricRequest,SliceRequest,TableRequest,FeatureFlagRequest,DruidResponse
 
+# List of allowed user roles, must provide value if uncommented
+# bard__user_roles=
+
 # Default wait to use when no asyncAfter parameter is specified. It can be the String 'never' (all requests are
 #synchronous), the String 'always' (all requests are asynchronous) or a time duration in milliseconds.
 # The asyncAfter parameter describes how long users should wait before a synchronous request becomes asynchronous.

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy

@@ -5,6 +5,8 @@ package com.yahoo.bard.webservice.web.filters;
 import com.yahoo.bard.webservice.config.SystemConfig
 import com.yahoo.bard.webservice.config.SystemConfigProvider
 
+import com.fasterxml.jackson.databind.ObjectMapper
+
 import spock.lang.Specification
 import spock.lang.Timeout
 
@@ -17,53 +19,59 @@ class RoleBasedAuthFilterSpec extends Specification {
 
     private final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance()
 
-    ContainerRequestContext containerRequestContext;
-    SecurityContext securityContext;
-    RoleBasedAuthFilter roleBasedAuthFilter;
+    ObjectMapper objectMapper
+    ContainerRequestContext containerRequestContext
+    SecurityContext securityContext
+    RoleBasedAuthFilter roleBasedAuthFilter
 
     def setup() {
+        objectMapper = Mock(ObjectMapper)
         containerRequestContext = Mock(ContainerRequestContext)
         securityContext = Mock(SecurityContext)
         containerRequestContext.getSecurityContext() >> securityContext
 
-        roleBasedAuthFilter = new RoleBasedAuthFilter()
+        roleBasedAuthFilter = new RoleBasedAuthFilter(objectMapper)
         SYSTEM_CONFIG.setProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"), "foo,bar");
     }
 
+    def cleanup() {
+        SYSTEM_CONFIG.clearProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"))
+    }
+
     def "GET request is not authorized if the user is not in an allowed role"() {
-        when: "received a request and that the user is not in the authorized roles"
+        given: "received a request and that the user is not in the authorized roles"
         containerRequestContext.getMethod() >> "GET"
-        securityContext.isUserInRole() >> false
+        securityContext.isUserInRole(_) >> false
 
-        then: "the request is not authorized"
-        roleBasedAuthFilter.isAuthorized(containerRequestContext) == false
+        expect: "the request is not authorized"
+        !roleBasedAuthFilter.isAuthorized(containerRequestContext)
     }
 
     def "GET request is authorized if the user is in an allowed role"() {
-        when: "received a request and that the user is in the authorized roles"
+        given: "received a request and that the user is in the authorized roles"
         containerRequestContext.getMethod() >> "GET"
         securityContext.isUserInRole("bar") >> true
 
-        then: "the request is authorized"
-        roleBasedAuthFilter.isAuthorized(containerRequestContext) == true
+        expect: "the request is authorized"
+        roleBasedAuthFilter.isAuthorized(containerRequestContext)
     }
 
     def "GET request is authorized if the `user_roles` setting is not set"() {
-        when: "received a request and that the user is not in the authorized roles"
+        given: "received a request and that the user is not in the authorized roles"
         containerRequestContext.getMethod() >> "GET"
         SYSTEM_CONFIG.clearProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"))
-        securityContext.isUserInRole() >> false
+        securityContext.isUserInRole(_) >> false
 
-        then: "the request is authorized"
-        roleBasedAuthFilter.isAuthorized(containerRequestContext) == true
+        expect: "the request is authorized"
+        roleBasedAuthFilter.isAuthorized(containerRequestContext)
     }
 
     def "OPTIONS request is authorized which bypasses the filter"() {
-        when: "received an OPTIONS request and that the user is in the authorized roles"
-        securityContext.isUserInRole() >> false
+        given: "received an OPTIONS request and that the user is in the authorized roles"
+        securityContext.isUserInRole(_) >> false
         containerRequestContext.getMethod() >> "OPTIONS"
 
-        then: "the request is authorized"
-        roleBasedAuthFilter.isAuthorized(containerRequestContext) == true
+        expect: "the request is authorized"
+        roleBasedAuthFilter.isAuthorized(containerRequestContext)
     }
 }