From commits-return-6802-archive-asf-public=cust-asf.ponee.io@fineract.apache.org Tue Dec 3 12:17:35 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 65A2D180629 for ; Tue, 3 Dec 2019 13:17:35 +0100 (CET) Received: (qmail 93630 invoked by uid 500); 3 Dec 2019 12:17:32 -0000 Mailing-List: contact commits-help@fineract.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@fineract.apache.org Delivered-To: mailing list commits@fineract.apache.org Received: (qmail 93577 invoked by uid 99); 3 Dec 2019 12:17:32 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 03 Dec 2019 12:17:32 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 4F7BD8D809; Tue, 3 Dec 2019 12:17:32 +0000 (UTC) Date: Tue, 03 Dec 2019 12:17:32 +0000 To: "commits@fineract.apache.org" Subject: [fineract] branch develop updated: FINERACT-803: validate username uniqueness MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <157537545207.11585.9110694417940479439@gitbox.apache.org> From: mosi@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: fineract X-Git-Refname: refs/heads/develop X-Git-Reftype: branch X-Git-Oldrev: a20f76d68612aae3a4384a43d99afbdaecd41404 X-Git-Newrev: d9a0a9ac63a7ebe8e4ca24a71fac840982f0d494 X-Git-Rev: d9a0a9ac63a7ebe8e4ca24a71fac840982f0d494 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. mosi 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 d9a0a9a FINERACT-803: validate username uniqueness new fb1e3d7 Merge pull request #660 from jamesidw/FINERACT-803 d9a0a9a is described below commit d9a0a9ac63a7ebe8e4ca24a71fac840982f0d494 Author: Sidney Wasibani AuthorDate: Wed Nov 27 04:15:31 2019 +0300 FINERACT-803: validate username uniqueness FINERACT-803: treat existing username in update, to same user, as valid FINERACT-803: skip username uniqueness validation for updates FINERACT-803: add integration test for username uniqueness validation FINERACT-803: change the validation error message to show the errant username FINERACT-803: restore username uniqueness validation for updates FINERACT-803: apply minor code clean up suggestions --- .../integrationtests/UserAdministrationTest.java | 122 +++++++++++++++++++++ .../useradministration/users/UserHelper.java | 29 ++++- .../core/data/DataValidatorBuilder.java | 6 + ...pUserWritePlatformServiceJpaRepositoryImpl.java | 2 +- .../service/UserDataValidator.java | 22 +++- 5 files changed, 173 insertions(+), 8 deletions(-) diff --git a/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/UserAdministrationTest.java b/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/UserAdministrationTest.java new file mode 100644 index 0000000..0985ee1 --- /dev/null +++ b/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/UserAdministrationTest.java @@ -0,0 +1,122 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fineract.integrationtests; + +import com.jayway.restassured.builder.RequestSpecBuilder; +import com.jayway.restassured.builder.ResponseSpecBuilder; +import com.jayway.restassured.http.ContentType; +import com.jayway.restassured.specification.RequestSpecification; +import com.jayway.restassured.specification.ResponseSpecification; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.apache.fineract.integrationtests.common.Utils; +import org.apache.fineract.integrationtests.common.organisation.StaffHelper; +import org.apache.fineract.integrationtests.useradministration.roles.RolesHelper; +import org.apache.fineract.integrationtests.useradministration.users.UserHelper; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class UserAdministrationTest { + + private ResponseSpecification responseSpec; + private RequestSpecification requestSpec; + private List transientUsers = new ArrayList<>(); + + private ResponseSpecification expectStatusCode(int code) { + return new ResponseSpecBuilder().expectStatusCode(code).build(); + } + + @Before + public void setup() { + Utils.initializeRESTAssured(); + this.requestSpec = new RequestSpecBuilder().setContentType(ContentType.JSON).build(); + this.requestSpec.header("Authorization", "Basic " + Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey()); + this.responseSpec = expectStatusCode(200); + } + + @After + public void tearDown() { + for(Integer userId : this.transientUsers) { + UserHelper.deleteUser(this.requestSpec, this.responseSpec, userId); + } + this.transientUsers.clear(); + } + + @Test + public void testCreateNewUserBlocksDuplicateUsername() { + final Integer roleId = RolesHelper.createRole(this.requestSpec, this.responseSpec); + Assert.assertNotNull(roleId); + + final Integer staffId = StaffHelper.createStaff(this.requestSpec, this.responseSpec); + Assert.assertNotNull(staffId); + + final Integer userId = (Integer) UserHelper.createUser(this.requestSpec, this.responseSpec, roleId, staffId, "alphabet", "resourceId"); + Assert.assertNotNull(userId); + this.transientUsers.add(userId); + + final List errors = (List) UserHelper.createUser(this.requestSpec, expectStatusCode(400), roleId, staffId, "alphabet", "errors"); + Map reason = (Map) errors.get(0); + Assert.assertEquals("User with username 'alphabet' already exists.", reason.get("defaultUserMessage")); + } + + @Test + public void testUpdateUserAcceptsNewOrSameUsername() { + final Integer roleId = RolesHelper.createRole(this.requestSpec, this.responseSpec); + Assert.assertNotNull(roleId); + + final Integer staffId = StaffHelper.createStaff(this.requestSpec, this.responseSpec); + Assert.assertNotNull(staffId); + + final Integer userId = (Integer) UserHelper.createUser(this.requestSpec, this.responseSpec, roleId, staffId, "alphabet", "resourceId"); + Assert.assertNotNull(userId); + this.transientUsers.add(userId); + + final Integer userId2 = (Integer) UserHelper.updateUser(this.requestSpec, this.responseSpec, userId, "renegade", "resourceId"); + Assert.assertNotNull(userId2); + + final Integer userId3 = (Integer) UserHelper.updateUser(this.requestSpec, this.responseSpec, userId, "renegade", "resourceId"); + Assert.assertNotNull(userId3); + } + + @Test + public void testUpdateUserBlockDuplicateUsername() { + final Integer roleId = RolesHelper.createRole(this.requestSpec, this.responseSpec); + Assert.assertNotNull(roleId); + + final Integer staffId = StaffHelper.createStaff(this.requestSpec, this.responseSpec); + Assert.assertNotNull(staffId); + + final Integer userId = (Integer) UserHelper.createUser(this.requestSpec, this.responseSpec, roleId, staffId, "alphabet", "resourceId"); + Assert.assertNotNull(userId); + this.transientUsers.add(userId); + + final Integer userId2 = (Integer) UserHelper.createUser(this.requestSpec, this.responseSpec, roleId, staffId, "bilingual", "resourceId"); + Assert.assertNotNull(userId2); + this.transientUsers.add(userId2); + + final List errors = (List) UserHelper.updateUser(this.requestSpec, expectStatusCode(400), userId2, "alphabet", "errors"); + Map reason = (Map) errors.get(0); + Assert.assertEquals("User with username 'alphabet' already exists.", reason.get("defaultUserMessage")); + } + +} diff --git a/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java b/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java index 8acabda..ea5b4c8 100644 --- a/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java +++ b/fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/useradministration/users/UserHelper.java @@ -32,21 +32,40 @@ public class UserHelper { return Utils.performServerPost(requestSpec, responseSpec, CREATE_USER_URL, getTestCreateUserAsJSON(roleId, staffId), "resourceId"); } + public static Object createUser(final RequestSpecification requestSpec, final ResponseSpecification responseSpec, int roleId, int staffId, String username, String attribute) { + return Utils.performServerPost(requestSpec, responseSpec, CREATE_USER_URL, getTestCreateUserAsJSON(roleId, staffId, username), attribute); + } + public static String getTestCreateUserAsJSON(int roleId, int staffId) { - String json = "{ \"username\": \"" + Utils.randomNameGenerator("User_Name_", 3) + return "{ \"username\": \"" + Utils.randomNameGenerator("User_Name_", 3) + "\", \"firstname\": \"Test\", \"lastname\": \"User\", \"email\": \"whatever@mifos.org\"," + " \"officeId\": \"1\", \"staffId\": " + "\"" - + Integer.toString(staffId)+"\",\"roles\": [\"" - + Integer.toString(roleId) + "\"], \"sendPasswordToEmail\": false}"; - System.out.println(json); - return json; + + staffId +"\",\"roles\": [\"" + + roleId + "\"], \"sendPasswordToEmail\": false}"; + } + private static String getTestCreateUserAsJSON(int roleId, int staffId, String username) { + return "{ \"username\": \"" + username + + "\", \"firstname\": \"Test\", \"lastname\": \"User\", \"email\": \"whatever@mifos.org\"," + + " \"officeId\": \"1\", \"staffId\": " + "\"" + + staffId +"\",\"roles\": [\"" + + roleId + "\"], \"sendPasswordToEmail\": false}"; + } + + private static String getTestUpdateUserAsJSON(String username) { + return "{ \"username\": \"" + username + + "\", \"firstname\": \"Test\", \"lastname\": \"User\", \"email\": \"whatever@mifos.org\"," + + " \"officeId\": \"1\"}"; } public static Integer deleteUser(final RequestSpecification requestSpec, final ResponseSpecification responseSpec, final Integer userId) { return Utils.performServerDelete(requestSpec, responseSpec, createRoleOperationURL(userId), "resourceId"); } + public static Object updateUser(final RequestSpecification requestSpec, final ResponseSpecification responseSpec, int userId, String username, String attribute) { + return Utils.performServerPut(requestSpec, responseSpec, createRoleOperationURL(userId), getTestUpdateUserAsJSON(username), attribute); + } + private static String createRoleOperationURL(final Integer userId) { return USER_URL + "/" + userId + "?" + Utils.TENANT_IDENTIFIER; } diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/DataValidatorBuilder.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/DataValidatorBuilder.java index 8191cbc..8824e28 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/DataValidatorBuilder.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/DataValidatorBuilder.java @@ -104,6 +104,12 @@ public class DataValidatorBuilder { return this; } + public void failWithMessage(final String errorCode, final String errorMessage, final Object... defaultUserMessageArgs) { + String validationErrorCode = "validation.msg." + this.resource + "." + this.parameter + "." + errorCode; + final ApiParameterError error = ApiParameterError.parameterError(validationErrorCode, errorMessage, this.parameter, this.value, defaultUserMessageArgs); + this.dataValidationErrors.add(error); + } + public void failWithCode(final String errorCode, final Object... defaultUserMessageArgs) { final StringBuilder validationErrorCode = new StringBuilder("validation.msg.").append(this.resource).append(".") .append(this.parameter).append(".").append(errorCode); 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 15fa769..0176c27 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 @@ -192,7 +192,7 @@ public class AppUserWritePlatformServiceJpaRepositoryImpl implements AppUserWrit this.context.authenticatedUser(new CommandWrapperBuilder().updateUser(null).build()); - this.fromApiJsonDeserializer.validateForUpdate(command.json()); + this.fromApiJsonDeserializer.validateForUpdate(command.json(), userId); final AppUser userToUpdate = this.appUserRepository.findById(userId) .orElseThrow(() -> new UserNotFoundException(userId)); 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 57a72c7..58f5bb9 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 @@ -32,6 +32,8 @@ 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.AppUserRepository; import org.apache.fineract.useradministration.domain.PasswordValidationPolicy; import org.apache.fineract.useradministration.domain.PasswordValidationPolicyRepository; import org.springframework.beans.factory.annotation.Autowired; @@ -55,10 +57,13 @@ public final class UserDataValidator { private final PasswordValidationPolicyRepository passwordValidationPolicy; + private final AppUserRepository appUserRepository; + @Autowired - public UserDataValidator(final FromJsonHelper fromApiJsonHelper, final PasswordValidationPolicyRepository passwordValidationPolicy) { + public UserDataValidator(final FromJsonHelper fromApiJsonHelper, final PasswordValidationPolicyRepository passwordValidationPolicy, AppUserRepository appUserRepository) { this.fromApiJsonHelper = fromApiJsonHelper; this.passwordValidationPolicy = passwordValidationPolicy; + this.appUserRepository = appUserRepository; } public void validateForCreate(final String json) { @@ -75,6 +80,14 @@ public final class UserDataValidator { final String username = this.fromApiJsonHelper.extractStringNamed("username", element); baseDataValidator.reset().parameter("username").value(username).notBlank().notExceedingLengthOf(100); + if (!StringUtils.isEmpty(username)) { + AppUser exists = appUserRepository.findAppUserByName(username); + if (exists != null) { + final String errorMessage = "User with username '" + username + "' already exists."; + baseDataValidator.reset().parameter("username").failWithMessage("constraint.violation.duplicate.username", errorMessage); + } + } + final String firstname = this.fromApiJsonHelper.extractStringNamed("firstname", element); baseDataValidator.reset().parameter("firstname").value(firstname).notBlank().notExceedingLengthOf(100); @@ -149,7 +162,7 @@ public final class UserDataValidator { if (!dataValidationErrors.isEmpty()) { throw new PlatformApiDataValidationException(dataValidationErrors); } } - public void validateForUpdate(final String json) { + public void validateForUpdate(final String json, final Long userId) { if (StringUtils.isBlank(json)) { throw new InvalidJsonException(); } final Type typeOfMap = new TypeToken>() {}.getType(); @@ -173,6 +186,11 @@ public final class UserDataValidator { if (this.fromApiJsonHelper.parameterExists("username", element)) { final String username = this.fromApiJsonHelper.extractStringNamed("username", element); baseDataValidator.reset().parameter("username").value(username).notBlank().notExceedingLengthOf(100); + AppUser current = appUserRepository.findAppUserByName(username); + if (current != null && !current.hasIdOf(userId)) { + final String errorMessage = "User with username '" + username + "' already exists."; + baseDataValidator.reset().parameter("username").failWithMessage("constraint.violation.duplicate.username", errorMessage); + } } if (this.fromApiJsonHelper.parameterExists("firstname", element)) {