From ddc962bf2384d0290ae11eb67ad038c0e3ba2b97 Mon Sep 17 00:00:00 2001 From: Krishna Bottla <40598480+kbottla@users.noreply.github.com> Date: Thu, 26 Oct 2023 11:15:20 +0100 Subject: [PATCH] PP-11205 Add DAO method to delete users (#2192) * PP-11205 Add DAO method to delete users - Added DAO method to delete historic users not associated with any service --- .secrets.baseline | 4 +- .../uk/gov/pay/adminusers/model/User.java | 21 ++++- .../adminusers/persistence/dao/UserDao.java | 23 +++++- .../adminusers/fixtures/UserDbFixture.java | 15 +++- .../adminusers/persistence/dao/UserDaoIT.java | 81 ++++++++++++++++++- .../adminusers/utils/DatabaseTestHelper.java | 7 +- 6 files changed, 140 insertions(+), 11 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index b40284852..ae59c05a4 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -317,7 +317,7 @@ "filename": "src/test/java/uk/gov/pay/adminusers/fixtures/UserDbFixture.java", "hashed_secret": "95578813f8acdd659caf14acd19b09c875addbb6", "is_verified": false, - "line_number": 26 + "line_number": 27 } ], "src/test/java/uk/gov/pay/adminusers/pact/ContractTest.java": [ @@ -466,5 +466,5 @@ } ] }, - "generated_at": "2023-09-13T11:11:05Z" + "generated_at": "2023-10-26T09:06:36Z" } diff --git a/src/main/java/uk/gov/pay/adminusers/model/User.java b/src/main/java/uk/gov/pay/adminusers/model/User.java index 14f462f57..924fd7cf6 100644 --- a/src/main/java/uk/gov/pay/adminusers/model/User.java +++ b/src/main/java/uk/gov/pay/adminusers/model/User.java @@ -50,6 +50,8 @@ public class User { @Schema(example = "2022-04-06T23:03:41.665Z") @JsonSerialize(using = ApiResponseDateTimeSerializer.class) private ZonedDateTime lastLoggedInAt; + @JsonIgnore + private ZonedDateTime createdAt; private List links = new ArrayList<>(); private Integer sessionVersion = 0; @@ -58,7 +60,15 @@ public static User from(Integer id, String externalId, String password, String e SecondFactorMethod secondFactor, String provisionalOtpKey, ZonedDateTime provisionalOtpKeyCreatedAt, ZonedDateTime lastLoggedInAt) { return new User(id, externalId, password, email, otpKey, telephoneNumber, serviceRoles, features, - secondFactor, provisionalOtpKey, provisionalOtpKeyCreatedAt, lastLoggedInAt); + secondFactor, provisionalOtpKey, provisionalOtpKeyCreatedAt, lastLoggedInAt, null); + } + + public static User from(Integer id, String externalId, String password, String email, String otpKey, + String telephoneNumber, List serviceRoles, String features, + SecondFactorMethod secondFactor, String provisionalOtpKey, + ZonedDateTime provisionalOtpKeyCreatedAt, ZonedDateTime lastLoggedInAt, ZonedDateTime createdAt) { + return new User(id, externalId, password, email, otpKey, telephoneNumber, serviceRoles, features, + secondFactor, provisionalOtpKey, provisionalOtpKeyCreatedAt, lastLoggedInAt, createdAt); } private User(Integer id, @JsonProperty("external_id") String externalId, @JsonProperty("password") String password, @JsonProperty("email") String email, @@ -67,7 +77,9 @@ private User(Integer id, @JsonProperty("external_id") String externalId, @JsonPr @JsonProperty("second_factor") SecondFactorMethod secondFactor, @JsonProperty("provisional_otp_key") String provisionalOtpKey, @JsonProperty("provisional_otp_key_created_at") ZonedDateTime provisionalOtpKeyCreatedAt, - @JsonProperty("last_logged_in_at") ZonedDateTime lastLoggedInAt) { + @JsonProperty("last_logged_in_at") ZonedDateTime lastLoggedInAt, + ZonedDateTime createdAt + ) { this.id = id; this.externalId = externalId; this.password = password; @@ -80,6 +92,7 @@ private User(Integer id, @JsonProperty("external_id") String externalId, @JsonPr this.provisionalOtpKey = provisionalOtpKey; this.provisionalOtpKeyCreatedAt = provisionalOtpKeyCreatedAt; this.lastLoggedInAt = lastLoggedInAt; + this.createdAt = createdAt; } @JsonIgnore @@ -173,6 +186,10 @@ public void setLastLoggedInAt(ZonedDateTime lastLoggedInAt) { this.lastLoggedInAt = lastLoggedInAt; } + public ZonedDateTime getCreatedAt() { + return createdAt; + } + @JsonProperty("_links") public List getLinks() { return links; diff --git a/src/main/java/uk/gov/pay/adminusers/persistence/dao/UserDao.java b/src/main/java/uk/gov/pay/adminusers/persistence/dao/UserDao.java index 35b7b7357..fbb9c0593 100644 --- a/src/main/java/uk/gov/pay/adminusers/persistence/dao/UserDao.java +++ b/src/main/java/uk/gov/pay/adminusers/persistence/dao/UserDao.java @@ -8,6 +8,7 @@ import javax.inject.Inject; import javax.persistence.EntityManager; import javax.persistence.Query; +import java.time.Instant; import java.util.AbstractMap.SimpleEntry; import java.util.List; import java.util.Map; @@ -15,6 +16,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; +import static java.util.Date.from; import static java.util.stream.Collectors.groupingBy; import static java.util.stream.Collectors.toUnmodifiableList; import static java.util.stream.Collectors.toUnmodifiableMap; @@ -47,7 +49,7 @@ public List findByExternalIds(List externalIds) { .setParameter("externalIds", lowerCaseExternalIds) .getResultList(); } - + public Map> getAdminUserEmailsForGatewayAccountIds(List gatewayAccountIds) { if (gatewayAccountIds.size() > 0) { String positionalParams = IntStream.rangeClosed(1, gatewayAccountIds.size()).mapToObj(Integer::toString) @@ -80,7 +82,7 @@ public Map> getAdminUserEmailsForGatewayAccountIds(List findByEmail(String email) { String query = "SELECT u FROM UserEntity u " + "WHERE LOWER(u.email) = LOWER(:email)"; @@ -103,4 +105,21 @@ public List findByServiceId(Integer serviceId) { .map(ServiceRoleEntity::getUser) .collect(toUnmodifiableList()); } + + public int deleteUsersNotAssociatedWithAnyService(Instant deleteRecordsBeforeDate) { + String query = "DELETE FROM users u" + + " WHERE u.id in (" + + " SELECT u.id FROM users u" + + " LEFT OUTER JOIN user_services_roles usr " + + " ON u.id = usr.user_id" + + " WHERE usr.user_id is null" + + " AND (last_logged_in_at < ?1 OR (\"createdAt\" < ?2 AND last_logged_in_at IS null))" + + " )"; + + return entityManager.get() + .createNativeQuery(query) + .setParameter(1, from(deleteRecordsBeforeDate)) + .setParameter(2, from(deleteRecordsBeforeDate)) + .executeUpdate(); + } } diff --git a/src/test/java/uk/gov/pay/adminusers/fixtures/UserDbFixture.java b/src/test/java/uk/gov/pay/adminusers/fixtures/UserDbFixture.java index 5fb0626e1..bc27e5b04 100644 --- a/src/test/java/uk/gov/pay/adminusers/fixtures/UserDbFixture.java +++ b/src/test/java/uk/gov/pay/adminusers/fixtures/UserDbFixture.java @@ -10,6 +10,7 @@ import uk.gov.pay.adminusers.model.User; import uk.gov.pay.adminusers.utils.DatabaseTestHelper; +import java.time.ZonedDateTime; import java.util.ArrayList; import java.util.List; @@ -29,6 +30,8 @@ public class UserDbFixture { private String features = "FEATURE_1, FEATURE_2"; private String provisionalOtpKey; private SecondFactorMethod secondFactorMethod = SecondFactorMethod.SMS; + private ZonedDateTime createdAt; + private ZonedDateTime lastLoggedInAt; private UserDbFixture(DatabaseTestHelper databaseTestHelper) { this.databaseTestHelper = databaseTestHelper; @@ -44,7 +47,7 @@ public User insertUser() { .collect(toUnmodifiableList()); User user = User.from(randomInt(), externalId, password, email, otpKey, telephoneNumber, - serviceRoles, features, secondFactorMethod, provisionalOtpKey, null, null); + serviceRoles, features, secondFactorMethod, provisionalOtpKey, null, lastLoggedInAt, createdAt); databaseTestHelper.add(user); serviceRoles.forEach(serviceRole -> @@ -103,4 +106,14 @@ public UserDbFixture withSecondFactorMethod(SecondFactorMethod secondFactorMetho this.secondFactorMethod = secondFactorMethod; return this; } + + public UserDbFixture withCreatedAt(ZonedDateTime createdAt) { + this.createdAt = createdAt; + return this; + } + + public UserDbFixture withLastLoggedInAt(ZonedDateTime lastLoggedInAt) { + this.lastLoggedInAt = lastLoggedInAt; + return this; + } } diff --git a/src/test/java/uk/gov/pay/adminusers/persistence/dao/UserDaoIT.java b/src/test/java/uk/gov/pay/adminusers/persistence/dao/UserDaoIT.java index af36701a6..5395d1f31 100644 --- a/src/test/java/uk/gov/pay/adminusers/persistence/dao/UserDaoIT.java +++ b/src/test/java/uk/gov/pay/adminusers/persistence/dao/UserDaoIT.java @@ -1,6 +1,7 @@ package uk.gov.pay.adminusers.persistence.dao; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import uk.gov.pay.adminusers.model.Role; import uk.gov.pay.adminusers.model.SecondFactorMethod; @@ -18,11 +19,14 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; import static java.lang.String.valueOf; +import static java.time.ZonedDateTime.parse; import static java.util.stream.Collectors.toUnmodifiableList; import static org.apache.commons.lang3.RandomUtils.nextInt; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.emptyOrNullString; import static org.hamcrest.Matchers.equalTo; @@ -50,7 +54,7 @@ public void before() { serviceDao = env.getInstance(ServiceDao.class); roleDao = env.getInstance(RoleDao.class); } - + @Test void getAdminUserEmailsForGatewayAccountIds_should_return_empty_map() { Map> map = userDao.getAdminUserEmailsForGatewayAccountIds(List.of()); @@ -377,4 +381,79 @@ public void shouldNotCreateAUserWithDifferentCaseEmail() { var thrown = assertThrows(javax.persistence.RollbackException.class, () -> userDao.persist(userEntity)); assertThat(thrown.getMessage(), containsString("ERROR: duplicate key value violates unique constraint \"lower_case_email_index\"")); } + + @Nested + class TestDeleteUsersNotAssociatedWithAnyService { + + @Test + void shouldDeleteUsersWithLastLoggedInAtDateCorrectly() { + ZonedDateTime deleteUsersUpToDate = parse("2023-01-31T00:00:00Z"); + Integer userId = userDbFixture(databaseHelper) + .withLastLoggedInAt(deleteUsersUpToDate.minusDays(1)) + .insertUser() + .getId(); + Integer userIdThatShouldNotDeleted = userDbFixture(databaseHelper) + .withLastLoggedInAt(deleteUsersUpToDate.plusDays(1)) + .insertUser() + .getId(); + + int recordsDeleted = userDao.deleteUsersNotAssociatedWithAnyService(deleteUsersUpToDate.toInstant()); + + assertThat(recordsDeleted, is(1)); + + Optional userEntity = userDao.findById(userId); + assertThat(userEntity.isPresent(), is(false)); + + userEntity = userDao.findById(userIdThatShouldNotDeleted); + assertThat(userEntity.isPresent(), is(true)); + } + + @Test + void shouldDeleteUsersWithoutLastLoggedInAtDateCorrectlyButCreatedDateBeforeTheExpungeDate() { + ZonedDateTime deleteUsersUpToDate = parse("2023-01-31T00:00:00Z"); + Integer userId = userDbFixture(databaseHelper) + .withLastLoggedInAt(null) + .withCreatedAt(deleteUsersUpToDate.minusDays(1)) + .insertUser() + .getId(); + Integer userIdThatShouldNotDeleted = userDbFixture(databaseHelper) + .withLastLoggedInAt(null) + .withCreatedAt(deleteUsersUpToDate.plusDays(1)) + .insertUser() + .getId(); + + int recordsDeleted = userDao.deleteUsersNotAssociatedWithAnyService(deleteUsersUpToDate.toInstant()); + + assertThat(recordsDeleted, is(1)); + + Optional userEntity = userDao.findById(userId); + assertThat(userEntity.isPresent(), is(false)); + + userEntity = userDao.findById(userIdThatShouldNotDeleted); + assertThat(userEntity.isPresent(), is(true)); + } + + @Test + void shouldNotDeleteUsersWithLoggedInOrCreatedIfDateIsAfterTheExpungingDate() { + ZonedDateTime deleteUsersUpToDate = parse("2023-01-31T00:00:00Z"); + String externalId1 = userDbFixture(databaseHelper) + .withLastLoggedInAt(deleteUsersUpToDate.plusDays(1)) + .insertUser() + .getExternalId(); + String externalId2 = userDbFixture(databaseHelper) + .withLastLoggedInAt(null) + .withCreatedAt(deleteUsersUpToDate.plusDays(1)) + .insertUser() + .getExternalId(); + + int recordsDeleted = userDao.deleteUsersNotAssociatedWithAnyService(deleteUsersUpToDate.toInstant()); + + assertThat(recordsDeleted, is(0)); + + List users = userDao.findByExternalIds(List.of(externalId1, externalId2)); + + assertThat(users.size(), is(2)); + assertThat(users.stream().map(UserEntity::getExternalId).collect(Collectors.toSet()), containsInAnyOrder(externalId1, externalId2)); + } + } } diff --git a/src/test/java/uk/gov/pay/adminusers/utils/DatabaseTestHelper.java b/src/test/java/uk/gov/pay/adminusers/utils/DatabaseTestHelper.java index 8f1b7ae0f..04a8822ea 100644 --- a/src/test/java/uk/gov/pay/adminusers/utils/DatabaseTestHelper.java +++ b/src/test/java/uk/gov/pay/adminusers/utils/DatabaseTestHelper.java @@ -117,9 +117,9 @@ public DatabaseTestHelper add(User user) { .createUpdate("INSERT INTO users(" + "id, external_id, password, email, otp_key, telephone_number, " + "second_factor, disabled, login_counter, version, " + - "\"createdAt\", \"updatedAt\", session_version, provisional_otp_key) " + + "\"createdAt\", \"updatedAt\", session_version, provisional_otp_key, last_logged_in_at) " + "VALUES (:id, :externalId, :password, :email, :otpKey, :telephoneNumber, " + - ":secondFactor, :disabled, :loginCounter, :version, :createdAt, :updatedAt, :session_version, :provisionalOtpKey)") + ":secondFactor, :disabled, :loginCounter, :version, :createdAt, :updatedAt, :session_version, :provisionalOtpKey, :lastLoggedInAt)") .bind("id", user.getId()) .bind("externalId", user.getExternalId()) .bind("password", user.getPassword()) @@ -131,9 +131,10 @@ public DatabaseTestHelper add(User user) { .bind("loginCounter", user.getLoginCounter()) .bind("version", 0) .bind("session_version", user.getSessionVersion()) - .bind("createdAt", now) + .bind("createdAt", user.getCreatedAt() == null ? now : user.getCreatedAt()) .bind("updatedAt", now) .bind("provisionalOtpKey", user.getProvisionalOtpKey()) + .bind("lastLoggedInAt", user.getLastLoggedInAt()) .execute() ); return this;