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 :/
TIER · Jul 23, 2021 · 3f350e0 · 3f350e0
1 parent 117fc98
commit 3f350e0
Show file tree

Hide file tree

Showing 6 changed files with 175 additions and 129 deletions.
diff --git a/...end/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy b/...end/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
@@ -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/...main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/UsersController.java b/...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
@@ -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<UserGroup> 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();