From f09de6aa56a805127e9f3c169b9fab7d50cd48fc Mon Sep 17 00:00:00 2001 From: Matteo Mara Date: Wed, 5 Oct 2022 15:55:52 +0200 Subject: [PATCH] SONAR-17296 improved handling of SAML Authentication errors preventing the handshake to start. --- .../sonar/auth/saml/SamlAuthenticator.java | 8 +++- .../sonar/auth/saml/SamlStatusChecker.java | 8 ++++ .../auth/saml/SamlAuthenticatorTest.java | 41 +++++++++++++++++++ .../auth/saml/SamlStatusCheckerTest.java | 10 +++++ .../server/saml/ws/ValidationInitAction.java | 11 ++++- .../saml/ws/ValidationInitActionTest.java | 18 ++++++++ 6 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlAuthenticatorTest.java diff --git a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlAuthenticator.java b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlAuthenticator.java index 584d95950e9..dba3c5b170f 100644 --- a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlAuthenticator.java +++ b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlAuthenticator.java @@ -209,8 +209,12 @@ public class SamlAuthenticator { } public String getAuthenticationStatusPage(HttpServletRequest request, HttpServletResponse response) { - Auth auth = this.initSamlAuth(request, response); - return getSamlAuthStatusHtml(getSamlAuthenticationStatus(auth, samlSettings)); + try { + Auth auth = this.initSamlAuth(request, response); + return getSamlAuthStatusHtml(getSamlAuthenticationStatus(auth, samlSettings)); + } catch (IllegalStateException e) { + return getSamlAuthStatusHtml(getSamlAuthenticationStatus(String.format("%s due to: %s", e.getMessage(), e.getCause().getMessage()))); + } } } diff --git a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlStatusChecker.java b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlStatusChecker.java index 337f061a299..d47394582e5 100644 --- a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlStatusChecker.java +++ b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlStatusChecker.java @@ -67,6 +67,14 @@ public final class SamlStatusChecker { } + public static SamlAuthenticationStatus getSamlAuthenticationStatus(String errorMessage) { + SamlAuthenticationStatus samlAuthenticationStatus = new SamlAuthenticationStatus(); + samlAuthenticationStatus.getErrors().add(errorMessage); + samlAuthenticationStatus.setStatus("error"); + + return samlAuthenticationStatus; + } + private static Map> getAttributesMapping(Auth auth, SamlSettings samlSettings) { Map> attributesMapping = new HashMap<>(); diff --git a/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlAuthenticatorTest.java b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlAuthenticatorTest.java new file mode 100644 index 00000000000..84ca8b0f333 --- /dev/null +++ b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlAuthenticatorTest.java @@ -0,0 +1,41 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.auth.saml; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.junit.Test; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; + +public class SamlAuthenticatorTest { + + @Test + public void authentication_status_with_errors_returned_when_init_fails() { + SamlAuthenticator samlAuthenticator = new SamlAuthenticator(mock(SamlSettings.class), mock(SamlMessageIdChecker.class)); + HttpServletRequest request = mock(HttpServletRequest.class); + HttpServletResponse response = mock(HttpServletResponse.class); + + String authenticationStatus = samlAuthenticator.getAuthenticationStatusPage(request, response); + + assertFalse(authenticationStatus.isEmpty()); + } +} diff --git a/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlStatusCheckerTest.java b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlStatusCheckerTest.java index f76529d3095..fd286fbae4d 100644 --- a/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlStatusCheckerTest.java +++ b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlStatusCheckerTest.java @@ -64,6 +64,16 @@ public class SamlStatusCheckerTest { when(auth.getAttributes()).thenReturn(getResponseAttributes()); } + @Test + public void authentication_status_has_errors_when_no_idp_certificate_is_provided() { + + samlAuthenticationStatus = getSamlAuthenticationStatus("error message"); + + assertEquals("error", samlAuthenticationStatus.getStatus()); + assertFalse(samlAuthenticationStatus.getErrors().isEmpty()); + assertEquals("error message", samlAuthenticationStatus.getErrors().get(0)); + } + @Test public void authentication_status_is_success_when_no_errors() { setSettings(); diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java index f882827cd07..4181fe6d83f 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationInitAction.java @@ -36,6 +36,9 @@ import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.user.UserSession; import org.sonar.server.ws.ServletFilterHandler; +import static org.sonar.server.authentication.SamlValidationRedirectionFilter.SAML_VALIDATION_CONTROLLER_CONTEXT; +import static org.sonar.server.authentication.SamlValidationRedirectionFilter.SAML_VALIDATION_KEY; + public class ValidationInitAction extends ServletFilter implements SamlAction { public static final String VALIDATION_RELAY_STATE = "validation-query"; @@ -79,7 +82,11 @@ public class ValidationInitAction extends ServletFilter implements SamlAction { return; } - samlAuthenticator.initLogin(oAuth2ContextFactory.generateCallbackUrl(SamlIdentityProvider.KEY), - VALIDATION_RELAY_STATE, request, response); + try { + samlAuthenticator.initLogin(oAuth2ContextFactory.generateCallbackUrl(SamlIdentityProvider.KEY), + VALIDATION_RELAY_STATE, request, response); + } catch (IllegalStateException e) { + response.sendRedirect("/" + SAML_VALIDATION_CONTROLLER_CONTEXT + "/" + SAML_VALIDATION_KEY); + } } } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java index 618dce62ef0..c284e4defec 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationInitActionTest.java @@ -36,6 +36,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.matches; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -80,6 +81,23 @@ public class ValidationInitActionTest { any(), any()); } + @Test + public void do_filter_as_admin_with_init_issues() throws IOException, ServletException { + userSession.logIn().setSystemAdministrator(); + HttpServletRequest servletRequest = mock(HttpServletRequest.class); + HttpServletResponse servletResponse = mock(HttpServletResponse.class); + FilterChain filterChain = mock(FilterChain.class); + String callbackUrl = "http://localhost:9000/api/validation_test"; + when(oAuth2ContextFactory.generateCallbackUrl(anyString())) + .thenReturn(callbackUrl); + + doThrow(new IllegalStateException()).when(samlAuthenticator).initLogin(any(), any(), any(), any()); + + underTest.doFilter(servletRequest, servletResponse, filterChain); + + verify(servletResponse).sendRedirect("/saml/validation"); + } + @Test public void do_filter_as_not_admin() throws IOException, ServletException { userSession.logIn(); -- 2.39.5