From 6b18287731f914d94327b7bae03c9ba1e351f99d Mon Sep 17 00:00:00 2001 From: ivinokur Date: Fri, 8 Mar 2024 13:06:15 +0200 Subject: [PATCH] Encode redirect URL if needed on oauth1 callback request --- .../che/security/oauth/EmbeddedOAuthAPI.java | 2 +- .../oauth1/OAuthAuthenticationService.java | 12 +++- .../OAuthAuthenticationServiceTest.java | 68 +++++++++++++++++-- 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth/EmbeddedOAuthAPI.java b/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth/EmbeddedOAuthAPI.java index ef644e7a67..a31cedde05 100644 --- a/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth/EmbeddedOAuthAPI.java +++ b/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth/EmbeddedOAuthAPI.java @@ -152,7 +152,7 @@ public class EmbeddedOAuthAPI implements OAuthAPI { * JSON, as a query parameter. This prevents passing unsupported characters, like '{' and '}' to * the {@link URI#create(String)} method. */ - private String encodeRedirectUrl(String url) { + public static String encodeRedirectUrl(String url) { try { String query = new URL(url).getQuery(); return url.substring(0, url.indexOf(query)) + URLEncoder.encode(query, UTF_8); diff --git a/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth1/OAuthAuthenticationService.java b/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth1/OAuthAuthenticationService.java index 0ecc4313be..f5c5800000 100644 --- a/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth1/OAuthAuthenticationService.java +++ b/wsmaster/che-core-api-auth/src/main/java/org/eclipse/che/security/oauth1/OAuthAuthenticationService.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2023 Red Hat, Inc. + * Copyright (c) 2012-2024 Red Hat, Inc. * This program and the accompanying materials are made * available under the terms of the Eclipse Public License 2.0 * which is available at https://www.eclipse.org/legal/epl-2.0/ @@ -16,6 +16,7 @@ import static org.eclipse.che.commons.lang.UrlUtils.getParameter; import static org.eclipse.che.commons.lang.UrlUtils.getQueryParametersFromState; import static org.eclipse.che.commons.lang.UrlUtils.getRequestUrl; import static org.eclipse.che.commons.lang.UrlUtils.getState; +import static org.eclipse.che.security.oauth.EmbeddedOAuthAPI.encodeRedirectUrl; import jakarta.ws.rs.GET; import jakarta.ws.rs.Path; @@ -73,7 +74,14 @@ public class OAuthAuthenticationService extends Service { final Map> parameters = getQueryParametersFromState(getState(requestUrl)); final String providerName = getParameter(parameters, "oauth_provider"); - final String redirectAfterLogin = getParameter(parameters, "redirect_after_login"); + String redirectAfterLogin = getParameter(parameters, "redirect_after_login"); + + try { + URI.create(redirectAfterLogin); + } catch (IllegalArgumentException e) { + // the redirectUrl was decoded by the CSM provider, so we need to encode it back. + redirectAfterLogin = encodeRedirectUrl(redirectAfterLogin); + } UriBuilder redirectUriBuilder = UriBuilder.fromUri(redirectAfterLogin); diff --git a/wsmaster/che-core-api-auth/src/test/java/org/eclipse/che/security/oauth1/OAuthAuthenticationServiceTest.java b/wsmaster/che-core-api-auth/src/test/java/org/eclipse/che/security/oauth1/OAuthAuthenticationServiceTest.java index ab1bcc9155..2d34c1e700 100644 --- a/wsmaster/che-core-api-auth/src/test/java/org/eclipse/che/security/oauth1/OAuthAuthenticationServiceTest.java +++ b/wsmaster/che-core-api-auth/src/test/java/org/eclipse/che/security/oauth1/OAuthAuthenticationServiceTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2021 Red Hat, Inc. + * Copyright (c) 2012-2024 Red Hat, Inc. * This program and the accompanying materials are made * available under the terms of the Eclipse Public License 2.0 * which is available at https://www.eclipse.org/legal/epl-2.0/ @@ -12,20 +12,26 @@ package org.eclipse.che.security.oauth1; import static io.restassured.RestAssured.given; +import static java.net.URLEncoder.encode; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.everrest.assured.JettyHttpServer.ADMIN_USER_NAME; import static org.everrest.assured.JettyHttpServer.ADMIN_USER_PASSWORD; import static org.everrest.assured.JettyHttpServer.SECURE_PATH; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import io.restassured.response.Response; +import jakarta.ws.rs.core.UriInfo; +import java.lang.reflect.Field; +import java.net.URI; import java.net.URL; +import org.eclipse.che.api.core.rest.Service; import org.everrest.assured.EverrestJetty; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.testng.MockitoTestNGListener; -import org.testng.annotations.BeforeMethod; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @@ -41,17 +47,15 @@ public class OAuthAuthenticationServiceTest { @Mock private OAuthAuthenticator oAuthAuthenticator; + @Mock private UriInfo uriInfo; + @Mock private OAuthAuthenticatorProvider oAuthProvider; @InjectMocks private OAuthAuthenticationService oAuthAuthenticationService; - @BeforeMethod - public void setUp() { - when(oAuthProvider.getAuthenticator("test-server")).thenReturn(oAuthAuthenticator); - } - @Test public void shouldResolveCallbackWithoutError() throws OAuthAuthenticationException { + when(oAuthProvider.getAuthenticator("test-server")).thenReturn(oAuthAuthenticator); when(oAuthAuthenticator.callback(any(URL.class))).thenReturn("user1"); final Response response = given() @@ -70,6 +74,7 @@ public class OAuthAuthenticationServiceTest { @Test public void shouldResolveCallbackWithAccessDeniedError() throws OAuthAuthenticationException { + when(oAuthProvider.getAuthenticator("test-server")).thenReturn(oAuthAuthenticator); when(oAuthAuthenticator.callback(any(URL.class))) .thenThrow(new UserDeniedOAuthAuthenticationException("Access denied")); final Response response = @@ -89,6 +94,7 @@ public class OAuthAuthenticationServiceTest { @Test public void shouldResolveCallbackWithInvalidRequestError() throws OAuthAuthenticationException { + when(oAuthProvider.getAuthenticator("test-server")).thenReturn(oAuthAuthenticator); when(oAuthAuthenticator.callback(any(URL.class))) .thenThrow(new OAuthAuthenticationException("Invalid request")); final Response response = @@ -105,4 +111,52 @@ public class OAuthAuthenticationServiceTest { assertEquals(response.statusCode(), 307); assertEquals(response.header("Location"), REDIRECT_URI + "?error_code=invalid_request"); } + + @Test + public void shouldEncodeRedirectUrl() throws Exception { + // given + Field uriInfoField = Service.class.getDeclaredField("uriInfo"); + uriInfoField.setAccessible(true); + uriInfoField.set(oAuthAuthenticationService, uriInfo); + when(uriInfo.getRequestUri()) + .thenReturn( + new URI( + "http://eclipse.che?state=oauth_provider" + + encode( + "=bitbucket-server&redirect_after_login=https://redirecturl.com?params=" + + encode("{}", UTF_8), + UTF_8))); + when(oAuthProvider.getAuthenticator("bitbucket-server")) + .thenReturn(mock(OAuthAuthenticator.class)); + + // when + jakarta.ws.rs.core.Response callback = oAuthAuthenticationService.callback(); + + // then + assertEquals(callback.getLocation().toString(), "https://redirecturl.com?params%3D%7B%7D"); + } + + @Test + public void shouldNotEncodeRedirectUrl() throws Exception { + // given + Field uriInfoField = Service.class.getDeclaredField("uriInfo"); + uriInfoField.setAccessible(true); + uriInfoField.set(oAuthAuthenticationService, uriInfo); + when(uriInfo.getRequestUri()) + .thenReturn( + new URI( + "http://eclipse.che?state=oauth_provider" + + encode( + "=bitbucket-server&redirect_after_login=https://redirecturl.com?params=" + + encode(encode("{}", UTF_8), UTF_8), + UTF_8))); + when(oAuthProvider.getAuthenticator("bitbucket-server")) + .thenReturn(mock(OAuthAuthenticator.class)); + + // when + jakarta.ws.rs.core.Response callback = oAuthAuthenticationService.callback(); + + // then + assertEquals(callback.getLocation().toString(), "https://redirecturl.com?params=%7B%7D"); + } }