modified again according to rick's comment

yahoo · Nov 17, 2016 · 9e641be · 9e641be
1 parent cf6928a
commit 9e641be
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,7 +24,7 @@ Current
 
 - [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)
+    * Discovered a bug where `user_roles` is declared but unset still reads as a list with empty string (included a temporary fix by commenting the variable declaration)
     * Refactored `RoleBasedAuthFilter` and `RoleBasedAuthFilterSpec` for better testing
 
 ### Known Issues:

diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilter.java b/fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilter.java
@@ -94,7 +94,7 @@ protected boolean isBypassMethod(ContainerRequestContext containerRequestContext
      */
     protected boolean isUserInRole (List<String> allowedUserRoles, ContainerRequestContext containerRequestContext) {
         SecurityContext securityContext = containerRequestContext.getSecurityContext();
-        return allowedUserRoles.isEmpty() ? true : allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);
+        return allowedUserRoles.isEmpty() || allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);
     }
 
     /**

diff --git a/...core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy b/...core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy
@@ -3,6 +3,7 @@
 package com.yahoo.bard.webservice.web.filters;
 
 import com.yahoo.bard.webservice.config.SystemConfig
+import com.yahoo.bard.webservice.config.SystemConfigException
 import com.yahoo.bard.webservice.config.SystemConfigProvider
 
 import com.fasterxml.jackson.databind.ObjectMapper
@@ -17,7 +18,11 @@ import javax.ws.rs.core.SecurityContext
 @Timeout(30)
 class RoleBasedAuthFilterSpec extends Specification {
 
-    private final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance()
+    private static final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance()
+    private static final String userRolesSetting = SYSTEM_CONFIG.getPackageVariableName("user_roles")
+
+    // Memorize pre-test setting value
+    Optional<String> oldUserRoles
 
     ObjectMapper objectMapper
     ContainerRequestContext containerRequestContext
@@ -29,13 +34,19 @@ class RoleBasedAuthFilterSpec extends Specification {
         containerRequestContext = Mock(ContainerRequestContext)
         securityContext = Mock(SecurityContext)
         containerRequestContext.getSecurityContext() >> securityContext
-
         roleBasedAuthFilter = new RoleBasedAuthFilter(objectMapper)
-        SYSTEM_CONFIG.setProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"), "foo,bar");
+
+        try {
+            oldUserRoles = Optional.of(SYSTEM_CONFIG.getStringProperty(userRolesSetting))
+        } catch(SystemConfigException e) {
+            oldUserRoles = Optional.empty()
+        }
+        SYSTEM_CONFIG.setProperty(userRolesSetting, "foo,bar");
     }
 
     def cleanup() {
-        SYSTEM_CONFIG.clearProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"))
+        // Restore pre-test setting value if exists
+        oldUserRoles.ifPresent({userRoles -> SYSTEM_CONFIG.resetProperty(userRolesSetting, userRoles)})
     }
 
     def "GET request is not authorized if the user is not in an allowed role"() {