SONAR-19085 Remove external_groups entries when GitHub provisioning is deactivated

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<ExternalGroupDto> expectedGroups = createAndInsertExternalGroupDtos("provider1", 3);
+    List<ExternalGroupDto> expectedGroups = createAndInsertExternalGroupDtos(PROVIDER, 3);
     createAndInsertExternalGroupDtos("provider2", 1);
-    List<ExternalGroupDto> savedGroup = underTest.selectByIdentityProvider(dbSession, "provider1");
+    List<ExternalGroupDto> savedGroup = underTest.selectByIdentityProvider(dbSession, PROVIDER);
     assertThat(savedGroup).containsExactlyInAnyOrderElementsOf(expectedGroups);
   }
 
@@ -96,15 +97,26 @@ public class ExternalGroupDaoIT {
 
   @Test
   public void deleteByGroupUuid_deletesTheGroup() {
-    List<ExternalGroupDto> insertedGroups = createAndInsertExternalGroupDtos("provider1", 3);
+    List<ExternalGroupDto> insertedGroups = createAndInsertExternalGroupDtos(PROVIDER, 3);
 
     ExternalGroupDto toRemove = insertedGroups.remove(0);
     underTest.deleteByGroupUuid(dbSession, toRemove.groupUuid());
-    List<ExternalGroupDto> remainingGroups = underTest.selectByIdentityProvider(dbSession, "provider1");
+    List<ExternalGroupDto> remainingGroups = underTest.selectByIdentityProvider(dbSession, PROVIDER);
     assertThat(remainingGroups).containsExactlyInAnyOrderElementsOf(insertedGroups);
   }
 
   @Test
+  public void deleteByExternalIdentityProvider_onlyDeletesGroupOfTheRequestedProvider() {
+    createAndInsertExternalGroupDtos(PROVIDER, 3);
+    List<ExternalGroupDto> 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);
     assertThat(filterManagedUser).isEqualTo(" exists (select group_uuid from external_groups eg where eg.group_uuid = uuid)");
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<ExternalGroupDto> 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>
 
+  <delete id="deleteByExternalIdentityProvider" parameterType="String">
+    delete from external_groups where external_identity_provider = #{externalIdentityProvider, jdbcType=VARCHAR}
+  </delete>
+
 </mapper>