From c303d46d8ef7a2f6534e094271e40aa065e65265 Mon Sep 17 00:00:00 2001 From: Maverick Studer Date: Fri, 14 Jun 2024 11:51:51 +0200 Subject: [PATCH] RED-8491: Hide all KNECON_* roles for any possible access in all endpoints && RED-9346: User without roles not displayed in GET endpoint --- .../api/external/UserResource.java | 6 +- .../service/UserService.java | 8 +- .../tenantusermanagement/tests/UserTest.java | 98 +++++++++++++++---- 3 files changed, 84 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/knecon/fforesight/tenantusermanagement/api/external/UserResource.java b/src/main/java/com/knecon/fforesight/tenantusermanagement/api/external/UserResource.java index 5639074..7738dff 100644 --- a/src/main/java/com/knecon/fforesight/tenantusermanagement/api/external/UserResource.java +++ b/src/main/java/com/knecon/fforesight/tenantusermanagement/api/external/UserResource.java @@ -61,7 +61,7 @@ public interface UserResource { @ResponseBody @ResponseStatus(value = HttpStatus.OK) @Operation(summary = "Update a user profile", description = "None") - @ApiResponses(value = {@ApiResponse(responseCode = "204", description = "OK"), @ApiResponse(responseCode = "400", description = "Failed to update profile, e-mail cannot be empty")}) + @ApiResponses(value = {@ApiResponse(responseCode = "204", description = "OK"), @ApiResponse(responseCode = "400", description = "Failed to update profile, e-mail invalid")}) @PostMapping(value = UPDATE_USER_PROFILE_PATH + USER_ID_PATH_VARIABLE, consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) User updateProfile(@PathVariable(USER_ID) String userId, @RequestBody UpdateProfileRequest updateProfileRequest); @@ -69,7 +69,7 @@ public interface UserResource { @ResponseBody @ResponseStatus(value = HttpStatus.OK) @Operation(summary = "Update your user profile", description = "None") - @ApiResponses(value = {@ApiResponse(responseCode = "204", description = "OK"), @ApiResponse(responseCode = "400", description = "Failed to update profile, e-mail cannot be empty")}) + @ApiResponses(value = {@ApiResponse(responseCode = "204", description = "OK"), @ApiResponse(responseCode = "400", description = "Failed to update profile, e-mail invalid")}) @PostMapping(value = UPDATE_MY_USER_PROFILE_PATH, consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) User updateMyProfile(@RequestBody UpdateMyProfileRequest updateProfileRequest); @@ -92,7 +92,7 @@ public interface UserResource { @ResponseBody @Operation(summary = "Create a new user", description = "None") - @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "OK"), @ApiResponse(responseCode = "400", description = "Invalid Data."), @ApiResponse(responseCode = "409", description = "User already exists.")}) + @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "OK"), @ApiResponse(responseCode = "400", description = "Invalid Data.")}) @PostMapping(value = USER_REST_PATH, consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) User createUser(@RequestBody CreateUserRequest user); diff --git a/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java b/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java index 50bcc6f..c3565b4 100644 --- a/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java +++ b/src/main/java/com/knecon/fforesight/tenantusermanagement/service/UserService.java @@ -83,7 +83,7 @@ public class UserService { } if (!EmailValidator.getInstance().isValid(user.getEmail())) { - throw new ResponseStatusException(HttpStatus.CONFLICT, "Email address format is not valid"); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Email address format is not valid"); } tenantUserManagementProperties.getKcRoleMapping().validateRoles(user.getRoles()); @@ -101,7 +101,7 @@ public class UserService { if (response.getStatusInfo().getFamily() != Response.Status.Family.SUCCESSFUL) { if (response.getStatusInfo().getStatusCode() == 409) { - throw new ResponseStatusException(HttpStatus.CONFLICT, response.getStatusInfo().getReasonPhrase()); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, response.getStatusInfo().getReasonPhrase()); } if (response.getStatusInfo().getStatusCode() == 400) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, response.getStatusInfo().getReasonPhrase()); @@ -282,7 +282,7 @@ public class UserService { user.update(userRepresentation); } catch (ClientErrorException e) { if (e.getResponse().getStatus() == 409) { - throw new ResponseStatusException(HttpStatus.CONFLICT, "E-mail already in use"); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "E-mail is not available"); } throw e; } @@ -454,7 +454,7 @@ public class UserService { } if (!EmailValidator.getInstance().isValid(updateProfileRequest.getEmail())) { - throw new ResponseStatusException(HttpStatus.CONFLICT, "Email address format is not valid"); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Email address format is not valid"); } userRepresentation.setFirstName(updateProfileRequest.getFirstName()); diff --git a/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java b/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java index 70c2419..d37b644 100644 --- a/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java +++ b/src/test/java/com/knecon/fforesight/tenantusermanagement/tests/UserTest.java @@ -57,8 +57,10 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { assertThat(allUsers).isNotEmpty(); assertThat(testUserFound).isTrue(); - var optionalUser = allUsers.stream().filter(user -> user.getUsername().equalsIgnoreCase("test@fforesight.com")).findFirst(); - assert(optionalUser.isPresent()); + var optionalUser = allUsers.stream() + .filter(user -> user.getUsername().equalsIgnoreCase("test@fforesight.com")) + .findFirst(); + assert (optionalUser.isPresent()); var testUser = userClient.getUserById(optionalUser.get().getUserId()); assertThat(testUser).isNotNull(); @@ -70,8 +72,8 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { assertThat(testUser.getLastName()).isEqualTo("updateTestLastName"); assertThat(testUser.getFirstName()).isEqualTo("updateTestFirstName"); - - Set allButKneconRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles().stream() + Set allButKneconRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles() + .stream() .filter(ApplicationRoles::isNoKneconRole) .collect(Collectors.toSet()); @@ -102,8 +104,7 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { .lastName("update test") .roles(allButKneconRoles) .build()); - assertThat(createdUser.getRoles()).containsExactly(allButKneconRoles - .toArray(new String[0])); + assertThat(createdUser.getRoles()).containsExactly(allButKneconRoles.toArray(new String[0])); userClient.deleteUsers(List.of(createdUser.getUserId())); @@ -300,7 +301,8 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { public void testCreateUserWithExistingUser() { TenantContext.setTenantId(AbstractTenantUserManagementIntegrationTest.TEST_TENANT_ID); - Set allButKneconRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles().stream() + Set allButKneconRoles = tenantUserManagementProperties.getKcRoleMapping().getAllRoles() + .stream() .filter(ApplicationRoles::isNoKneconRole) .collect(Collectors.toSet()); @@ -406,14 +408,19 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { var allUsers = userClient.getAllUsers(true); // should not contain the knecon user but the others - assertTrue(allUsers.stream().anyMatch(u -> u.getUserId().equals(user.getUserId()))); - assertTrue(allUsers.stream().anyMatch(u -> u.getUserId().equals(noKneconUser.getUserId()))); - assertFalse(allUsers.stream().anyMatch(u -> u.getUserId().equals(onlyKneconUser.getUserId()))); + assertTrue(allUsers.stream() + .anyMatch(u -> u.getUserId().equals(user.getUserId()))); + assertTrue(allUsers.stream() + .anyMatch(u -> u.getUserId().equals(noKneconUser.getUserId()))); + assertFalse(allUsers.stream() + .anyMatch(u -> u.getUserId().equals(onlyKneconUser.getUserId()))); var foundUser = allUsers.stream() .filter(u -> u.getUserId().equals(user.getUserId())) .findFirst(); assertTrue(foundUser.isPresent()); - assertFalse(foundUser.get().getRoles().stream().anyMatch(ApplicationRoles::isKneconRole)); + assertFalse(foundUser.get().getRoles() + .stream() + .anyMatch(ApplicationRoles::isKneconRole)); // should not be found var e = assertThrows(FeignException.class, () -> userClient.getUserById(onlyKneconUser.getUserId())); @@ -421,22 +428,33 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { // should not show any knecon roles User userById = userClient.getUserById(user.getUserId()); - assertTrue(userById.getRoles().stream().noneMatch(ApplicationRoles::isKneconRole)); + assertTrue(userById.getRoles() + .stream() + .noneMatch(ApplicationRoles::isKneconRole)); List applicationSpecificUsers = userClient.getApplicationSpecificUsers(true); // should not contain the knecon user but the others - assertTrue(applicationSpecificUsers.stream().anyMatch(u -> u.getUserId().equals(user.getUserId()))); - assertTrue(applicationSpecificUsers.stream().anyMatch(u -> u.getUserId().equals(noKneconUser.getUserId()))); - assertFalse(applicationSpecificUsers.stream().anyMatch(u -> u.getUserId().equals(onlyKneconUser.getUserId()))); + assertTrue(applicationSpecificUsers.stream() + .anyMatch(u -> u.getUserId().equals(user.getUserId()))); + assertTrue(applicationSpecificUsers.stream() + .anyMatch(u -> u.getUserId().equals(noKneconUser.getUserId()))); + assertFalse(applicationSpecificUsers.stream() + .anyMatch(u -> u.getUserId().equals(onlyKneconUser.getUserId()))); foundUser = applicationSpecificUsers.stream() .filter(u -> u.getUserId().equals(user.getUserId())) .findFirst(); assertTrue(foundUser.isPresent()); - assertFalse(foundUser.get().getRoles().stream().anyMatch(ApplicationRoles::isKneconRole)); + assertFalse(foundUser.get().getRoles() + .stream() + .anyMatch(ApplicationRoles::isKneconRole)); // no knecon roles are visible - assertEquals(allUsers.stream().filter(u -> u.getRoles().stream().anyMatch(ApplicationRoles::isKneconRole)).count(), 0); + assertEquals(allUsers.stream() + .filter(u -> u.getRoles() + .stream() + .anyMatch(ApplicationRoles::isKneconRole)) + .count(), 0); // we can get this user because of other roles being present userClient.getUserById(noKneconUser.getUserId()); @@ -520,7 +538,8 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { userClient.deleteUsers(List.of(user4.getUserId())); allUsers = userClient.getAllUsers(true); - assertTrue(allUsers.stream().anyMatch(u -> u.getUserId().equals(user4.getUserId()))); + assertTrue(allUsers.stream() + .anyMatch(u -> u.getUserId().equals(user4.getUserId()))); // and should not have changed var user5 = userClient.getUserById(user4.getUserId()); assertEquals(user4, user5); @@ -528,7 +547,6 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { } - @Test public void testShowUserWithoutRoles() { @@ -559,11 +577,49 @@ public class UserTest extends AbstractTenantUserManagementIntegrationTest { User userById = userClient.getUserById(noRolesUser.getUserId()); List allUsers = userClient.getAllUsers(true); - assertTrue(allUsers.stream().anyMatch(u -> u.getUserId().equals(noRolesUser.getUserId()))); + assertTrue(allUsers.stream() + .anyMatch(u -> u.getUserId().equals(noRolesUser.getUserId()))); List applicationSpecificUsers = userClient.getApplicationSpecificUsers(true); - assertFalse(applicationSpecificUsers.stream().anyMatch(u -> u.getUserId().equals(noRolesUser.getUserId()))); + assertFalse(applicationSpecificUsers.stream() + .anyMatch(u -> u.getUserId().equals(noRolesUser.getUserId()))); + } + + + @Test + public void testCreateUserWithInvalidEmailFormat() { + + // set context and user + TenantContext.setTenantId(AbstractTenantUserManagementIntegrationTest.TEST_TENANT_ID); + tokenService.setUser("admin@knecon.com", "secret"); + + // create several users with different roles for testing + var createUserRequest = new CreateUserRequest(); + createUserRequest.setFirstName("My nice"); + createUserRequest.setLastName("User"); + createUserRequest.setUsername("MyNiceUser"); + createUserRequest.setRoles(new HashSet<>()); + + createUserRequest.setEmail(null); + var e = assertThrows(FeignException.class, () -> userClient.createUser(createUserRequest)); + assertEquals(400, e.status()); + + createUserRequest.setEmail(""); + e = assertThrows(FeignException.class, () -> userClient.createUser(createUserRequest)); + assertEquals(400, e.status()); + + createUserRequest.setEmail("myniceuser@notknecon"); + e = assertThrows(FeignException.class, () -> userClient.createUser(createUserRequest)); + assertEquals(400, e.status()); + + createUserRequest.setEmail("myniceuser.com"); + e = assertThrows(FeignException.class, () -> userClient.createUser(createUserRequest)); + assertEquals(400, e.status()); + + createUserRequest.setEmail("myniceuser@notknecon@at.com"); + e = assertThrows(FeignException.class, () -> userClient.createUser(createUserRequest)); + assertEquals(400, e.status()); }