diff --git a/CHANGELOG.md b/CHANGELOG.md index e2bbc47699..ea603bac17 100644 --- a/CHANGELOG.md +++ b/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: 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 index 36640deb06..aa5f2787c2 100644 --- 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 @@ -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 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 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 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); - } } diff --git a/fili-core/src/main/resources/moduleConfig.properties b/fili-core/src/main/resources/moduleConfig.properties index c80d423e9d..3c754e84bc 100644 --- a/fili-core/src/main/resources/moduleConfig.properties +++ b/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. diff --git a/fili-core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy b/fili-core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy index 5ca65dbbbd..02a462da78 100644 --- a/fili-core/src/test/groovy/com/yahoo/bard/webservice/web/filters/RoleBasedAuthFilterSpec.groovy +++ b/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) } }