From f235dcec41d3fad489d16318bb59b8869f064ac0 Mon Sep 17 00:00:00 2001 From: Matteo Mara Date: Fri, 2 Dec 2022 17:56:14 +0100 Subject: SONAR-17694 fix SSF-323 --- .../org/sonar/server/saml/ws/ValidationAction.java | 17 ++++++++++- .../sonar/server/saml/ws/ValidationInitAction.java | 10 +++++-- .../sonar/server/saml/ws/ValidationActionTest.java | 34 +++++++++++++++++++++- .../server/saml/ws/ValidationInitActionTest.java | 15 ++++++++-- 4 files changed, 68 insertions(+), 8 deletions(-) (limited to 'server/sonar-webserver-webapi') diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java index 4b58458513c..3b1c5e37a86 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/saml/ws/ValidationAction.java @@ -34,7 +34,9 @@ import org.sonar.auth.saml.SamlAuthenticator; import org.sonar.auth.saml.SamlIdentityProvider; import org.sonar.server.authentication.AuthenticationError; import org.sonar.server.authentication.OAuth2ContextFactory; +import org.sonar.server.authentication.OAuthCsrfVerifier; import org.sonar.server.authentication.SamlValidationRedirectionFilter; +import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ThreadLocalUserSession; import org.sonar.server.ws.ServletFilterHandler; @@ -44,11 +46,16 @@ public class ValidationAction extends ServletFilter implements SamlAction { private final ThreadLocalUserSession userSession; private final SamlAuthenticator samlAuthenticator; private final OAuth2ContextFactory oAuth2ContextFactory; + private final SamlIdentityProvider samlIdentityProvider; + private final OAuthCsrfVerifier oAuthCsrfVerifier; - public ValidationAction(ThreadLocalUserSession userSession, SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory) { + public ValidationAction(ThreadLocalUserSession userSession, SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory, + SamlIdentityProvider samlIdentityProvider, OAuthCsrfVerifier oAuthCsrfVerifier) { this.samlAuthenticator = samlAuthenticator; this.userSession = userSession; this.oAuth2ContextFactory = oAuth2ContextFactory; + this.samlIdentityProvider = samlIdentityProvider; + this.oAuthCsrfVerifier = oAuthCsrfVerifier; } @Override @@ -60,6 +67,14 @@ public class ValidationAction extends ServletFilter implements SamlAction { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { HttpServletResponse httpResponse = (HttpServletResponse) response; HttpServletRequest httpRequest = (HttpServletRequest) request; + + try { + oAuthCsrfVerifier.verifyState(httpRequest, httpResponse, samlIdentityProvider, "CSRFToken"); + } catch (AuthenticationException exception) { + AuthenticationError.handleError(httpRequest, httpResponse, exception.getMessage()); + return; + } + if (!userSession.hasSession() || !userSession.isSystemAdministrator()) { AuthenticationError.handleError(httpRequest, httpResponse, "User needs to be logged in as system administrator to access this page."); return; 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 4181fe6d83f..d25122d5ea8 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 @@ -32,6 +32,7 @@ import org.sonar.auth.saml.SamlAuthenticator; import org.sonar.auth.saml.SamlIdentityProvider; import org.sonar.server.authentication.AuthenticationError; import org.sonar.server.authentication.OAuth2ContextFactory; +import org.sonar.server.authentication.OAuthCsrfVerifier; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.user.UserSession; import org.sonar.server.ws.ServletFilterHandler; @@ -44,12 +45,13 @@ public class ValidationInitAction extends ServletFilter implements SamlAction { public static final String VALIDATION_RELAY_STATE = "validation-query"; public static final String VALIDATION_INIT_KEY = "validation_init"; private final SamlAuthenticator samlAuthenticator; - + private final OAuthCsrfVerifier oAuthCsrfVerifier; private final OAuth2ContextFactory oAuth2ContextFactory; private final UserSession userSession; - public ValidationInitAction(SamlAuthenticator samlAuthenticator, OAuth2ContextFactory oAuth2ContextFactory, UserSession userSession) { + public ValidationInitAction(SamlAuthenticator samlAuthenticator, OAuthCsrfVerifier oAuthCsrfVerifier, OAuth2ContextFactory oAuth2ContextFactory, UserSession userSession) { this.samlAuthenticator = samlAuthenticator; + this.oAuthCsrfVerifier = oAuthCsrfVerifier; this.oAuth2ContextFactory = oAuth2ContextFactory; this.userSession = userSession; } @@ -82,9 +84,11 @@ public class ValidationInitAction extends ServletFilter implements SamlAction { return; } + String csrfState = oAuthCsrfVerifier.generateState(request,response); + try { samlAuthenticator.initLogin(oAuth2ContextFactory.generateCallbackUrl(SamlIdentityProvider.KEY), - VALIDATION_RELAY_STATE, request, response); + VALIDATION_RELAY_STATE + "/" + csrfState, 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/ValidationActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java index a00371db1d6..beb73abc605 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/saml/ws/ValidationActionTest.java @@ -31,12 +31,17 @@ import org.junit.Before; import org.junit.Test; import org.sonar.api.server.ws.WebService; import org.sonar.auth.saml.SamlAuthenticator; +import org.sonar.auth.saml.SamlIdentityProvider; import org.sonar.server.authentication.OAuth2ContextFactory; +import org.sonar.server.authentication.OAuthCsrfVerifier; +import org.sonar.server.authentication.event.AuthenticationEvent; +import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.user.ThreadLocalUserSession; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; @@ -48,12 +53,18 @@ public class ValidationActionTest { private SamlAuthenticator samlAuthenticator; private ThreadLocalUserSession userSession; + private OAuthCsrfVerifier oAuthCsrfVerifier; + + private SamlIdentityProvider samlIdentityProvider; + @Before public void setup() { samlAuthenticator = mock(SamlAuthenticator.class); userSession = mock(ThreadLocalUserSession.class); + oAuthCsrfVerifier = mock(OAuthCsrfVerifier.class); + samlIdentityProvider = mock(SamlIdentityProvider.class); var oAuth2ContextFactory = mock(OAuth2ContextFactory.class); - underTest = new ValidationAction(userSession, samlAuthenticator, oAuth2ContextFactory); + underTest = new ValidationAction(userSession, samlAuthenticator, oAuth2ContextFactory, samlIdentityProvider, oAuthCsrfVerifier); } @Test @@ -98,6 +109,27 @@ public class ValidationActionTest { verifyNoInteractions(samlAuthenticator); } + @Test + public void do_filter_failed_csrf_verification() throws ServletException, IOException { + HttpServletRequest servletRequest = spy(HttpServletRequest.class); + HttpServletResponse servletResponse = mock(HttpServletResponse.class); + StringWriter stringWriter = new StringWriter(); + doReturn(new PrintWriter(stringWriter)).when(servletResponse).getWriter(); + FilterChain filterChain = mock(FilterChain.class); + + doReturn("IdentityProviderName").when(samlIdentityProvider).getName(); + doThrow(AuthenticationException.newBuilder() + .setSource(AuthenticationEvent.Source.oauth2(samlIdentityProvider)) + .setMessage("Cookie is missing").build()).when(oAuthCsrfVerifier).verifyState(any(),any(),any(), any()); + + doReturn(true).when(userSession).hasSession(); + doReturn(true).when(userSession).isSystemAdministrator(); + + underTest.doFilter(servletRequest, servletResponse, filterChain); + + verifyNoInteractions(samlAuthenticator); + } + @Test public void verify_definition() { String controllerKey = "foo"; 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 c284e4defec..a12e56dbb66 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 @@ -30,6 +30,7 @@ import org.junit.Test; import org.sonar.api.server.ws.WebService; import org.sonar.auth.saml.SamlAuthenticator; import org.sonar.server.authentication.OAuth2ContextFactory; +import org.sonar.server.authentication.OAuthCsrfVerifier; import org.sonar.server.tester.UserSessionRule; import static org.assertj.core.api.Assertions.assertThat; @@ -48,12 +49,14 @@ public class ValidationInitActionTest { private ValidationInitAction underTest; private SamlAuthenticator samlAuthenticator; private OAuth2ContextFactory oAuth2ContextFactory; + private OAuthCsrfVerifier oAuthCsrfVerifier; @Before public void setUp() throws Exception { samlAuthenticator = mock(SamlAuthenticator.class); oAuth2ContextFactory = mock(OAuth2ContextFactory.class); - underTest = new ValidationInitAction(samlAuthenticator, oAuth2ContextFactory, userSession); + oAuthCsrfVerifier = mock(OAuthCsrfVerifier.class); + underTest = new ValidationInitAction(samlAuthenticator, oAuthCsrfVerifier, oAuth2ContextFactory, userSession); } @Test @@ -71,8 +74,9 @@ public class ValidationInitActionTest { HttpServletResponse servletResponse = mock(HttpServletResponse.class); FilterChain filterChain = mock(FilterChain.class); String callbackUrl = "http://localhost:9000/api/validation_test"; - when(oAuth2ContextFactory.generateCallbackUrl(anyString())) - .thenReturn(callbackUrl); + + mockCsrfTokenGeneration(servletRequest, servletResponse); + when(oAuth2ContextFactory.generateCallbackUrl(anyString())).thenReturn(callbackUrl); underTest.doFilter(servletRequest, servletResponse, filterChain); @@ -91,6 +95,7 @@ public class ValidationInitActionTest { when(oAuth2ContextFactory.generateCallbackUrl(anyString())) .thenReturn(callbackUrl); + mockCsrfTokenGeneration(servletRequest, servletResponse); doThrow(new IllegalStateException()).when(samlAuthenticator).initLogin(any(), any(), any(), any()); underTest.doFilter(servletRequest, servletResponse, filterChain); @@ -143,4 +148,8 @@ public class ValidationInitActionTest { assertThat(validationInitAction.description()).isNotEmpty(); assertThat(validationInitAction.handler()).isNotNull(); } + + private void mockCsrfTokenGeneration(HttpServletRequest servletRequest, HttpServletResponse servletResponse) { + when(oAuthCsrfVerifier.generateState(servletRequest, servletResponse)).thenReturn("CSRF_TOKEN"); + } } -- cgit v1.2.3