From dde228471386431661c93ec0ab2700e27477e7d3 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Fri, 14 Dec 2018 09:49:52 -0700 Subject: [PATCH] [SHIBUI-1031] Added tests for POST/PUT. --- .../security/controller/UsersController.java | 64 ++++++++----- .../admin/ui/security/model/Role.java | 4 + .../admin/ui/security/model/User.java | 10 +- .../UsersControllerIntegrationTests.groovy | 93 +++++++++++++++++++ 4 files changed, 144 insertions(+), 27 deletions(-) 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 272e178dc..bd479b1c8 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 @@ -1,25 +1,29 @@ 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.security.model.Role; import edu.internet2.tier.shibboleth.admin.ui.security.model.User; +import edu.internet2.tier.shibboleth.admin.ui.security.repository.RoleRepository; import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; -import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; -import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.PutMapping; +import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; -import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.HttpClientErrorException; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import static org.springframework.http.HttpStatus.NOT_FOUND; @@ -29,29 +33,33 @@ * @author Dmitriy Kopylenko */ @RestController -@RequestMapping("/api/admin") +@RequestMapping("/api/admin/users") public class UsersController { + private static final Logger logger = LoggerFactory.getLogger(UsersController.class); + private UserRepository userRepository; + private RoleRepository roleRepository; - public UsersController(UserRepository userRepository) { + public UsersController(UserRepository userRepository, RoleRepository roleRepository) { this.userRepository = userRepository; + this.roleRepository = roleRepository; } @Transactional(readOnly = true) - @GetMapping("/users") + @GetMapping public List getAll() { return userRepository.findAll(); } @Transactional(readOnly = true) - @GetMapping("/users/{username}") + @GetMapping("/{username}") public ResponseEntity getOne(@PathVariable String username) { return ResponseEntity.ok(findUserOrThrowHttp404(username)); } @Transactional - @DeleteMapping("/users/{username}") + @DeleteMapping("/{username}") public ResponseEntity deleteOne(@PathVariable String username) { User user = findUserOrThrowHttp404(username); userRepository.delete(user); @@ -59,39 +67,47 @@ public ResponseEntity deleteOne(@PathVariable String username) { } @Transactional - @PostMapping("/users") - ResponseEntity saveOne(@RequestParam User user) { + @PostMapping + ResponseEntity saveOne(@RequestBody User user) { Optional persistedUser = userRepository.findByUsername(user.getUsername()); if (persistedUser.isPresent()) { return ResponseEntity .status(HttpStatus.CONFLICT) .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), - String.format("A user with username [ %s ] already exists within the system.", user.getUsername()))); + String.format("A user with username [%s] already exists within the system.", user.getUsername()))); } - //TODO: encrypt password? + user.setRoles(getPersistedRoles(user.getUsername(), user.getRoles())); + //TODO: encrypt password? Or is it sent to us encrypted? User savedUser = userRepository.save(user); return ResponseEntity.ok(savedUser); } @Transactional - @PutMapping("/users/{username}") - ResponseEntity updateOne(@PathVariable(value = "username") String username, @RequestParam User user) { - Optional userSearchResult = userRepository.findByUsername(username); - if (!userSearchResult.isPresent()) { - return ResponseEntity.status(HttpStatus.BAD_REQUEST) - .body(new ErrorResponse(String.valueOf(HttpStatus.BAD_REQUEST.value()), - String.format("No user with username [ %s ] exists within the system.", username))); - } - PasswordEncoder passwordEncoder = new BCryptPasswordEncoder(); - User persistedUser = userSearchResult.get(); - persistedUser.setPassword(passwordEncoder.encode(user.getPassword())); + @PutMapping("/{username}") + ResponseEntity updateOne(@PathVariable(value = "username") String username, @RequestBody User user) { + User persistedUser = findUserOrThrowHttp404(username); + persistedUser.setPassword(user.getPassword()); //TODO: encrypt password? persistedUser.setFirstName(user.getFirstName()); persistedUser.setLastName(user.getLastName()); - persistedUser.setRoles(user.getRoles()); + persistedUser.setEmailAddress(user.getEmailAddress()); + persistedUser.setRoles(getPersistedRoles(user.getUsername(), user.getRoles())); User savedUser = userRepository.save(persistedUser); return ResponseEntity.ok(savedUser); } + private Set getPersistedRoles(String username, Set userRolesToBeUpdated) { + Set newRoles = new HashSet<>(); + for (Role role : userRolesToBeUpdated) { + Optional persistedRole = roleRepository.findByName(role.getName()); + if (!persistedRole.isPresent()) { + logger.warn("Role [%s] is not present in the system. Not setting role for user [%s].", role.getName(), username); + continue; + } + newRoles.add(persistedRole.get()); + } + return newRoles; + } + private User findUserOrThrowHttp404(String username) { return userRepository.findByUsername(username) .orElseThrow(() -> new HttpClientErrorException(NOT_FOUND, String.format("User with username [%s] not found", username))); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java index fcb893c20..e35ae1047 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java @@ -29,6 +29,10 @@ @ToString(exclude = "users") public class Role extends AbstractAuditable { + public Role(String name) { + this.name = name; + } + @Column(unique = true) private String name; 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 index 029b712e6..992b55413 100644 --- 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 @@ -1,9 +1,12 @@ package edu.internet2.tier.shibboleth.admin.ui.security.model; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; import edu.internet2.tier.shibboleth.admin.ui.domain.AbstractAuditable; -import lombok.*; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import lombok.ToString; import javax.persistence.CascadeType; import javax.persistence.Column; @@ -30,7 +33,8 @@ public class User extends AbstractAuditable { @Column(nullable = false, unique = true) private String username; - @JsonProperty(access = JsonProperty.Access.WRITE_ONLY) + //TODO: Need to figure out the right way to protect this property + //@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) @Column(nullable = false) private String password; 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 7c97f7475..075be3d0e 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,8 +1,14 @@ package edu.internet2.tier.shibboleth.admin.ui.security.controller +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.databind.SerializationFeature +import edu.internet2.tier.shibboleth.admin.ui.security.model.Role +import edu.internet2.tier.shibboleth.admin.ui.security.model.User import org.springframework.beans.factory.annotation.Autowired 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.test.annotation.DirtiesContext import org.springframework.test.context.ActiveProfiles import spock.lang.Specification @@ -19,6 +25,13 @@ class UsersControllerIntegrationTests extends Specification { static RESOURCE_URI = '/api/admin/users' + ObjectMapper mapper + + def setup() { + mapper = new ObjectMapper() + mapper.enable(SerializationFeature.INDENT_OUTPUT) + } + def 'GET ALL users (when there are existing users)'() { 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) @@ -64,4 +77,84 @@ class UsersControllerIntegrationTests extends Specification { then: 'The deleted user is gone' result.statusCodeValue == 404 } + + def 'POST new user persists properly'() { + given: + def newUser = new User().with { + it.firstName = 'Foo' + it.lastName = 'Bar' + it.username = 'FooBar' + it.password = 'somepass' + it.roles = [new Role().with {it.name = 'ROLE_USER'}] as Set + it + } + + when: + def result = this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + + then: + result.statusCodeValue == 200 + //TODO: Compare body? Or do that in a service-level unit test? + } + + def 'POST new duplicate username returns 409'() { + given: + def newUser = new User().with { + it.firstName = 'Foo' + it.lastName = 'Bar' + it.username = 'DuplicateUser' + it.password = 'somepass' + it.roles = [new Role().with {it.name = 'ROLE_USER'}] as Set + it + } + + when: + this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + def result = this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + + then: + result.statusCodeValue == 409 + } + + def 'PUT updates user properly'() { + given: + def newUser = new User().with { + it.firstName = 'Foo' + it.lastName = 'Bar' + it.username = 'FooBar' + it.password = 'somepass' + it.roles = [new Role().with {it.name = 'ROLE_USER'}] as Set + it + } + + when: + this.restTemplate.postForEntity("$RESOURCE_URI", createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + newUser.setFirstName('Bob') + def result = this.restTemplate.exchange("$RESOURCE_URI/$newUser.username", org.springframework.http.HttpMethod.PUT, createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + + then: + result.statusCodeValue == 200 + } + + def 'PUT detects unknown username'() { + given: + def newUser = new User().with { + it.firstName = 'Foo' + it.lastName = 'Bar' + it.username = 'FooBar' + it.password = 'somepass' + it.roles = [new Role().with {it.name = 'ROLE_USER'}] as Set + it + } + + when: + def result = this.restTemplate.exchange("$RESOURCE_URI/$newUser.username", org.springframework.http.HttpMethod.PUT, createRequestHttpEntityFor { mapper.writeValueAsString(newUser) }, Map) + + then: + result.statusCodeValue == 404 + } + + private HttpEntity createRequestHttpEntityFor(Closure jsonBodySupplier) { + new HttpEntity(jsonBodySupplier(), ['Content-Type': 'application/json'] as HttpHeaders) + } } \ No newline at end of file