Skip to content

Commit 74cb1ee

Browse files
committed
fix(review): address PR #262 review feedback
Security: - WebSecurityConfig now checks environment.matchesProfiles("local") before adding /dev/** to unprotected and CSRF-ignored URIs, matching the controller's own @Profile("local") guard Code quality: - DevLoginController: replace sendRedirect()+null return with ResponseEntity.status(FOUND).header("Location", ...) — cleaner, no IOException, no HttpServletResponse dependency - DevLoginController: use UserRepository.findAllByEnabledTrue() instead of findAll().filter() to filter at the database level - DevLoginController: use .toList() instead of Collectors.toList() - WebSecurityConfig: rename disableCSRFURIsArray → baseDisableCSRFURIs - DevLoginStartupWarning: fix {{email}} typo → {email} in log message Tests: - UserServiceTest: remove dead authCaptor variable - DevLoginControllerTest: update for new loginAs() signature - DevLoginControllerTest: update listUsers() to use findAllByEnabledTrue() - DevLoginIntegrationTest: strengthen shouldListEnabledUsers() to assert email content, not just array type Deferred (follow-up issue): DevLoginConfigProperties registered via @component; architectural consistency with WebAuthnConfigProperties needs a broader discussion before changing.
1 parent ad8ace7 commit 74cb1ee

7 files changed

Lines changed: 41 additions & 43 deletions

File tree

src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginController.java

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package com.digitalsanctuary.spring.user.dev;
22

3-
import java.io.IOException;
43
import java.util.List;
5-
import java.util.stream.Collectors;
64
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
75
import org.springframework.context.annotation.Profile;
86
import org.springframework.http.HttpStatus;
@@ -15,7 +13,6 @@
1513
import com.digitalsanctuary.spring.user.persistence.repository.UserRepository;
1614
import com.digitalsanctuary.spring.user.service.UserService;
1715
import com.digitalsanctuary.spring.user.util.JSONResponse;
18-
import jakarta.servlet.http.HttpServletResponse;
1916
import lombok.RequiredArgsConstructor;
2017
import lombok.extern.slf4j.Slf4j;
2118

@@ -45,16 +42,13 @@ public class DevLoginController {
4542

4643
/**
4744
* Logs in as the specified user by email without requiring a password.
48-
* After successful authentication, redirects to the configured redirect URL.
45+
* After successful authentication, returns a redirect response to the configured URL.
4946
*
50-
* @param email the email of the user to log in as
51-
* @param response the HTTP servlet response for redirect
52-
* @return a ResponseEntity with error details if authentication fails
53-
* @throws IOException if the redirect fails
47+
* @param email the email of the user to log in as
48+
* @return a redirect response on success, or an error response with 404/403 status
5449
*/
5550
@GetMapping("/login-as/{email}")
56-
public ResponseEntity<JSONResponse> loginAs(@PathVariable String email, HttpServletResponse response)
57-
throws IOException {
51+
public ResponseEntity<JSONResponse> loginAs(@PathVariable String email) {
5852
log.warn("Dev login attempt for user: {}", email);
5953

6054
User user = userService.findUserByEmail(email);
@@ -74,8 +68,9 @@ public ResponseEntity<JSONResponse> loginAs(@PathVariable String email, HttpServ
7468
userService.authWithoutPassword(user);
7569
log.warn("Dev login successful for user: {}", email);
7670

77-
response.sendRedirect(devLoginConfigProperties.getLoginRedirectUrl());
78-
return null;
71+
return ResponseEntity.status(HttpStatus.FOUND)
72+
.header("Location", devLoginConfigProperties.getLoginRedirectUrl())
73+
.build();
7974
}
8075

8176
/**
@@ -85,10 +80,11 @@ public ResponseEntity<JSONResponse> loginAs(@PathVariable String email, HttpServ
8580
*/
8681
@GetMapping("/users")
8782
public ResponseEntity<JSONResponse> listUsers() {
88-
List<String> enabledEmails = userRepository.findAll().stream().filter(User::isEnabled).map(User::getEmail)
89-
.collect(Collectors.toList());
83+
List<String> enabledEmails = userRepository.findAllByEnabledTrue().stream()
84+
.map(User::getEmail)
85+
.toList();
9086

91-
return ResponseEntity.ok(JSONResponse.builder().success(true).message("Found " + enabledEmails.size()
92-
+ " enabled users").data(enabledEmails).build());
87+
return ResponseEntity.ok(JSONResponse.builder().success(true)
88+
.message("Found " + enabledEmails.size() + " enabled users").data(enabledEmails).build());
9389
}
9490
}

src/main/java/com/digitalsanctuary/spring/user/dev/DevLoginStartupWarning.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public class DevLoginStartupWarning {
2020
public void logWarning() {
2121
log.warn("========================================================");
2222
log.warn(" DEV LOGIN IS ACTIVE");
23-
log.warn(" Passwordless authentication is enabled at /dev/login-as/{{email}}");
23+
log.warn(" Passwordless authentication is enabled at /dev/login-as/{email}");
2424
log.warn(" DO NOT enable this in production!");
2525
log.warn("========================================================");
2626
}

src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.digitalsanctuary.spring.user.persistence.repository;
22

3+
import java.util.List;
34
import org.springframework.data.jpa.repository.JpaRepository;
45
import com.digitalsanctuary.spring.user.persistence.model.User;
56

@@ -16,6 +17,13 @@ public interface UserRepository extends JpaRepository<User, Long> {
1617
*/
1718
User findByEmail(String email);
1819

20+
/**
21+
* Find all enabled users.
22+
*
23+
* @return list of enabled users
24+
*/
25+
List<User> findAllByEnabledTrue();
26+
1927
/**
2028
* Delete.
2129
*

src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.springframework.context.ApplicationEventPublisher;
1111
import org.springframework.context.annotation.Bean;
1212
import org.springframework.context.annotation.Configuration;
13+
import org.springframework.core.env.Environment;
1314
import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
1415
import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler;
1516
import org.springframework.security.access.hierarchicalroles.RoleHierarchy;
@@ -123,6 +124,7 @@ public class WebSecurityConfig {
123124
private final DSOAuth2UserService dsOAuth2UserService;
124125
private final DSOidcUserService dsOidcUserService;
125126
private final WebAuthnConfigProperties webAuthnConfigProperties;
127+
private final Environment environment;
126128

127129
/**
128130
*
@@ -151,9 +153,9 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti
151153
.deleteCookies("JSESSIONID"));
152154

153155
// If we have URIs to disable CSRF validation on, do so here
154-
String[] disableCSRFURIsArray = getDisableCSRFURIsArray();
155-
List<String> csrfIgnoreList = new ArrayList<>(Arrays.asList(disableCSRFURIsArray));
156-
if (devAutoLoginEnabled) {
156+
String[] baseDisableCSRFURIs = getDisableCSRFURIsArray();
157+
List<String> csrfIgnoreList = new ArrayList<>(Arrays.asList(baseDisableCSRFURIs));
158+
if (devAutoLoginEnabled && environment.matchesProfiles("local")) {
157159
csrfIgnoreList.add("/dev/**");
158160
}
159161
if (!csrfIgnoreList.isEmpty()) {
@@ -264,7 +266,7 @@ private List<String> getUnprotectedURIsList() {
264266
unprotectedURIs.add(registrationSuccessURI);
265267
unprotectedURIs.add(forgotPasswordPendingURI);
266268
unprotectedURIs.add(forgotPasswordChangeURI);
267-
if (devAutoLoginEnabled) {
269+
if (devAutoLoginEnabled && environment.matchesProfiles("local")) {
268270
unprotectedURIs.add("/dev/**");
269271
}
270272
unprotectedURIs.removeAll(Collections.emptyList());

src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginControllerTest.java

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.mockito.ArgumentMatchers.any;
5-
import static org.mockito.Mockito.mock;
65
import static org.mockito.Mockito.never;
76
import static org.mockito.Mockito.verify;
87
import static org.mockito.Mockito.when;
@@ -21,7 +20,6 @@
2120
import com.digitalsanctuary.spring.user.test.annotations.ServiceTest;
2221
import com.digitalsanctuary.spring.user.test.builders.UserTestDataBuilder;
2322
import com.digitalsanctuary.spring.user.util.JSONResponse;
24-
import jakarta.servlet.http.HttpServletResponse;
2523

2624
@ServiceTest
2725
class DevLoginControllerTest {
@@ -49,33 +47,31 @@ void setUp() {
4947

5048
@Test
5149
@DisplayName("loginAs - should authenticate and redirect for valid enabled user")
52-
void shouldAuthenticateAndRedirectWhenValidUser() throws Exception {
50+
void shouldAuthenticateAndRedirectWhenValidUser() {
5351
// Given
5452
when(userService.findUserByEmail("dev@test.com")).thenReturn(enabledUser);
5553
when(devLoginConfigProperties.getLoginRedirectUrl()).thenReturn("/dashboard");
56-
HttpServletResponse response = mock(HttpServletResponse.class);
5754

5855
// When
59-
ResponseEntity<JSONResponse> result = devLoginController.loginAs("dev@test.com", response);
56+
ResponseEntity<JSONResponse> result = devLoginController.loginAs("dev@test.com");
6057

6158
// Then
6259
verify(userService).authWithoutPassword(enabledUser);
63-
verify(response).sendRedirect("/dashboard");
64-
assertThat(result).isNull();
60+
assertThat(result.getStatusCode()).isEqualTo(HttpStatus.FOUND);
61+
assertThat(result.getHeaders().getFirst("Location")).isEqualTo("/dashboard");
62+
assertThat(result.getBody()).isNull();
6563
}
6664

6765
@Test
6866
@DisplayName("loginAs - should return 404 for unknown user")
69-
void shouldReturn404WhenUserNotFound() throws Exception {
67+
void shouldReturn404WhenUserNotFound() {
7068
// Given
7169
when(userService.findUserByEmail("unknown@test.com")).thenReturn(null);
72-
HttpServletResponse response = mock(HttpServletResponse.class);
7370

7471
// When
75-
ResponseEntity<JSONResponse> result = devLoginController.loginAs("unknown@test.com", response);
72+
ResponseEntity<JSONResponse> result = devLoginController.loginAs("unknown@test.com");
7673

7774
// Then
78-
assertThat(result).isNotNull();
7975
assertThat(result.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
8076
assertThat(result.getBody()).isNotNull();
8177
assertThat(result.getBody().isSuccess()).isFalse();
@@ -84,28 +80,25 @@ void shouldReturn404WhenUserNotFound() throws Exception {
8480

8581
@Test
8682
@DisplayName("loginAs - should return 403 for disabled user")
87-
void shouldReturn403WhenUserDisabled() throws Exception {
83+
void shouldReturn403WhenUserDisabled() {
8884
// Given
8985
when(userService.findUserByEmail("disabled@test.com")).thenReturn(disabledUser);
90-
HttpServletResponse response = mock(HttpServletResponse.class);
9186

9287
// When
93-
ResponseEntity<JSONResponse> result = devLoginController.loginAs("disabled@test.com", response);
88+
ResponseEntity<JSONResponse> result = devLoginController.loginAs("disabled@test.com");
9489

9590
// Then
96-
assertThat(result).isNotNull();
9791
assertThat(result.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN);
9892
assertThat(result.getBody()).isNotNull();
9993
assertThat(result.getBody().isSuccess()).isFalse();
10094
verify(userService, never()).authWithoutPassword(any());
10195
}
10296

10397
@Test
104-
@DisplayName("listUsers - should return enabled user emails")
98+
@DisplayName("listUsers - should return only enabled user emails")
10599
void shouldReturnEnabledUserEmails() {
106100
// Given
107-
List<User> allUsers = Arrays.asList(enabledUser, disabledUser);
108-
when(userRepository.findAll()).thenReturn(allUsers);
101+
when(userRepository.findAllByEnabledTrue()).thenReturn(Arrays.asList(enabledUser));
109102

110103
// When
111104
ResponseEntity<JSONResponse> result = devLoginController.listUsers();

src/test/java/com/digitalsanctuary/spring/user/dev/DevLoginIntegrationTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
5+
import static org.hamcrest.Matchers.hasItem;
56
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
67
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrl;
78
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
@@ -105,6 +106,7 @@ void shouldListEnabledUsers() throws Exception {
105106
mockMvc.perform(get("/dev/users"))
106107
.andExpect(status().isOk())
107108
.andExpect(jsonPath("$.success").value(true))
108-
.andExpect(jsonPath("$.data").isArray());
109+
.andExpect(jsonPath("$.data").isArray())
110+
.andExpect(jsonPath("$.data", hasItem("dev-test@test.com")));
109111
}
110112
}

src/test/java/com/digitalsanctuary/spring/user/service/UserServiceTest.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -479,9 +479,6 @@ void authWithoutPassword_authenticatesValidUser() {
479479
SecurityContext securityContext = mock(SecurityContext.class);
480480
mockedSecurityHolder.when(SecurityContextHolder::getContext).thenReturn(securityContext);
481481

482-
// Capture the authentication set on the context and return it on getAuthentication()
483-
ArgumentCaptor<Authentication> authCaptor = ArgumentCaptor.forClass(Authentication.class);
484-
485482
// Use thenAnswer to capture and store the authentication set
486483
final Authentication[] storedAuth = new Authentication[1];
487484
org.mockito.Mockito.doAnswer(invocation -> {

0 commit comments

Comments
 (0)