From 990757eda35116723e769f84cf8c4de997d3224c Mon Sep 17 00:00:00 2001
From: Chris Lemaire <c.lemaire@student.tudelft.nl>
Date: Thu, 14 May 2020 10:44:06 +0200
Subject: [PATCH] Add test for TOTPController
Additionally rename a few methods to fit in the new naming scheme and
remove the need to check the availability of key information.
---
.../labracore/config/WebMvcConfiguration.java | 5 +
.../labracore/controller/TOTPController.java | 19 +-
.../nl/tudelft/labracore/model/TOTPKey.java | 2 +-
.../security/AuthorizationService.java | 2 +-
.../AuthenticatedPersonParameterResolver.java | 2 +
.../labracore/service/PermissionService.java | 2 +-
.../labracore/service/TOTPService.java | 4 +-
.../controller/TOTPControllerTest.java | 214 ++++++++++++++++++
.../test/MockUserDetailsService.java | 35 ++-
9 files changed, 262 insertions(+), 23 deletions(-)
create mode 100644 src/test/java/nl/tudelft/labracore/controller/TOTPControllerTest.java
diff --git a/src/main/java/nl/tudelft/labracore/config/WebMvcConfiguration.java b/src/main/java/nl/tudelft/labracore/config/WebMvcConfiguration.java
index 4d78ea94..c1a28293 100644
--- a/src/main/java/nl/tudelft/labracore/config/WebMvcConfiguration.java
+++ b/src/main/java/nl/tudelft/labracore/config/WebMvcConfiguration.java
@@ -19,6 +19,7 @@ package nl.tudelft.labracore.config;
import java.util.List;
+import nl.tudelft.labracore.security.user.AuthenticatedPersonParameterResolver;
import nl.tudelft.labracore.versioning.VersionArgumentResolver;
import org.springframework.beans.factory.annotation.Autowired;
@@ -31,8 +32,12 @@ public class WebMvcConfiguration implements WebMvcConfigurer {
@Autowired
private VersionArgumentResolver versionArgumentResolver;
+ @Autowired
+ private AuthenticatedPersonParameterResolver authenticatedPersonParameterResolver;
+
@Override
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
resolvers.add(versionArgumentResolver);
+ resolvers.add(authenticatedPersonParameterResolver);
}
}
diff --git a/src/main/java/nl/tudelft/labracore/controller/TOTPController.java b/src/main/java/nl/tudelft/labracore/controller/TOTPController.java
index f238161b..7170b5d6 100644
--- a/src/main/java/nl/tudelft/labracore/controller/TOTPController.java
+++ b/src/main/java/nl/tudelft/labracore/controller/TOTPController.java
@@ -17,8 +17,6 @@
*/
package nl.tudelft.labracore.controller;
-import javax.persistence.EntityNotFoundException;
-
import nl.tudelft.labracore.dto.view.TOTPKeyInfoViewDTO;
import nl.tudelft.labracore.model.Person;
import nl.tudelft.labracore.security.user.AuthenticatedPerson;
@@ -41,7 +39,7 @@ public class TOTPController {
*
* @param person The authenticated Person that wants to set their TOTP key.
*/
- @PreAuthorize("#authorizationService.canCreateTOTPKey()")
+ @PreAuthorize("@authorizationService.canCreateTOTPKey()")
@PutMapping("create")
public TOTPKeyInfoViewDTO createTOTPKey(@AuthenticatedPerson Person person) {
totpService.disableTOTP(person);
@@ -57,9 +55,9 @@ public class TOTPController {
* @param person The authenticated Person that wants to see their TOTP key.
* @return The TOTP key as an array of bytes.
*/
- @PreAuthorize("#authorizationService.canReadTOTPKey()")
+ @PreAuthorize("@authorizationService.canReadTOTPKeySecret()")
@GetMapping("secret")
- public String getKey(@AuthenticatedPerson Person person) {
+ public String getTOTPSecret(@AuthenticatedPerson Person person) {
return person.getTwoFA().activeTOTPKey().get().getSecret();
}
@@ -70,12 +68,9 @@ public class TOTPController {
* @return The TOTP key as an array of bytes.
*/
@GetMapping("key-info")
- @PreAuthorize("#authorizationService.canReadTOTPKey()")
+ @PreAuthorize("@authorizationService.canReadTOTPKeyInfo()")
public TOTPKeyInfoViewDTO getTOTPKeyInfo(@AuthenticatedPerson Person person) {
- return View.convert(person.getTwoFA().activeTOTPKey().orElseThrow(
- () -> new EntityNotFoundException(
- "Could not find active TOTP Key for user: " + person.getUsername())),
- TOTPKeyInfoViewDTO.class);
+ return View.convert(person.getTwoFA().activeTOTPKey().get(), TOTPKeyInfoViewDTO.class);
}
/**
@@ -86,9 +81,9 @@ public class TOTPController {
* @param password The current TOTP password the user entered.
* @return {@code true} when the password was correct, {@code false} otherwise.
*/
- @PreAuthorize("#authorizationService.canUseTOTPKey()")
+ @PreAuthorize("@authorizationService.canUseTOTPKey()")
@PostMapping("confirm")
- public boolean confirmPassword(@AuthenticatedPerson Person person, int password) {
+ public boolean confirmTOTPPassword(@AuthenticatedPerson Person person, @RequestParam int password) {
return totpService.confirmTOTPPassword(person, password);
}
diff --git a/src/main/java/nl/tudelft/labracore/model/TOTPKey.java b/src/main/java/nl/tudelft/labracore/model/TOTPKey.java
index f4d3660e..7d76c21e 100644
--- a/src/main/java/nl/tudelft/labracore/model/TOTPKey.java
+++ b/src/main/java/nl/tudelft/labracore/model/TOTPKey.java
@@ -81,7 +81,7 @@ public class TOTPKey {
}
/**
- * @return The secret of this key decoded into a (160-bit) byte-array.
+ * @return The secret of this key decoded into a (192-bit) byte-array.
*/
public byte[] getSecretAsBytes() {
return new Base32().decode(secret);
diff --git a/src/main/java/nl/tudelft/labracore/security/AuthorizationService.java b/src/main/java/nl/tudelft/labracore/security/AuthorizationService.java
index bbb70adc..8d3a0ec6 100644
--- a/src/main/java/nl/tudelft/labracore/security/AuthorizationService.java
+++ b/src/main/java/nl/tudelft/labracore/security/AuthorizationService.java
@@ -128,7 +128,7 @@ public class AuthorizationService {
/**
* @return Whether the service can see the key secret of the authenticated user.
*/
- public boolean canReadTOTPKey() {
+ public boolean canReadTOTPKeySecret() {
return withAuthenticatedUser(person -> {
boolean isCurrent = person.getTwoFA().activeTOTPKey()
.map(key -> LocalDateTime.now().isBefore(key.getCreatedAt().plusMinutes(5L)))
diff --git a/src/main/java/nl/tudelft/labracore/security/user/AuthenticatedPersonParameterResolver.java b/src/main/java/nl/tudelft/labracore/security/user/AuthenticatedPersonParameterResolver.java
index 04f1d723..ea0f8b5d 100644
--- a/src/main/java/nl/tudelft/labracore/security/user/AuthenticatedPersonParameterResolver.java
+++ b/src/main/java/nl/tudelft/labracore/security/user/AuthenticatedPersonParameterResolver.java
@@ -23,11 +23,13 @@ import nl.tudelft.labracore.security.api.APIUserDetailsWithPerson;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.core.MethodParameter;
import org.springframework.security.core.context.SecurityContextHolder;
+import org.springframework.stereotype.Component;
import org.springframework.web.bind.support.WebDataBinderFactory;
import org.springframework.web.context.request.NativeWebRequest;
import org.springframework.web.method.support.HandlerMethodArgumentResolver;
import org.springframework.web.method.support.ModelAndViewContainer;
+@Component
public class AuthenticatedPersonParameterResolver implements HandlerMethodArgumentResolver {
@Autowired
private PersonRepository pr;
diff --git a/src/main/java/nl/tudelft/labracore/service/PermissionService.java b/src/main/java/nl/tudelft/labracore/service/PermissionService.java
index f80bccb2..bdcec942 100644
--- a/src/main/java/nl/tudelft/labracore/service/PermissionService.java
+++ b/src/main/java/nl/tudelft/labracore/service/PermissionService.java
@@ -39,7 +39,7 @@ public class PermissionService {
private final List<String> permissionsDomains = List.of(
"ASSIGNMENT", "BUILDING", "CLUSTER", "COURSE",
"EDITION", "MODULE", "PERSON", "ROLE", "ROOM",
- "SESSION", "STUDENT_GROUP", "MODULE_DIVISION");
+ "SESSION", "STUDENT_GROUP", "MODULE_DIVISION", "TOTP");
/**
* A list of permission types (the ending of an authority name). Can also be interpreted as the action a
diff --git a/src/main/java/nl/tudelft/labracore/service/TOTPService.java b/src/main/java/nl/tudelft/labracore/service/TOTPService.java
index 03bc7fdb..4ae76e6c 100644
--- a/src/main/java/nl/tudelft/labracore/service/TOTPService.java
+++ b/src/main/java/nl/tudelft/labracore/service/TOTPService.java
@@ -50,7 +50,7 @@ public class TOTPService {
private TOTPKeyRepository tkr;
/**
- * Sets up a new TOTP key for the given authenticated {@link Person}. For this, a 160-bit secret is
+ * Sets up a new TOTP key for the given authenticated {@link Person}. For this, a 192-bit secret is
* generated and stored in a new TOTP key object.
*
* @param person The authenticated Person for which a key needs to be set up.
@@ -60,7 +60,7 @@ public class TOTPService {
public TOTPKey setupTOTP(Person person) {
try {
KeyGenerator keyGen = KeyGenerator.getInstance("AES");
- keyGen.init(160);
+ keyGen.init(192);
Key key = keyGen.generateKey();
return tkr.save(new TOTPKey(person, key.getEncoded()));
diff --git a/src/test/java/nl/tudelft/labracore/controller/TOTPControllerTest.java b/src/test/java/nl/tudelft/labracore/controller/TOTPControllerTest.java
new file mode 100644
index 00000000..7c6dc18d
--- /dev/null
+++ b/src/test/java/nl/tudelft/labracore/controller/TOTPControllerTest.java
@@ -0,0 +1,214 @@
+/*
+ * Labracore - A connecting core service for Labrador products
+ * Copyright (C) 2020- Delft University of Technology
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+package nl.tudelft.labracore.controller;
+
+import static java.time.LocalDateTime.now;
+import static java.time.temporal.ChronoUnit.SECONDS;
+import static nl.tudelft.labracore.test.JsonContentMatcher.jsonContent;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.within;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+
+import java.util.List;
+
+import javax.transaction.Transactional;
+
+import nl.tudelft.labracore.dto.view.PersonViewDTO;
+import nl.tudelft.labracore.dto.view.TOTPKeyInfoViewDTO;
+import nl.tudelft.labracore.model.Person;
+import nl.tudelft.labracore.model.TOTPKey;
+import nl.tudelft.labracore.repository.PersonRepository;
+import nl.tudelft.labracore.service.TOTPService;
+import nl.tudelft.labracore.test.RestControllerTest;
+import nl.tudelft.labracore.test.TestDatabaseLoader;
+import nl.tudelft.librador.dto.view.View;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+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.mock.mockito.SpyBean;
+import org.springframework.security.test.context.support.WithUserDetails;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+
+@Transactional
+@SpringBootTest
+@AutoConfigureMockMvc
+public class TOTPControllerTest extends RestControllerTest {
+
+ @Autowired
+ private MockMvc mvc;
+
+ @Autowired
+ private TestDatabaseLoader db;
+
+ @Autowired
+ private PersonRepository pr;
+
+ @SpyBean
+ private TOTPService ts;
+
+ private Person p;
+ private TOTPKey key;
+
+ @BeforeEach
+ void setup() {
+ p = pr.getOne(db.getVision().getId());
+ key = p.getTwoFA().activeTOTPKey().get();
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void createTOTPKeyShouldDisableCurrentTOTP() throws Exception {
+ mvc.perform(put("/api/2fa/totp/create").with(csrf()));
+
+ verify(ts).disableTOTP(eq(p));
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void createTOTPKeyShouldSetupTOTP() throws Exception {
+ mvc.perform(put("/api/2fa/totp/create").with(csrf()));
+
+ verify(ts).setupTOTP(eq(p));
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void createTOTPKeyShouldCreateNewKeyAndReturnIt() throws Exception {
+ mvc.perform(put("/api/2fa/totp/create").with(csrf()))
+ .andExpect(status().isOk())
+ .andExpect(jsonContent(TOTPKeyInfoViewDTO.class)
+ .test(info -> {
+ assertThat(info.getRetractedAt()).isNull();
+ assertThat(info.getPerson()).isEqualTo(View.convert(p, PersonViewDTO.class));
+ assertThat(info.getCreatedAt()).isCloseTo(now(), within(5, SECONDS));
+ }));
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void getSecretShouldBeAllowedForNewKey() throws Exception {
+ key.setCreatedAt(now());
+ mvc.perform(get("/api/2fa/totp/secret").with(csrf()))
+ .andExpect(status().isOk());
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void getSecretShouldNotBeAllowedForOldKey() throws Exception {
+ key.setCreatedAt(now().minusMinutes(5));
+ mvc.perform(get("/api/2fa/totp/secret").with(csrf()))
+ .andExpect(status().isForbidden());
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void getSecretShouldActuallyReturnSecret() throws Exception {
+ key.setCreatedAt(now());
+ mvc.perform(get("/api/2fa/totp/secret").with(csrf()))
+ .andExpect(status().isOk())
+ .andExpect(content().string(key.getSecret()));
+ }
+
+ @WithUserDetails("All-access Key:antman")
+ @Test
+ void getSecretNonExistentShouldResultInForbidden() throws Exception {
+ mvc.perform(get("/api/2fa/totp/secret").with(csrf()))
+ .andExpect(status().isForbidden());
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void getKeyInfoShouldResultInInfo() throws Exception {
+ mvc.perform(get("/api/2fa/totp/key-info").with(csrf()))
+ .andExpect(status().isOk())
+ .andExpect(jsonContent(TOTPKeyInfoViewDTO.class)
+ .test(info -> assertThat(info)
+ .isEqualTo(View.convert(key, TOTPKeyInfoViewDTO.class))));
+ }
+
+ @WithUserDetails("All-access Key:antman")
+ @Test
+ void getKeyInfoNonExistentShouldResultIn403() throws Exception {
+ mvc.perform(get("/api/2fa/totp/key-info").with(csrf()))
+ .andExpect(status().isForbidden());
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void confirmPasswordDelegatesToService() throws Exception {
+ mvc.perform(post("/api/2fa/totp/confirm")
+ .param("password", "123456")
+ .with(csrf()));
+
+ verify(ts).confirmTOTPPassword(eq(p), eq(123456));
+ }
+
+ @WithUserDetails("All-access Key:antman")
+ @Test
+ void confirmPasswordChecksWhetherUserHasKey() throws Exception {
+ mvc.perform(post("/api/2fa/totp/confirm")
+ .param("password", "123456")
+ .with(csrf())).andExpect(status().isForbidden());
+ }
+
+ @WithUserDetails("All-access Key:vision")
+ @Test
+ void checkTOTPEnabledForReturnsTrue() throws Exception {
+ mvc.perform(get("/api/2fa/totp/is-enabled"))
+ .andExpect(status().isOk())
+ .andExpect(content().string("true"));
+ }
+
+ @WithUserDetails("All-access Key:antman")
+ @Test
+ void checkTOTPEnabledForReturnsFalse() throws Exception {
+ mvc.perform(get("/api/2fa/totp/is-enabled"))
+ .andExpect(status().isOk())
+ .andExpect(content().string("false"));
+ }
+
+ @WithUserDetails("No-access Key:vision")
+ @MethodSource("protectedEndpoints")
+ @ParameterizedTest
+ void endpointsShouldBeAuthorityProtected(MockHttpServletRequestBuilder request) throws Exception {
+ mvc.perform(request.with(csrf()))
+ .andExpect(status().isForbidden());
+ }
+
+ private static List<MockHttpServletRequestBuilder> protectedEndpoints() throws JsonProcessingException {
+ return List.of(
+ put("/api/2fa/totp/create"),
+ get("/api/2fa/totp/secret"),
+ get("/api/2fa/totp/key-info"),
+ // post("/api/2fa/totp/confirm").param("password", "123456"),
+ get("/api/2fa/totp/is-enabled"));
+ }
+}
diff --git a/src/test/java/nl/tudelft/labracore/test/MockUserDetailsService.java b/src/test/java/nl/tudelft/labracore/test/MockUserDetailsService.java
index 3e59dc6d..fe96ac00 100644
--- a/src/test/java/nl/tudelft/labracore/test/MockUserDetailsService.java
+++ b/src/test/java/nl/tudelft/labracore/test/MockUserDetailsService.java
@@ -20,8 +20,11 @@ package nl.tudelft.labracore.test;
import java.util.stream.Collectors;
import nl.tudelft.labracore.model.APIKey;
+import nl.tudelft.labracore.model.Person;
import nl.tudelft.labracore.repository.APIKeyRepository;
+import nl.tudelft.labracore.repository.PersonRepository;
import nl.tudelft.labracore.security.api.APIUserDetails;
+import nl.tudelft.labracore.security.api.APIUserDetailsWithPerson;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
@@ -33,17 +36,37 @@ public class MockUserDetailsService implements UserDetailsService {
@Autowired
private APIKeyRepository apiKeyRepository;
+ @Autowired
+ private PersonRepository personRepository;
+
@Override
public UserDetails loadUserByUsername(String username)
throws UsernameNotFoundException {
- APIKey key = apiKeyRepository.findByName(username);
+ String keyName = username;
+ Person authUser = null;
+ if (username.contains(":")) {
+ keyName = username.split("[:]")[0];
+
+ String u = username.split("[:]")[1];
+ authUser = personRepository.findByUsernameOrThrow(u);
+ }
+
+ APIKey key = apiKeyRepository.findByName(keyName);
if (key == null) {
- throw new UsernameNotFoundException("Could not key with name: " + username);
+ throw new UsernameNotFoundException("Could not find key with name: " + keyName);
}
- return new APIUserDetails(key,
- key.getPermissions().stream()
- .map(p -> new SimpleGrantedAuthority(p.getName()))
- .collect(Collectors.toList()));
+ if (authUser == null) {
+ return new APIUserDetails(key,
+ key.getPermissions().stream()
+ .map(p -> new SimpleGrantedAuthority(p.getName()))
+ .collect(Collectors.toList()));
+ } else {
+ return new APIUserDetailsWithPerson(key,
+ key.getPermissions().stream()
+ .map(p -> new SimpleGrantedAuthority(p.getName()))
+ .collect(Collectors.toList()),
+ authUser);
+ }
}
}
--
GitLab