From 6bbed03e56db2565e429f6db16b00fe871ee0e19 Mon Sep 17 00:00:00 2001 From: ChrisS1512 <87066931+ChrisS1512@users.noreply.github.com> Date: Wed, 4 Dec 2024 09:19:30 +0000 Subject: [PATCH 1/5] PUB-2681 - Updated System Admin --- build.gradle | 2 +- .../SystemAdminB2CAccountController.java | 2 + .../service/AuthorisationService.java | 13 +++++ .../service/SystemAdminB2CAccountService.java | 51 ++++++------------- 4 files changed, 32 insertions(+), 36 deletions(-) diff --git a/build.gradle b/build.gradle index fb9ba4b0..dfcfc6b1 100644 --- a/build.gradle +++ b/build.gradle @@ -221,7 +221,7 @@ dependencies { implementation group: 'com.opencsv', name: 'opencsv', version: '5.9' implementation group: 'commons-validator', name: 'commons-validator', version: '1.9.0' - implementation group: 'com.github.hmcts', name: 'pip-data-models', version: '2.1.32', { + implementation group: 'com.github.hmcts', name: 'pip-data-models', version: '2.1.34', { exclude group: 'org.springframework.boot', module: 'spring-boot-starter-data-jpa' } implementation group: 'io.hypersistence', name: 'hypersistence-utils-hibernate-63', version: '3.8.3' diff --git a/src/main/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountController.java b/src/main/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountController.java index 5af1b895..dfdea391 100644 --- a/src/main/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountController.java +++ b/src/main/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountController.java @@ -5,6 +5,7 @@ import io.swagger.v3.oas.annotations.tags.Tag; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; @@ -48,6 +49,7 @@ public SystemAdminB2CAccountController(SystemAdminB2CAccountService systemAdminB @ApiResponse(responseCode = OK_CODE, description = PI_USER) @ApiResponse(responseCode = BAD_REQUEST_CODE, description = "{ErroredSystemAdminAccount}") @PostMapping("/add/system-admin") + @PreAuthorize("@authorisationService.userCanCreateSystemAdmin(#issuerId)") public ResponseEntity createSystemAdminAccount(//NOSONAR @RequestHeader(ISSUER_ID) String issuerId, @RequestBody SystemAdminAccount account) { return ResponseEntity.ok(systemAdminB2CAccountService.addSystemAdminAccount(account, issuerId)); diff --git a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationService.java b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationService.java index 0907d9f2..4026ee82 100644 --- a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationService.java +++ b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationService.java @@ -10,6 +10,7 @@ import uk.gov.hmcts.reform.pip.model.account.UserProvenances; import java.util.List; +import java.util.Optional; import java.util.UUID; import static uk.gov.hmcts.reform.pip.model.LogBuilder.writeLog; @@ -70,6 +71,18 @@ public boolean userCanUpdateAccount(UUID userId, UUID adminUserId) { return isAuthorised; } + public boolean userCanCreateSystemAdmin(UUID userId) { + Optional adminUser = userRepository.findByUserId(userId); + boolean isSystemAdmin = adminUser.isPresent() && adminUser.get().getRoles().equals(Roles.SYSTEM_ADMIN); + + if (!isSystemAdmin) { + log.error(writeLog( + String.format("User with ID %s is forbidden to create a B2C system admin", userId) + )); + } + return isSystemAdmin; + } + private boolean isAuthorisedRole(UUID userId, UUID adminUserId) { PiUser user = getUser(userId); if (UserProvenances.SSO.equals(user.getUserProvenance())) { diff --git a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java index 7961ada8..61eccda9 100644 --- a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java +++ b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java @@ -38,20 +38,20 @@ public class SystemAdminB2CAccountService { private final AzureUserService azureUserService; private final UserRepository userRepository; private final PublicationService publicationService; - private final AzureAccountService azureAccountService; + private final AccountService accountService; private final Integer maxSystemAdminValue; @Autowired public SystemAdminB2CAccountService(Validator validator, AzureUserService azureUserService, UserRepository userRepository, PublicationService publicationService, @Value("${admin.max-system-admin}")Integer maxSystemAdminValue, - AzureAccountService azureAccountService) { + AccountService accountService) { this.validator = validator; this.azureUserService = azureUserService; this.userRepository = userRepository; this.publicationService = publicationService; this.maxSystemAdminValue = maxSystemAdminValue; - this.azureAccountService = azureAccountService; + this.accountService = accountService; } /** @@ -61,18 +61,12 @@ public SystemAdminB2CAccountService(Validator validator, AzureUserService azureU * @return The PiUser of the created system admin account. */ public PiUser addSystemAdminAccount(SystemAdminAccount account, String issuerId) { - - String displayName = ""; - String provenanceUserId = verifyAdminUser(issuerId); - if (!provenanceUserId.isEmpty()) { - displayName = azureAccountService.retrieveAzureAccount(provenanceUserId).getDisplayName(); - } - - validateSystemAdminAccount(account, issuerId, displayName); + PiUser piUser = accountService.getUserById(UUID.fromString(issuerId)); + validateSystemAdminAccount(account, issuerId, piUser.getEmail()); try { User user = azureUserService.createUser(account.convertToAzureAccount(), false); PiUser createdUser = userRepository.save(account.convertToPiUser(user.getId())); - handleNewSystemAdminAccountAction(account, issuerId, ActionResult.SUCCEEDED, displayName); + handleNewSystemAdminAccountAction(account, issuerId, ActionResult.SUCCEEDED, piUser.getEmail()); publicationService.sendNotificationEmail( account.getEmail(), @@ -83,19 +77,20 @@ public PiUser addSystemAdminAccount(SystemAdminAccount account, String issuerId) } catch (AzureCustomException e) { ErroredSystemAdminAccount erroredSystemAdminAccount = new ErroredSystemAdminAccount(account); erroredSystemAdminAccount.setErrorMessages(List.of(e.getLocalizedMessage())); - handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, displayName); + handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, piUser.getEmail()); throw new SystemAdminAccountException(erroredSystemAdminAccount); } + } /** * This method handles the logging and publishing that a new system admin account has been created. * @param systemAdminAccount The system admin account that has been created * @param adminId The ID of the admin user who is creating the account. - * @param name The name of the admin user who is creating the account + * @param email The email of the admin user who is creating the account */ public void handleNewSystemAdminAccountAction(SystemAdminAccount systemAdminAccount, String adminId, - ActionResult result, String name) { + ActionResult result, String email) { log.info(writeLog(UUID.fromString(adminId), "has attempted to create a System Admin account, which has: " + result.toString())); @@ -105,7 +100,7 @@ public void handleNewSystemAdminAccountAction(SystemAdminAccount systemAdminAcco CreateSystemAdminAction createSystemAdminAction = new CreateSystemAdminAction(); createSystemAdminAction.setAccountEmail(systemAdminAccount.getEmail()); createSystemAdminAction.setEmailList(existingAdminEmails); - createSystemAdminAction.setRequesterName(name); + createSystemAdminAction.setRequesterEmail(email); createSystemAdminAction.setActionResult(result); publicationService.sendSystemAdminAccountAction(createSystemAdminAction); @@ -115,9 +110,9 @@ public void handleNewSystemAdminAccountAction(SystemAdminAccount systemAdminAcco * A helper method which specifically handles validation failures on the system admin account. * @param account The system admin account to validate. * @param issuerId The ID of the admin user that is issuing the account. - * @param name The name of the admin user requesting the account. + * @param email The email of the admin user requesting the account. */ - private void validateSystemAdminAccount(SystemAdminAccount account, String issuerId, String name) { + private void validateSystemAdminAccount(SystemAdminAccount account, String issuerId, String email) { Set> constraintViolationSet = validator.validate(account); if (!constraintViolationSet.isEmpty()) { @@ -126,14 +121,14 @@ private void validateSystemAdminAccount(SystemAdminAccount account, String issue .stream().map(constraint -> constraint.getPropertyPath() + ": " + constraint.getMessage()).toList()); - handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, name); + handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, email); throw new SystemAdminAccountException(erroredSystemAdminAccount); } if (userRepository.findByEmailAndUserProvenance(account.getEmail(), UserProvenances.PI_AAD).isPresent()) { ErroredSystemAdminAccount erroredSystemAdminAccount = new ErroredSystemAdminAccount(account); erroredSystemAdminAccount.setDuplicate(true); - handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, name); + handleNewSystemAdminAccountAction(account, issuerId, ActionResult.FAILED, email); throw new SystemAdminAccountException(erroredSystemAdminAccount); } @@ -144,22 +139,8 @@ private void validateSystemAdminAccount(SystemAdminAccount account, String issue if (systemAdminUsers.size() >= maxSystemAdminValue) { ErroredSystemAdminAccount erroredSystemAdminAccount = new ErroredSystemAdminAccount(account); erroredSystemAdminAccount.setAboveMaxSystemAdmin(true); - handleNewSystemAdminAccountAction(account, issuerId, ActionResult.ATTEMPTED, name); + handleNewSystemAdminAccountAction(account, issuerId, ActionResult.ATTEMPTED, email); throw new SystemAdminAccountException(erroredSystemAdminAccount); } } - - /** - * Method to find whether user is SYSTEM_ADMIN or not. - * @param issuerId The ID of the admin user - * @return Boolean user is SYSTEM_ADMIN or not - */ - private String verifyAdminUser(String issuerId) { - Optional adminUser = userRepository.findByUserId(UUID.fromString(issuerId)); - if (adminUser.isPresent() && adminUser.get().getRoles().equals(Roles.SYSTEM_ADMIN)) { - return adminUser.get().getProvenanceUserId(); - } - - return ""; - } } From aecbd63aef03df69a04235d70eaa2bb895870ef2 Mon Sep 17 00:00:00 2001 From: ChrisS1512 <87066931+ChrisS1512@users.noreply.github.com> Date: Wed, 4 Dec 2024 14:41:41 +0000 Subject: [PATCH 2/5] PUB-2681 - Add unit and integration tests --- .../SystemAdminB2CAccountTest.java | 23 ++++ .../resources/add-admin-users.sql | 3 +- .../service/SystemAdminB2CAccountService.java | 1 - .../service/AuthorisationServiceTest.java | 42 +++++++ .../SystemAdminB2CAccountServiceTest.java | 107 ++++++++---------- 5 files changed, 115 insertions(+), 61 deletions(-) diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java index 21089fb0..30f83303 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java +++ b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java @@ -54,6 +54,7 @@ class SystemAdminB2CAccountTest { private static final String ISSUER_ID = "1234-1234-1234-1234"; private static final String SYSTEM_ADMIN_ISSUER_ID = "87f907d2-eb28-42cc-b6e1-ae2b03f7bba2"; + private static final String SUPER_ADMIN_ISSUER_ID = "87f907d2-eb28-42cc-b6e1-ae2b03f7bba3"; private static final String ISSUER_HEADER = "x-issuer-id"; private static final String GIVEN_NAME = "Given Name"; private static final String ID = "1234"; @@ -232,4 +233,26 @@ void testUnauthorizedCreateSystemAdminAccount() throws Exception { FORBIDDEN_STATUS_CODE ); } + + @Test + void testCreateSystemAdminUserWhenNotSystemAdmin() throws Exception { + SystemAdminAccount systemAdmin = new SystemAdminAccount(); + systemAdmin.setFirstName(TEST_SYS_ADMIN_FIRSTNAME); + systemAdmin.setSurname(TEST_SYS_ADMIN_SURNAME); + systemAdmin.setEmail(TEST_SYS_ADMIN_EMAIL); + + MockHttpServletRequestBuilder createRequest = + MockMvcRequestBuilders + .post(CREATE_SYSTEM_ADMIN_URL) + .content(OBJECT_MAPPER.writeValueAsString(systemAdmin)) + .header(ISSUER_HEADER, SUPER_ADMIN_ISSUER_ID) + .contentType(MediaType.APPLICATION_JSON); + + MvcResult responseCreateSystemAdminUser = mockMvc.perform(createRequest) + .andExpect(status().isForbidden()).andReturn(); + + assertEquals(FORBIDDEN.value(), responseCreateSystemAdminUser.getResponse().getStatus(), + FORBIDDEN_STATUS_CODE + ); + } } diff --git a/src/integrationTest/resources/add-admin-users.sql b/src/integrationTest/resources/add-admin-users.sql index d978c131..4a548321 100644 --- a/src/integrationTest/resources/add-admin-users.sql +++ b/src/integrationTest/resources/add-admin-users.sql @@ -1,5 +1,4 @@ INSERT INTO pi_user (user_id, email, provenance_user_id,user_provenance,roles,forenames,surname) VALUES -('87f907d2-eb28-42cc-b6e1-ae2b03f7bba2', 'SyestemAdmin@justice.gov.uk', 'e5f1cc77-6e9a-40ab-8da0-a9666b328464','PI_AAD','SYSTEM_ADMIN','System','Admin'), +('87f907d2-eb28-42cc-b6e1-ae2b03f7bba2', 'SystemAdmin@justice.gov.uk', 'e5f1cc77-6e9a-40ab-8da0-a9666b328464','PI_AAD','SYSTEM_ADMIN','System','Admin'), ('87f907d2-eb28-42cc-b6e1-ae2b03f7bba3', 'SuperAdminCtsc@justice.gov.uk', 'e5f1cc77-6e9a-40ab-8da0-a9666b328465','PI_AAD','INTERNAL_SUPER_ADMIN_CTSC','Super','Admin'), ('87f907d2-eb28-42cc-b6e1-ae2b03f7bba4', 'SyestemAdminSso@justice.gov.uk', 'e5f1cc77-6e9a-40ab-8da0-a9666b328466','SSO','SYSTEM_ADMIN','System','Admin'); - diff --git a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java index 61eccda9..ff5e8303 100644 --- a/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java +++ b/src/main/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountService.java @@ -19,7 +19,6 @@ import uk.gov.hmcts.reform.pip.model.system.admin.CreateSystemAdminAction; import java.util.List; -import java.util.Optional; import java.util.Set; import java.util.UUID; diff --git a/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationServiceTest.java b/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationServiceTest.java index abbd93e8..41f25e43 100644 --- a/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationServiceTest.java +++ b/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/AuthorisationServiceTest.java @@ -20,6 +20,7 @@ import java.util.Optional; import java.util.UUID; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.verifyNoInteractions; @@ -667,4 +668,45 @@ void testExceptionThrowIfAdminUserNotFound() { .isInstanceOf(NotFoundException.class) .hasMessage(String.format("User with supplied user id: %s could not be found", ADMIN_USER_ID)); } + + @Test + void testUserCanCreateSystemAdmin() { + UUID userId = UUID.randomUUID(); + PiUser user = new PiUser(); + user.setRoles(Roles.SYSTEM_ADMIN); + + when(userRepository.findByUserId(userId)).thenReturn(Optional.of(user)); + + assertThat(authorisationService.userCanCreateSystemAdmin(userId)).isTrue(); + } + + @Test + void testUserCannotCreateSystemAdminIfAccountNotFound() { + UUID userId = UUID.randomUUID(); + + when(userRepository.findByUserId(userId)).thenReturn(Optional.empty()); + + try (LogCaptor logCaptor = LogCaptor.forClass(AuthorisationService.class)) { + assertThat(authorisationService.userCanCreateSystemAdmin(userId)).isFalse(); + + assertThat(logCaptor.getErrorLogs().get(0)).contains( + String.format("User with ID %s is forbidden to create a B2C system admin", userId)); + } + } + + @Test + void testUserCannotCreateSystemAdminIfUserIsNotSystemAdmin() { + UUID userId = UUID.randomUUID(); + PiUser user = new PiUser(); + user.setRoles(Roles.INTERNAL_ADMIN_LOCAL); + + when(userRepository.findByUserId(userId)).thenReturn(Optional.of(user)); + + try (LogCaptor logCaptor = LogCaptor.forClass(AuthorisationService.class)) { + assertThat(authorisationService.userCanCreateSystemAdmin(userId)).isFalse(); + + assertThat(logCaptor.getErrorLogs().get(0)).contains( + String.format("User with ID %s is forbidden to create a B2C system admin", userId)); + } + } } diff --git a/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountServiceTest.java b/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountServiceTest.java index 38bbf390..8cdec77e 100644 --- a/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountServiceTest.java +++ b/src/test/java/uk/gov/hmcts/reform/pip/account/management/service/SystemAdminB2CAccountServiceTest.java @@ -46,7 +46,7 @@ class SystemAdminB2CAccountServiceTest { private PublicationService publicationService; @Mock - private AzureAccountService azureAccountService; + private AccountService accountService; @Mock private UserRepository userRepository; @@ -62,7 +62,7 @@ class SystemAdminB2CAccountServiceTest { private SystemAdminB2CAccountService systemAdminAccountService; - private static final String ID = UUID.randomUUID().toString(); + private static final UUID ID = UUID.randomUUID(); private static final String EMAIL = "test@email.com"; private static final String FORENAME = "Test"; private static final String SURNAME = "Surname"; @@ -74,16 +74,17 @@ class SystemAdminB2CAccountServiceTest { private final User expectedUser = new User(); private final PiUser expectedPiUser = new PiUser(); private final PiUser ssoUser = new PiUser(); + private final PiUser systemAdminUser = new PiUser(); @BeforeEach void setup() { expectedUser.setGivenName(FORENAME); - expectedUser.setId(ID); + expectedUser.setId(ID.toString()); expectedUser.setSurname(SURNAME); expectedPiUser.setUserId(UUID.randomUUID()); expectedPiUser.setEmail(EMAIL); - expectedPiUser.setProvenanceUserId(ID); + expectedPiUser.setProvenanceUserId(ID.toString()); expectedPiUser.setRoles(Roles.SYSTEM_ADMIN); expectedPiUser.setUserProvenance(UserProvenances.PI_AAD); @@ -93,9 +94,13 @@ void setup() { ssoUser.setRoles(Roles.SYSTEM_ADMIN); ssoUser.setUserProvenance(UserProvenances.SSO); + systemAdminUser.setRoles(Roles.SYSTEM_ADMIN); + systemAdminUser.setUserId(ID); + systemAdminUser.setEmail(EMAIL); + systemAdminAccountService = new SystemAdminB2CAccountService(validator, azureUserService, userRepository, publicationService, 4, - azureAccountService); + accountService); } @@ -107,14 +112,16 @@ void testAddSystemAdminAccount() throws AzureCustomException { when(azureUserService.createUser(argThat(user -> EMAIL.equals(user.getEmail())), anyBoolean())) .thenReturn(expectedUser); when(userRepository.save(any())).thenReturn(expectedPiUser); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); when(publicationService.sendNotificationEmail(EMAIL, FORENAME, SURNAME)).thenReturn(Boolean.TRUE); - when(userRepository.findByUserId(any())).thenReturn(Optional.ofNullable(expectedPiUser)); + + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> EMAIL.equals(arg.getRequesterEmail()))); + + when(accountService.getUserById(ID)).thenReturn(systemAdminUser); when(validator.validate(SYSTEM_ADMIN_ACCOUNT)).thenReturn(Set.of()); when(userRepository.findByRoles(Roles.SYSTEM_ADMIN)).thenReturn(List.of(expectedPiUser)); - PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID); + PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString()); assertEquals(expectedPiUser, returnedUser, USER_MESSAGE); } @@ -124,16 +131,13 @@ void testAddSystemAdminAccountThrowsException() throws AzureCustomException { AzureAccount azUser = new AzureAccount(); azUser.setDisplayName(FORENAME); - when(userRepository.findByUserId(any())) - .thenReturn(Optional.ofNullable(expectedPiUser)); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); + when(accountService.getUserById(ID)).thenReturn(systemAdminUser); when(azureUserService.createUser(argThat(user -> EMAIL.equals(user.getEmail())), anyBoolean())) .thenThrow(new AzureCustomException("Test error")); SystemAdminAccountException systemAdminAccountException = assertThrows(SystemAdminAccountException.class, () -> - systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID)); + systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString())); assertEquals("Test error", @@ -146,41 +150,22 @@ void testConstraintViolationException() { AzureAccount azUser = new AzureAccount(); azUser.setDisplayName(FORENAME); - when(userRepository.findByUserId(any())) - .thenReturn(Optional.ofNullable(expectedPiUser)); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); + when(accountService.getUserById(ID)).thenReturn(systemAdminUser); when(validator.validate(any())).thenReturn(Set.of(constraintViolation)); when(constraintViolation.getMessage()).thenReturn("This is a message"); when(constraintViolation.getPropertyPath()).thenReturn(path); + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> EMAIL.equals(arg.getRequesterEmail()))); + SystemAdminAccountException systemAdminAccountException = assertThrows(SystemAdminAccountException.class, () -> - systemAdminAccountService.addSystemAdminAccount(ERRORED_SYSTEM_ADMIN_ACCOUNT, ID)); + systemAdminAccountService.addSystemAdminAccount(ERRORED_SYSTEM_ADMIN_ACCOUNT, ID.toString())); assertNotEquals(0, systemAdminAccountException.getErroredSystemAdminAccount().getErrorMessages().size(), "Constraint violation error messages not displayed"); } - @Test - void testAddSystemAdminAccountNotVerified() throws AzureCustomException { - AzureAccount azUser = new AzureAccount(); - azUser.setDisplayName(FORENAME); - - expectedPiUser.setRoles(Roles.VERIFIED); - when(azureUserService.createUser(argThat(user -> EMAIL.equals(user.getEmail())), anyBoolean())) - .thenReturn(expectedUser); - when(userRepository.save(any())).thenReturn(expectedPiUser); - when(publicationService.sendNotificationEmail(EMAIL, FORENAME, SURNAME)).thenReturn(Boolean.FALSE); - when(userRepository.findByUserId(any())).thenReturn(Optional.ofNullable(expectedPiUser)); - when(validator.validate(SYSTEM_ADMIN_ACCOUNT)).thenReturn(Set.of()); - when(userRepository.findByRoles(Roles.SYSTEM_ADMIN)).thenReturn(List.of(expectedPiUser)); - - PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID); - - assertEquals(expectedPiUser, returnedUser, USER_MESSAGE); - } - @Test void testAddSystemAdminAccountNotExists() throws AzureCustomException { AzureAccount azUser = new AzureAccount(); @@ -191,11 +176,13 @@ void testAddSystemAdminAccountNotExists() throws AzureCustomException { .thenReturn(expectedUser); when(userRepository.save(any())).thenReturn(expectedPiUser); when(publicationService.sendNotificationEmail(EMAIL, FORENAME, SURNAME)).thenReturn(Boolean.FALSE); - when(userRepository.findByUserId(any())).thenReturn(Optional.empty()); + when(accountService.getUserById(ID)).thenReturn(new PiUser()); when(validator.validate(SYSTEM_ADMIN_ACCOUNT)).thenReturn(Set.of()); when(userRepository.findByRoles(Roles.SYSTEM_ADMIN)).thenReturn(List.of(expectedPiUser)); + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> arg.getRequesterEmail() == null)); - PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID); + PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString()); assertEquals(expectedPiUser, returnedUser, USER_MESSAGE); } @@ -206,14 +193,14 @@ void testUserAlreadyExists() { azUser.setDisplayName(FORENAME); when(userRepository.findByEmailAndUserProvenance(EMAIL, UserProvenances.PI_AAD)) .thenReturn(Optional.of(expectedPiUser)); - when(userRepository.findByUserId(any())) - .thenReturn(Optional.ofNullable(expectedPiUser)); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> EMAIL.equals(arg.getRequesterEmail()))); + when(accountService.getUserById(ID)) + .thenReturn(systemAdminUser); SystemAdminAccountException systemAdminAccountException = assertThrows(SystemAdminAccountException.class, () -> - systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID)); + systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString())); assertTrue(systemAdminAccountException.getErroredSystemAdminAccount().isDuplicate(), "Duplicate account flag " + "not set"); @@ -225,16 +212,17 @@ void testAboveMaxAllowsUsersWithAllAadUsers() { azUser.setDisplayName(FORENAME); when(userRepository.findByEmailAndUserProvenance(EMAIL, UserProvenances.PI_AAD)) .thenReturn(Optional.empty()); - when(userRepository.findByUserId(any())) - .thenReturn(Optional.ofNullable(expectedPiUser)); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); + when(accountService.getUserById(ID)) + .thenReturn(systemAdminUser); when(userRepository.findByRoles(Roles.SYSTEM_ADMIN)).thenReturn(List.of(expectedPiUser, expectedPiUser, expectedPiUser, expectedPiUser)); + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> EMAIL.equals(arg.getRequesterEmail()))); + SystemAdminAccountException systemAdminAccountException = assertThrows(SystemAdminAccountException.class, () -> - systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID)); + systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString())); assertTrue(systemAdminAccountException.getErroredSystemAdminAccount().isAboveMaxSystemAdmin(), "Max system " + "admin flag not set"); @@ -246,17 +234,18 @@ void testAboveMaxAllowsUsersNotIncludingSsoUser() throws AzureCustomException { azUser.setDisplayName(FORENAME); when(userRepository.findByEmailAndUserProvenance(EMAIL, UserProvenances.PI_AAD)) .thenReturn(Optional.empty()); - when(userRepository.findByUserId(any())) - .thenReturn(Optional.ofNullable(expectedPiUser)); - when(azureAccountService.retrieveAzureAccount(any())) - .thenReturn(azUser); + when(accountService.getUserById(ID)) + .thenReturn(systemAdminUser); when(userRepository.findByRoles(Roles.SYSTEM_ADMIN)).thenReturn(List.of(expectedPiUser, expectedPiUser, expectedPiUser, ssoUser)); + doNothing().when(publicationService) + .sendSystemAdminAccountAction(argThat(arg -> EMAIL.equals(arg.getRequesterEmail()))); + when(azureUserService.createUser(argThat(user -> EMAIL.equals(user.getEmail())), anyBoolean())) .thenReturn(expectedUser); when(userRepository.save(any())).thenReturn(expectedPiUser); - PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID); + PiUser returnedUser = systemAdminAccountService.addSystemAdminAccount(SYSTEM_ADMIN_ACCOUNT, ID.toString()); assertEquals(expectedPiUser, returnedUser, USER_MESSAGE); } @@ -270,13 +259,15 @@ void testHandleNewSystemAdminAccountAction() { doNothing().when(publicationService).sendSystemAdminAccountAction(systemAdminAccountArgumentCaptor.capture()); - systemAdminAccountService.handleNewSystemAdminAccountAction(SYSTEM_ADMIN_ACCOUNT, ID, ActionResult.ATTEMPTED, - FORENAME); + systemAdminAccountService.handleNewSystemAdminAccountAction(SYSTEM_ADMIN_ACCOUNT, + ID.toString(), + ActionResult.ATTEMPTED, + EMAIL); CreateSystemAdminAction createSystemAdminAction = systemAdminAccountArgumentCaptor.getValue(); assertEquals(EMAIL, createSystemAdminAction.getAccountEmail(), "Unknown email retrieved"); - assertEquals(FORENAME, createSystemAdminAction.getRequesterName(), "Unknown requester name retrieved"); + assertEquals(EMAIL, createSystemAdminAction.getRequesterEmail(), "Unknown requester name retrieved"); assertEquals(List.of(EMAIL, EMAIL), createSystemAdminAction.getEmailList(), "Unknown email list retrieved"); assertEquals(ActionResult.ATTEMPTED, createSystemAdminAction.getActionResult(), "Action result not as expected"); From 180b67b2a7e958c0be12b5858429c92703ec4b16 Mon Sep 17 00:00:00 2001 From: ChrisS1512 <87066931+ChrisS1512@users.noreply.github.com> Date: Thu, 5 Dec 2024 13:40:27 +0000 Subject: [PATCH 3/5] PUB-2681 - Updated integration tests --- .../reform/pip/account/management/controllers/AccountTest.java | 2 ++ .../management/controllers/SystemAdminB2CAccountTest.java | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java index f6913d0e..701ab8ea 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java +++ b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java @@ -610,6 +610,7 @@ void testDeleteAccountNotFound() throws Exception { } @Test + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") void testV2SystemAdminDeletesVerifiedUser() throws Exception { validUser.setUserProvenance(UserProvenances.CFT_IDAM); validUser.setRoles(Roles.VERIFIED); @@ -675,6 +676,7 @@ void testV2SystemAdminDeletesThirdPartyUser() throws Exception { } @Test + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") void testV2SystemAdminDeletesSuperAdminUser() throws Exception { superAdminUser.setUserProvenance(UserProvenances.CFT_IDAM); String superAdminUserId = getSuperAdminUserId(superAdminUser); diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java index 30f83303..f347f36f 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java +++ b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountTest.java @@ -32,6 +32,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.UUID; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; @@ -223,7 +224,7 @@ void testUnauthorizedCreateSystemAdminAccount() throws Exception { MockMvcRequestBuilders .post(CREATE_SYSTEM_ADMIN_URL) .content(OBJECT_MAPPER.writeValueAsString(systemAdmin)) - .header(ISSUER_HEADER, ISSUER_ID) + .header(ISSUER_HEADER, UUID.randomUUID().toString()) .contentType(MediaType.APPLICATION_JSON); MvcResult responseCreateSystemAdminUser = mockMvc.perform(createRequest) From 0cfd0a1e0be1dc46732745aacefdd15411d2bd2c Mon Sep 17 00:00:00 2001 From: ChrisS1512 <87066931+ChrisS1512@users.noreply.github.com> Date: Fri, 13 Dec 2024 11:15:22 +0000 Subject: [PATCH 4/5] PUB-2681 - Fixed PMD issue --- .../account/management/controllers/AccountTest.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java index 701ab8ea..100c98ee 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java +++ b/src/integrationTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/AccountTest.java @@ -119,6 +119,7 @@ class AccountTest { private static final String FORBIDDEN_STATUS_CODE = "Status code does not match forbidden"; private static final String DELETE_USER_FAILURE = "Failed to delete user account"; private static final String DELETE_USER_SUCCESS = "User deleted"; + private static final String ADD_USERS_SCRIPT = "classpath:add-admin-users.sql"; private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); @@ -610,7 +611,7 @@ void testDeleteAccountNotFound() throws Exception { } @Test - @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = ADD_USERS_SCRIPT) void testV2SystemAdminDeletesVerifiedUser() throws Exception { validUser.setUserProvenance(UserProvenances.CFT_IDAM); validUser.setRoles(Roles.VERIFIED); @@ -644,7 +645,7 @@ void testV2SystemAdminDeletesVerifiedUser() throws Exception { } @Test - @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = ADD_USERS_SCRIPT) void testV2SystemAdminDeletesThirdPartyUser() throws Exception { PiUser thirdPartyUser = createThirdPartyUser(); @@ -676,7 +677,7 @@ void testV2SystemAdminDeletesThirdPartyUser() throws Exception { } @Test - @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = ADD_USERS_SCRIPT) void testV2SystemAdminDeletesSuperAdminUser() throws Exception { superAdminUser.setUserProvenance(UserProvenances.CFT_IDAM); String superAdminUserId = getSuperAdminUserId(superAdminUser); @@ -897,7 +898,7 @@ void testUpdateAccountRoleByIdNotFound() throws Exception { } @Test - @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = ADD_USERS_SCRIPT) void testCreateThirdPartyUser() throws Exception { PiUser thirdPartyUser = createThirdPartyUser(); MockHttpServletRequestBuilder mockHttpServletRequestBuilder = MockMvcRequestBuilders @@ -920,7 +921,7 @@ void testCreateThirdPartyUser() throws Exception { } @Test - @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = "classpath:add-admin-users.sql") + @Sql(executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD, scripts = ADD_USERS_SCRIPT) void testUnauthorizedCreateThirdPartyUser() throws Exception { PiUser thirdPartyUser = createUser(true, Roles.GENERAL_THIRD_PARTY); MockHttpServletRequestBuilder mockHttpServletRequestBuilder = MockMvcRequestBuilders From fbd9ae94759c7cd626dcc22fc5fd06ee9d4b9bd3 Mon Sep 17 00:00:00 2001 From: ChrisS1512 <87066931+ChrisS1512@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:14:46 +0000 Subject: [PATCH 5/5] PUB-2681 - Updated functional tests --- .../SystemAdminB2CAccountCreationTest.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/functionalTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountCreationTest.java b/src/functionalTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountCreationTest.java index 0143962b..6063802b 100644 --- a/src/functionalTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountCreationTest.java +++ b/src/functionalTest/java/uk/gov/hmcts/reform/pip/account/management/controllers/SystemAdminB2CAccountCreationTest.java @@ -27,14 +27,17 @@ class SystemAdminB2CAccountCreationTest extends FunctionalTestBase { "pip-am-test-email-%s", ThreadLocalRandom.current().nextInt(1000, 9999)); private static final String TEST_USER_EMAIL_PREFIX_2 = String.format( "pip-am-test-email-%s", ThreadLocalRandom.current().nextInt(1000, 9999)); + private static final String TEST_USER_EMAIL_PREFIX_3 = String.format( + "pip-am-test-email-%s", ThreadLocalRandom.current().nextInt(1000, 9999)); private static final String TEST_USER_EMAIL_1 = TEST_USER_EMAIL_PREFIX_1 + "@justice.gov.uk"; private static final String TEST_USER_EMAIL_2 = TEST_USER_EMAIL_PREFIX_2 + "@justice.gov.uk"; + private static final String TEST_USER_EMAIL_3 = TEST_USER_EMAIL_PREFIX_3 + "@justice.gov.uk"; private static final String TEST_USER_PROVENANCE_ID = UUID.randomUUID().toString(); - private static final String USER_ID = UUID.randomUUID().toString(); private static final String TESTING_SUPPORT_ACCOUNT_URL = "/testing-support/account/"; private static final String ACCOUNT_URL = "/account"; private static final String SYSTEM_ADMIN_B2C_URL = ACCOUNT_URL + "/add/system-admin"; + private static final String SYSTEM_ADMIN_SSO_URL = ACCOUNT_URL + "/system-admin"; private static final String BEARER = "Bearer "; private static final String ISSUER_ID = "x-issuer-id"; @@ -44,13 +47,25 @@ class SystemAdminB2CAccountCreationTest extends FunctionalTestBase { @BeforeAll public void startUp() { bearer = Map.of(HttpHeaders.AUTHORIZATION, BEARER + accessToken); - issuerId = Map.of(ISSUER_ID, USER_ID); + + String requestBody = """ + { + "email": "%s", + "provenanceUserId": "%s" + } + """.formatted(TEST_USER_EMAIL_3, UUID.randomUUID().toString()); + + String userId = doPostRequest(SYSTEM_ADMIN_SSO_URL, bearer, requestBody) + .jsonPath().getString("userId"); + + issuerId = Map.of(ISSUER_ID, userId); } @AfterAll public void teardown() { doDeleteRequest(TESTING_SUPPORT_ACCOUNT_URL + TEST_USER_EMAIL_1, bearer); doDeleteRequest(TESTING_SUPPORT_ACCOUNT_URL + TEST_USER_EMAIL_2, bearer); + doDeleteRequest(TESTING_SUPPORT_ACCOUNT_URL + TEST_USER_EMAIL_3, bearer); } @Test