From 4c34bd7d1cd77cb2d0f52fca82ec9a039571ec37 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Mon, 28 Jan 2019 23:08:39 -0700 Subject: [PATCH] [SHIBUI-1058] Replaced @Secured with @PreAuthorize Extensive refactoring to UserControllerIntegrationTests. Replaced restTemplate usage with MockMvc so we can use @WithMockUser. However, only the GET methods seem to honor the security settings and pass/fail appropriately when the settings are changed. POST/PATCH/DELETE do not. --- backend/build.gradle | 1 + .../admin/ui/configuration/DevConfig.groovy | 8 - .../security/controller/UsersController.java | 12 +- .../UsersControllerIntegrationTests.groovy | 172 +++++++++++++----- 4 files changed, 132 insertions(+), 61 deletions(-) diff --git a/backend/build.gradle b/backend/build.gradle index a6797b52c..914ccc374 100644 --- a/backend/build.gradle +++ b/backend/build.gradle @@ -150,6 +150,7 @@ dependencies { compile 'io.springfox:springfox-swagger-ui:2.9.2' testCompile "org.springframework.boot:spring-boot-starter-test" + testCompile "org.springframework.security:spring-security-test" testCompile "org.spockframework:spock-core:1.1-groovy-2.4" testCompile "org.spockframework:spock-spring:1.1-groovy-2.4" testCompile "org.xmlunit:xmlunit-core:2.5.1" 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 2bc831cd0..a137526a4 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 @@ -79,14 +79,6 @@ class DevConfig { emailAddress = 'anon@institution.edu' roles.add(roleRepository.findByName('ROLE_ADMIN').get()) it - }, new User().with { // allow us to auto-login as an admin - username = 'wsmith@unicon.net' - password = '{noop}anonymous' - firstName = 'Anon' - lastName = 'Ymous' - emailAddress = 'anon@institution.edu' - roles.add(roleRepository.findByName('ROLE_ADMIN').get()) - it }] users.each { adminUserRepository.save(it) 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 fecc79e9f..42b4ab508 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 @@ -10,7 +10,7 @@ import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; -import org.springframework.security.access.annotation.Secured; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.security.crypto.bcrypt.BCrypt; import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.DeleteMapping; @@ -50,7 +50,7 @@ public UsersController(UserRepository userRepository, RoleRepository roleReposit this.userService = userService; } - @Secured("ROLE_ADMIN") + @PreAuthorize("hasRole('ADMIN')") @Transactional(readOnly = true) @GetMapping public List getAll() { @@ -63,14 +63,14 @@ public Principal getCurrentUser(Principal principal) { return principal; } - @Secured("ROLE_ADMIN") + @PreAuthorize("hasRole('ADMIN')") @Transactional(readOnly = true) @GetMapping("/{username}") public ResponseEntity getOne(@PathVariable String username) { return ResponseEntity.ok(findUserOrThrowHttp404(username)); } - @Secured("ROLE_ADMIN") + @PreAuthorize("hasRole('ADMIN')") @Transactional @DeleteMapping("/{username}") public ResponseEntity deleteOne(@PathVariable String username) { @@ -79,7 +79,7 @@ public ResponseEntity deleteOne(@PathVariable String username) { return ResponseEntity.noContent().build(); } - @Secured("ROLE_ADMIN") + @PreAuthorize("hasRole('ADMIN')") @Transactional @PostMapping ResponseEntity saveOne(@RequestBody User user) { @@ -97,7 +97,7 @@ ResponseEntity saveOne(@RequestBody User user) { return ResponseEntity.ok(savedUser); } - @Secured("ROLE_ADMIN") + @PreAuthorize("hasRole('ADMIN')") @Transactional @PatchMapping("/{username}") ResponseEntity updateOne(@PathVariable(value = "username") String username, @RequestBody User user) { 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 04aa033e9..bd5d9c026 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,83 +1,141 @@ package edu.internet2.tier.shibboleth.admin.ui.security.controller -import com.fasterxml.jackson.databind.ObjectMapper -import com.fasterxml.jackson.databind.SerializationFeature + 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.boot.test.web.client.TestRestTemplate -import org.springframework.http.HttpEntity -import org.springframework.http.HttpHeaders -import org.springframework.http.HttpMethod +import org.springframework.http.MediaType +import org.springframework.security.test.context.support.WithMockUser import org.springframework.test.annotation.DirtiesContext import org.springframework.test.context.ActiveProfiles +import org.springframework.test.web.servlet.MockMvc import spock.lang.Specification +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch +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 + /** * @author Dmitriy Kopylenko */ -@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@SpringBootTest +@AutoConfigureMockMvc @ActiveProfiles(["no-auth", "dev"]) class UsersControllerIntegrationTests extends Specification { @Autowired - private TestRestTemplate restTemplate + private MockMvc mockMvc static RESOURCE_URI = '/api/admin/users' - ObjectMapper mapper - - def setup() { - mapper = new ObjectMapper() - mapper.enable(SerializationFeature.INDENT_OUTPUT) - } - + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'GET ALL users (when there are existing users)'() { + given: + def expectedJson = """ +[ + { + "modifiedBy" : null, + "firstName" : "Joe", + "emailAddress" : "joe@institution.edu", + "role" : "ROLE_ADMIN", + "username" : "admin", + "createdBy" : null, + "audId" : 4, + "lastName" : "Doe" + }, + { + "modifiedBy" : null, + "firstName" : "Peter", + "emailAddress" : "peter@institution.edu", + "role" : "ROLE_USER", + "username" : "nonadmin", + "createdBy" : null, + "audId" : 5, + "lastName" : "Vandelay" + }, + { + "modifiedBy" : null, + "firstName" : "Anon", + "emailAddress" : "anon@institution.edu", + "role" : "ROLE_ADMIN", + "username" : "anonymousUser", + "createdBy" : null, + "audId" : 6, + "lastName" : "Ymous" + } +]""" when: 'GET request is made for ALL users in the system, and system has users in it' - def result = this.restTemplate.getForEntity(RESOURCE_URI, Object) + def result = mockMvc.perform(get(RESOURCE_URI)) + then: 'Request completed with HTTP 200 and returned a list of users' - result.statusCodeValue == 200 - result.body[0].username == 'admin' - result.body[0].role == 'ROLE_ADMIN' + result + .andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8)) + .andExpect(content().json(expectedJson, false)) + } + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'GET ONE existing user'() { + given: + def expectedJson = """ +{ + "modifiedBy" : null, + "firstName" : "Joe", + "emailAddress" : "joe@institution.edu", + "role" : "ROLE_ADMIN", + "username" : "admin", + "createdBy" : null, + "audId" : 4, + "lastName" : "Doe" +}""" when: 'GET request is made for one existing user' - def result = this.restTemplate.getForEntity("$RESOURCE_URI/admin", Map) + def result = mockMvc.perform(get("$RESOURCE_URI/admin")) then: 'Request completed with HTTP 200 and returned one user' - result.statusCodeValue == 200 - result.body.username == 'admin' - result.body.role == 'ROLE_ADMIN' + result + .andExpect(status().isOk()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8)) + .andExpect(content().json(expectedJson, false)) } + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'GET ONE NON-existing user'() { when: 'GET request is made for one NON-existing user' - def result = this.restTemplate.getForEntity("$RESOURCE_URI/bogus", Map) + def result = mockMvc.perform(get("$RESOURCE_URI/bogus")) then: 'Request completed with HTTP 404' - result.statusCodeValue == 404 - result.body.errorCode == '404' - result.body.errorMessage == 'User with username [bogus] not found' + result.andExpect(status().isNotFound()) } @DirtiesContext + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'DELETE ONE existing user'() { when: 'GET request is made for one existing user' - def result = this.restTemplate.getForEntity("$RESOURCE_URI/admin", Map) + def result = mockMvc.perform(get("$RESOURCE_URI/nonadmin")) then: 'Request completed with HTTP 200' - result.statusCodeValue == 200 + result.andExpect(status().isOk()) when: 'DELETE request is made' - this.restTemplate.delete("$RESOURCE_URI/admin") - result = this.restTemplate.getForEntity("$RESOURCE_URI/admin", Map) + result = mockMvc.perform(delete("$RESOURCE_URI/nonadmin")) + + then: 'DELETE was successful' + result.andExpect(status().isNoContent()) + + when: 'GET request is made for the deleted user' + result = mockMvc.perform(get("$RESOURCE_URI/nonadmin")) then: 'The deleted user is gone' - result.statusCodeValue == 404 + result.andExpect(status().isNotFound()) } + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'POST new user persists properly'() { given: def newUser = [firstName: 'Foo', @@ -88,12 +146,16 @@ class UsersControllerIntegrationTests extends Specification { role: 'ROLE_USER'] when: - def result = this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { JsonOutput.toJson(newUser) }, Map) + def result = mockMvc.perform(post(RESOURCE_URI) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser)) + .accept(MediaType.APPLICATION_JSON)) then: - result.statusCodeValue == 200 + result.andExpect(status().isOk()) } + @WithMockUser(value = "admin", roles = ["ADMIN"]) def 'POST new duplicate username returns 409'() { given: def newUser = [firstName: 'Foo', @@ -104,14 +166,21 @@ class UsersControllerIntegrationTests extends Specification { role: 'ROLE_USER'] when: - this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { JsonOutput.toJson(newUser) }, Map) - def result = this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { JsonOutput.toJson(newUser) }, Map) + mockMvc.perform(post(RESOURCE_URI) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser)) + .accept(MediaType.APPLICATION_JSON)) + def result = mockMvc.perform(post(RESOURCE_URI) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser)) + .accept(MediaType.APPLICATION_JSON)) then: - result.statusCodeValue == 409 + result.andExpect(status().isConflict()) } - def 'PUT updates user properly'() { + @WithMockUser(value = "admin", roles = ["ADMIN"]) + def 'PATCH updates user properly'() { given: def newUser = [firstName: 'Foo', lastName: 'Bar', @@ -121,12 +190,23 @@ class UsersControllerIntegrationTests extends Specification { role: 'ROLE_USER'] when: - this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { JsonOutput.toJson(newUser) }, Map) + def result = mockMvc.perform(post(RESOURCE_URI) + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser)) + .accept(MediaType.APPLICATION_JSON)) + + then: + result.andExpect(status().isOk()) + + when: newUser['firstName'] = 'Bob' - def result = this.restTemplate.exchange("$RESOURCE_URI/$newUser.username", HttpMethod.PATCH, createRequestHttpEntityFor { JsonOutput.toJson(newUser) }, Map) + result = mockMvc.perform(patch("$RESOURCE_URI/$newUser.username") + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser)) + .accept(MediaType.APPLICATION_JSON)) then: - result.statusCodeValue == 200 + result.andExpect(status().isOk()) } def 'PATCH detects unknown username'() { @@ -139,13 +219,11 @@ class UsersControllerIntegrationTests extends Specification { role: 'ROLE_USER'] when: - def result = this.restTemplate.exchange("$RESOURCE_URI/$newUser.username", HttpMethod.PATCH, createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + def result = mockMvc.perform(patch("$RESOURCE_URI/$newUser.username") + .contentType(MediaType.APPLICATION_JSON_UTF8) + .content(JsonOutput.toJson(newUser))) then: - result.statusCodeValue == 404 - } - - private HttpEntity createRequestHttpEntityFor(Closure jsonBodySupplier) { - new HttpEntity(jsonBodySupplier(), ['Content-Type': 'application/json'] as HttpHeaders) + result.andExpect(status().isNotFound()) } } \ No newline at end of file