From 95a1529bdbbaff4a211a8e4d53d6b53af810aca4 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Sat, 3 Jul 2021 15:38:17 -0700 Subject: [PATCH] SHIBUI-1740 Group controller refactoring --- .../security/controller/GroupController.java | 69 +++---------------- .../GroupControllerExceptionHandler.java | 46 +++++++++++++ .../exception/GroupDeleteException.java | 7 ++ .../GroupExistsConflictException.java | 7 ++ .../ui/security/service/GroupServiceImpl.java | 32 ++++++++- .../ui/security/service/IGroupService.java | 9 ++- .../GroupsControllerIntegrationTests.groovy | 32 ++++----- 7 files changed, 123 insertions(+), 79 deletions(-) create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java index 6ebe985c6..9f7a2f5c9 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java @@ -17,6 +17,9 @@ import org.springframework.web.servlet.support.ServletUriComponentsBuilder; 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.GroupDeleteException; +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.service.IGroupService; @@ -29,41 +32,16 @@ public class GroupController { @Secured("ROLE_ADMIN") @PostMapping @Transactional - public ResponseEntity create(@RequestBody Group group) { - // If already defined, we can't create a new one, nor will this call update the definition - Group foundGroup = groupService.find(group.getResourceId()); - - if (foundGroup != null) { - HttpHeaders headers = new HttpHeaders(); - headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri()); - - return ResponseEntity.status(HttpStatus.METHOD_NOT_ALLOWED).headers(headers) - .body(new ErrorResponse(String.valueOf(HttpStatus.METHOD_NOT_ALLOWED.value()), - String.format("The group with resource id: [%s] and name: [%s] already exists.", - group.getResourceId(), group.getName()))); - } - - Group result = groupService.createOrUpdateGroup(group); + public ResponseEntity create(@RequestBody Group group) throws GroupExistsConflictException { + Group result = groupService.createGroup(group); return ResponseEntity.status(HttpStatus.CREATED).body(result); } @Secured("ROLE_ADMIN") @PutMapping @Transactional - public ResponseEntity update(@RequestBody Group group) { - Group g = groupService.find(group.getResourceId()); - - if (g == null) { - HttpHeaders headers = new HttpHeaders(); - headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri()); - - return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers) - .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()), - String.format("Unable to find group with resource id: [%s] and name: [%s]", - group.getResourceId(), group.getName()))); - } - - Group result = groupService.createOrUpdateGroup(group); + public ResponseEntity update(@RequestBody Group group) throws EntityNotFoundException { + Group result = groupService.updateGroup(group); return ResponseEntity.ok(result); } @@ -75,16 +53,10 @@ public ResponseEntity getAll() { @GetMapping("/{resourceId}") @Transactional(readOnly = true) - public ResponseEntity getOne(@PathVariable String resourceId) { + public ResponseEntity getOne(@PathVariable String resourceId) throws EntityNotFoundException { Group g = groupService.find(resourceId); - if (g == null) { - HttpHeaders headers = new HttpHeaders(); - headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri()); - - return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers) - .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()), - String.format("Unable to find group with resource id: [%s]", resourceId))); + throw new EntityNotFoundException(String.format("Unable to find group with resource id: [%s]", resourceId)); } return ResponseEntity.ok(g); } @@ -92,27 +64,8 @@ public ResponseEntity getOne(@PathVariable String resourceId) { @Secured("ROLE_ADMIN") @DeleteMapping("/{resourceId}") @Transactional - public ResponseEntity delete(@PathVariable String resourceId) { - Group g = groupService.find(resourceId); - - if (g == null) { - HttpHeaders headers = new HttpHeaders(); - headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri()); - - return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers) - .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()), - String.format("Unable to find group with resource id: [%s]", resourceId))); - } - if (!g.getUsers().isEmpty() || !g.getEntityDescriptors().isEmpty()) { - HttpHeaders headers = new HttpHeaders(); - headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri()); - - return ResponseEntity.status(HttpStatus.CONFLICT).headers(headers) - .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), String.format( - "Unable to delete group with resource id: [%s] - remove all users and entities from group first", - resourceId))); - } - groupService.deleteDefinition(g); + public ResponseEntity delete(@PathVariable String resourceId) throws EntityNotFoundException, GroupDeleteException { + groupService.deleteDefinition(resourceId); return ResponseEntity.noContent().build(); } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java new file mode 100644 index 000000000..e8bb3b4af --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java @@ -0,0 +1,46 @@ +package edu.internet2.tier.shibboleth.admin.ui.security.controller; + +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ControllerAdvice; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.context.request.WebRequest; +import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; +import org.springframework.web.servlet.support.ServletUriComponentsBuilder; + +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.GroupDeleteException; +import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException; + +@ControllerAdvice(assignableTypes = {GroupController.class}) +public class GroupControllerExceptionHandler extends ResponseEntityExceptionHandler { + + @ExceptionHandler({ EntityNotFoundException.class }) + public ResponseEntity handleEntityNotFoundException(EntityNotFoundException e, WebRequest request) { + HttpHeaders headers = new HttpHeaders(); + headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri()); + + return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers) + .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()), e.getMessage())); + } + + @ExceptionHandler({ GroupDeleteException.class }) + public ResponseEntity handleForbiddenAccess(GroupDeleteException e, WebRequest request) { + HttpHeaders headers = new HttpHeaders(); + headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri()); + + return ResponseEntity.status(HttpStatus.CONFLICT).headers(headers) + .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), e.getMessage())); + } + + @ExceptionHandler({ GroupExistsConflictException.class }) + public ResponseEntity handleGroupExistsConflict(GroupExistsConflictException e, WebRequest request) { + HttpHeaders headers = new HttpHeaders(); + headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri()); + + return ResponseEntity.status(HttpStatus.METHOD_NOT_ALLOWED).headers(headers) + .body(new ErrorResponse(String.valueOf(HttpStatus.METHOD_NOT_ALLOWED.value()), e.getMessage())); + } +} diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java new file mode 100644 index 000000000..f95d09142 --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java @@ -0,0 +1,7 @@ +package edu.internet2.tier.shibboleth.admin.ui.security.exception; + +public class GroupDeleteException extends Exception { + public GroupDeleteException(String message) { + super(message); + } +} diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java new file mode 100644 index 000000000..5566f034c --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java @@ -0,0 +1,7 @@ +package edu.internet2.tier.shibboleth.admin.ui.security.exception; + +public class GroupExistsConflictException extends Exception { + public GroupExistsConflictException(String message) { + super(message); + } +} 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 f4c3a1344..fd544cc38 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 @@ -5,6 +5,9 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; +import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException; +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.Group; import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository; @@ -14,13 +17,26 @@ public class GroupServiceImpl implements IGroupService { private GroupsRepository repo; @Override - public Group createOrUpdateGroup(Group group) { + public Group createGroup(Group group) throws GroupExistsConflictException { + Group foundGroup = find(group.getResourceId()); + // If already defined, we can't 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())); + } return repo.save(group); } @Override - public void deleteDefinition(Group group) { - repo.delete(group); + public void deleteDefinition(String resourceId) throws EntityNotFoundException, GroupDeleteException { + Group g = find(resourceId); + if (!g.getUsers().isEmpty() || !g.getEntityDescriptors().isEmpty()) { + throw new GroupDeleteException(String.format( + "Unable to delete group with resource id: [%s] - remove all users and entities from group first", + resourceId)); + } + repo.delete(g); } @Override @@ -33,4 +49,14 @@ public List findAll() { return repo.findAll(); } + @Override + public Group updateGroup(Group group) throws EntityNotFoundException { + Group g = find(group.getResourceId()); + if (g == null) { + throw new EntityNotFoundException(String.format("Unable to find group with resource id: [%s] and name: [%s]", + group.getResourceId(), group.getName())); + } + return repo.save(group); + } + } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java index 1e040a9d1..9d9a51f87 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java @@ -2,16 +2,21 @@ import java.util.List; +import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException; +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.Group; public interface IGroupService { - Group createOrUpdateGroup(Group g); + Group createGroup(Group group) throws GroupExistsConflictException; - void deleteDefinition(Group g); + void deleteDefinition(String resourceId) throws EntityNotFoundException, GroupDeleteException; Group find(String resourceId); List findAll(); + Group updateGroup(Group g) throws EntityNotFoundException; + } 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 a1199bf21..a9948b72c 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 @@ -13,6 +13,9 @@ import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.result.MockMvcResultHandlers import org.springframework.transaction.annotation.Transactional +import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException +import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException +import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException import spock.lang.Ignore import spock.lang.Specification @@ -68,13 +71,11 @@ class GroupsControllerIntegrationTests extends Specification { .andExpect(content().json(expectedJson, false)) when: 'Try to create with an existing resource id' - result = mockMvc.perform(post(RESOURCE_URI) - .contentType(MediaType.APPLICATION_JSON) - .content(JsonOutput.toJson(newGroup)) - .accept(MediaType.APPLICATION_JSON)) + def exceptionExpected = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup)) + .accept(MediaType.APPLICATION_JSON)).andReturn().getResolvedException() then: 'Expecting method not allowed' - result.andExpect(status().isMethodNotAllowed()) + exceptionExpected instanceof GroupExistsConflictException == true } @Rollback @@ -107,13 +108,11 @@ class GroupsControllerIntegrationTests extends Specification { def newGroup = [name: 'XXXXX', description: 'should not work', resourceId: 'XXXX'] - def notFoundresult = mockMvc.perform(put(RESOURCE_URI) - .contentType(MediaType.APPLICATION_JSON) - .content(JsonOutput.toJson(newGroup)) - .accept(MediaType.APPLICATION_JSON)) - + def exceptionExpected = mockMvc.perform(put(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup)) + .accept(MediaType.APPLICATION_JSON)).andReturn().getResolvedException() + then: 'Expecting nothing happened because the object was not found' - notFoundresult.andExpect(status().isNotFound()) + exceptionExpected instanceof EntityNotFoundException == true } @WithMockUser(value = "admin", roles = ["ADMIN"]) @@ -147,10 +146,10 @@ class GroupsControllerIntegrationTests extends Specification { singleGroupRequest.andExpect(status().isOk()) when: 'GET request for a single non-existent group in a system that has groups' - def nonexistentGroupRequest = mockMvc.perform(get("$RESOURCE_URI/CCC")) + def exceptionExpected = mockMvc.perform(get("$RESOURCE_URI/CCC")).andReturn().getResolvedException() then: 'The group not found' - nonexistentGroupRequest.andExpect(status().isNotFound()) + exceptionExpected instanceof EntityNotFoundException == true } @Rollback @@ -197,9 +196,10 @@ class GroupsControllerIntegrationTests extends Specification { when: 'DELETE request is made' entityManager.flush() entityManager.clear() - result = mockMvc.perform(delete("$RESOURCE_URI/$group.resourceId")) + + def exceptionExpected = mockMvc.perform(delete("$RESOURCE_URI/$group.resourceId")).andReturn().getResolvedException() - then: 'DELETE was not successful' - result.andExpect(status().isConflict()) + then: 'Expecting method not allowed' + exceptionExpected instanceof GroupDeleteException == true } }