Skip to content

Commit

Permalink
FINERACT-1971 Improve Request Validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@reluxa @adamsaghy
reluxa authored and adamsaghy committed Dec 11, 2023
1 parent 31a7e36 commit 6dbab17
Show file tree
Hide file tree
Showing 7 changed files with 160 additions and 31 deletions.
26 changes: 23 additions & 3 deletions ...der/src/main/java/org/apache/fineract/useradministration/api/UsersApiResourceSwagger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,14 @@ private PostUsersRequest() {
@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;
}

Expand All @@ -156,10 +160,26 @@ private PutUsersUserIdRequest() {

@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")
Expand Down
12 changes: 7 additions & 5 deletions ...che/fineract/useradministration/service/AppUserWritePlatformServiceJpaRepositoryImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -112,8 +114,8 @@ public CommandProcessingResult createUser(final JsonCommand command) {
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());
Expand Down Expand Up @@ -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));

Expand All @@ -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<Long> clientIds = new HashSet<>();
for (JsonElement clientElement : clientsArray) {
clientIds.add(clientElement.getAsLong());
Expand Down
59 changes: 45 additions & 14 deletions ...vider/src/main/java/org/apache/fineract/useradministration/service/UserDataValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}
}
}
Expand All @@ -166,7 +170,33 @@ private void throwExceptionIfValidationWarningsExist(final List<ApiParameterErro
}
}

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();
}
Expand Down Expand Up @@ -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);
}
}
2 changes: 1 addition & 1 deletion ...gration-tests/src/test/java/org/apache/fineract/integrationtests/NotificationApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public void setUp() {
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());
Expand Down
74 changes: 73 additions & 1 deletion ...tion-tests/src/test/java/org/apache/fineract/integrationtests/UserAdministrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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("[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"));
}

}
16 changes: 10 additions & 6 deletions ...tion-tests/src/test/java/org/apache/fineract/integrationtests/client/IntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down
2 changes: 1 addition & 1 deletion ...c/test/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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("[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());
Expand Down

0 comments on commit 6dbab17

Please sign in to comment.