Juan Hernandez has uploaded a new change for review. Change subject: core: Parse Prefer according to RFC 7240 ......................................................................
core: Parse Prefer according to RFC 7240 Currently we assume that the "Prefer" header contains only the value "persistent-auth" and we check it directly. This prevents using the header for other purposes, like including several preferences in the same header, as described in RFC 7240. This will be required by later patches, in particular for the patches implementing CSRF protection in the RESTAPI. This patch changes the engine so that it parses correctly the hader, according to the RFC, using the Apache HttpComponents library. A module for this library is already provided by the application server. Change-Id: I917aa9e56a50a0a3f85447003676ffd59752749d Related: https://bugzilla.redhat.com/1077441 Signed-off-by: Juan Hernandez <[email protected]> (cherry picked from commit 15e3b2cb39752a4914847365fc49c5b9289d4a52) --- M backend/manager/modules/aaa/pom.xml M backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java M backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml A backend/manager/modules/aaa/src/test/java/org/ovirt/engine/core/aaa/filters/FiltersHelperTest.java M pom.xml 5 files changed, 139 insertions(+), 2 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/48/29848/1 diff --git a/backend/manager/modules/aaa/pom.xml b/backend/manager/modules/aaa/pom.xml index 23f9bec..ec2b0fe 100644 --- a/backend/manager/modules/aaa/pom.xml +++ b/backend/manager/modules/aaa/pom.xml @@ -50,6 +50,25 @@ <artifactId>commons-lang</artifactId> </dependency> + <dependency> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpcore</artifactId> + </dependency> + + <dependency> + <groupId>junit</groupId> + <artifactId>junit</artifactId> + <version>${junit.version}</version> + <scope>test</scope> + </dependency> + + <dependency> + <groupId>org.mockito</groupId> + <artifactId>mockito-all</artifactId> + <version>${mockito.version}</version> + <scope>test</scope> + </dependency> + </dependencies> <build> diff --git a/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java b/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java index 8dd828f..d2dbbce 100644 --- a/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java +++ b/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/FiltersHelper.java @@ -1,11 +1,13 @@ package org.ovirt.engine.core.aaa.filters; -import java.util.Collections; +import java.util.Enumeration; import javax.naming.Context; import javax.naming.NamingException; import javax.servlet.http.HttpServletRequest; +import org.apache.http.HeaderElement; +import org.apache.http.message.BasicHeaderValueParser; import org.ovirt.engine.core.common.constants.SessionConstants; import org.ovirt.engine.core.common.interfaces.BackendLocal; @@ -38,7 +40,20 @@ } public static boolean isPersistentAuth(HttpServletRequest req) { - return Collections.list(req.getHeaders(FiltersHelper.Constants.HEADER_PREFER)).contains("persistent-auth"); + Enumeration<String> headerValues = req.getHeaders(Constants.HEADER_PREFER); + while (headerValues.hasMoreElements()) { + String headerValue = headerValues.nextElement(); + HeaderElement[] headerElements = BasicHeaderValueParser.parseElements(headerValue, null); + if (headerElements != null) { + for (HeaderElement headerElement : headerElements) { + String elementName = headerElement.getName(); + if ("persistent-auth".equalsIgnoreCase(elementName)) { + return true; + } + } + } + } + return false; } } diff --git a/backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml b/backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml index 5b730b1..5dce55b6 100644 --- a/backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml +++ b/backend/manager/modules/aaa/src/main/modules/org/ovirt/engine/core/aaa/main/module.xml @@ -11,6 +11,7 @@ <module name="javax.servlet.api"/> <module name="org.apache.commons.codec"/> <module name="org.apache.commons.lang"/> + <module name="org.apache.httpcomponents"/> <module name="org.ovirt.engine.core.common"/> <module name="org.ovirt.engine.core.extensions-manager"/> <module name="org.ovirt.engine.core.utils"/> diff --git a/backend/manager/modules/aaa/src/test/java/org/ovirt/engine/core/aaa/filters/FiltersHelperTest.java b/backend/manager/modules/aaa/src/test/java/org/ovirt/engine/core/aaa/filters/FiltersHelperTest.java new file mode 100644 index 0000000..583e93d --- /dev/null +++ b/backend/manager/modules/aaa/src/test/java/org/ovirt/engine/core/aaa/filters/FiltersHelperTest.java @@ -0,0 +1,96 @@ +package org.ovirt.engine.core.aaa.filters; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import java.util.Vector; +import javax.servlet.http.HttpServletRequest; + +import org.junit.Test; + +public class FiltersHelperTest { + + /** + * Check that the persistent authentication preference is recognized when there are more than one {@code Prefer} + * headers. + */ + @Test + public void testPersistentAuthWithSeveralHeaders() { + assertTrue(isPersistentAuth("persistent-auth", "x", "y")); + assertTrue(isPersistentAuth("x", "persistent-auth", "y")); + assertTrue(isPersistentAuth("x", "y", "persistent-auth")); + } + + /** + * Check that the persistent authentication preference is recognized regardless of case. + */ + @Test + public void testPersistentAuthIgnoresCase() { + assertTrue(isPersistentAuth("Persistent-Auth")); + assertTrue(isPersistentAuth("PERSISTENT-AUTH")); + } + + /** + * Check that the persistent authentication preference is recognized when there are other preferences in the same + * header. + */ + @Test + public void testPersistentAuthOtherPreferencesInSameHeader() { + assertTrue(isPersistentAuth("persistent-auth, x, y")); + assertTrue(isPersistentAuth("x, persistent-auth, y")); + assertTrue(isPersistentAuth("x, y, persistent-auth")); + } + + /** + * Check that the persistent authentication preference is recognized even it is has a value (the value should be + * ignored). + */ + @Test + public void testPersistentAuthWithValue() { + assertTrue(isPersistentAuth("persistent-auth=false")); + assertTrue(isPersistentAuth("persistent-auth=true")); + assertTrue(isPersistentAuth("persistent-auth=junk")); + } + + /** + * Check that the persistent authentication preference is recognized even it is has parameters (the parameters + * should be ignored). + */ + @Test + public void testPersistentAuthWithParameters() { + assertTrue(isPersistentAuth("persistent-auth; x=0; y=0")); + } + + /** + * Check that the persistent authentication isn't enabled if the preference isn't present. + */ + @Test + public void testPersistentAuthDisabled() { + assertFalse(isPersistentAuth()); + assertFalse(isPersistentAuth("x", "y")); + assertFalse(isPersistentAuth("x", "y")); + } + + /** + * This method constructs a mocked HTTP request, populates it with values for the {@cod Prefer} header, and then + * calls the method that checks if persistent authentication is enabled. It is intended to simplify other tests. + * + * @param values the values of the {@code Prefer} header + */ + private boolean isPersistentAuth(String... values) { + // Create a vector containing the values of the header: + Vector<String> vector = new Vector<>(); + Collections.addAll(vector, values); + + // Create the mocked request: + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getHeaders(FiltersHelper.Constants.HEADER_PREFER)).thenReturn(vector.elements()); + + // Call the method that checks for persistent authentication: + return FiltersHelper.isPersistentAuth(request); + } + +} diff --git a/pom.xml b/pom.xml index ea22e45..4a1b84e 100644 --- a/pom.xml +++ b/pom.xml @@ -55,6 +55,7 @@ <javax.activation.version>1.1</javax.activation.version> <xmlrpc-client.version>3.1.3</xmlrpc-client.version> <httpclient.version>3.1</httpclient.version> + <httpcomponents.version>4.2.1</httpcomponents.version> <spring.version>3.1.1.RELEASE</spring.version> <spring.core.version>3.1.1.RELEASE</spring.core.version> <spring.jdbc.version>3.1.1.RELEASE</spring.jdbc.version> @@ -153,6 +154,11 @@ <version>${httpclient.version}</version> </dependency> <dependency> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpcore</artifactId> + <version>${httpcomponents.version}</version> + </dependency> + <dependency> <groupId>org.springframework.ldap</groupId> <artifactId>spring-ldap-core</artifactId> <version>${spring.ldap.version}</version> -- To view, visit http://gerrit.ovirt.org/29848 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I917aa9e56a50a0a3f85447003676ffd59752749d Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Juan Hernandez <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
