From 6dbab1737ed64ef632afeb24de47ce21d5d95b90 Mon Sep 17 00:00:00 2001 From: Peter Bagrij Date: Sun, 10 Dec 2023 12:57:15 +0100 Subject: [PATCH] FINERACT-1971 Improve Request Validation --- .../api/UsersApiResourceSwagger.java | 26 ++++++- ...WritePlatformServiceJpaRepositoryImpl.java | 12 +-- .../service/UserDataValidator.java | 59 +++++++++++---- .../integrationtests/NotificationApiTest.java | 2 +- .../UserAdministrationTest.java | 74 ++++++++++++++++++- .../client/IntegrationTest.java | 16 ++-- .../useradministration/users/UserHelper.java | 2 +- 7 files changed, 160 insertions(+), 31 deletions(-) diff --git a/fineract-provider/src/main/java/org/apache/fineract/useradministration/api/UsersApiResourceSwagger.java b/fineract-provider/src/main/java/org/apache/fineract/useradministration/api/UsersApiResourceSwagger.java index f1564222fb1..35a13cb93fa 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/useradministration/api/UsersApiResourceSwagger.java +++ b/fineract-provider/src/main/java/org/apache/fineract/useradministration/api/UsersApiResourceSwagger.java @@ -127,10 +127,14 @@ private PostUsersRequest() { @Schema(example = "1") public Long staffId; @Schema(example = "[2,3]") - public List roles; + public List roles; + @Schema(example = "[2,3]") + public List clients; @Schema(example = "true") public Boolean sendPasswordToEmail; @Schema(example = "true") + public Boolean passwordNeverExpires; + @Schema(example = "true") public Boolean isSelfServiceUser; } @@ -156,10 +160,26 @@ private PutUsersUserIdRequest() { @Schema(example = "Test") public String firstname; - @Schema(example = "window75") + @Schema(example = "User") + public String lastname; + @Schema(example = "whatever@mifos.org") + public String email; + @Schema(example = "1") + public Long officeId; + @Schema(example = "1") + public Long staffId; + @Schema(example = "[2,3]") + public List roles; + @Schema(example = "[2,3]") + public List clients; + @Schema(example = "password") public String password; - @Schema(example = "window75") + @Schema(example = "repeatPassword") public String repeatPassword; + @Schema(example = "true") + public Boolean sendPasswordToEmail; + @Schema(example = "true") + public Boolean isSelfServiceUser; } @Schema(description = "PutUsersUserIdResponse") diff --git a/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java b/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java index 10f09fafd3b..6805f90ea46 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java +++ b/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java @@ -18,6 +18,8 @@ */ package org.apache.fineract.useradministration.service; +import static org.apache.fineract.useradministration.service.AppUserConstants.CLIENTS; + import com.google.gson.JsonArray; import com.google.gson.JsonElement; import jakarta.persistence.PersistenceException; @@ -112,8 +114,8 @@ public CommandProcessingResult createUser(final JsonCommand command) { Collection clients; if (command.hasParameter(AppUserConstants.IS_SELF_SERVICE_USER) && command.booleanPrimitiveValueOfParameterNamed(AppUserConstants.IS_SELF_SERVICE_USER) - && command.hasParameter(AppUserConstants.CLIENTS)) { - JsonArray clientsArray = command.arrayOfParameterNamed(AppUserConstants.CLIENTS); + && command.hasParameter(CLIENTS)) { + JsonArray clientsArray = command.arrayOfParameterNamed(CLIENTS); Collection clientIds = new HashSet<>(); for (JsonElement clientElement : clientsArray) { clientIds.add(clientElement.getAsLong()); @@ -159,7 +161,7 @@ public CommandProcessingResult updateUser(final Long userId, final JsonCommand c try { this.context.authenticatedUser(new CommandWrapperBuilder().updateUser(null).build()); - this.fromApiJsonDeserializer.validateForUpdate(command.json()); + this.fromApiJsonDeserializer.validateForUpdate(command.json(), this.context.authenticatedUser()); final AppUser userToUpdate = this.appUserRepository.findById(userId).orElseThrow(() -> new UserNotFoundException(userId)); @@ -171,8 +173,8 @@ public CommandProcessingResult updateUser(final Long userId, final JsonCommand c isSelfServiceUser = command.booleanPrimitiveValueOfParameterNamed(AppUserConstants.IS_SELF_SERVICE_USER); } - if (isSelfServiceUser && command.hasParameter(AppUserConstants.CLIENTS)) { - JsonArray clientsArray = command.arrayOfParameterNamed(AppUserConstants.CLIENTS); + if (isSelfServiceUser && command.hasParameter(CLIENTS)) { + JsonArray clientsArray = command.arrayOfParameterNamed(CLIENTS); Collection clientIds = new HashSet<>(); for (JsonElement clientElement : clientsArray) { clientIds.add(clientElement.getAsLong()); diff --git a/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java b/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java index c98f7aa0281..03575747b3c 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java +++ b/fineract-provider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java @@ -18,8 +18,11 @@ */ package org.apache.fineract.useradministration.service; +import static org.apache.fineract.useradministration.service.AppUserConstants.CLIENTS; + import com.google.gson.JsonArray; import com.google.gson.JsonElement; +import com.google.gson.JsonObject; import com.google.gson.reflect.TypeToken; import java.lang.reflect.Type; import java.util.ArrayList; @@ -34,6 +37,7 @@ import org.apache.fineract.infrastructure.core.exception.InvalidJsonException; import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException; import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper; +import org.apache.fineract.useradministration.domain.AppUser; import org.apache.fineract.useradministration.domain.PasswordValidationPolicy; import org.apache.fineract.useradministration.domain.PasswordValidationPolicyRepository; import org.springframework.beans.factory.annotation.Autowired; @@ -57,9 +61,9 @@ public final class UserDataValidator { /** * The parameters supported for this command. */ - private static final Set SUPPORTED_PARAMETERS = new HashSet<>(Arrays.asList(USERNAME, FIRSTNAME, LASTNAME, PASSWORD, - REPEAT_PASSWORD, EMAIL, OFFICE_ID, NOT_SELECTED_ROLES, ROLES, SEND_PASSWORD_TO_EMAIL, STAFF_ID, PASSWORD_NEVER_EXPIRES, - AppUserConstants.IS_SELF_SERVICE_USER, AppUserConstants.CLIENTS)); + private static final Set SUPPORTED_PARAMETERS = new HashSet<>( + Arrays.asList(USERNAME, FIRSTNAME, LASTNAME, PASSWORD, REPEAT_PASSWORD, EMAIL, OFFICE_ID, NOT_SELECTED_ROLES, ROLES, + SEND_PASSWORD_TO_EMAIL, STAFF_ID, PASSWORD_NEVER_EXPIRES, AppUserConstants.IS_SELF_SERVICE_USER, CLIENTS)); public static final String PASSWORD_NEVER_EXPIRE = "passwordNeverExpire"; private final FromJsonHelper fromApiJsonHelper; @@ -139,17 +143,17 @@ public void validateForCreate(final String json) { } } - if (this.fromApiJsonHelper.parameterExists(AppUserConstants.CLIENTS, element)) { + if (this.fromApiJsonHelper.parameterExists(CLIENTS, element)) { if (isSelfServiceUser == null || !isSelfServiceUser) { - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).failWithCode("not.supported.when.isSelfServiceUser.is.false", + baseDataValidator.reset().parameter(CLIENTS).failWithCode("not.supported.when.isSelfServiceUser.is.false", "clients parameter is not supported when isSelfServiceUser parameter is false"); } else { - final JsonArray clientsArray = this.fromApiJsonHelper.extractJsonArrayNamed(AppUserConstants.CLIENTS, element); - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).value(clientsArray).jsonArrayNotEmpty(); + final JsonArray clientsArray = this.fromApiJsonHelper.extractJsonArrayNamed(CLIENTS, element); + baseDataValidator.reset().parameter(CLIENTS).value(clientsArray).jsonArrayNotEmpty(); for (JsonElement client : clientsArray) { Long clientId = client.getAsLong(); - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).value(clientId).longGreaterThanZero(); + baseDataValidator.reset().parameter(CLIENTS).value(clientId).longGreaterThanZero(); } } } @@ -166,7 +170,33 @@ private void throwExceptionIfValidationWarningsExist(final List getParamNamesFromRequest(final String json) { + final JsonElement element = this.fromApiJsonHelper.parse(json); + if (element.isJsonObject()) { + return ((JsonObject) element).keySet(); + } + return Set.of(); + } + + void validateFieldLevelACL(final String json, AppUser authenticatedUser) { + if (!authenticatedUser.hasAnyPermission("ALL_FUNCTIONS", "UPDATE_USER")) { + Set paramNamesFromRequest = getParamNamesFromRequest(json); + if (authenticatedUser.isSelfServiceUser()) { + // selfService user can change the clients and the password + paramNamesFromRequest.removeAll(Set.of(CLIENTS, PASSWORD, REPEAT_PASSWORD)); + } else { + // user without permission can only change the password + paramNamesFromRequest.removeAll(Set.of(PASSWORD, REPEAT_PASSWORD)); + } + if (paramNamesFromRequest.size() > 0) { + throw new PlatformApiDataValidationException( + List.of(ApiParameterError.parameterError("not.enough.permission.to.update.fields", + "Current user has no permission to update fields", String.join(",", paramNamesFromRequest)))); + } + } + } + + public void validateForUpdate(final String json, AppUser authenticatedUser) { if (StringUtils.isBlank(json)) { throw new InvalidJsonException(); } @@ -243,21 +273,22 @@ public void validateForUpdate(final String json) { } } - if (this.fromApiJsonHelper.parameterExists(AppUserConstants.CLIENTS, element)) { + if (this.fromApiJsonHelper.parameterExists(CLIENTS, element)) { if (isSelfServiceUser != null && !isSelfServiceUser) { - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).failWithCode("not.supported.when.isSelfServiceUser.is.false", + baseDataValidator.reset().parameter(CLIENTS).failWithCode("not.supported.when.isSelfServiceUser.is.false", "clients parameter is not supported when isSelfServiceUser parameter is false"); } else { - final JsonArray clientsArray = this.fromApiJsonHelper.extractJsonArrayNamed(AppUserConstants.CLIENTS, element); - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).value(clientsArray).jsonArrayNotEmpty(); + final JsonArray clientsArray = this.fromApiJsonHelper.extractJsonArrayNamed(CLIENTS, element); + baseDataValidator.reset().parameter(CLIENTS).value(clientsArray).jsonArrayNotEmpty(); for (JsonElement client : clientsArray) { Long clientId = client.getAsLong(); - baseDataValidator.reset().parameter(AppUserConstants.CLIENTS).value(clientId).longGreaterThanZero(); + baseDataValidator.reset().parameter(CLIENTS).value(clientId).longGreaterThanZero(); } } } throwExceptionIfValidationWarningsExist(dataValidationErrors); + validateFieldLevelACL(json, authenticatedUser); } } diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/NotificationApiTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/NotificationApiTest.java index 81152fdbc91..307b7bc418a 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/NotificationApiTest.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/NotificationApiTest.java @@ -65,7 +65,7 @@ public void setUp() { PostUsersRequest createUserRequest = new PostUsersRequest().username(username) .firstname(Utils.randomStringGenerator("NotificationFN", 4)).lastname(Utils.randomStringGenerator("NotificationLN", 4)) .email("whatever@mifos.org").password(password).repeatPassword(password).sendPasswordToEmail(false) - .roles(List.of(Long.toString(SUPER_USER_ROLE_ID))).officeId(headOffice.getId()); + .roles(List.of(SUPER_USER_ROLE_ID)).officeId(headOffice.getId()); PostUsersResponse userCreationResponse = UserHelper.createUser(requestSpec, responseSpec, createUserRequest); Assertions.assertNotNull(userCreationResponse.getResourceId()); diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/UserAdministrationTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/UserAdministrationTest.java index 00fad579e7e..b54b96e7fbd 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/UserAdministrationTest.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/UserAdministrationTest.java @@ -27,7 +27,16 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import org.apache.fineract.client.models.GetOfficesResponse; +import org.apache.fineract.client.models.GetUsersUserIdResponse; +import org.apache.fineract.client.models.PostUsersRequest; +import org.apache.fineract.client.models.PostUsersResponse; +import org.apache.fineract.client.models.PutUsersUserIdRequest; +import org.apache.fineract.client.models.PutUsersUserIdResponse; +import org.apache.fineract.client.util.CallFailedRuntimeException; +import org.apache.fineract.integrationtests.client.IntegrationTest; import org.apache.fineract.integrationtests.common.ClientHelper; +import org.apache.fineract.integrationtests.common.OfficeHelper; import org.apache.fineract.integrationtests.common.Utils; import org.apache.fineract.integrationtests.common.organisation.StaffHelper; import org.apache.fineract.integrationtests.useradministration.roles.RolesHelper; @@ -40,7 +49,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class UserAdministrationTest { +public class UserAdministrationTest extends IntegrationTest { private static final Logger LOG = LoggerFactory.getLogger(UserAdministrationTest.class); private ResponseSpecification responseSpec; @@ -171,4 +180,67 @@ public void testModifySystemUser() { final List errors = (List) UserHelper.updateUser(this.requestSpec, expectStatusCode(403), userId, "systemtest", "errors"); } + + @Test + public void testApplicationUserCanChangeOwnPassword() { + // Admin creates a new user with an empty role + Integer roleId = RolesHelper.createRole(requestSpec, responseSpec); + String originalPassword = "aA1qwerty56"; + String simpleUsername = Utils.uniqueRandomStringGenerator("NotificationUser", 4); + GetOfficesResponse headOffice = OfficeHelper.getHeadOffice(requestSpec, responseSpec); + PostUsersRequest createUserRequest = new PostUsersRequest().username(simpleUsername) + .firstname(Utils.randomStringGenerator("NotificationFN", 4)).lastname(Utils.randomStringGenerator("NotificationLN", 4)) + .email("whatever@mifos.org").password(originalPassword).repeatPassword(originalPassword).sendPasswordToEmail(false) + .officeId(headOffice.getId()).roles(List.of(Long.valueOf(roleId))); + + PostUsersResponse userCreationResponse = UserHelper.createUser(requestSpec, responseSpec, createUserRequest); + Long userId = userCreationResponse.getResourceId(); + Assertions.assertNotNull(userId); + + // User updates its own password + String updatedPassword = "aA1qwerty56!"; + PutUsersUserIdResponse putUsersUserIdResponse = ok(newFineract(simpleUsername, originalPassword).users.update26(userId, + new PutUsersUserIdRequest().password(updatedPassword).repeatPassword(updatedPassword))); + Assertions.assertNotNull(putUsersUserIdResponse.getResourceId()); + + // From then on the originalPassword is not working anymore + CallFailedRuntimeException callFailedRuntimeException = Assertions.assertThrows(CallFailedRuntimeException.class, () -> { + ok(newFineract(simpleUsername, originalPassword).users.retrieveOne31(userId)); + }); + Assertions.assertEquals(401, callFailedRuntimeException.getResponse().raw().code()); + Assertions.assertTrue(callFailedRuntimeException.getMessage().contains("Unauthorized")); + + // The update password is still working perfectly + GetUsersUserIdResponse ok = ok(newFineract(simpleUsername, updatedPassword).users.retrieveOne31(userId)); + } + + @Test + public void testApplicationUserShallNotBeAbleToChangeItsOwnRoles() { + // Admin creates a new user with one role assigned + Integer roleId = RolesHelper.createRole(requestSpec, responseSpec); + String password = "aA1qwerty56"; + String simpleUsername = Utils.uniqueRandomStringGenerator("NotificationUser", 4); + GetOfficesResponse headOffice = OfficeHelper.getHeadOffice(requestSpec, responseSpec); + PostUsersRequest createUserRequest = new PostUsersRequest().username(simpleUsername) + .firstname(Utils.randomStringGenerator("NotificationFN", 4)).lastname(Utils.randomStringGenerator("NotificationLN", 4)) + .email("whatever@mifos.org").password(password).repeatPassword(password).sendPasswordToEmail(false) + .officeId(headOffice.getId()).roles(List.of(Long.valueOf(roleId))); + + PostUsersResponse userCreationResponse = UserHelper.createUser(requestSpec, responseSpec, createUserRequest); + Long userId = userCreationResponse.getResourceId(); + Assertions.assertNotNull(userId); + + // Admin creates a second role + Integer roleId2 = RolesHelper.createRole(requestSpec, responseSpec); + + // User tries to update it's own roles + CallFailedRuntimeException callFailedRuntimeException = Assertions.assertThrows(CallFailedRuntimeException.class, () -> { + ok(newFineract(simpleUsername, password).users.update26(userId, + new PutUsersUserIdRequest().roles(List.of(Long.valueOf(roleId2))))); + }); + + Assertions.assertEquals(400, callFailedRuntimeException.getResponse().raw().code()); + Assertions.assertTrue(callFailedRuntimeException.getMessage().contains("not.enough.permission.to.update.fields")); + } + } diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/IntegrationTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/IntegrationTest.java index f3bee6ac49b..2b127acbca2 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/IntegrationTest.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/IntegrationTest.java @@ -62,16 +62,20 @@ public abstract class IntegrationTest { protected FineractClient fineract() { if (fineract == null) { - String url = System.getProperty("fineract.it.url", "https://localhost:8443/fineract-provider/api/"); - // insecure(true) should *ONLY* ever be used for https://localhost:8443, NOT in real clients!! - FineractClient.Builder builder = FineractClient.builder().insecure(true).baseURL(url).tenant("default") - .basicAuth("mifos", "password").logging(Level.NONE); - customizeFineractClient(builder); - fineract = builder.build(); + fineract = newFineract("mifos", "password"); } return fineract; } + protected FineractClient newFineract(String username, String password) { + String url = System.getProperty("fineract.it.url", "https://localhost:8443/fineract-provider/api/"); + // insecure(true) should *ONLY* ever be used for https://localhost:8443, NOT in real clients!! + FineractClient.Builder builder = FineractClient.builder().insecure(true).baseURL(url).tenant("default") + .basicAuth(username, password).logging(Level.NONE); + customizeFineractClient(builder); + return builder.build(); + } + /** * Callback to customize FineractClient * diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java index 67ae409261e..23ea7177fe9 100644 --- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java +++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java @@ -149,7 +149,7 @@ public static RequestSpecification getSimpleUserWithoutBypassPermission(final Re PostUsersRequest createUserRequest = new PostUsersRequest().username(simpleUsername) .firstname(Utils.randomStringGenerator("NotificationFN", 4)).lastname(Utils.randomStringGenerator("NotificationLN", 4)) .email("whatever@mifos.org").password(password).repeatPassword(password).sendPasswordToEmail(false) - .roles(List.of(simpleRoleId)).officeId(headOffice.getId()); + .roles(List.of(Long.valueOf(simpleRoleId))).officeId(headOffice.getId()); PostUsersResponse userCreationResponse = UserHelper.createUser(requestSpec, responseSpec, createUserRequest); Assertions.assertNotNull(userCreationResponse.getResourceId());