Skip to content

Commit

Permalink
Delegated authentication issue when serviceId contains blanks
Browse files Browse the repository at this point in the history
The + character is not correcly encoded by Cas when
using delegated authenication.

This leads to the following error when CAS redirects to a delegated
authentication server:

```
ERROR [org.apereo.cas.support.oauth.services.OAuth20AuthenticationServiceSelectionStrategy] -
<Illegal character in query at index xxx:
https://casserver.fr:443/cas/oauth2.0/callbackAuthorize?client_id=myapp&redirect_uri=http%3A%2F%2Fmyapp.fr%3A4200%2Fauth%2Fauth-callback&response_type=id_token token&client_name=CasOAuthClient>
```

This error originates from UriComponentsBuilder usage.

See spring-projects/spring-framework#21577
and https://docs.spring.io/spring/docs/current/spring-framework-reference/web.html#web-uri-encoding
  • Loading branch information
gonzalad committed Mar 27, 2020
1 parent 7d5303d commit 543acb8
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import org.pac4j.core.util.Pac4jConstants;
import org.springframework.web.util.UriComponentsBuilder;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -52,31 +54,36 @@ public Optional<DelegatedClientIdentityProviderConfiguration> resolve() {
val uriBuilder = UriComponentsBuilder
.fromUriString(ENDPOINT_URL_REDIRECT)
.queryParam(Pac4jConstants.DEFAULT_CLIENT_NAME_PARAMETER, name);
Map<String, String> queryParams = new HashMap<>();

if (service != null) {
val sourceParam = service.getSource();
val serviceParam = service.getOriginalUrl();
if (StringUtils.isNotBlank(sourceParam) && StringUtils.isNotBlank(serviceParam)) {
uriBuilder.queryParam(sourceParam, serviceParam);
uriBuilder.queryParam(sourceParam, "{service}");
queryParams.put("service", serviceParam);
}
}

val methodParam = webContext.getRequestParameter(CasProtocolConstants.PARAMETER_METHOD)
.map(String::valueOf).orElse(StringUtils.EMPTY);
if (StringUtils.isNotBlank(methodParam)) {
uriBuilder.queryParam(CasProtocolConstants.PARAMETER_METHOD, methodParam);
uriBuilder.queryParam(CasProtocolConstants.PARAMETER_METHOD, "{method}");
queryParams.put("method", methodParam);
}
val localeParam = webContext.getRequestParameter(casProperties.getLocale().getParamName())
.map(String::valueOf).orElse(StringUtils.EMPTY);
if (StringUtils.isNotBlank(localeParam)) {
uriBuilder.queryParam(casProperties.getLocale().getParamName(), localeParam);
uriBuilder.queryParam(casProperties.getLocale().getParamName(), "{locale}");
queryParams.put("locale", localeParam);
}
val themeParam = webContext.getRequestParameter(casProperties.getTheme().getParamName())
.map(String::valueOf).orElse(StringUtils.EMPTY);
if (StringUtils.isNotBlank(themeParam)) {
uriBuilder.queryParam(casProperties.getTheme().getParamName(), themeParam);
uriBuilder.queryParam(casProperties.getTheme().getParamName(), "{theme}");
queryParams.put("theme", themeParam);
}
val redirectUrl = uriBuilder.toUriString();
val redirectUrl = uriBuilder.build(queryParams).toString();
val autoRedirect = (Boolean) client.getCustomProperties()
.getOrDefault(ClientCustomPropertyConstants.CLIENT_CUSTOM_PROPERTY_AUTO_REDIRECT, Boolean.FALSE);
val p = new DelegatedClientIdentityProviderConfiguration(name, redirectUrl, type, getCssClass(client), autoRedirect);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package org.apereo.cas.web;

import org.apereo.cas.CasProtocolConstants;
import org.apereo.cas.authentication.principal.SimpleWebApplicationServiceImpl;
import org.apereo.cas.configuration.CasConfigurationProperties;
import org.junit.Before;
import org.junit.Test;
import org.pac4j.cas.client.CasClient;
import org.pac4j.cas.config.CasConfiguration;
import org.pac4j.core.context.WebContext;

import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Optional;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class DelegatedClientIdentityProviderConfigurationFactoryTests {

private CasConfigurationProperties config;
private WebContext webContextMock;
private CasClient client;
private SimpleWebApplicationServiceImpl service;

@Before
public void setUp() {
config = new CasConfigurationProperties();
webContextMock = mock(WebContext.class);
service = new SimpleWebApplicationServiceImpl();
client = new CasClient(new CasConfiguration());
}

@Test
public void verifyRedirectUrl() {
String method = "some-method";
when(webContextMock.getRequestParameter(CasProtocolConstants.PARAMETER_METHOD)).thenReturn(Optional.of(method));
String locale = "some-locale";
when(webContextMock.getRequestParameter(config.getLocale().getParamName())).thenReturn(Optional.of(locale));
String theme = "some-theme";
when(webContextMock.getRequestParameter(config.getTheme().getParamName())).thenReturn(Optional.of(theme));
service.setSource("source");
service.setOriginalUrl("http://service.original.url.com");
DelegatedClientIdentityProviderConfigurationFactory factory = newFactory();

Optional<DelegatedClientIdentityProviderConfiguration> actual = factory.resolve();

assertTrue(actual.isPresent());
assertEquals(client.getName(), actual.get().getName());
assertEquals("cas", actual.get().getType());
String redirectUrl = actual.get().getRedirectUrl();
assertNotNull(redirectUrl);
String invalidRedirectUrlMessage = "invalid " + redirectUrl;
assertTrue(redirectUrl.startsWith("clientredirect?"), invalidRedirectUrlMessage);
assertTrue(redirectUrl.contains("client_name=" + client.getName()), invalidRedirectUrlMessage);
assertTrue(redirectUrl.contains("method=" + method), invalidRedirectUrlMessage);
assertTrue(redirectUrl.contains("locale=" + locale), invalidRedirectUrlMessage);
assertTrue(redirectUrl.contains("theme=" + theme), invalidRedirectUrlMessage);
assertTrue(redirectUrl.contains("source=" + URLEncoder.encode(service.getOriginalUrl(), StandardCharsets.UTF_8)),
invalidRedirectUrlMessage);
}

@Test
public void verifyRedirectUrlCorrectlyEncoded() {
service.setSource("source");
// check that we encode correctly the + sign
service.setOriginalUrl("http://service.original.url.com?response_type=idtoken+token");
DelegatedClientIdentityProviderConfigurationFactory factory = newFactory();

Optional<DelegatedClientIdentityProviderConfiguration> actual = factory.resolve();

assertTrue(actual.isPresent());
String redirectUrl = actual.get().getRedirectUrl();
assertNotNull(redirectUrl);
assertTrue(redirectUrl.contains("source=" + URLEncoder.encode(service.getOriginalUrl(), StandardCharsets.UTF_8)));
}

private DelegatedClientIdentityProviderConfigurationFactory newFactory() {
return DelegatedClientIdentityProviderConfigurationFactory.builder()
.casProperties(config)
.client(client)
.service(service)
.webContext(webContextMock)
.build();
}
}

0 comments on commit 543acb8

Please sign in to comment.