From 9e676fce7ed2a481466bb5e1d3ce5e31c0d56514 Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Sun, 27 Jun 2021 23:31:12 -0300 Subject: [PATCH] [KEYCLOAK-18559] Fix SAML adapters so they allow unescaped characters in URIs - Makes adapters bahavior consistent with containers that allow unescaped characters in URIs --- .../adapters/OAuthRequestAuthenticator.java | 6 ++--- .../org/keycloak/adapters/ServerRequest.java | 2 +- .../undertow/ServletSamlSessionStore.java | 2 +- .../saml/elytron/ElytronSamlSessionStore.java | 6 +---- .../adapters/undertow/UndertowHttpFacade.java | 2 +- .../common/util/KeycloakUriBuilder.java | 25 +++++++++++-------- .../org/keycloak/AbstractOAuthClient.java | 2 +- 7 files changed, 23 insertions(+), 22 deletions(-) diff --git a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java index cbd19b1156..1ad69adb7c 100755 --- a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java +++ b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java @@ -151,7 +151,7 @@ public class OAuthRequestAuthenticator { } KeycloakUriBuilder secureUrl = KeycloakUriBuilder.fromUri(url).scheme("https").port(-1); if (port != 443) secureUrl.port(port); - url = secureUrl.build().toString(); + url = secureUrl.buildAsString(); } String loginHint = getQueryParamValue("login_hint"); @@ -197,7 +197,7 @@ public class OAuthRequestAuthenticator { scope = TokenUtil.attachOIDCScope(scope); redirectUriBuilder.queryParam(OAuth2Constants.SCOPE, scope); - return redirectUriBuilder.build().toString(); + return redirectUriBuilder.buildAsString(); } protected int sslRedirectPort() { @@ -385,7 +385,7 @@ public class OAuthRequestAuthenticator { .replaceQueryParam(OAuth2Constants.CODE, null) .replaceQueryParam(OAuth2Constants.STATE, null) .replaceQueryParam(OAuth2Constants.SESSION_STATE, null); - return builder.build().toString(); + return builder.buildAsString(); } private String rewrittenRedirectUri(String originalUri) { diff --git a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/ServerRequest.java b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/ServerRequest.java index 12c2462694..222b5e8603 100755 --- a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/ServerRequest.java +++ b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/ServerRequest.java @@ -292,7 +292,7 @@ public class ServerRequest { KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(uri) .replaceQueryParam(OAuth2Constants.CODE, null) .replaceQueryParam(OAuth2Constants.STATE, null); - return builder.build().toString(); + return builder.buildAsString(); } diff --git a/adapters/saml/undertow/src/main/java/org/keycloak/adapters/saml/undertow/ServletSamlSessionStore.java b/adapters/saml/undertow/src/main/java/org/keycloak/adapters/saml/undertow/ServletSamlSessionStore.java index f9e52c18f7..449a876c77 100755 --- a/adapters/saml/undertow/src/main/java/org/keycloak/adapters/saml/undertow/ServletSamlSessionStore.java +++ b/adapters/saml/undertow/src/main/java/org/keycloak/adapters/saml/undertow/ServletSamlSessionStore.java @@ -230,7 +230,7 @@ public class ServletSamlSessionStore implements SamlSessionStore { KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(exchange.getRequestURI()) .replaceQuery(exchange.getQueryString()); if (!exchange.isHostIncludedInRequestURI()) uriBuilder.scheme(exchange.getRequestScheme()).host(exchange.getHostAndPort()); - String uri = uriBuilder.build().toString(); + String uri = uriBuilder.buildAsString(); session.setAttribute(SAML_REDIRECT_URI, uri); diff --git a/adapters/saml/wildfly-elytron/src/main/java/org/keycloak/adapters/saml/elytron/ElytronSamlSessionStore.java b/adapters/saml/wildfly-elytron/src/main/java/org/keycloak/adapters/saml/elytron/ElytronSamlSessionStore.java index d6993d43f1..26dc328477 100644 --- a/adapters/saml/wildfly-elytron/src/main/java/org/keycloak/adapters/saml/elytron/ElytronSamlSessionStore.java +++ b/adapters/saml/wildfly-elytron/src/main/java/org/keycloak/adapters/saml/elytron/ElytronSamlSessionStore.java @@ -212,11 +212,7 @@ public class ElytronSamlSessionStore implements SamlSessionStore, ElytronTokeSto if (!scope.exists()) { scope.create(); } - - KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(exchange.getURI()).replaceQuery(exchange.getURI().getQuery()); - String uri = uriBuilder.build().toString(); - - scope.setAttachment(SAML_REDIRECT_URI, uri); + scope.setAttachment(SAML_REDIRECT_URI, exchange.getRequest().getURI()); } @Override diff --git a/adapters/spi/undertow-adapter-spi/src/main/java/org/keycloak/adapters/undertow/UndertowHttpFacade.java b/adapters/spi/undertow-adapter-spi/src/main/java/org/keycloak/adapters/undertow/UndertowHttpFacade.java index c8d812b1f8..497f772c31 100755 --- a/adapters/spi/undertow-adapter-spi/src/main/java/org/keycloak/adapters/undertow/UndertowHttpFacade.java +++ b/adapters/spi/undertow-adapter-spi/src/main/java/org/keycloak/adapters/undertow/UndertowHttpFacade.java @@ -96,7 +96,7 @@ public class UndertowHttpFacade implements HttpFacade { KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(exchange.getRequestURI()) .replaceQuery(exchange.getQueryString()); if (!exchange.isHostIncludedInRequestURI()) uriBuilder.scheme(exchange.getRequestScheme()).host(exchange.getHostAndPort()); - return uriBuilder.build().toString(); + return uriBuilder.buildAsString(); } @Override diff --git a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java index dd5590ef9a..b85e257836 100755 --- a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java +++ b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java @@ -571,28 +571,33 @@ public class KeycloakUriBuilder { return buildFromValues(true, false, values); } + public String buildAsString(Object... values) throws IllegalArgumentException { + if (values == null) throw new IllegalArgumentException("values parameter is null"); + return buildFromValuesAsString(true, false, values); + } + protected URI buildFromValues(boolean encodeSlash, boolean encoded, Object... values) { + String buf = buildFromValuesAsString(encodeSlash, encoded, values); + try { + return new URI(buf); + } catch (Exception e) { + throw new RuntimeException("Failed to create URI: " + buf, e); + } + } + + protected String buildFromValuesAsString(boolean encodeSlash, boolean encoded, Object... values) { List params = getPathParamNamesInDeclarationOrder(); if (values.length < params.size()) throw new IllegalArgumentException("You did not supply enough values to fill path parameters"); Map pathParams = new HashMap(); - - for (int i = 0; i < params.size(); i++) { String pathParam = params.get(i); Object val = values[i]; if (val == null) throw new IllegalArgumentException("A value was null"); pathParams.put(pathParam, val.toString()); } - String buf = null; - try { - buf = buildString(pathParams, encoded, false, encodeSlash); - return new URI(buf); - //return URI.create(buf); - } catch (Exception e) { - throw new RuntimeException("Failed to create URI: " + buf, e); - } + return buildString(pathParams, encoded, false, encodeSlash); } public KeycloakUriBuilder matrixParam(String name, Object... values) throws IllegalArgumentException { diff --git a/core/src/main/java/org/keycloak/AbstractOAuthClient.java b/core/src/main/java/org/keycloak/AbstractOAuthClient.java index 5eeb399d08..40ada8a3ee 100644 --- a/core/src/main/java/org/keycloak/AbstractOAuthClient.java +++ b/core/src/main/java/org/keycloak/AbstractOAuthClient.java @@ -130,7 +130,7 @@ public class AbstractOAuthClient { KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(uri) .replaceQueryParam(OAuth2Constants.CODE, null) .replaceQueryParam(OAuth2Constants.STATE, null); - return builder.build().toString(); + return builder.buildAsString(); } }