[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.
TIER · Jan 29, 2019 · 4c34bd7 · 4c34bd7
1 parent b30e1ff
commit 4c34bd7
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 61 deletions.
diff --git 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/...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
@@ -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/...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
@@ -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<User> 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/...rnet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy b/...rnet2/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<String> createRequestHttpEntityFor(Closure jsonBodySupplier) {
-        new HttpEntity<String>(jsonBodySupplier(), ['Content-Type': 'application/json'] as HttpHeaders)
+        result.andExpect(status().isNotFound())
     }
 }