Fix and refactor role based filter to allow CORS #99

garyluoex · 2016-11-17T00:02:09Z

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

cdeszaq

Needs CHANGELOG entry

cdeszaq · 2016-11-17T15:45:02Z

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

+        return allowedUserRoles.isEmpty() ? true : allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);
+    }
+
+    /**\


s// (ie. typo... \ shouldn't be there)

cdeszaq · 2016-11-17T15:47:03Z

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

        }
    }

    /**
+     * Request authorization logic.


It would be good to highlight what that logic is, rather than forcing the user to read the code to understand it. In particular: If the HTTP method is OPTIONS or if the user has a role that's in the configured allowed list

cdeszaq · 2016-11-17T15:47:36Z

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

+    }
+
+    /**\
+     *  By default, returns true to bypass the filter if the request is a Http OPTIONS method to allow CORS.


No need to have byDefault, since this method (being private) can't be overridden.

Making it protected to allow for override

cdeszaq · 2016-11-17T16:11:57Z

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

-        // Release the test web container
-        jtb.tearDown()
+        roleBasedAuthFilter = new RoleBasedAuthFilter()
+        SYSTEM_CONFIG.setProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"), "foo,bar");


We need to make sure this gets cleaned up and re-set to it's original value. Property leaks are SUCH a pain to find.

cdeszaq · 2016-11-17T16:12:14Z

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

-    }
+    ContainerRequestContext containerRequestContext;
+    SecurityContext securityContext;
+    RoleBasedAuthFilter roleBasedAuthFilter;


No semicolons in groovy

cdeszaq · 2016-11-17T16:17:18Z

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

+    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"
+        containerRequestContext.getMethod() >> "GET"
+        securityContext.isUserInRole() >> false


This mock won't work as you're expecting. It shouldn't ever trigger because it's not valid to call that method with no parameters. If you want the mock to trigger on all calls to that method, you should use the wildcard parameter syntax: securityContext.isUserInRole(*_)

cdeszaq · 2016-11-17T16:17:44Z

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

+        when: "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


See previous note on making this trigger the way I think you want it to.

cdeszaq · 2016-11-17T16:19:07Z

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

+        securityContext.isUserInRole() >> false
+
+        then: "the request is not authorized"
+        roleBasedAuthFilter.isAuthorized(containerRequestContext) == false


These assertions can be simplified, since they are all pretty much boolean responses. Instead of == false, you can just prepend ! to the call, and you don't need == true if you're asserting truth.

cdeszaq · 2016-11-17T16:21:02Z

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

+        containerRequestContext.getMethod() >> "GET"
+        securityContext.isUserInRole() >> false
+
+        then: "the request is not authorized"


None of these tests are "stimulus / assertion on the state of the world" tests, but instead they are all "given the state of the world, assert a behavior", which lends itself much more to the given / expect style of tests, rather than the when / then style. So, it would be good to convert all of these tests to given / expect

cdeszaq · 2016-11-17T16:23:21Z

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

-        RoleBasedAuthFilter roleBasedAuthFilter = new RoleBasedAuthFilter()
+    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


May not be triggering when you expect it to

cdeszaq

CHANGELOG still needed

cdeszaq · 2016-11-17T17:33:29Z

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

+     */
+    protected boolean isUserInRole (List<String> allowedUserRoles, ContainerRequestContext containerRequestContext) {
+        SecurityContext securityContext = containerRequestContext.getSecurityContext();
+        return allowedUserRoles.isEmpty() ? true : allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);


Can simplify this using || like so: allowedUserRoles.isEmpty() || allowedUserRoles.stream().anyMatch(securityContext::isUserInRole);

