refactor(auth): replace @Autowired(required=false) with SessionRevocationPort + constructor injection
Extract SessionRevocationPort interface with JdbcSessionRevocationAdapter (@ConditionalOnBean) and NoOpSessionRevocationAdapter (@ConditionalOnMissingBean). AuthService now uses @RequiredArgsConstructor with final fields for both LoginRateLimiter and SessionRevocationPort, removing all null guards. AuthServiceTest drops ReflectionTestUtils.setField and uses @Mock on the port. Fixes Felix's blocker: @Autowired(required=false) field injection in AuthService. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -7,12 +7,10 @@ import org.raddatz.familienarchiv.audit.AuditService;
|
||||
import org.raddatz.familienarchiv.exception.DomainException;
|
||||
import org.raddatz.familienarchiv.user.AppUser;
|
||||
import org.raddatz.familienarchiv.user.UserService;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.security.authentication.AuthenticationManager;
|
||||
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
|
||||
import org.springframework.security.core.Authentication;
|
||||
import org.springframework.security.core.AuthenticationException;
|
||||
import org.springframework.session.jdbc.JdbcIndexedSessionRepository;
|
||||
import org.springframework.stereotype.Service;
|
||||
|
||||
import java.util.Map;
|
||||
@@ -26,28 +24,17 @@ public class AuthService {
|
||||
private final AuthenticationManager authenticationManager;
|
||||
private final UserService userService;
|
||||
private final AuditService auditService;
|
||||
private final LoginRateLimiter loginRateLimiter;
|
||||
private final SessionRevocationPort sessionRevocationPort;
|
||||
|
||||
@Autowired(required = false)
|
||||
private JdbcIndexedSessionRepository sessionRepository;
|
||||
|
||||
@Autowired(required = false)
|
||||
private LoginRateLimiter loginRateLimiter;
|
||||
|
||||
/**
|
||||
* Validates credentials and returns the authenticated user plus the Spring Security
|
||||
* Authentication object. The caller is responsible for persisting the Authentication
|
||||
* to the session via SecurityContextRepository.
|
||||
*/
|
||||
public LoginResult login(String email, String password, String ip, String ua) {
|
||||
if (loginRateLimiter != null) {
|
||||
try {
|
||||
loginRateLimiter.checkAndConsume(ip, email);
|
||||
} catch (DomainException ex) {
|
||||
auditService.log(AuditKind.LOGIN_RATE_LIMITED, null, null, Map.of(
|
||||
"ip", ip,
|
||||
"email", email));
|
||||
throw ex;
|
||||
}
|
||||
try {
|
||||
loginRateLimiter.checkAndConsume(ip, email);
|
||||
} catch (DomainException ex) {
|
||||
auditService.log(AuditKind.LOGIN_RATE_LIMITED, null, null, Map.of(
|
||||
"ip", ip,
|
||||
"email", email));
|
||||
throw ex;
|
||||
}
|
||||
try {
|
||||
Authentication auth = authenticationManager.authenticate(
|
||||
@@ -58,9 +45,7 @@ public class AuthService {
|
||||
"userId", user.getId().toString(),
|
||||
"ip", ip,
|
||||
"ua", truncateUa(ua)));
|
||||
if (loginRateLimiter != null) {
|
||||
loginRateLimiter.invalidateOnSuccess(ip, email);
|
||||
}
|
||||
loginRateLimiter.invalidateOnSuccess(ip, email);
|
||||
return new LoginResult(user, auth);
|
||||
} catch (AuthenticationException ex) {
|
||||
// Audit login failure — intentionally does NOT log the attempted password.
|
||||
@@ -75,22 +60,11 @@ public class AuthService {
|
||||
}
|
||||
|
||||
public int revokeOtherSessions(String currentSessionId, String principalName) {
|
||||
if (sessionRepository == null) return 0;
|
||||
int count = 0;
|
||||
for (String id : sessionRepository.findByPrincipalName(principalName).keySet()) {
|
||||
if (!id.equals(currentSessionId)) {
|
||||
sessionRepository.deleteById(id);
|
||||
count++;
|
||||
}
|
||||
}
|
||||
return count;
|
||||
return sessionRevocationPort.revokeOtherSessions(currentSessionId, principalName);
|
||||
}
|
||||
|
||||
public int revokeAllSessions(String principalName) {
|
||||
if (sessionRepository == null) return 0;
|
||||
var sessions = sessionRepository.findByPrincipalName(principalName);
|
||||
sessions.keySet().forEach(sessionRepository::deleteById);
|
||||
return sessions.size();
|
||||
return sessionRevocationPort.revokeAllSessions(principalName);
|
||||
}
|
||||
|
||||
public void logout(String email, String ip, String ua) {
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
package org.raddatz.familienarchiv.auth;
|
||||
|
||||
import lombok.RequiredArgsConstructor;
|
||||
import org.springframework.boot.autoconfigure.condition.ConditionalOnBean;
|
||||
import org.springframework.session.jdbc.JdbcIndexedSessionRepository;
|
||||
import org.springframework.stereotype.Service;
|
||||
|
||||
@Service
|
||||
@ConditionalOnBean(JdbcIndexedSessionRepository.class)
|
||||
@RequiredArgsConstructor
|
||||
class JdbcSessionRevocationAdapter implements SessionRevocationPort {
|
||||
|
||||
private final JdbcIndexedSessionRepository sessionRepository;
|
||||
|
||||
@Override
|
||||
public int revokeOtherSessions(String currentSessionId, String principalName) {
|
||||
int count = 0;
|
||||
for (String id : sessionRepository.findByPrincipalName(principalName).keySet()) {
|
||||
if (!id.equals(currentSessionId)) {
|
||||
sessionRepository.deleteById(id);
|
||||
count++;
|
||||
}
|
||||
}
|
||||
return count;
|
||||
}
|
||||
|
||||
@Override
|
||||
public int revokeAllSessions(String principalName) {
|
||||
var sessions = sessionRepository.findByPrincipalName(principalName);
|
||||
sessions.keySet().forEach(sessionRepository::deleteById);
|
||||
return sessions.size();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,19 @@
|
||||
package org.raddatz.familienarchiv.auth;
|
||||
|
||||
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
|
||||
import org.springframework.stereotype.Service;
|
||||
|
||||
@Service
|
||||
@ConditionalOnMissingBean(SessionRevocationPort.class)
|
||||
class NoOpSessionRevocationAdapter implements SessionRevocationPort {
|
||||
|
||||
@Override
|
||||
public int revokeOtherSessions(String currentSessionId, String principalName) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
@Override
|
||||
public int revokeAllSessions(String principalName) {
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,6 @@
|
||||
package org.raddatz.familienarchiv.auth;
|
||||
|
||||
public interface SessionRevocationPort {
|
||||
int revokeOtherSessions(String currentSessionId, String principalName);
|
||||
int revokeAllSessions(String principalName);
|
||||
}
|
||||
@@ -15,17 +15,10 @@ import org.springframework.security.authentication.AuthenticationManager;
|
||||
import org.springframework.security.authentication.BadCredentialsException;
|
||||
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
|
||||
import org.springframework.security.core.Authentication;
|
||||
import org.springframework.session.jdbc.JdbcIndexedSessionRepository;
|
||||
import org.raddatz.familienarchiv.exception.ErrorCode;
|
||||
|
||||
import java.util.HashMap;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.UUID;
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.springframework.test.util.ReflectionTestUtils;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.mockito.ArgumentMatchers.*;
|
||||
@@ -37,19 +30,13 @@ class AuthServiceTest {
|
||||
@Mock AuthenticationManager authenticationManager;
|
||||
@Mock UserService userService;
|
||||
@Mock AuditService auditService;
|
||||
@Mock JdbcIndexedSessionRepository sessionRepository;
|
||||
@Mock LoginRateLimiter loginRateLimiter;
|
||||
@Mock SessionRevocationPort sessionRevocationPort;
|
||||
@InjectMocks AuthService authService;
|
||||
|
||||
private static final String IP = "127.0.0.1";
|
||||
private static final String UA = "Mozilla/5.0 (Test)";
|
||||
|
||||
@BeforeEach
|
||||
void injectOptionalFields() {
|
||||
ReflectionTestUtils.setField(authService, "sessionRepository", sessionRepository);
|
||||
ReflectionTestUtils.setField(authService, "loginRateLimiter", loginRateLimiter);
|
||||
}
|
||||
|
||||
@Test
|
||||
void login_returns_user_on_valid_credentials() {
|
||||
UUID userId = UUID.randomUUID();
|
||||
@@ -159,7 +146,6 @@ class AuthServiceTest {
|
||||
|
||||
@Test
|
||||
void login_fires_LOGIN_RATE_LIMITED_audit_when_rate_limited() {
|
||||
UUID userId = UUID.randomUUID();
|
||||
doThrow(DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS, "rate limited"))
|
||||
.when(loginRateLimiter).checkAndConsume(IP, "user@test.de");
|
||||
|
||||
@@ -183,55 +169,23 @@ class AuthServiceTest {
|
||||
verify(loginRateLimiter).invalidateOnSuccess(IP, "user@test.de");
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void revokeOtherSessions_preserves_current_and_deletes_N_minus_1() {
|
||||
var sessions = new HashMap<String, Object>();
|
||||
sessions.put("session-keep", null);
|
||||
sessions.put("session-del-1", null);
|
||||
sessions.put("session-del-2", null);
|
||||
doReturn(sessions).when(sessionRepository).findByPrincipalName("user@test.de");
|
||||
void revokeOtherSessions_delegates_to_port() {
|
||||
when(sessionRevocationPort.revokeOtherSessions("session-keep", "user@test.de")).thenReturn(2);
|
||||
|
||||
int count = authService.revokeOtherSessions("session-keep", "user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(2);
|
||||
verify(sessionRepository, never()).deleteById("session-keep");
|
||||
verify(sessionRepository).deleteById("session-del-1");
|
||||
verify(sessionRepository).deleteById("session-del-2");
|
||||
verify(sessionRevocationPort).revokeOtherSessions("session-keep", "user@test.de");
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void revokeAllSessions_deletes_all_sessions_for_principal() {
|
||||
var sessions = new HashMap<String, Object>();
|
||||
sessions.put("session-1", null);
|
||||
sessions.put("session-2", null);
|
||||
doReturn(sessions).when(sessionRepository).findByPrincipalName("user@test.de");
|
||||
void revokeAllSessions_delegates_to_port() {
|
||||
when(sessionRevocationPort.revokeAllSessions("user@test.de")).thenReturn(3);
|
||||
|
||||
int count = authService.revokeAllSessions("user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(2);
|
||||
verify(sessionRepository).deleteById("session-1");
|
||||
verify(sessionRepository).deleteById("session-2");
|
||||
}
|
||||
|
||||
// ─── null-guard when sessionRepository is unavailable ────────────────────
|
||||
|
||||
@Test
|
||||
void revokeAllSessions_returns_zero_when_sessionRepository_is_null() {
|
||||
ReflectionTestUtils.setField(authService, "sessionRepository", null);
|
||||
|
||||
int count = authService.revokeAllSessions("user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(0);
|
||||
}
|
||||
|
||||
@Test
|
||||
void revokeOtherSessions_returns_zero_when_sessionRepository_is_null() {
|
||||
ReflectionTestUtils.setField(authService, "sessionRepository", null);
|
||||
|
||||
int count = authService.revokeOtherSessions("session-keep", "user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(0);
|
||||
assertThat(count).isEqualTo(3);
|
||||
verify(sessionRevocationPort).revokeAllSessions("user@test.de");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
package org.raddatz.familienarchiv.auth;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.extension.ExtendWith;
|
||||
import org.mockito.InjectMocks;
|
||||
import org.mockito.Mock;
|
||||
import org.mockito.junit.jupiter.MockitoExtension;
|
||||
import org.springframework.session.jdbc.JdbcIndexedSessionRepository;
|
||||
|
||||
import java.util.HashMap;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.Mockito.*;
|
||||
|
||||
@ExtendWith(MockitoExtension.class)
|
||||
class JdbcSessionRevocationAdapterTest {
|
||||
|
||||
@Mock JdbcIndexedSessionRepository sessionRepository;
|
||||
@InjectMocks JdbcSessionRevocationAdapter adapter;
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void revokeOtherSessions_preserves_current_and_deletes_N_minus_1() {
|
||||
var sessions = new HashMap<String, Object>();
|
||||
sessions.put("session-keep", null);
|
||||
sessions.put("session-del-1", null);
|
||||
sessions.put("session-del-2", null);
|
||||
doReturn(sessions).when(sessionRepository).findByPrincipalName("user@test.de");
|
||||
|
||||
int count = adapter.revokeOtherSessions("session-keep", "user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(2);
|
||||
verify(sessionRepository, never()).deleteById("session-keep");
|
||||
verify(sessionRepository).deleteById("session-del-1");
|
||||
verify(sessionRepository).deleteById("session-del-2");
|
||||
}
|
||||
|
||||
@SuppressWarnings("unchecked")
|
||||
@Test
|
||||
void revokeAllSessions_deletes_all_sessions_for_principal() {
|
||||
var sessions = new HashMap<String, Object>();
|
||||
sessions.put("session-1", null);
|
||||
sessions.put("session-2", null);
|
||||
doReturn(sessions).when(sessionRepository).findByPrincipalName("user@test.de");
|
||||
|
||||
int count = adapter.revokeAllSessions("user@test.de");
|
||||
|
||||
assertThat(count).isEqualTo(2);
|
||||
verify(sessionRepository).deleteById("session-1");
|
||||
verify(sessionRepository).deleteById("session-2");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user