From 4e7453d5ce1a0d54a45dd28a1c3631df0d1ebcb4 Mon Sep 17 00:00:00 2001 From: Antoine Vigneau Date: Tue, 23 Jan 2024 15:40:05 +0100 Subject: [PATCH] SONAR-21483 Fix SSF-530 --- .../auth/gitlab/GitLabIdentityProvider.java | 31 ++- .../org/sonar/auth/gitlab/GitLabSettings.java | 18 +- .../gitlab/GitLabIdentityProviderTest.java | 2 +- .../sonar/auth/gitlab/GitLabModuleTest.java | 2 +- .../sonar/auth/gitlab/IntegrationTest.java | 201 +++++++++++++----- 5 files changed, 192 insertions(+), 62 deletions(-) diff --git a/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabIdentityProvider.java b/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabIdentityProvider.java index 31a6e481baf..fdd76dcd932 100644 --- a/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabIdentityProvider.java +++ b/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabIdentityProvider.java @@ -33,6 +33,7 @@ import java.util.stream.Stream; import javax.servlet.http.HttpServletRequest; import org.sonar.api.server.authentication.Display; import org.sonar.api.server.authentication.OAuth2IdentityProvider; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.api.server.authentication.UserIdentity; import static com.google.common.base.Preconditions.checkState; @@ -83,16 +84,16 @@ public class GitLabIdentityProvider implements OAuth2IdentityProvider { @Override public void init(InitContext context) { String state = context.generateCsrfState(); - OAuth20Service scribe = newScribeBuilder(context, gitLabSettings.syncUserGroups()).build(scribeApi); + OAuth20Service scribe = newScribeBuilder(context).build(scribeApi); String url = scribe.getAuthorizationUrl(state); context.redirectTo(url); } - private ServiceBuilderOAuth20 newScribeBuilder(OAuth2Context context, boolean syncUserGroups) { + private ServiceBuilderOAuth20 newScribeBuilder(OAuth2Context context) { checkState(isEnabled(), "GitLab authentication is disabled"); return new ServiceBuilder(gitLabSettings.applicationId()) .apiSecret(gitLabSettings.secret()) - .defaultScope(syncUserGroups ? API_SCOPE : READ_USER_SCOPE) + .defaultScope(API_SCOPE) .callback(context.getCallbackUrl()); } @@ -110,7 +111,7 @@ public class GitLabIdentityProvider implements OAuth2IdentityProvider { private void onCallback(CallbackContext context) throws InterruptedException, ExecutionException, IOException { HttpServletRequest request = context.getRequest(); - OAuth20Service scribe = newScribeBuilder(context, gitLabSettings.syncUserGroups()).build(scribeApi); + OAuth20Service scribe = newScribeBuilder(context).build(scribeApi); String code = request.getParameter(OAuthConstants.CODE); OAuth2AccessToken accessToken = scribe.getAccessToken(code); @@ -122,14 +123,34 @@ public class GitLabIdentityProvider implements OAuth2IdentityProvider { .setName(user.getName()) .setEmail(user.getEmail()); + + Set userGroups = getGroups(scribe, accessToken); + + if (!gitLabSettings.allowedGroups().isEmpty()) { + validateUserInAllowedGroups(userGroups, gitLabSettings.allowedGroups()); + } + if (gitLabSettings.syncUserGroups()) { - builder.setGroups(getGroups(scribe, accessToken)); + builder.setGroups(userGroups); } context.authenticate(builder.build()); context.redirectToRequestedPage(); } + private static void validateUserInAllowedGroups(Set userGroups, Set allowedGroups) { + boolean allowedUser = userGroups.stream() + .anyMatch(userGroup -> isAllowedGroup(userGroup, allowedGroups)); + + if (!allowedUser) { + throw new UnauthorizedException("You are not allowed to authenticate"); + } + } + + private static boolean isAllowedGroup(String group, Set allowedGroups) { + return allowedGroups.stream().anyMatch(group::startsWith); + } + private Set getGroups(OAuth20Service scribe, OAuth2AccessToken accessToken) { List groups = gitLabRestClient.getGroups(scribe, accessToken); return Stream.of(groups) diff --git a/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabSettings.java b/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabSettings.java index 1334a1c3c8a..696ec15dfbd 100644 --- a/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabSettings.java +++ b/server/sonar-auth-gitlab/src/main/java/org/sonar/auth/gitlab/GitLabSettings.java @@ -21,6 +21,7 @@ package org.sonar.auth.gitlab; import java.util.Arrays; import java.util.List; +import java.util.Set; import org.sonar.api.PropertyType; import org.sonar.api.config.Configuration; import org.sonar.api.config.PropertyDefinition; @@ -36,6 +37,7 @@ public class GitLabSettings { public static final String GITLAB_AUTH_APPLICATION_ID = "sonar.auth.gitlab.applicationId.secured"; public static final String GITLAB_AUTH_SECRET = "sonar.auth.gitlab.secret.secured"; public static final String GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP = "sonar.auth.gitlab.allowUsersToSignUp"; + public static final String GITLAB_AUTH_ALLOWED_GROUPS = "sonar.auth.gitlab.allowedGroups"; public static final String GITLAB_AUTH_SYNC_USER_GROUPS = "sonar.auth.gitlab.groupsSync"; private static final String CATEGORY = "authentication"; @@ -71,6 +73,10 @@ public class GitLabSettings { return configuration.getBoolean(GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP).orElse(false); } + public Set allowedGroups() { + return Set.of(configuration.getStringArray(GITLAB_AUTH_ALLOWED_GROUPS)); + } + public boolean syncUserGroups() { return configuration.getBoolean(GITLAB_AUTH_SYNC_USER_GROUPS).orElse(false); } @@ -118,6 +124,16 @@ public class GitLabSettings { .defaultValue(valueOf(true)) .index(5) .build(), + PropertyDefinition.builder(GITLAB_AUTH_ALLOWED_GROUPS) + .name("Allowed groups") + .description("Only members of these groups (and sub-groups) will be allowed to authenticate. " + + "Please enter the group slug as it appears in the GitLab URL, for instance `my-gitlab-group`. " + + "⚠ if not set, any GitLab user will be able to authenticate to the server.") + .multiValues(true) + .category(CATEGORY) + .subCategory(SUBCATEGORY) + .index(6) + .build(), PropertyDefinition.builder(GITLAB_AUTH_SYNC_USER_GROUPS) .deprecatedKey("sonar.auth.gitlab.sync_user_groups") .name("Synchronize user groups") @@ -127,7 +143,7 @@ public class GitLabSettings { .subCategory(SUBCATEGORY) .type(PropertyType.BOOLEAN) .defaultValue(valueOf(false)) - .index(6) + .index(7) .build()); } } diff --git a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabIdentityProviderTest.java b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabIdentityProviderTest.java index 49399eb64e7..3371b3188a1 100644 --- a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabIdentityProviderTest.java +++ b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabIdentityProviderTest.java @@ -85,7 +85,7 @@ public class GitLabIdentityProviderTest { gitLabIdentityProvider.init(initContext); - verify(initContext).redirectTo("http://server/oauth/authorize?response_type=code&client_id=123&redirect_uri=http%3A%2F%2Fserver%2Fcallback&scope=read_user"); + verify(initContext).redirectTo("http://server/oauth/authorize?response_type=code&client_id=123&redirect_uri=http%3A%2F%2Fserver%2Fcallback&scope=api"); } @Test diff --git a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabModuleTest.java b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabModuleTest.java index 2a67f1ed071..e2d35424656 100644 --- a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabModuleTest.java +++ b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/GitLabModuleTest.java @@ -33,7 +33,7 @@ public class GitLabModuleTest { public void verify_count_of_added_components() { ListContainer container = new ListContainer(); new GitLabModule().configure(container); - assertThat(container.getAddedObjects()).hasSize(10); + assertThat(container.getAddedObjects()).hasSize(11); } private static class ListContainer implements Container { diff --git a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java index 0dbe60399ad..0ba4c414cc4 100644 --- a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java +++ b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java @@ -22,7 +22,6 @@ package org.sonar.auth.gitlab; import javax.servlet.http.HttpServletRequest; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; -import org.assertj.core.api.Assertions; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -30,12 +29,16 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.server.authentication.OAuth2IdentityProvider; +import org.sonar.api.server.authentication.UnauthorizedException; import org.sonar.api.server.authentication.UserIdentity; import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOWED_GROUPS; import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP; import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_APPLICATION_ID; import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ENABLED; @@ -68,23 +71,17 @@ public class IntegrationTest { .setProperty(GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP, "true") .setProperty(GITLAB_AUTH_URL, gitLabUrl) .setProperty(GITLAB_AUTH_APPLICATION_ID, "123") - .setProperty(GITLAB_AUTH_SECRET, "456"); + .setProperty(GITLAB_AUTH_SECRET, "456") + .setProperty(GITLAB_AUTH_ALLOWED_GROUPS, "group1,group2"); } @Test - public void authenticate_user() { - OAuth2IdentityProvider.CallbackContext callbackContext = Mockito.mock(OAuth2IdentityProvider.CallbackContext.class); - when(callbackContext.getCallbackUrl()).thenReturn("http://server/callback"); - - HttpServletRequest httpServletRequest = Mockito.mock(HttpServletRequest.class); - when(httpServletRequest.getParameter("code")).thenReturn(ANY_CODE_VALUE); - when(callbackContext.getRequest()).thenReturn(httpServletRequest); + public void callback_whenAllowedUser_shouldAuthenticate() { + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); - gitlab.enqueue(new MockResponse().setBody( - "{\n" + " \"access_token\": \"de6780bc506a0446309bd9362820ba8aed28aa506c71eedbe1c5c4f9dd350e54\",\n" + " \"token_type\": \"bearer\",\n" + " \"expires_in\": 7200,\n" - + " \"refresh_token\": \"8257e65c97202ed1726cf9571600918f3bffb2544b26e00a61df9897668c33a1\"\n" + "}")); - // response of /user - gitlab.enqueue(new MockResponse().setBody("{\"id\": 123, \"username\":\"toto\", \"name\":\"Toto Toto\",\"email\":\"toto@toto.com\"}")); + mockAccessTokenResponse(); + mockUserResponse(); + mockSingleGroupReponse("group1"); gitLabIdentityProvider.callback(callbackContext); @@ -99,22 +96,53 @@ public class IntegrationTest { } @Test - public void synchronize_groups() throws InterruptedException { - mapSettings.setProperty(GITLAB_AUTH_SYNC_USER_GROUPS, "true"); - OAuth2IdentityProvider.CallbackContext callbackContext = Mockito.mock(OAuth2IdentityProvider.CallbackContext.class); - when(callbackContext.getCallbackUrl()).thenReturn("http://server/callback"); + public void callback_whenNotAllowedUser_shouldThrow() { + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); + + mockAccessTokenResponse(); + mockUserResponse(); + mockSingleGroupReponse("wrong-group"); + + assertThatThrownBy(() -> gitLabIdentityProvider.callback(callbackContext)) + .isInstanceOf((UnauthorizedException.class)) + .hasMessage("You are not allowed to authenticate"); + } + + @Test + public void callback_whenAllowedUserBySubgroupMembership_shouldAuthenticate() { + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); + + mockAccessTokenResponse(); + mockUserResponse(); + mockSingleGroupReponse("group1/subgroup"); + + gitLabIdentityProvider.callback(callbackContext); + + verify(callbackContext).authenticate(any()); + verify(callbackContext).redirectToRequestedPage(); + } - HttpServletRequest httpServletRequest = Mockito.mock(HttpServletRequest.class); - when(httpServletRequest.getParameter("code")).thenReturn(ANY_CODE_VALUE); - when(callbackContext.getRequest()).thenReturn(httpServletRequest); - gitlab.enqueue(new MockResponse().setBody( - "{\n" + " \"access_token\": \"de6780bc506a0446309bd9362820ba8aed28aa506c71eedbe1c5c4f9dd350e54\",\n" + " \"token_type\": \"bearer\",\n" + " \"expires_in\": 7200,\n" - + " \"refresh_token\": \"8257e65c97202ed1726cf9571600918f3bffb2544b26e00a61df9897668c33a1\"\n" + "}")); - // response of /user - gitlab.enqueue(new MockResponse().setBody("{\"id\": 123, \"username\": \"username\", \"name\": \"name\"}")); - // response of /groups - gitlab.enqueue(new MockResponse().setBody("[{\"full_path\": \"group1\"}, {\"full_path\": \"group2\"}]")); + @Test + public void callback_shouldSynchronizeGroups() throws InterruptedException { + mapSettings.setProperty(GITLAB_AUTH_SYNC_USER_GROUPS, "true"); + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); + + mockAccessTokenResponse(); + mockUserResponse(); + // Response for /groups + gitlab.enqueue(new MockResponse().setBody(""" + [ + { + "id": 1, + "full_path": "group1" + }, + { + "id": 2, + "full_path": "group2" + } + ] + """)); gitLabIdentityProvider.callback(callbackContext); @@ -128,35 +156,60 @@ public class IntegrationTest { } @Test - public void synchronize_groups_on_many_pages() { + public void callback_whenMultiplePagesOfGroups_shouldSynchronizeAllGroups() { mapSettings.setProperty(GITLAB_AUTH_SYNC_USER_GROUPS, "true"); - OAuth2IdentityProvider.CallbackContext callbackContext = Mockito.mock(OAuth2IdentityProvider.CallbackContext.class); - when(callbackContext.getCallbackUrl()).thenReturn("http://server/callback"); - - HttpServletRequest httpServletRequest = Mockito.mock(HttpServletRequest.class); - when(httpServletRequest.getParameter("code")).thenReturn(ANY_CODE_VALUE); - when(callbackContext.getRequest()).thenReturn(httpServletRequest); + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); - gitlab.enqueue(new MockResponse().setBody( - "{\n" + " \"access_token\": \"de6780bc506a0446309bd9362820ba8aed28aa506c71eedbe1c5c4f9dd350e54\",\n" + " \"token_type\": \"bearer\",\n" + " \"expires_in\": 7200,\n" - + " \"refresh_token\": \"8257e65c97202ed1726cf9571600918f3bffb2544b26e00a61df9897668c33a1\"\n" + "}")); - // response of /user - gitlab.enqueue(new MockResponse().setBody("{\"id\": 123, \"username\": \"username\", \"name\": \"name\"}")); - // response of /groups, first page + mockAccessTokenResponse(); + mockUserResponse(); + // Response for /groups, first page gitlab.enqueue(new MockResponse() - .setBody("[{\"full_path\": \"group1\"}, {\"full_path\": \"group2\"}]") + .setBody(""" + [ + { + "id": 1, + "full_path": "group1" + }, + { + "id": 2, + "full_path": "group2" + } + ] + """) .setHeader("Link", format(" <%s/groups?per_page=100&page=2>; rel=\"next\"," + " <%s/groups?per_page=100&&page=3>; rel=\"last\"," + " <%s/groups?per_page=100&&page=1>; rel=\"first\"", gitLabUrl, gitLabUrl, gitLabUrl))); - // response of /groups, page 2 + // Response for /groups, page 2 gitlab.enqueue(new MockResponse() - .setBody("[{\"full_path\": \"group3\"}, {\"full_path\": \"group4\"}]") + .setBody(""" + [ + { + "id": 3, + "full_path": "group3" + }, + { + "id": 4, + "full_path": "group4" + } + ] + """) .setHeader("Link", format("<%s/groups?per_page=100&page=3>; rel=\"next\"," + " <%s/groups?per_page=100&&page=3>; rel=\"last\"," + " <%s/groups?per_page=100&&page=1>; rel=\"first\"", gitLabUrl, gitLabUrl, gitLabUrl))); - // response of /groups, page 3 + // Response for /groups, page 3 gitlab.enqueue(new MockResponse() - .setBody("[{\"full_path\": \"group5\"}, {\"full_path\": \"group6\"}]") + .setBody(""" + [ + { + "id": 5, + "full_path": "group5" + }, + { + "id": 6, + "full_path": "group6" + } + ] + """) .setHeader("Link", format("<%s/groups?per_page=100&&page=3>; rel=\"last\"," + " <%s/groups?per_page=100&&page=1>; rel=\"first\"", gitLabUrl, gitLabUrl))); @@ -169,22 +222,62 @@ public class IntegrationTest { } @Test - public void fail_to_authenticate() { + public void callback_whenNoUser_shouldThrow() { + OAuth2IdentityProvider.CallbackContext callbackContext = mockCallbackContext(); + + mockAccessTokenResponse(); + // Response for /user + gitlab.enqueue(new MockResponse().setResponseCode(404).setBody("empty")); + + assertThatThrownBy(() -> gitLabIdentityProvider.callback(callbackContext)) + .hasMessage("Fail to execute request '" + gitLabSettings.url() + "/api/v4/user'. HTTP code: 404, response: empty") + .isInstanceOf((IllegalStateException.class)); + } + + private static OAuth2IdentityProvider.CallbackContext mockCallbackContext() { OAuth2IdentityProvider.CallbackContext callbackContext = Mockito.mock(OAuth2IdentityProvider.CallbackContext.class); when(callbackContext.getCallbackUrl()).thenReturn("http://server/callback"); HttpServletRequest httpServletRequest = Mockito.mock(HttpServletRequest.class); when(httpServletRequest.getParameter("code")).thenReturn(ANY_CODE_VALUE); when(callbackContext.getRequest()).thenReturn(httpServletRequest); + return callbackContext; + } - gitlab.enqueue(new MockResponse().setBody( - "{\n" + " \"access_token\": \"de6780bc506a0446309bd9362820ba8aed28aa506c71eedbe1c5c4f9dd350e54\",\n" + " \"token_type\": \"bearer\",\n" + " \"expires_in\": 7200,\n" - + " \"refresh_token\": \"8257e65c97202ed1726cf9571600918f3bffb2544b26e00a61df9897668c33a1\"\n" + "}")); - gitlab.enqueue(new MockResponse().setResponseCode(404).setBody("empty")); + private void mockAccessTokenResponse() { + // Response for OAuth access token + gitlab.enqueue(new MockResponse().setBody(""" + { + "access_token": "de6780bc506a0446309bd9362820ba8aed28aa506c71eedbe1c5c4f9dd350e54", + "token_type": "bearer", + "expires_in": 7200, + "refresh_token": "8257e65c97202ed1726cf9571600918f3bffb2544b26e00a61df9897668c33a1" + } + """)); + } - Assertions.assertThatThrownBy(() -> gitLabIdentityProvider.callback(callbackContext)) - .hasMessage("Fail to execute request '" + gitLabSettings.url() + "/api/v4/user'. HTTP code: 404, response: empty") - .isInstanceOf((IllegalStateException.class)); + private void mockUserResponse() { + // Response for /user + gitlab.enqueue(new MockResponse().setBody(""" + { + "id": 123, + "username": "toto", + "name": "Toto Toto", + "email": "toto@toto.com" + } + """)); + } + + private void mockSingleGroupReponse(String group) { + // Response for /groups + gitlab.enqueue(new MockResponse().setBody(""" + [ + { + "id": 1, + "full_path": "%s" + } + ] + """.formatted(group))); } } -- 2.39.5