From 86ed2e5290ec4692bb9a23201658d723193d25dc Mon Sep 17 00:00:00 2001 From: Pierre Guillot Date: Wed, 5 Feb 2020 17:50:16 +0100 Subject: [PATCH] SONAR-13001 apply review feedback --- .../java/org/sonar/db/alm/pat/AlmPatDao.java | 20 +++++--- .../org/sonar/db/alm/pat/AlmPatMapper.java | 8 +-- .../org/sonar/db/alm/pat/AlmPatMapper.xml | 16 +++--- server/sonar-db-dao/src/schema/schema-sq.ddl | 2 +- .../org/sonar/db/alm/pat/ALMPatDaoTest.java | 50 +++++++++++-------- .../version/v82/CreateAlmPATsTableTest.java | 2 +- .../server/user/ws/DeactivateAction.java | 3 +- .../server/user/ws/DeactivateActionTest.java | 18 +++++++ .../almintegration/AlmIntegrationService.java | 2 - 9 files changed, 78 insertions(+), 43 deletions(-) diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatDao.java index 44447b942a0..7f0164253ae 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatDao.java @@ -19,13 +19,13 @@ */ package org.sonar.db.alm.pat; -import java.util.List; import java.util.Optional; import org.sonar.api.utils.System2; import org.sonar.core.util.UuidFactory; import org.sonar.db.Dao; import org.sonar.db.DbSession; import org.sonar.db.alm.setting.AlmSettingDto; +import org.sonar.db.user.UserDto; public class AlmPatDao implements Dao { @@ -45,12 +45,8 @@ public class AlmPatDao implements Dao { return Optional.ofNullable(getMapper(dbSession).selectByUuid(uuid)); } - public Optional selectByAlmSetting(DbSession dbSession, String userUuid, AlmSettingDto almSettingDto) { - return Optional.ofNullable(getMapper(dbSession).selectByAlmSetting(userUuid, almSettingDto.getUuid())); - } - - public List selectAll(DbSession dbSession) { - return getMapper(dbSession).selectAll(); + public Optional selectByUserAndAlmSetting(DbSession dbSession, String userUuid, AlmSettingDto almSettingDto) { + return Optional.ofNullable(getMapper(dbSession).selectByUserAndAlmSetting(userUuid, almSettingDto.getUuid())); } public void insert(DbSession dbSession, AlmPatDto almPatDto) { @@ -72,4 +68,14 @@ public class AlmPatDao implements Dao { getMapper(dbSession).deleteByUuid(almPatDto.getUuid()); } + public void deleteByUser(DbSession dbSession, UserDto user) { + getMapper(dbSession).deleteByUser(user.getUuid()); + } + + public void deleteByAlmSetting(DbSession dbSession, AlmSettingDto almSetting) { + getMapper(dbSession).deleteByAlmSetting(almSetting.getUuid()); + } + + + } diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatMapper.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatMapper.java index eaf84b038e1..e4e5b6318b1 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatMapper.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/pat/AlmPatMapper.java @@ -19,7 +19,6 @@ */ package org.sonar.db.alm.pat; -import java.util.List; import javax.annotation.CheckForNull; import org.apache.ibatis.annotations.Param; @@ -29,9 +28,7 @@ public interface AlmPatMapper { AlmPatDto selectByUuid(@Param("uuid") String uuid); @CheckForNull - AlmPatDto selectByAlmSetting(@Param("userUuid") String userUuid, @Param("almSettingUuid") String almSettingUuid); - - List selectAll(); + AlmPatDto selectByUserAndAlmSetting(@Param("userUuid") String userUuid, @Param("almSettingUuid") String almSettingUuid); void insert(@Param("dto") AlmPatDto almPatDto, @Param("uuid") String uuid, @Param("now") long now); @@ -39,4 +36,7 @@ public interface AlmPatMapper { void deleteByUuid(@Param("uuid") String uuid); + void deleteByUser(@Param("userUuid") String userUuid); + + void deleteByAlmSetting(@Param("almSettingUuid") String almSettingUuid); } diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/pat/AlmPatMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/pat/AlmPatMapper.xml index 248c2c03ef0..68f1fb44b89 100644 --- a/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/pat/AlmPatMapper.xml +++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/alm/pat/AlmPatMapper.xml @@ -20,7 +20,7 @@ a.uuid = #{uuid, jdbcType=VARCHAR} - select from alm_pats a @@ -30,12 +30,6 @@ a.user_uuid = #{userUuid, jdbcType=VARCHAR} - - - INSERT INTO alm_pats ( @@ -71,5 +65,13 @@ DELETE FROM alm_pats WHERE uuid = #{uuid, jdbcType=VARCHAR} + + DELETE FROM alm_pats WHERE user_uuid = #{userUuid, jdbcType=VARCHAR} + + + + DELETE FROM alm_pats WHERE alm_setting_uuid = #{almSettingUuid, jdbcType=VARCHAR} + + diff --git a/server/sonar-db-dao/src/schema/schema-sq.ddl b/server/sonar-db-dao/src/schema/schema-sq.ddl index 15d5484b25e..eb922de2031 100644 --- a/server/sonar-db-dao/src/schema/schema-sq.ddl +++ b/server/sonar-db-dao/src/schema/schema-sq.ddl @@ -53,7 +53,7 @@ CREATE INDEX "ALM_APP_INSTALLS_EXTERNAL_ID" ON "ALM_APP_INSTALLS"("USER_EXTERNAL CREATE TABLE "ALM_PATS"( "UUID" VARCHAR(40) NOT NULL, "PAT" VARCHAR(2000) NOT NULL, - "USER_UUID" VARCHAR(40) NOT NULL, + "USER_UUID" VARCHAR(256) NOT NULL, "ALM_SETTING_UUID" VARCHAR(40) NOT NULL, "UPDATED_AT" BIGINT NOT NULL, "CREATED_AT" BIGINT NOT NULL diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/pat/ALMPatDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/pat/ALMPatDaoTest.java index d30ed17f5ed..ee087042404 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/pat/ALMPatDaoTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/pat/ALMPatDaoTest.java @@ -19,8 +19,6 @@ */ package org.sonar.db.alm.pat; -import java.util.List; -import org.assertj.core.api.Assertions; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -28,10 +26,9 @@ import org.sonar.api.utils.System2; import org.sonar.core.util.UuidFactory; import org.sonar.db.DbSession; import org.sonar.db.DbTester; -import org.sonar.db.alm.pat.AlmPatDao; -import org.sonar.db.alm.pat.AlmPatDto; import org.sonar.db.alm.setting.AlmSettingDao; import org.sonar.db.alm.setting.AlmSettingDto; +import org.sonar.db.user.UserDto; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; @@ -39,7 +36,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.sonar.db.alm.integration.pat.AlmPatsTesting.newAlmPatDto; import static org.sonar.db.almsettings.AlmSettingsTesting.newGithubAlmSettingDto; -import static org.sonar.db.user.UserTesting.newUserDto; public class ALMPatDaoTest { @@ -89,27 +85,14 @@ public class ALMPatDaoTest { almPatDto.setUserUuid(userUuid); underTest.insert(dbSession, almPatDto); - assertThat(underTest.selectByAlmSetting(dbSession, userUuid, almSetting).get()) + assertThat(underTest.selectByUserAndAlmSetting(dbSession, userUuid, almSetting).get()) .extracting(AlmPatDto::getUuid, AlmPatDto::getPersonalAccessToken, AlmPatDto::getUserUuid, AlmPatDto::getAlmSettingUuid, AlmPatDto::getCreatedAt, AlmPatDto::getUpdatedAt) .containsExactly(A_UUID, almPatDto.getPersonalAccessToken(), userUuid, almSetting.getUuid(), NOW, NOW); - assertThat(underTest.selectByAlmSetting(dbSession, randomAlphanumeric(40), newGithubAlmSettingDto())).isNotPresent(); - } - - @Test - public void selectAll() { - when(uuidFactory.create()).thenReturn(A_UUID); - when(system2.now()).thenReturn(NOW); - underTest.insert(dbSession, newAlmPatDto()); - when(uuidFactory.create()).thenReturn(A_UUID + "bis"); - underTest.insert(dbSession, newAlmPatDto()); - - List almPats = underTest.selectAll(dbSession); - - Assertions.assertThat(almPats).size().isEqualTo(2); + assertThat(underTest.selectByUserAndAlmSetting(dbSession, randomAlphanumeric(40), newGithubAlmSettingDto())).isNotPresent(); } @Test @@ -146,7 +129,34 @@ public class ALMPatDaoTest { underTest.delete(dbSession, almPat); assertThat(underTest.selectByUuid(dbSession, almPat.getUuid()).isPresent()).isFalse(); + } + + @Test + public void deleteByUser() { + when(uuidFactory.create()).thenReturn(A_UUID); + when(system2.now()).thenReturn(NOW); + UserDto userDto = db.users().insertUser(); + AlmPatDto almPat = newAlmPatDto(); + almPat.setUserUuid(userDto.getUuid()); + underTest.insert(dbSession, almPat); + + underTest.deleteByUser(dbSession, userDto); + + assertThat(underTest.selectByUuid(dbSession, almPat.getUuid()).isPresent()).isFalse(); + } + @Test + public void deleteByAlmSetting() { + when(uuidFactory.create()).thenReturn(A_UUID); + when(system2.now()).thenReturn(NOW); + AlmSettingDto almSettingDto = db.almSettings().insertBitbucketAlmSetting(); + AlmPatDto almPat = newAlmPatDto(); + almPat.setAlmSettingUuid(almSettingDto.getUuid()); + underTest.insert(dbSession, almPat); + + underTest.deleteByAlmSetting(dbSession, almSettingDto); + + assertThat(underTest.selectByUuid(dbSession, almPat.getUuid()).isPresent()).isFalse(); } } diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v82/CreateAlmPATsTableTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v82/CreateAlmPATsTableTest.java index bdffd01bb1d..464f86a19fa 100644 --- a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v82/CreateAlmPATsTableTest.java +++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v82/CreateAlmPATsTableTest.java @@ -51,7 +51,7 @@ public class CreateAlmPATsTableTest { dbTester.assertColumnDefinition(TABLE_NAME, "uuid", VARCHAR, 40, false); dbTester.assertColumnDefinition(TABLE_NAME, "pat", VARCHAR, 2000, false); - dbTester.assertColumnDefinition(TABLE_NAME, "user_uuid", VARCHAR, 40, false); + dbTester.assertColumnDefinition(TABLE_NAME, "user_uuid", VARCHAR, 256, false); dbTester.assertColumnDefinition(TABLE_NAME, "alm_setting_uuid", VARCHAR, 40, false); dbTester.assertColumnDefinition(TABLE_NAME, "updated_at", BIGINT, 20, false); dbTester.assertColumnDefinition(TABLE_NAME, "created_at", BIGINT, 20, false); diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/DeactivateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/DeactivateAction.java index e0231623046..2c0254899b1 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/DeactivateAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/DeactivateAction.java @@ -119,10 +119,11 @@ public class DeactivateAction implements UsersWsAction { dbClient.qProfileEditUsersDao().deleteByUser(dbSession, user); dbClient.organizationMemberDao().deleteByUserId(dbSession, userId); dbClient.userPropertiesDao().deleteByUser(dbSession, user); + dbClient.almPatDao().deleteByUser(dbSession, user); deactivateUser(dbSession, user); userIndexer.commitAndIndex(dbSession, user); - LOGGER.info("Deactivate user: {}; by admin: {}", login, userSession.isSystemAdministrator()); + LOGGER.debug("Deactivate user: {}; by admin: {}", login, userSession.isSystemAdministrator()); } writeResponse(response, login); diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java index b190790464f..04a29d08ae3 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/DeactivateActionTest.java @@ -30,6 +30,7 @@ import org.sonar.api.utils.System2; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.DbTester; +import org.sonar.db.alm.setting.AlmSettingDto; import org.sonar.db.component.ComponentDto; import org.sonar.db.organization.OrganizationDto; import org.sonar.db.permission.template.PermissionTemplateDto; @@ -243,6 +244,23 @@ public class DeactivateActionTest { assertThat(db.getDbClient().userPropertiesDao().selectByUser(dbSession, anotherUser)).hasSize(1); } + @Test + public void deactivate_user_deletes_his_alm_pat() { + logInAsSystemAdministrator(); + + AlmSettingDto almSettingDto = db.almSettings().insertBitbucketAlmSetting(); + + UserDto user = db.users().insertUser(); + db.almPats().insert(p -> p.setUserUuid(user.getUuid()), p -> p.setAlmSettingUuid(almSettingDto.getUuid())); + UserDto anotherUser = db.users().insertUser(); + db.almPats().insert(p -> p.setUserUuid(anotherUser.getUuid()), p -> p.setAlmSettingUuid(almSettingDto.getUuid())); + + deactivate(user.getLogin()); + + assertThat(db.getDbClient().almPatDao().selectByUserAndAlmSetting(dbSession, user.getUuid(), almSettingDto)).isEmpty(); + assertThat(db.getDbClient().almPatDao().selectByUserAndAlmSetting(dbSession, anotherUser.getUuid(), almSettingDto)).isNotNull(); + } + @Test public void user_can_deactivate_itself_on_sonarcloud() { WsActionTester customWs = newSonarCloudWs(); diff --git a/sonar-ws/src/main/java/org/sonarqube/ws/client/almintegration/AlmIntegrationService.java b/sonar-ws/src/main/java/org/sonarqube/ws/client/almintegration/AlmIntegrationService.java index 9a159675c55..e68e67138ac 100644 --- a/sonar-ws/src/main/java/org/sonarqube/ws/client/almintegration/AlmIntegrationService.java +++ b/sonar-ws/src/main/java/org/sonarqube/ws/client/almintegration/AlmIntegrationService.java @@ -61,6 +61,4 @@ public class AlmIntegrationService extends BaseService { .setMediaType(MediaTypes.JSON)).content(); } - //xx add projectList and repoList - } -- 2.39.5