From 70d66e680652a45b1b4ecca3c3792fd2fcf92bc4 Mon Sep 17 00:00:00 2001 From: anhefti Date: Tue, 18 Dec 2018 16:11:28 +0100 Subject: [PATCH] SEBSERV-8 #more tests and bug fixes --- .../datalayer => gbl/model}/APIMessage.java | 23 +++++-- .../sebserver/gbl/model/user/UserInfo.java | 8 +-- .../seb/sebserver/gbl/model/user/UserMod.java | 2 +- .../webservice/servicelayer/dao/UserDAO.java | 12 +++- .../servicelayer/dao/impl/UserDaoImpl.java | 21 ++++--- .../weblayer/api/APIExceptionHandler.java | 14 ++++- .../weblayer/api/UserAccountController.java | 34 +++++----- .../weblayer/oauth/RevokeTokenEndpoint.java | 2 +- .../integration/api/UserAPITest.java | 62 ++++++++++++++++++- 9 files changed, 130 insertions(+), 48 deletions(-) rename src/main/java/ch/ethz/seb/sebserver/{webservice/datalayer => gbl/model}/APIMessage.java (82%) diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/datalayer/APIMessage.java b/src/main/java/ch/ethz/seb/sebserver/gbl/model/APIMessage.java similarity index 82% rename from src/main/java/ch/ethz/seb/sebserver/webservice/datalayer/APIMessage.java rename to src/main/java/ch/ethz/seb/sebserver/gbl/model/APIMessage.java index 6092ac98..ad2cd0fe 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/datalayer/APIMessage.java +++ b/src/main/java/ch/ethz/seb/sebserver/gbl/model/APIMessage.java @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -package ch.ethz.seb.sebserver.webservice.datalayer; +package ch.ethz.seb.sebserver.gbl.model; import java.io.Serializable; import java.util.Collections; @@ -17,6 +17,9 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.validation.FieldError; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + import ch.ethz.seb.sebserver.gbl.util.Utils; public class APIMessage implements Serializable { @@ -75,16 +78,21 @@ public class APIMessage implements Serializable { } } + @JsonProperty("messageCode") public final String messageCode; + @JsonProperty("systemMessage") public final String systemMessage; + @JsonProperty("details") public final String details; + @JsonProperty("attributes") public final List attributes; + @JsonCreator public APIMessage( - final String messageCode, - final String systemMessage, - final String details, - final List attributes) { + @JsonProperty("messageCode") final String messageCode, + @JsonProperty("systemMessage") final String systemMessage, + @JsonProperty("details") final String details, + @JsonProperty("attributes") final List attributes) { this.messageCode = messageCode; this.systemMessage = systemMessage; @@ -139,6 +147,11 @@ public class APIMessage implements Serializable { this.apiMessage = errorMessage.of(); } + public APIMessageException(final ErrorMessage errorMessage, final String detail, final String... attributes) { + super(); + this.apiMessage = errorMessage.of(detail, attributes); + } + public APIMessage getAPIMessage() { return this.apiMessage; } diff --git a/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserInfo.java b/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserInfo.java index e2233956..c5312834 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserInfo.java +++ b/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserInfo.java @@ -51,18 +51,18 @@ public final class UserInfo implements GrantEntity, Serializable { /** Full name of the user */ @NotNull - @Size(min = 3, max = 255, message = "userInfo:name:size:{min}:{max}:${validatedValue}") + @Size(min = 3, max = 255, message = "user:name:size:{min}:{max}:${validatedValue}") @JsonProperty(USER.ATTR_NAME) public final String name; /** The internal user name */ @NotNull - @Size(min = 3, max = 255, message = "userInfo:username:size:{min}:{max}:${validatedValue}") + @Size(min = 3, max = 255, message = "user:username:size:{min}:{max}:${validatedValue}") @JsonProperty(USER.ATTR_USER_NAME) public final String userName; /** E-mail address of the user */ - @Email(message = "userInfo:email:email:_:_:${validatedValue}") + @Email(message = "user:email:email:_:_:${validatedValue}") @JsonProperty(USER.ATTR_EMAIL) public final String email; @@ -83,7 +83,7 @@ public final class UserInfo implements GrantEntity, Serializable { /** The users roles in a unmodifiable set. Is never null */ @NotNull - @NotEmpty(message = "userInfo:roles:notEmpty:_:_:_") + @NotEmpty(message = "user:roles:notEmpty:_:_:_") @JsonProperty(USER_ROLE.REFERENCE_NAME) public final Set roles; diff --git a/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserMod.java b/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserMod.java index 12585f34..25b657f0 100644 --- a/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserMod.java +++ b/src/main/java/ch/ethz/seb/sebserver/gbl/model/user/UserMod.java @@ -26,7 +26,7 @@ public final class UserMod implements GrantEntity { @JsonProperty(ATTR_NAME_USER_INFO) private final UserInfo userInfo; - @Size(min = 8, max = 255, message = "userInfo:password:size:{min}:{max}:${validatedValue}") + @Size(min = 8, max = 255, message = "user:password:size:{min}:{max}:${validatedValue}") @JsonProperty(ATTR_NAME_NEW_PASSWORD) private final String newPassword; diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/UserDAO.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/UserDAO.java index 52663e30..c43b48a7 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/UserDAO.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/UserDAO.java @@ -11,6 +11,7 @@ package ch.ethz.seb.sebserver.webservice.servicelayer.dao; import java.util.Collection; import java.util.function.Predicate; +import ch.ethz.seb.sebserver.gbl.model.Entity; import ch.ethz.seb.sebserver.gbl.model.user.UserFilter; import ch.ethz.seb.sebserver.gbl.model.user.UserInfo; import ch.ethz.seb.sebserver.gbl.model.user.UserMod; @@ -77,11 +78,16 @@ public interface UserDAO extends EntityDAO { * not null on UserMod instance are updated within the existing user record. * * @param userMod UserMod instance containing new user record data - * @param principal the user principal that requests the save/modification * @return A Result of UserInfo where the successfully saved/modified user data is available or a reported * exception on error case */ - Result save(SEBServerUser principal, UserMod userMod); + Result save(UserMod userMod); - Result delete(SEBServerUser principal, Long id); + /** Use this to delete a user and all its relationships by id + * + * @param id the identifier if the user to delete + * @param archive indicates whether the user and all its relations should be archived (inactive and anonymous) or + * hard deleted + * @return Result of a collection of all entities that has been deleted (or archived) */ + Result> delete(Long id, boolean archive); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/UserDaoImpl.java b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/UserDaoImpl.java index cc54c94f..dbe4eb2b 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/UserDaoImpl.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/servicelayer/dao/impl/UserDaoImpl.java @@ -34,14 +34,15 @@ import org.springframework.transaction.interceptor.TransactionInterceptor; import org.springframework.util.CollectionUtils; import ch.ethz.seb.sebserver.WebSecurityConfig; +import ch.ethz.seb.sebserver.gbl.model.Entity; import ch.ethz.seb.sebserver.gbl.model.EntityType; +import ch.ethz.seb.sebserver.gbl.model.APIMessage.APIMessageException; +import ch.ethz.seb.sebserver.gbl.model.APIMessage.ErrorMessage; import ch.ethz.seb.sebserver.gbl.model.user.UserFilter; import ch.ethz.seb.sebserver.gbl.model.user.UserInfo; import ch.ethz.seb.sebserver.gbl.model.user.UserMod; import ch.ethz.seb.sebserver.gbl.util.Result; import ch.ethz.seb.sebserver.gbl.util.Utils; -import ch.ethz.seb.sebserver.webservice.datalayer.APIMessage.APIMessageException; -import ch.ethz.seb.sebserver.webservice.datalayer.APIMessage.ErrorMessage; import ch.ethz.seb.sebserver.webservice.datalayer.batis.mapper.RoleRecordDynamicSqlSupport; import ch.ethz.seb.sebserver.webservice.datalayer.batis.mapper.RoleRecordMapper; import ch.ethz.seb.sebserver.webservice.datalayer.batis.mapper.UserRecordDynamicSqlSupport; @@ -160,7 +161,7 @@ public class UserDaoImpl implements UserDAO { @Override @Transactional - public Result save(final SEBServerUser principal, final UserMod userMod) { + public Result save(final UserMod userMod) { if (userMod == null) { return Result.ofError(new NullPointerException("userMod has null-reference")); } @@ -168,11 +169,9 @@ public class UserDaoImpl implements UserDAO { try { final UserInfo userInfo = userMod.getUserInfo(); - if (userInfo.uuid != null) { - return updateUser(userMod); - } else { - return createNewUser(principal, userMod); - } + return (userInfo.uuid != null) + ? updateUser(userMod) + : createNewUser(userMod); } catch (final Throwable t) { log.error("Unexpected error while saving User data: ", t); @@ -183,8 +182,10 @@ public class UserDaoImpl implements UserDAO { @Override @Transactional - public Result delete(final SEBServerUser principal, final Long id) { + public Result> delete(final Long id, final boolean force) { + // TODO clarify within discussion about inactivate, archive and delete user related data + return Result.ofError(new RuntimeException("TODO")); } @@ -234,7 +235,7 @@ public class UserDaoImpl implements UserDAO { }); } - private Result createNewUser(final SEBServerUser principal, final UserMod userMod) { + private Result createNewUser(final UserMod userMod) { final UserInfo userInfo = userMod.getUserInfo(); if (!userMod.newPasswordMatch()) { diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/APIExceptionHandler.java b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/APIExceptionHandler.java index b98e900f..73830d6f 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/APIExceptionHandler.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/APIExceptionHandler.java @@ -8,6 +8,7 @@ package ch.ethz.seb.sebserver.webservice.weblayer.api; +import java.util.Arrays; import java.util.Collection; import java.util.stream.Collectors; @@ -25,7 +26,8 @@ import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.context.request.WebRequest; import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; -import ch.ethz.seb.sebserver.webservice.datalayer.APIMessage; +import ch.ethz.seb.sebserver.gbl.model.APIMessage; +import ch.ethz.seb.sebserver.gbl.model.APIMessage.APIMessageException; import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.PermissionDeniedException; @Order(Ordered.HIGHEST_PRECEDENCE) @@ -92,4 +94,14 @@ public class APIExceptionHandler extends ResponseEntityExceptionHandler { .createErrorResponse(ex.getMessage()); } + @ExceptionHandler(APIMessageException.class) + public ResponseEntity handleAPIMessageException( + final APIMessageException ex, + final WebRequest request) { + + return new ResponseEntity<>( + Arrays.asList(ex.getAPIMessage()), + HttpStatus.BAD_REQUEST); + } + } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/UserAccountController.java b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/UserAccountController.java index 7b029c52..bab96393 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/UserAccountController.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/api/UserAccountController.java @@ -13,6 +13,8 @@ import java.util.Collection; import java.util.function.Predicate; import java.util.stream.Collectors; +import javax.validation.Valid; + import org.springframework.context.ApplicationEventPublisher; import org.springframework.security.core.Authentication; import org.springframework.web.bind.annotation.PathVariable; @@ -29,7 +31,6 @@ import ch.ethz.seb.sebserver.gbl.profile.WebServiceProfile; import ch.ethz.seb.sebserver.gbl.util.Result; import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.AuthorizationGrantService; import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.PrivilegeType; -import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.SEBServerUser; import ch.ethz.seb.sebserver.webservice.servicelayer.authorization.UserService; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.UserActivityLogDAO; import ch.ethz.seb.sebserver.webservice.servicelayer.dao.UserActivityLogDAO.ActivityType; @@ -126,33 +127,24 @@ public class UserAccountController { @RequestMapping(value = "/create", method = RequestMethod.PUT) public UserInfo createUser( - @RequestBody final UserMod userData, + @Valid @RequestBody final UserMod userData, final Principal principal) { - return _saveUser( - userData, - this.userService.extractFromPrincipal(principal), - PrivilegeType.WRITE) - .getOrThrow(); + return _saveUser(userData, PrivilegeType.WRITE) + .getOrThrow(); } @RequestMapping(value = "/save", method = RequestMethod.POST) public UserInfo saveUser( - @RequestBody final UserMod userData, + @Valid @RequestBody final UserMod userData, final Principal principal) { - return _saveUser( - userData, - this.userService.extractFromPrincipal(principal), - PrivilegeType.MODIFY) - .getOrThrow(); + return _saveUser(userData, PrivilegeType.MODIFY) + .getOrThrow(); } - private Result _saveUser( - final UserMod userData, - final SEBServerUser admin, - final PrivilegeType privilegeType) { + private Result _saveUser(final UserMod userData, final PrivilegeType privilegeType) { final ActivityType actionType = (userData.getUserInfo().uuid == null) ? ActivityType.CREATE @@ -160,12 +152,14 @@ public class UserAccountController { return this.authorizationGrantService .checkGrantOnEntity(userData, privilegeType) - .flatMap(data -> this.userDao.save(admin, data)) + .flatMap(this.userDao::save) .flatMap(userInfo -> this.userActivityLogDAO.logUserActivity(actionType, userInfo)) .flatMap(userInfo -> { // handle password change; revoke access tokens if password has changed - this.applicationEventPublisher.publishEvent( - new RevokeTokenEndpoint.RevokeTokenEvent(this, userInfo.userName)); + if (userData.passwordChangeRequest() && userData.newPasswordMatch()) { + this.applicationEventPublisher.publishEvent( + new RevokeTokenEndpoint.RevokeTokenEvent(this, userInfo.userName)); + } return Result.of(userInfo); }); } diff --git a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/oauth/RevokeTokenEndpoint.java b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/oauth/RevokeTokenEndpoint.java index 82dade25..bb855b84 100644 --- a/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/oauth/RevokeTokenEndpoint.java +++ b/src/main/java/ch/ethz/seb/sebserver/webservice/weblayer/oauth/RevokeTokenEndpoint.java @@ -54,7 +54,7 @@ public class RevokeTokenEndpoint { } @EventListener(RevokeTokenEvent.class) - private void revokeAccessToken(final RevokeTokenEvent event) { + void revokeAccessToken(final RevokeTokenEvent event) { final String clientId = this.adminAPIClientDetails.getClientId(); final Collection tokens = this.tokenStore .findTokensByClientIdAndUserName(clientId, event.userName); diff --git a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/UserAPITest.java b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/UserAPITest.java index c1e11e04..82dee79b 100644 --- a/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/UserAPITest.java +++ b/src/test/java/ch/ethz/seb/sebserver/webservice/integration/api/UserAPITest.java @@ -28,6 +28,7 @@ import org.springframework.test.context.jdbc.Sql; import com.fasterxml.jackson.core.type.TypeReference; +import ch.ethz.seb.sebserver.gbl.model.APIMessage; import ch.ethz.seb.sebserver.gbl.model.user.UserActivityLog; import ch.ethz.seb.sebserver.gbl.model.user.UserFilter; import ch.ethz.seb.sebserver.gbl.model.user.UserInfo; @@ -207,7 +208,7 @@ public class UserAPITest extends AdministrationAPIIntegrationTest { null, 1L, "NewTestUser", "NewTestUser", "", true, Locale.CANADA, DateTimeZone.UTC, new HashSet<>(Arrays.asList(UserRole.EXAM_ADMIN.name()))); - final UserMod newUser = new UserMod(userInfo, "123", "123"); + final UserMod newUser = new UserMod(userInfo, "12345678", "12345678"); final String newUserJson = this.jsonMapper.writeValueAsString(newUser); final String token = getSebAdminAccess(); @@ -347,7 +348,7 @@ public class UserAPITest extends AdministrationAPIIntegrationTest { null, 2L, "NewTestUser", "NewTestUser", "", true, Locale.CANADA, DateTimeZone.UTC, new HashSet<>(Arrays.asList(UserRole.EXAM_ADMIN.name()))); - final UserMod newUser = new UserMod(userInfo, "123", "123"); + final UserMod newUser = new UserMod(userInfo, "12345678", "12345678"); final String newUserJson = this.jsonMapper.writeValueAsString(newUser); final String token = getAdminInstitution1Access(); @@ -372,7 +373,7 @@ public class UserAPITest extends AdministrationAPIIntegrationTest { null, 2L, "NewTestUser", "NewTestUser", "", true, Locale.CANADA, DateTimeZone.UTC, new HashSet<>(Arrays.asList(UserRole.EXAM_ADMIN.name()))); - final UserMod newUser = new UserMod(userInfo, "123", "123"); + final UserMod newUser = new UserMod(userInfo, "12345678", "12345678"); final String newUserJson = this.jsonMapper.writeValueAsString(newUser); final String token = getExamAdmin1(); @@ -441,6 +442,61 @@ public class UserAPITest extends AdministrationAPIIntegrationTest { .andReturn().getResponse().getContentAsString(); } + @Test + public void modifyUserPasswordInvalidPasswords() throws Exception { + final String sebAdminToken = getSebAdminAccess(); + final UserInfo examAdmin1 = this.jsonMapper.readValue( + this.mockMvc.perform(get(this.endpoint + RestAPI.ENDPOINT_USER_ACCOUNT + "/user4") + .header("Authorization", "Bearer " + sebAdminToken)) + .andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(), + new TypeReference() { + }); + + // must be longer then 8 chars + UserMod modifiedUser = new UserMod( + UserInfo.of(examAdmin1), + "new", + "new"); + String modifiedUserJson = this.jsonMapper.writeValueAsString(modifiedUser); + + List messages = this.jsonMapper.readValue( + this.mockMvc.perform(post(this.endpoint + RestAPI.ENDPOINT_USER_ACCOUNT + "/save") + .header("Authorization", "Bearer " + sebAdminToken) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(modifiedUserJson)) + .andExpect(status().isBadRequest()) + .andReturn().getResponse().getContentAsString(), + new TypeReference>() { + }); + + assertNotNull(messages); + assertTrue(1 == messages.size()); + assertEquals("1200", messages.get(0).messageCode); + assertEquals("[user, password, size, 8, 255, new]", String.valueOf(messages.get(0).getAttributes())); + + // wrong password retype + modifiedUser = new UserMod( + UserInfo.of(examAdmin1), + "12345678", + "87654321"); + modifiedUserJson = this.jsonMapper.writeValueAsString(modifiedUser); + + messages = this.jsonMapper.readValue( + this.mockMvc.perform(post(this.endpoint + RestAPI.ENDPOINT_USER_ACCOUNT + "/save") + .header("Authorization", "Bearer " + sebAdminToken) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(modifiedUserJson)) + .andExpect(status().isBadRequest()) + .andReturn().getResponse().getContentAsString(), + new TypeReference>() { + }); + + assertNotNull(messages); + assertTrue(1 == messages.size()); + assertEquals("1300", messages.get(0).messageCode); + } + private UserInfo getUserInfo(final String name, final Collection infos) { return infos .stream()