From 3f350e0f2f6cd22fd65c95ac8fc96e8df3b84768 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Thu, 22 Jul 2021 23:01:03 -0700 Subject: [PATCH] SHIBUI-1740 Fixing some bugs related to updating users/user's group. Re-enable a bunch of tests in the UserControllerIntegrationTests that would have shown the bugs had they been working :/ --- .../admin/ui/configuration/DevConfig.groovy | 112 +++++++++--------- .../admin/ui/domain/AbstractAuditable.java | 37 +++--- .../security/controller/UsersController.java | 32 ++--- .../admin/ui/security/model/User.java | 2 + .../ui/security/service/UserService.java | 49 +++++--- .../UsersControllerIntegrationTests.groovy | 72 +++++++---- 6 files changed, 175 insertions(+), 129 deletions(-) diff --git a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy index 6080b2f20..65fcb7ed4 100644 --- a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy +++ b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy @@ -18,8 +18,10 @@ import edu.internet2.tier.shibboleth.admin.ui.security.model.User import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository import edu.internet2.tier.shibboleth.admin.ui.security.repository.RoleRepository import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository +import edu.internet2.tier.shibboleth.admin.ui.security.service.UserService import edu.internet2.tier.shibboleth.admin.util.ModelRepresentationConversions +import org.springframework.beans.factory.annotation.Autowired import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Profile import org.springframework.stereotype.Component @@ -30,14 +32,15 @@ import javax.annotation.PostConstruct @Component @Profile('dev') class DevConfig { - private final UserRepository adminUserRepository + private final EntityDescriptorRepository entityDescriptorRepository private final GroupsRepository groupsRepository - private final RoleRepository roleRepository - private final MetadataResolverRepository metadataResolverRepository - private final EntityDescriptorRepository entityDescriptorRepository - private final OpenSamlObjects openSamlObjects + private final RoleRepository roleRepository + private final UserRepository userRepository + + @Autowired + private UserService userService DevConfig(UserRepository adminUserRepository, GroupsRepository groupsRepository, @@ -46,7 +49,7 @@ class DevConfig { EntityDescriptorRepository entityDescriptorRepository, OpenSamlObjects openSamlObjects) { - this.adminUserRepository = adminUserRepository + this.userRepository = adminUserRepository this.metadataResolverRepository = metadataResolverRepository this.roleRepository = roleRepository this.entityDescriptorRepository = entityDescriptorRepository @@ -57,24 +60,26 @@ class DevConfig { @Transactional @PostConstruct void createDevUsersAndGroups() { - if (groupsRepository.count() == 0) { - def groups = [ - new Group().with { - it.name = "A1" - it.description = "AAA Group" - it.resourceId = "AAA" - it - }, - new Group().with { - it.name = "B1" - it.description = "BBB Group" - it.resourceId = "BBB" - it - }] - groups.each { + def groups = [ + new Group().with { + it.name = "A1" + it.description = "AAA Group" + it.resourceId = "AAA" + it + }, + new Group().with { + it.name = "B1" + it.description = "BBB Group" + it.resourceId = "BBB" + it + }] + groups.each { + try { groupsRepository.save(it) - } - } + } catch (Throwable e) { + // Must already exist (from a unit test) + } + } groupsRepository.flush() if (roleRepository.count() == 0) { @@ -93,7 +98,7 @@ class DevConfig { } } roleRepository.flush() - if (adminUserRepository.count() == 0) { + if (userRepository.count() == 0) { def users = [new User().with { username = 'admin' password = '{noop}adminpass' @@ -128,40 +133,11 @@ class DevConfig { it }] users.each { - adminUserRepository.save(it) + userService.save(it) } - adminUserRepository.flush() } } - @Transactional - @Profile('fbhmr') - @Bean - MetadataResolver fbhmr(ModelRepresentationConversions modelRepresentationConversions) { - return this.metadataResolverRepository.save(new FileBackedHttpMetadataResolver().with { - enabled = true - xmlId = 'test-fbhmr' - name = 'test-fbhmr' - metadataURL = 'http://md.incommon.org/InCommon/InCommon-metadata.xml' - backingFile = '%{idp.home}/test-fbhmr.xml' - reloadableMetadataResolverAttributes = new ReloadableMetadataResolverAttributes() - httpMetadataResolverAttributes = new HttpMetadataResolverAttributes() - it.metadataFilters.add(new EntityAttributesFilter().with { - it.name = 'test' - it.filterEnabled = true - it.entityAttributesFilterTarget = new EntityAttributesFilterTarget().with { - it.entityAttributesFilterTargetType = EntityAttributesFilterTarget.EntityAttributesFilterTargetType.ENTITY - it.value = ["https://carmenwiki.osu.edu/shibboleth"] - return it - } - it.attributeRelease = ['eduPersonPrincipalName', 'givenName', 'surname', 'mail'] - it.relyingPartyOverrides = null - return it - }) - return it - }) - } - @Profile('dhmr') @Transactional @Bean @@ -199,4 +175,32 @@ class DevConfig { it }) } + + @Transactional + @Profile('fbhmr') + @Bean + MetadataResolver fbhmr(ModelRepresentationConversions modelRepresentationConversions) { + return this.metadataResolverRepository.save(new FileBackedHttpMetadataResolver().with { + enabled = true + xmlId = 'test-fbhmr' + name = 'test-fbhmr' + metadataURL = 'http://md.incommon.org/InCommon/InCommon-metadata.xml' + backingFile = '%{idp.home}/test-fbhmr.xml' + reloadableMetadataResolverAttributes = new ReloadableMetadataResolverAttributes() + httpMetadataResolverAttributes = new HttpMetadataResolverAttributes() + it.metadataFilters.add(new EntityAttributesFilter().with { + it.name = 'test' + it.filterEnabled = true + it.entityAttributesFilterTarget = new EntityAttributesFilterTarget().with { + it.entityAttributesFilterTargetType = EntityAttributesFilterTarget.EntityAttributesFilterTargetType.ENTITY + it.value = ["https://carmenwiki.osu.edu/shibboleth"] + return it + } + it.attributeRelease = ['eduPersonPrincipalName', 'givenName', 'surname', 'mail'] + it.relyingPartyOverrides = null + return it + }) + return it + }) + } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/AbstractAuditable.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/AbstractAuditable.java index 3d2ed1391..445d4cc92 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/AbstractAuditable.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/domain/AbstractAuditable.java @@ -1,16 +1,9 @@ package edu.internet2.tier.shibboleth.admin.ui.domain; -import com.fasterxml.jackson.annotation.JsonGetter; -import com.fasterxml.jackson.annotation.JsonProperty; -import lombok.EqualsAndHashCode; -import org.hibernate.annotations.CreationTimestamp; -import org.hibernate.annotations.UpdateTimestamp; -import org.hibernate.envers.Audited; -import org.springframework.data.annotation.CreatedBy; -import org.springframework.data.annotation.CreatedDate; -import org.springframework.data.annotation.LastModifiedBy; -import org.springframework.data.annotation.LastModifiedDate; -import org.springframework.data.jpa.domain.support.AuditingEntityListener; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.time.ZoneOffset; +import java.time.ZonedDateTime; import javax.persistence.Column; import javax.persistence.EntityListeners; @@ -20,12 +13,20 @@ import javax.persistence.MappedSuperclass; import javax.persistence.Transient; import javax.validation.constraints.NotNull; -import java.time.Instant; -import java.time.LocalDateTime; -import java.time.OffsetDateTime; -import java.time.ZoneId; -import java.time.ZoneOffset; -import java.time.ZonedDateTime; + +import org.hibernate.annotations.CreationTimestamp; +import org.hibernate.annotations.UpdateTimestamp; +import org.hibernate.envers.Audited; +import org.springframework.data.annotation.CreatedBy; +import org.springframework.data.annotation.CreatedDate; +import org.springframework.data.annotation.LastModifiedBy; +import org.springframework.data.annotation.LastModifiedDate; +import org.springframework.data.jpa.domain.support.AuditingEntityListener; + +import com.fasterxml.jackson.annotation.JsonFormat; +import com.fasterxml.jackson.annotation.JsonProperty; + +import lombok.EqualsAndHashCode; @MappedSuperclass @@ -41,11 +42,13 @@ public abstract class AbstractAuditable implements Auditable { @CreationTimestamp @CreatedDate @Column(nullable = false, updatable = false) + @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSSSS") private LocalDateTime createdDate; @UpdateTimestamp @LastModifiedDate @Column(nullable = false) + @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSSSS") private LocalDateTime modifiedDate; @Column(name = "created_by") diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersController.java index 027de990a..354f0f1df 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersController.java @@ -1,11 +1,14 @@ package edu.internet2.tier.shibboleth.admin.ui.security.controller; import edu.internet2.tier.shibboleth.admin.ui.controller.ErrorResponse; +import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException; +import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException; import edu.internet2.tier.shibboleth.admin.ui.security.model.Group; import edu.internet2.tier.shibboleth.admin.ui.security.model.User; import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.RoleRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository; +import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService; import edu.internet2.tier.shibboleth.admin.ui.security.service.UserService; import groovy.util.logging.Slf4j; import jline.internal.Log; @@ -42,6 +45,10 @@ public class UsersController { @Autowired private GroupsRepository groupRepo; + + @Autowired + private IGroupService groupService; + private UserRepository userRepository; private UserService userService; @@ -54,8 +61,12 @@ public UsersController(UserRepository userRepository, RoleRepository roleReposit @Transactional @DeleteMapping("/{username}") public ResponseEntity deleteOne(@PathVariable String username) { - User user = findUserOrThrowHttp404(username); - userRepository.delete(user); + try { + userService.delete(username); + } + catch (EntityNotFoundException e) { + throw new HttpClientErrorException(NOT_FOUND, String.format("User with username [%s] not found", username)); + } return ResponseEntity.noContent().build(); } @@ -113,22 +124,11 @@ ResponseEntity saveOne(@RequestBody User user) { //TODO: modify this such that additional encoders can be used user.setPassword(BCrypt.hashpw(user.getPassword(), BCrypt.gensalt())); userService.updateUserRole(user); - findAndSetGroup(user); - User savedUser = userRepository.save(user); + User savedUser = userService.save(user); return ResponseEntity.ok(savedUser); } - private User findAndSetGroup(User user) { - // Ensure we have the full group detail from the db - if (user.getGroupId() != null || user.getGroup() != null) { - String resourceId = user.getGroupId() == null ? user.getGroup().getResourceId() : user.getGroupId(); - Group groupFromDb = groupRepo.findByResourceId(resourceId); - user.setGroup(groupFromDb); - } - return user; - } - @PreAuthorize("hasRole('ADMIN')") @Transactional @PatchMapping("/{username}") @@ -150,8 +150,8 @@ ResponseEntity updateOne(@PathVariable(value = "username") String username, @ persistedUser.setRole(user.getRole()); userService.updateUserRole(persistedUser); } - persistedUser.setGroup(findAndSetGroup(user).getGroup()); - User savedUser = userRepository.save(persistedUser); + persistedUser.setGroupId(user.getGroupId()); + User savedUser = userService.save(persistedUser); return ResponseEntity.ok(savedUser); } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java index 2e44957ac..994c6b353 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java @@ -67,6 +67,7 @@ public class User extends AbstractAuditable { @OneToMany(mappedBy = "user", fetch = FetchType.EAGER, cascade = CascadeType.ALL) @EqualsAndHashCode.Exclude + @JsonIgnore // do not remove this or all hell will break loose private Set userGroups = new HashSet<>(); @Column(nullable = false, unique = true) @@ -80,6 +81,7 @@ public void clearOldUserGroups() { * @return the initial implementation, while supporting a user having multiple groups in the db side, acts as if the * user can only belong to a single group */ + @JsonIgnore public Group getGroup() { return userGroups.isEmpty() ? null : ((UserGroup)userGroups.toArray()[0]).getGroup(); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/UserService.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/UserService.java index dd3bad1b1..097491947 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/UserService.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/UserService.java @@ -11,6 +11,7 @@ import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException; import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException; import edu.internet2.tier.shibboleth.admin.ui.security.model.Group; import edu.internet2.tier.shibboleth.admin.ui.security.model.Role; @@ -28,10 +29,10 @@ public class UserService implements InitializingBean { private RoleRepository roleRepository; @Autowired - private UserRepository userRepository; + UserGroupRepository userGroupRepository; @Autowired - UserGroupRepository userGroupRepository; + private UserRepository userRepository; public UserService() { } @@ -44,15 +45,27 @@ public UserService(RoleRepository roleRepository, UserRepository userRepository) this.userRepository = userRepository; } + @Override + @Transactional + public void afterPropertiesSet() { + // TODO: Ensure all the db users have a group - migration task + } + public boolean currentUserIsAdmin() { User user = getCurrentUser(); return user != null && user.getRole().equals("ROLE_ADMIN"); } - - @Override + @Transactional - public void afterPropertiesSet() { - // TODO: Ensure all the db users have a group - migration task + public void delete(String username) throws EntityNotFoundException { + Optional userToRemove = userRepository.findByUsername(username); + if (userToRemove.isEmpty()) throw new EntityNotFoundException("User does not exist"); + User user = userToRemove.get(); + // remove all group references from the user + user.getUserGroups().forEach(userGroup -> userGroupRepository.delete(userGroup)); + user.getUserGroups().clear(); + + userRepository.delete(user); } public User getCurrentUser() { @@ -69,15 +82,6 @@ public User getCurrentUser() { } return user; } - - public Group getCurrentUserGroup() { - switch (getCurrentUserAccess()) { - case ADMIN: - return Group.ADMIN_GROUP; - default: - return getCurrentUser().getGroup(); - } - } public UserAccess getCurrentUserAccess() { User user = getCurrentUser(); @@ -93,11 +97,20 @@ public UserAccess getCurrentUserAccess() { return UserAccess.NONE; } + public Group getCurrentUserGroup() { + switch (getCurrentUserAccess()) { + case ADMIN: + return Group.ADMIN_GROUP; + default: + return getCurrentUser().getGroup(); + } + } + public boolean isAuthorizedFor(Group objectGroup) { String objectGroupId = objectGroup == null ? Group.ADMIN_GROUP.getResourceId() : objectGroup.getResourceId(); return isAuthorizedFor(objectGroupId); } - + public boolean isAuthorizedFor(String objectGroupResourceId) { switch (getCurrentUserAccess()) { // no user returns NONE case ADMIN: @@ -111,7 +124,7 @@ public boolean isAuthorizedFor(String objectGroupResourceId) { return false; } } - + /** * Creating users should always have a group. If the user isn't assigned to a group, create one based on their name. * If the user has the ADMIN role, they are always solely assigned to the admin group. @@ -161,7 +174,7 @@ public User save(User user) { return userRepository.save(user); } - + /** * Given a user with a defined User.role, update the User.roles collection with that role. * diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy index 5ecb34dc0..a9e3144fb 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy @@ -1,18 +1,26 @@ package edu.internet2.tier.shibboleth.admin.ui.security.controller +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.databind.SerializationFeature +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule +import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateTimeDeserializer import groovy.json.JsonOutput import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc import org.springframework.boot.test.context.SpringBootTest import org.springframework.http.MediaType +import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder import org.springframework.security.test.context.support.WithMockUser import org.springframework.test.annotation.DirtiesContext +import org.springframework.test.annotation.Rollback import org.springframework.test.context.ActiveProfiles import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.result.MockMvcResultHandlers import spock.lang.Ignore import spock.lang.Specification +import edu.internet2.tier.shibboleth.admin.ui.security.model.User +import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get @@ -20,6 +28,10 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.* + +import java.time.LocalDateTime +import java.time.format.DateTimeFormatter /** * @author Dmitriy Kopylenko @@ -27,16 +39,33 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. @SpringBootTest @AutoConfigureMockMvc @ActiveProfiles(["no-auth", "dev"]) +@DirtiesContext class UsersControllerIntegrationTests extends Specification { + @Autowired + IGroupService groupService + @Autowired private MockMvc mockMvc static RESOURCE_URI = '/api/admin/users' + + def ObjectMapper mapper + + def setup() { + JavaTimeModule module = new JavaTimeModule(); + LocalDateTimeDeserializer localDateTimeDeserializer = new LocalDateTimeDeserializer(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSS")); + module.addDeserializer(LocalDateTime.class, localDateTimeDeserializer); + mapper = Jackson2ObjectMapperBuilder.json() + .modules(module) + .featuresToDisable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS) + .build() + } @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'GET ALL users (when there are existing users)'() { given: + // The list of users created by the "dev" configuration def expectedJson = """ [ { @@ -119,9 +148,7 @@ class UsersControllerIntegrationTests extends Specification { result.andExpect(status().isNotFound()) } - //TODO: These are broken due to a bug in Spring Boot. Unignore these after we update to spring boot 2.0.8+. - @Ignore - @DirtiesContext + @Rollback @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'DELETE ONE existing user'() { when: 'GET request is made for one existing user' @@ -143,7 +170,7 @@ class UsersControllerIntegrationTests extends Specification { result.andExpect(status().isNotFound()) } - @Ignore + @Rollback @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'POST new user persists properly'() { given: @@ -164,7 +191,7 @@ class UsersControllerIntegrationTests extends Specification { result.andExpect(status().isOk()) } - @Ignore + @Rollback @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'POST new duplicate username returns 409'() { given: @@ -189,38 +216,35 @@ class UsersControllerIntegrationTests extends Specification { result.andExpect(status().isConflict()) } - @Ignore + @Rollback @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'PATCH updates user properly'() { given: - def newUser = [firstName: 'Foo', - lastName: 'Bar', - username: 'FooBar', - password: 'somepass', - emailAddress: 'foo@institution.edu', - role: 'ROLE_USER'] - + def String userString = mockMvc.perform(get("$RESOURCE_URI/none")).andReturn().getResponse().getContentAsString() + def User user = mapper.readValue(userString, User.class); + user.setFirstName("somethingnew") + when: - def result = mockMvc.perform(post(RESOURCE_URI) + def result = mockMvc.perform(patch("$RESOURCE_URI/$user.username") .contentType(MediaType.APPLICATION_JSON) - .content(JsonOutput.toJson(newUser)) + .content(mapper.writeValueAsString(user)) .accept(MediaType.APPLICATION_JSON)) then: result.andExpect(status().isOk()) - + when: - newUser['firstName'] = 'Bob' - result = mockMvc.perform(patch("$RESOURCE_URI/$newUser.username") - .contentType(MediaType.APPLICATION_JSON) - .content(JsonOutput.toJson(newUser)) - .accept(MediaType.APPLICATION_JSON)) - + user.setGroupId("AAA") + def resultNewGroup = mockMvc.perform(patch("$RESOURCE_URI/$user.username").contentType(MediaType.APPLICATION_JSON) + .content(mapper.writeValueAsString(user)).accept(MediaType.APPLICATION_JSON)) + then: - result.andExpect(status().isOk()) + resultNewGroup.andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(jsonPath("\$.groupId").value("AAA")) } - @Ignore + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'PATCH detects unknown username'() { given: def newUser = [firstName: 'Foo',