cdeszaq · 2016-11-17T17:35:07Z

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

    }

    def cleanup() {
-        // Release the test web container
-        jtb.tearDown()
+        SYSTEM_CONFIG.clearProperty(SYSTEM_CONFIG.getPackageVariableName("user_roles"))


We can't assume that this was unset when the test started. We need to capture the value of the property (as a String) in the setup, and then use that captured value to reset(capturedValue) in the cleanup.

cdeszaq

Just those last three small things and this will be ready!

cdeszaq · 2016-11-17T18:19:52Z

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

+
+        try {
+            oldUserRoles = Optional.of(SYSTEM_CONFIG.getStringProperty(userRolesSetting))
+        } catch(SystemConfigException e) {


For catch blocks where we're ignoring the exception, it's best to call the exception ignored (or something like that) to make it clear that we're intentionally not doing anything with the exception.

cdeszaq · 2016-11-17T18:20:24Z

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

@@ -15,49 +18,71 @@ import javax.ws.rs.core.SecurityContext
 @Timeout(30)
 class RoleBasedAuthFilterSpec extends Specification {

-    JerseyTestBinder jtb
+    private static final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance()
+    private static final String userRolesSetting = SYSTEM_CONFIG.getPackageVariableName("user_roles")


Since this variable is static, it should be in STATIC_CASE

cdeszaq · 2016-11-17T18:21:01Z

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

-        // Release the test web container
-        jtb.tearDown()
+        // Restore pre-test setting value if exists
+        oldUserRoles.ifPresent({userRoles -> SYSTEM_CONFIG.resetProperty(userRolesSetting, userRoles)})


This needs to clear the setting if the Optional is empty, which this doesn't do.

cdeszaq · 2016-11-17T19:04:01Z

👍

RVRSKumar

Just minor comments

RVRSKumar · 2016-11-18T01:04:11Z

CHANGELOG.md

@@ -28,12 +28,16 @@ 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


Can we expand what CORS is?

I think changelog is not the place to explain things, google CORS will give plenty of answers. What do you think? @cdeszaq

RVRSKumar · 2016-11-18T01:13:39Z

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

+     * @return true if request is authorized, false otherwise
+     */
+    private boolean isAuthorized(ContainerRequestContext containerRequestContext) {
+        return isBypassMethod(containerRequestContext) || isUserInRole(getAllowedUserRoles(), containerRequestContext);


In getAllowedUserRoles() what is the list that is returned when the bard__user_roles variable is uncommented. Is it not an empty list?

returns it is a list with an empty string

This is a good point. It might not be a bad idea to add this behavior description to the getList* methods in the SystemConfig interface (and possibly add a test asserting the behavior).

That said, I'm also OK making this piece an issue and coming back to it.

I was thinking we should just make it a bug issue and fix it later on, because it should be obvious that this behavior is undesirable.

RVRSKumar · 2016-11-18T16:30:34Z

👍

garyluoex added NEED 2 REVIEWS REVIEWABLE labels Nov 17, 2016

garyluoex force-pushed the FixOptions branch 2 times, most recently from 05ab769 to 33f7810 Compare November 17, 2016 00:15

cdeszaq requested changes Nov 17, 2016

View reviewed changes

garyluoex force-pushed the FixOptions branch from 985e810 to cf6928a Compare November 17, 2016 17:35

cdeszaq requested changes Nov 17, 2016

View reviewed changes

cdeszaq added NEED REBASE NEED SQUASH labels Nov 17, 2016

cdeszaq requested changes Nov 17, 2016

View reviewed changes

garyluoex force-pushed the FixOptions branch from 9e641be to bf007a7 Compare November 17, 2016 18:31

garyluoex removed NEED REBASE NEED SQUASH labels Nov 17, 2016

garyluoex force-pushed the FixOptions branch from bf007a7 to ebedd47 Compare November 17, 2016 18:37

Fix and refactor role based filter to allow CORS

26d4ae0

garyluoex force-pushed the FixOptions branch from ebedd47 to 26d4ae0 Compare November 17, 2016 18:45

cdeszaq added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Nov 17, 2016

RVRSKumar reviewed Nov 18, 2016

View reviewed changes

garyluoex added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Nov 18, 2016

garyluoex mentioned this pull request Nov 18, 2016

SystemConfig getListProperty reads empty declared variable as List with one element #101

Open

cdeszaq merged commit e91de00 into master Nov 18, 2016

cdeszaq deleted the FixOptions branch November 19, 2016 16:31

cdeszaq modified the milestone: v0.7.x Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and refactor role based filter to allow CORS #99

Fix and refactor role based filter to allow CORS #99

garyluoex commented Nov 17, 2016 •

edited

cdeszaq left a comment

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

garyluoex Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq left a comment

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq left a comment

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq Nov 17, 2016

cdeszaq commented Nov 17, 2016

RVRSKumar left a comment

RVRSKumar Nov 18, 2016

garyluoex Nov 18, 2016

RVRSKumar Nov 18, 2016

garyluoex Nov 18, 2016

cdeszaq Nov 18, 2016

garyluoex Nov 18, 2016

RVRSKumar commented Nov 18, 2016

Fix and refactor role based filter to allow CORS #99

Fix and refactor role based filter to allow CORS #99

Conversation

garyluoex commented Nov 17, 2016 • edited

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq commented Nov 17, 2016

RVRSKumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RVRSKumar commented Nov 18, 2016

garyluoex commented Nov 17, 2016 •

edited