Skip to content
Permalink
Browse files Browse the repository at this point in the history
OIDC-118: Fix bad handling of request parameters
  • Loading branch information
tmortagne committed Jan 27, 2022
1 parent 7d7cca1 commit 0247af1
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 15 deletions.
Expand Up @@ -37,11 +37,11 @@
import javax.servlet.http.HttpSession;

import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.collections4.SetUtils;
import org.apache.commons.lang3.StringUtils;
import org.joda.time.LocalDateTime;
import org.slf4j.Logger;
import org.xwiki.component.annotation.Component;
import org.xwiki.configuration.ConfigurationSource;
import org.xwiki.container.Container;
import org.xwiki.container.Request;
import org.xwiki.container.Session;
Expand Down Expand Up @@ -234,6 +234,8 @@ public Map<String, Set<String>> getProviderMapping()

private static final String XWIKI_GROUP_PREFIX = "XWiki.";

private static final Set<String> SAFE_PROPERTIES = SetUtils.hashSet(PROP_SKIPPED);

@Inject
private InstanceIdManager instance;

Expand All @@ -249,10 +251,6 @@ public Map<String, Set<String>> getProviderMapping()
@Inject
private Logger logger;

@Inject
// TODO: store configuration in custom objects
private ConfigurationSource configuration;

private HttpSession getHttpSession()
{
Session session = this.container.getSession();
Expand Down Expand Up @@ -335,10 +333,12 @@ public Map<String, String> getMap(String key)
@Override
protected <T> T getProperty(String key, Class<T> valueClass)
{
// Get property from request
String requestValue = getRequestParameter(key);
if (requestValue != null) {
return this.converter.convert(valueClass, requestValue);
if (SAFE_PROPERTIES.contains(key)) {
// Get property from request
String requestValue = getRequestParameter(key);
if (requestValue != null) {
return this.converter.convert(valueClass, requestValue);
}
}

// Get property from session
Expand All @@ -354,10 +354,12 @@ protected <T> T getProperty(String key, Class<T> valueClass)
@Override
protected <T> T getProperty(String key, T def)
{
// Get property from request
String requestValue = getRequestParameter(key);
if (requestValue != null) {
return this.converter.convert(def.getClass(), requestValue);
if (SAFE_PROPERTIES.contains(key)) {
// Get property from request
String requestValue = getRequestParameter(key);
if (requestValue != null) {
return this.converter.convert(def.getClass(), requestValue);
}
}

// Get property from session
Expand Down Expand Up @@ -426,9 +428,19 @@ private Endpoint getEndPoint(String hint) throws URISyntaxException
uri = new URI(uriString);
}

// If we still don't have any endpoint URI, return null
// If we still don't have any endpoint URI, try the request
if (uri == null) {
return null;
uriString = getRequestParameter(PROPPREFIX_ENDPOINT + hint);
if (uriString == null) {
String provider = getRequestParameter(PROP_XWIKIPROVIDER);
if (provider == null) {
return null;
}

uri = this.manager.createEndPointURI(provider, hint);
} else {
uri = new URI(uriString);
}
}

// Find custom headers
Expand Down
Expand Up @@ -19,20 +19,30 @@
*/
package org.xwiki.contrib.oidc.auth.internal;

import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import org.junit.jupiter.api.Test;
import org.xwiki.configuration.ConfigurationSource;
import org.xwiki.container.Container;
import org.xwiki.container.servlet.ServletRequest;
import org.xwiki.contrib.oidc.provider.internal.OIDCManager;
import org.xwiki.contrib.oidc.provider.internal.endpoint.TokenOIDCEndpoint;
import org.xwiki.properties.ConverterManager;
import org.xwiki.test.junit5.mockito.ComponentTest;
import org.xwiki.test.junit5.mockito.InjectMockComponents;
import org.xwiki.test.junit5.mockito.MockComponent;

import com.xpn.xwiki.web.XWikiServletRequestStub;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.when;
Expand All @@ -51,6 +61,15 @@
@MockComponent
private ConfigurationSource sourceConfiguration;

@MockComponent
private Container container;

@MockComponent
private OIDCManager manager;

@MockComponent
private ConverterManager converterManager;

@Test
void getUserInfoOIDCEndpoint() throws URISyntaxException
{
Expand Down Expand Up @@ -78,6 +97,39 @@ void getUserInfoOIDCEndpoint() throws URISyntaxException

assertEquals(uri, endpoint.getURI());
assertEquals(headers, endpoint.getHeaders());
}

@Test
void getPropertyOrder() throws MalformedURLException, URISyntaxException
{
String provider = "http://urlprovider";
URI urlauthorization = new URI("http://urlauthorization");

XWikiServletRequestStub requestStub = new XWikiServletRequestStub(new URL("http://url"), null);

when(this.container.getRequest()).thenReturn(new ServletRequest(requestStub));
when(this.sourceConfiguration.getProperty(OIDCClientConfiguration.PROP_SKIPPED, false)).thenReturn(false);

assertFalse(this.configuration.isSkipped());
assertNull(this.configuration.getXWikiProvider());
assertNull(this.configuration.getAuthorizationOIDCEndpoint());
assertNull(this.configuration.getAuthorizationOIDCEndpoint());
assertNull(this.configuration.getTokenOIDCEndpoint());

requestStub.put(OIDCClientConfiguration.PROP_SKIPPED, "true");
when(this.converterManager.convert(Boolean.class, "true")).thenReturn(true);

assertTrue(this.configuration.isSkipped());

requestStub.put(OIDCClientConfiguration.PROP_GROUPS_ALLOWED, "true");

assertNull(this.configuration.getAllowedGroups());

requestStub.put(OIDCClientConfiguration.PROP_XWIKIPROVIDER, provider.toString());
requestStub.put(OIDCClientConfiguration.PROP_ENDPOINT_AUTHORIZATION, urlauthorization.toString());
when(this.manager.createEndPointURI(provider, TokenOIDCEndpoint.HINT)).thenReturn(new URI(provider));

assertEquals(urlauthorization, this.configuration.getAuthorizationOIDCEndpoint().getURI());
assertEquals(provider, this.configuration.getTokenOIDCEndpoint().getURI().toString());
}
}

0 comments on commit 0247af1

Please sign in to comment.