From 5f7d8368cb5fdedd5df68fede07b7b4d98d77807 Mon Sep 17 00:00:00 2001 From: Aurelien Poscia Date: Wed, 10 May 2023 10:40:24 +0200 Subject: [PATCH] SONAR-19085 Remove external_groups entries when GitHub provisioning is deactivated --- .../org/sonar/auth/github/GitHubSettings.java | 15 +++++++++++++- .../github/GitHubIdentityProviderTest.java | 3 ++- .../sonar/auth/github/GitHubSettingsTest.java | 20 +++++++++++++++++-- .../sonar/auth/github/IntegrationTest.java | 2 +- .../org/sonar/db/user/ExternalGroupDaoIT.java | 20 +++++++++++++++---- .../org/sonar/db/user/ExternalGroupDao.java | 4 ++++ .../sonar/db/user/ExternalGroupMapper.java | 2 ++ .../org/sonar/db/user/ExternalGroupMapper.xml | 4 ++++ 8 files changed, 61 insertions(+), 9 deletions(-) diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java index f3fb6f07304..32465a2d999 100644 --- a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java +++ b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java @@ -27,6 +27,8 @@ import javax.annotation.Nullable; import org.sonar.api.PropertyType; import org.sonar.api.config.Configuration; import org.sonar.api.config.PropertyDefinition; +import org.sonar.db.DbClient; +import org.sonar.db.DbSession; import org.sonar.server.property.InternalProperties; import static java.lang.String.format; @@ -60,10 +62,12 @@ public class GitHubSettings { private final Configuration configuration; private final InternalProperties internalProperties; + private final DbClient dbClient; - public GitHubSettings(Configuration configuration, InternalProperties internalProperties) { + public GitHubSettings(Configuration configuration, InternalProperties internalProperties, DbClient dbClient) { this.configuration = configuration; this.internalProperties = internalProperties; + this.dbClient = dbClient; } String clientId() { @@ -119,10 +123,19 @@ public class GitHubSettings { public void setProvisioning(boolean enableProvisioning) { if (enableProvisioning) { checkGithubConfigIsCompleteForProvisioning(); + } else { + removeExternalGroupsForGithub(); } internalProperties.write(PROVISIONING, String.valueOf(enableProvisioning)); } + private void removeExternalGroupsForGithub() { + try (DbSession dbSession = dbClient.openSession(false)) { + dbClient.externalGroupDao().deleteByExternalIdentityProvider(dbSession, GitHubIdentityProvider.KEY); + dbSession.commit(); + } + } + private void checkGithubConfigIsCompleteForProvisioning() { checkState(isEnabled(), getErrorMessage("GitHub authentication must be enabled")); checkState(isNotBlank(appId()), getErrorMessage("Application ID must be provided")); diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java index 491413a39ec..c702c39b3fe 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java @@ -22,6 +22,7 @@ package org.sonar.auth.github; import org.junit.Test; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.server.authentication.OAuth2IdentityProvider; +import org.sonar.db.DbClient; import org.sonar.server.property.InternalProperties; import static org.assertj.core.api.Assertions.assertThat; @@ -35,7 +36,7 @@ public class GitHubIdentityProviderTest { private MapSettings settings = new MapSettings(); private InternalProperties internalProperties = mock(InternalProperties.class); - private GitHubSettings gitHubSettings = new GitHubSettings(settings.asConfig(), internalProperties); + private GitHubSettings gitHubSettings = new GitHubSettings(settings.asConfig(), internalProperties, mock(DbClient.class)); private UserIdentityFactoryImpl userIdentityFactory = mock(UserIdentityFactoryImpl.class); private ScribeGitHubApi scribeApi = new ScribeGitHubApi(gitHubSettings); private GitHubRestClient gitHubRestClient = new GitHubRestClient(gitHubSettings); diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java index c5291b5babc..8fb0125b7f8 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java @@ -20,11 +20,14 @@ package org.sonar.auth.github; import java.util.Optional; +import org.junit.Rule; import org.junit.Test; import org.sonar.api.config.PropertyDefinition; import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.utils.System2; +import org.sonar.db.DbTester; +import org.sonar.db.user.GroupDto; import org.sonar.server.property.InternalProperties; import static org.assertj.core.api.Assertions.assertThat; @@ -34,11 +37,13 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class GitHubSettingsTest { + @Rule + public DbTester db = DbTester.create(System2.INSTANCE); private MapSettings settings = new MapSettings(new PropertyDefinitions(System2.INSTANCE, GitHubSettings.definitions())); private InternalProperties internalProperties = mock(InternalProperties.class); - private GitHubSettings underTest = new GitHubSettings(settings.asConfig(), internalProperties); + private GitHubSettings underTest = new GitHubSettings(settings.asConfig(), internalProperties, db.getDbClient()); @Test public void is_enabled() { @@ -133,11 +138,22 @@ public class GitHubSettingsTest { } @Test - public void setProvisioning_whenPassedFalse_delegatesToInternalPropertiesWrite() { + public void setProvisioning_whenPassedFalse_delegatesToInternalPropertiesWriteAndCleansUpExternalGroups() { + GroupDto groupDto = createGithubManagedGroup(); + underTest.setProvisioning(false); + verify(internalProperties).write(GitHubSettings.PROVISIONING, Boolean.FALSE.toString()); + assertThat(db.getDbClient().externalGroupDao().selectByGroupUuid(db.getSession(), groupDto.getUuid())).isEmpty(); } + private GroupDto createGithubManagedGroup() { + GroupDto groupDto = db.users().insertGroup(); + db.users().markGroupAsGithubManaged(groupDto.getUuid()); + return groupDto; + } + + @Test public void return_client_id() { settings.setProperty("sonar.auth.github.clientId.secured", "id"); diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java index 9af6681ab46..1cccec8eab1 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java @@ -64,7 +64,7 @@ public class IntegrationTest { // load settings with default values private MapSettings settings = new MapSettings(new PropertyDefinitions(System2.INSTANCE, GitHubSettings.definitions())); private InternalProperties internalProperties = new InternalPropertiesImpl(db.getDbClient()); - private GitHubSettings gitHubSettings = new GitHubSettings(settings.asConfig(), internalProperties); + private GitHubSettings gitHubSettings = new GitHubSettings(settings.asConfig(), internalProperties, db.getDbClient()); private UserIdentityFactoryImpl userIdentityFactory = new UserIdentityFactoryImpl(); private ScribeGitHubApi scribeApi = new ScribeGitHubApi(gitHubSettings); private GitHubRestClient gitHubRestClient = new GitHubRestClient(gitHubSettings); diff --git a/server/sonar-db-dao/src/it/java/org/sonar/db/user/ExternalGroupDaoIT.java b/server/sonar-db-dao/src/it/java/org/sonar/db/user/ExternalGroupDaoIT.java index 6de7ba2c279..57c31bc2ccf 100644 --- a/server/sonar-db-dao/src/it/java/org/sonar/db/user/ExternalGroupDaoIT.java +++ b/server/sonar-db-dao/src/it/java/org/sonar/db/user/ExternalGroupDaoIT.java @@ -34,6 +34,7 @@ public class ExternalGroupDaoIT { private static final String GROUP_UUID = "uuid"; private static final String EXTERNAL_ID = "external_id"; private static final String EXTERNAL_IDENTITY_PROVIDER = "external_identity_provider"; + private static final String PROVIDER = "provider1"; @Rule public final DbTester db = DbTester.create(); @@ -69,9 +70,9 @@ public class ExternalGroupDaoIT { @Test public void selectByIdentityProvider_returnOnlyGroupForTheIdentityProvider() { - List expectedGroups = createAndInsertExternalGroupDtos("provider1", 3); + List expectedGroups = createAndInsertExternalGroupDtos(PROVIDER, 3); createAndInsertExternalGroupDtos("provider2", 1); - List savedGroup = underTest.selectByIdentityProvider(dbSession, "provider1"); + List savedGroup = underTest.selectByIdentityProvider(dbSession, PROVIDER); assertThat(savedGroup).containsExactlyInAnyOrderElementsOf(expectedGroups); } @@ -96,14 +97,25 @@ public class ExternalGroupDaoIT { @Test public void deleteByGroupUuid_deletesTheGroup() { - List insertedGroups = createAndInsertExternalGroupDtos("provider1", 3); + List insertedGroups = createAndInsertExternalGroupDtos(PROVIDER, 3); ExternalGroupDto toRemove = insertedGroups.remove(0); underTest.deleteByGroupUuid(dbSession, toRemove.groupUuid()); - List remainingGroups = underTest.selectByIdentityProvider(dbSession, "provider1"); + List remainingGroups = underTest.selectByIdentityProvider(dbSession, PROVIDER); assertThat(remainingGroups).containsExactlyInAnyOrderElementsOf(insertedGroups); } + @Test + public void deleteByExternalIdentityProvider_onlyDeletesGroupOfTheRequestedProvider() { + createAndInsertExternalGroupDtos(PROVIDER, 3); + List groupsProvider2 = createAndInsertExternalGroupDtos("provider2", 3); + + underTest.deleteByExternalIdentityProvider(dbSession, PROVIDER); + + assertThat(underTest.selectByIdentityProvider(dbSession, PROVIDER)).isEmpty(); + assertThat(underTest.selectByIdentityProvider(dbSession, "provider2")).containsExactlyInAnyOrderElementsOf(groupsProvider2); + } + @Test public void getManagedGroupsSqlFilter_whenFilterByManagedIsTrue_returnsCorrectQuery() { String filterManagedUser = underTest.getManagedGroupSqlFilter(true); diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupDao.java index 743236638fb..748260d9ae1 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupDao.java @@ -53,4 +53,8 @@ public class ExternalGroupDao implements Dao { public String getManagedGroupSqlFilter(boolean filterByManaged) { return String.format("%s exists (select group_uuid from external_groups eg where eg.group_uuid = uuid)", filterByManaged ? "" : "not"); } + + public void deleteByExternalIdentityProvider(DbSession dbSession, String externalIdentityProvider) { + mapper(dbSession).deleteByExternalIdentityProvider(externalIdentityProvider); + } } diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupMapper.java b/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupMapper.java index 908aae7664f..6a5c89a26b5 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupMapper.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/user/ExternalGroupMapper.java @@ -34,4 +34,6 @@ public interface ExternalGroupMapper { Optional selectByExternalIdAndIdentityProvider(@Param("externalId") String externalId, @Param("identityProvider") String identityProvider); void deleteByGroupUuid(@Param("groupUuid") String groupUuid); + + void deleteByExternalIdentityProvider(String externalIdentityProvider); } diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/user/ExternalGroupMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/user/ExternalGroupMapper.xml index a63497ece33..e82fce1efdd 100644 --- a/server/sonar-db-dao/src/main/resources/org/sonar/db/user/ExternalGroupMapper.xml +++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/user/ExternalGroupMapper.xml @@ -50,4 +50,8 @@ delete from external_groups where group_uuid = #{groupUuid, jdbcType=VARCHAR} + + delete from external_groups where external_identity_provider = #{externalIdentityProvider, jdbcType=VARCHAR} + + -- 2.39.5