diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java index 904f3abc5..ac02c5067 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java @@ -9,6 +9,7 @@ import javax.persistence.Column; import javax.persistence.Entity; import javax.persistence.EntityListeners; +import javax.persistence.FetchType; import javax.persistence.Id; import javax.persistence.OneToMany; import javax.persistence.Transient; @@ -54,7 +55,7 @@ public class Group implements Owner { @Column(name = "validation_regex") private String validationRegex; - @OneToMany + @OneToMany(fetch = FetchType.LAZY) private List approversList = new ArrayList<>(); /** diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/repository/ApproversRepository.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/repository/ApproversRepository.java index d7d20cef4..642a18481 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/repository/ApproversRepository.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/repository/ApproversRepository.java @@ -4,4 +4,5 @@ import org.springframework.data.jpa.repository.JpaRepository; public interface ApproversRepository extends JpaRepository { + Approvers findByResourceId(String resourceId); } \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java index f329a5be2..6eac2fde7 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java @@ -4,7 +4,9 @@ import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException; import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException; import edu.internet2.tier.shibboleth.admin.ui.security.exception.InvalidGroupRegexException; +import edu.internet2.tier.shibboleth.admin.ui.security.model.Approvers; import edu.internet2.tier.shibboleth.admin.ui.security.model.Group; +import edu.internet2.tier.shibboleth.admin.ui.security.repository.ApproversRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.OwnershipRepository; import lombok.NoArgsConstructor; @@ -16,6 +18,7 @@ import javax.script.ScriptEngine; import javax.script.ScriptEngineManager; import javax.script.ScriptException; +import java.util.ArrayList; import java.util.List; @Service @@ -25,28 +28,29 @@ public class GroupServiceImpl implements IGroupService { private static final String REGEX_MATCHER = "function validate(r, s){ return RegExp(r).test(s);};validate(rgx, str);"; private final ScriptEngine engine = new ScriptEngineManager().getEngineByName("JavaScript"); + @Autowired + protected ApproversRepository approversRepository; + @Autowired protected GroupsRepository groupRepository; @Autowired protected OwnershipRepository ownershipRepository; - public GroupServiceImpl(GroupsRepository repo, OwnershipRepository ownershipRepository) { - this.groupRepository = repo; - this.ownershipRepository = ownershipRepository; - } +// public GroupServiceImpl(GroupsRepository repo, OwnershipRepository ownershipRepository) { +// this.groupRepository = repo; +// this.ownershipRepository = ownershipRepository; +// } @Override @Transactional public Group createGroup(Group group) throws GroupExistsConflictException, InvalidGroupRegexException { - Group foundGroup = find(group.getResourceId()); // If already defined, we don't want to create a new one, nor do we want this call update the definition - if (foundGroup != null) { - throw new GroupExistsConflictException( - String.format("Call update (PUT) to modify the group with resource id: [%s] and name: [%s]", - foundGroup.getResourceId(), foundGroup.getName())); + if (groupRepository.existsById(group.getResourceId())) { + throw new GroupExistsConflictException(String.format("Call update (PUT) to modify the group with resource id: [%s] and name: [%s]", group.getResourceId(), group.getName())); } validateGroupRegex(group); + manageApproversList(group); return groupRepository.save(group); } @@ -59,6 +63,7 @@ public void deleteDefinition(String resourceId) throws PersistentEntityNotFound, "Unable to delete group with resource id: [%s] - remove all items owned by / associated with the group first", resourceId)); } + approversRepository.deleteAll(group.getApproversList()); groupRepository.delete(group); } @@ -117,6 +122,7 @@ public List findAll() { @Override public Group updateGroup(Group group) throws PersistentEntityNotFound, InvalidGroupRegexException { + manageApproversList(group); // have to make sure that approvers have been saved before a fetch or we can get data integrity errors on lookup... Group g = find(group.getResourceId()); if (g == null) { throw new PersistentEntityNotFound(String.format("Unable to find group with resource id: [%s] and name: [%s]", @@ -126,6 +132,21 @@ public Group updateGroup(Group group) throws PersistentEntityNotFound, InvalidGr return groupRepository.save(group); } + private void manageApproversList(Group group) { + if (group.getApproversList().isEmpty()) { + return; + } + List updatedApprovers = new ArrayList<>(); + group.getApproversList().forEach(approvers -> { + Approvers savedApprovers = approversRepository.findByResourceId(approvers.getResourceId()); + savedApprovers = savedApprovers == null ? approvers : savedApprovers; + savedApprovers.setApproverGroups(approvers.getApproverGroups()); + Approvers updatedApp = approversRepository.save(savedApprovers); + updatedApprovers.add(updatedApp); + }); + group.setApproversList(updatedApprovers); + } + /** * If the regex is blank simply return */ diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy index bb4613f6b..6fae2ed87 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy @@ -4,6 +4,7 @@ import edu.internet2.tier.shibboleth.admin.ui.AbstractBaseDataJpaTest import edu.internet2.tier.shibboleth.admin.ui.exception.PersistentEntityNotFound import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException +import edu.internet2.tier.shibboleth.admin.ui.security.model.Approvers import edu.internet2.tier.shibboleth.admin.ui.security.model.Group import edu.internet2.tier.shibboleth.admin.ui.security.model.Role import edu.internet2.tier.shibboleth.admin.ui.security.model.User @@ -47,36 +48,65 @@ class GroupsControllerIntegrationTests extends AbstractBaseDataJpaTest { @WithMockAdmin def 'POST new group persists properly'() { given: - def newGroup = [name: 'Foo', - description: 'Bar', - resourceId: 'FooBar'] - - def expectedJson = """ - { - "name":"Foo", - "description":"Bar", - "resourceId":"FooBar" - } -""" + def newGroup = [name: 'Foo', description: 'Bar', resourceId: 'FooBar'] + when: - def result = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON) - .content(JsonOutput.toJson(newGroup)).accept(MediaType.APPLICATION_JSON)) + def result = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup)).accept(MediaType.APPLICATION_JSON)) then: result.andExpect(status().isCreated()) - .andExpect(content().contentType(MediaType.APPLICATION_JSON)) - .andExpect(content().json(expectedJson, false)) - + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(jsonPath("\$.name").value("Foo")) + .andExpect(jsonPath("\$.resourceId").value("FooBar")) + .andExpect(jsonPath("\$.description").value("Bar")) - //'Try to create with an existing resource id' + //'Try to create with an existing resource id' try { mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON) .content(JsonOutput.toJson(newGroup)) .accept(MediaType.APPLICATION_JSON)) - false + false // failure if the call didn't throw } catch (Throwable expected) { expected instanceof GroupExistsConflictException } + + when: "POST new group with approvers" + groupService.clearAllForTesting() + List apprGroups = new ArrayList<>() + String[] groupNames = ['AAA', 'BBB', 'CCC', 'DDD'] + groupNames.each {name -> { + Group group = new Group().with({ + it.name = name + it.description = name + it.resourceId = name + it + }) + groupRepository.save(group) + }} + entityManager.flush() + entityManager.clear() + + groupNames.each {name ->{ + if (!name.equals('AAA')) { + apprGroups.add(groupRepository.findByResourceId(name)) + } + }} + Approvers approvers = new Approvers() + approvers.setApproverGroups(apprGroups) + def apprList = new ArrayList<>() + apprList.add(approvers) + def newGroup2 = [name: 'Foo', description: 'Bar', resourceId: 'FooBar', approversList: apprList] + def result2 = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup2)).accept(MediaType.APPLICATION_JSON)) + + then: + result2.andExpect(status().isCreated()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect(jsonPath("\$.name").value("Foo")) + .andExpect(jsonPath("\$.resourceId").value("FooBar")) + .andExpect(jsonPath("\$.description").value("Bar")) + .andExpect(jsonPath("\$.approversList[0].approverGroups[0].resourceId").value("BBB")) + .andExpect(jsonPath("\$.approversList[0].approverGroups[1].resourceId").value("CCC")) + .andExpect(jsonPath("\$.approversList[0].approverGroups[2].resourceId").value("DDD")) } @WithMockAdmin diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy index a81443166..f0dcfa68d 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy @@ -176,11 +176,11 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { def groupFromDb = gList.get(0) as Group groupFromDb == group - // update check + // update check - equality for groups should be by id groupFromDb.with { it.description = "some new text that wasn't there before" } - groupFromDb.equals(group) == false + groupFromDb.equals(group) == true when: groupsRepo.save(groupFromDb) @@ -189,7 +189,7 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest { def gList2 = groupsRepo.findAll() gList2.size() == 1 def groupFromDb2 = gList2.get(0) as Group - groupFromDb2.equals(group) == false + groupFromDb2.equals(group) == true groupFromDb2 == groupFromDb // delete tests diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy index 608e8065a..16045bca7 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy @@ -84,7 +84,7 @@ class GroupServiceTests extends AbstractBaseDataJpaTest { def "CRUD operations - approver groups" () { given: - groupService.clearAllForTesting(); + groupService.clearAllForTesting() List apprGroups = new ArrayList<>() String[] groupNames = ['AAA', 'BBB', 'CCC', 'DDD'] groupNames.each {name -> { @@ -94,38 +94,62 @@ class GroupServiceTests extends AbstractBaseDataJpaTest { it.resourceId = name it }) - group = groupRepository.save(group) + groupRepository.save(group) }} entityManager.flush() entityManager.clear() + when: "Adding approval list to a group" groupNames.each {name ->{ if (!name.equals('AAA')) { apprGroups.add(groupRepository.findByResourceId(name)) } }} - - when: "Adding approval list to a group" Approvers approvers = new Approvers() approvers.setApproverGroups(apprGroups) - approvers = approversRepository.save(approvers) - entityManager.flush() - entityManager.clear() - - List apprList = new ArrayList<>() + def apprList = new ArrayList<>() apprList.add(approvers) Group aaaGroup = groupService.find('AAA') aaaGroup.setApproversList(apprList) groupService.updateGroup(aaaGroup) - Group lookupGroup = groupService.find('AAA') then: + def lookupGroup = groupService.find('AAA') lookupGroup.getApproversList().size() == 1 - List approvalGroups = lookupGroup.getApproversList().get(0).getApproverGroups() + def approvalGroups = lookupGroup.getApproversList().get(0).getApproverGroups() approvalGroups.size() == 3 apprGroups.each {group -> { assert approvalGroups.contains(group)} } + + when: "removing approver group from existing list" + approvers.getApproverGroups().remove(groupService.find('BBB')) + apprList = new ArrayList<>() + apprList.add(approvers) + aaaGroup.setApproversList(apprList) + groupService.updateGroup(aaaGroup) + + then: + def lookupGroup2 = groupService.find('AAA') + lookupGroup2.getApproversList().size() == 1 + def approvalGroups2 = lookupGroup2.getApproversList().get(0).getApproverGroups() + approvalGroups2.size() == 2 + apprGroups.each { group -> + { + if (group.getResourceId() != 'BBB') { + assert approvalGroups2.contains(group) + } + } + } + + when: "removing all approver groups" + apprList = new ArrayList<>() + aaaGroup.setApproversList(apprList) + groupService.updateGroup(aaaGroup) + + then: + def lookupGroup3 = groupService.find('AAA') + lookupGroup3.getApproversList().isEmpty() == true } } \ No newline at end of file