This is an automated email from the ASF dual-hosted git repository.
adamsaghy pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/fineract.git
The following commit(s) were added to refs/heads/develop by this push:
new 6dbab1737 FINERACT-1971 Improve Request Validation
6dbab1737 is described below
commit 6dbab1737ed64ef632afeb24de47ce21d5d95b90
Author: Peter Bagrij <[email protected]>
AuthorDate: Sun Dec 10 12:57:15 2023 +0100
FINERACT-1971 Improve Request Validation
---
.../api/UsersApiResourceSwagger.java | 26 +++++++-
...pUserWritePlatformServiceJpaRepositoryImpl.java | 12 ++--
.../service/UserDataValidator.java | 59 +++++++++++++----
.../integrationtests/NotificationApiTest.java | 2 +-
.../integrationtests/UserAdministrationTest.java | 74 +++++++++++++++++++++-
.../integrationtests/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 f1564222f..35a13cb93 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 @@ final class UsersApiResourceSwagger {
@Schema(example = "1")
public Long staffId;
@Schema(example = "[2,3]")
- public List<String> roles;
+ public List<Long> roles;
+ @Schema(example = "[2,3]")
+ public List<Long> clients;
@Schema(example = "true")
public Boolean sendPasswordToEmail;
@Schema(example = "true")
+ public Boolean passwordNeverExpires;
+ @Schema(example = "true")
public Boolean isSelfServiceUser;
}
@@ -156,10 +160,26 @@ final class UsersApiResourceSwagger {
@Schema(example = "Test")
public String firstname;
- @Schema(example = "window75")
+ @Schema(example = "User")
+ public String lastname;
+ @Schema(example = "[email protected]")
+ public String email;
+ @Schema(example = "1")
+ public Long officeId;
+ @Schema(example = "1")
+ public Long staffId;
+ @Schema(example = "[2,3]")
+ public List<Long> roles;
+ @Schema(example = "[2,3]")
+ public List<Long> 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 10f09fafd..6805f90ea 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 class AppUserWritePlatformServiceJpaRepositoryImpl
implements AppUserWrit
Collection<Client> 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<Long> clientIds = new HashSet<>();
for (JsonElement clientElement : clientsArray) {
clientIds.add(clientElement.getAsLong());
@@ -159,7 +161,7 @@ public class AppUserWritePlatformServiceJpaRepositoryImpl
implements AppUserWrit
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 class AppUserWritePlatformServiceJpaRepositoryImpl
implements AppUserWrit
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<Long> 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 c98f7aa02..03575747b 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.data.DataValidatorBuilder;
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<String> 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<String> 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 final class UserDataValidator {
}
}
- 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 @@ public final class UserDataValidator {
}
}
- public void validateForUpdate(final String json) {
+ private Set<String> 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<String> 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 final class UserDataValidator {
}
}
- 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 81152fdbc..307b7bc41 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 class NotificationApiTest {
PostUsersRequest createUserRequest = new
PostUsersRequest().username(username)
.firstname(Utils.randomStringGenerator("NotificationFN",
4)).lastname(Utils.randomStringGenerator("NotificationLN", 4))
.email("[email protected]").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 00fad579e..b54b96e7f 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 io.restassured.specification.ResponseSpecification;
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.junit.jupiter.api.Test;
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 class UserAdministrationTest {
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("[email protected]").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("[email protected]").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 f3bee6ac4..2b127acbc 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 67ae40926..23ea7177f 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 final class UserHelper {
PostUsersRequest createUserRequest = new
PostUsersRequest().username(simpleUsername)
.firstname(Utils.randomStringGenerator("NotificationFN",
4)).lastname(Utils.randomStringGenerator("NotificationLN", 4))
.email("[email protected]").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());
