From 1f94c87a20fe73de1de1b5d1cdcc368566f1e24d Mon Sep 17 00:00:00 2001 From: lobo Date: Wed, 30 Sep 2026 13:41:57 +0200 Subject: [PATCH] fix(security): refuse the access token of a blocked or deleted user activeUsers cache, 30 s. Core still accepts the token until it expires; X4 (batch 4) tells Core. --- readme.md | 17 +++++++----- .../configuration/CacheConfig.java | 7 +++++ .../security/JwtAuthenticationFilter.java | 6 ++++- .../account/service/AccountService.java | 7 +++-- .../admin/service/AdminUserService.java | 4 ++- .../login/service/ActiveUserService.java | 26 +++++++++++++++++++ .../shared/security/ActiveUserCheck.java | 8 ++++++ .../security/RoleCheckFlowTest.java | 17 +++++++++++- .../account/api/DeletedAccountFlowTest.java | 4 +++ .../modules/admin/api/AdminFlowTest.java | 25 ++++++++++++++++++ 10 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 src/main/java/auth_service/modules/login/service/ActiveUserService.java create mode 100644 src/main/java/auth_service/shared/security/ActiveUserCheck.java diff --git a/readme.md b/readme.md index d08f4a9..994d2c6 100644 --- a/readme.md +++ b/readme.md @@ -63,17 +63,17 @@ To Be Played is split into three repositories: guard. ArchUnit fails the build for an unmarked method, two markers, a controller calling an `@Async` method, and a service method calling another service's method that needs more. - **Admin users page** (`/api/admin/**`, active role PLATFORM_ADMIN): search and page all users, block and unblock - (`users.enabled`; blocking ends every login, the login then answers `403 ACCOUNT_BLOCKED`, but only after the - right password), delete (the same flag and nightly cleanup as "Delete my account"). No action on the own account + (`users.enabled`; blocking ends every login and, within 30 s, the access token at Auth; Core accepts the token + until it expires, max 15 min; the login then answers `403 ACCOUNT_BLOCKED`, but only after the right password), delete (the same flag and nightly cleanup as "Delete my account"). No action on the own account or on another admin. Every action and every "view as" is a row in `admin_audit`. - **Dashboard hints**: a PLATFORM_ADMIN writes a hint for all users (plain text in English, German optional, start and end time, type `DISMISS` or `ACCEPT`). The dashboard shows the running hints one at a time, newest first. "Do not show again" on a `DISMISS` hint or "OK" on an `ACCEPT` hint is a row in `hint_done`, so the hint never comes back for this user on any device. Creating and ending a hint are rows in `admin_audit`. - **Delete my account** with the password (5 wrong tries per 15 minutes, then `429`). It only sets - `users.deleted_at` and revokes every refresh token: from then on no login, no refresh, no public profile, no Steam - link, not in the landing stats. Email and display name stay taken; the Steam ID is freed, so an undo needs a - new Steam link. A nightly job (04:15, Europe/Berlin) removes + `users.deleted_at` and revokes every refresh token: from then on no login, no refresh, no access token, no public + profile, no Steam link, not in the landing stats. Email and display name stay taken; the Steam ID is freed, so an + undo needs a new Steam link. A nightly job (04:15, Europe/Berlin) removes users flagged 30 days ago, in Core first and then in Auth. It calls Core's `/internal/accounts/*` with a **service token** (EdDSA JWT, 60 s, `aud=tbp-internal`). `app.account-cleanup.dry-run` (default `true`) only logs. @@ -136,8 +136,10 @@ layers: may log in and is shown to others" uses `shared/account/ActiveUser`. A settings change is one `UserRepository.update(userId, change)`: load the user, change the model, write back the display name and the settings. A new setting needs no new repository method. -Two Caffeine caches in `CacheConfig`: `users` (by userId) and `publicStats`. Every write evicts what it changes; -entries also expire 10 minutes after the write. +Three Caffeine caches in `CacheConfig`: `users` (by userId) and `publicStats` expire 10 minutes after the write, +`activeUsers` (the filter's "blocked or deleted?" check per request) after 30 seconds. Every write evicts what it +changes. A request that read the database just before a block or delete can put the old answer back, so the short +TTL is the real limit: a blocked or deleted user keeps a valid access token for up to 30 s. `profile` and `stats` only read data. They have no entities and use `JdbcTemplate` queries in a `repo` package. `version` has only an `api` package: `GET /api/version` answers the commit and build time of the running image. `account` (delete and nightly cleanup) sets the flag with `JdbcTemplate` in a `repo` package too, and calls Core @@ -214,6 +216,7 @@ sequenceDiagram A->>A: users.deleted_at = now, revoke refresh tokens A->>C: POST /internal/accounts/deleted (service token) A-->>B: 204, both cookies cleared + Note over A: an old access token gets 401 within 30 s (activeUsers cache) Note over A,C: nightly 04:15 A->>C: PUT /internal/accounts/deleted (all flagged users) A->>C: POST /internal/accounts/purge (flagged 30+ days) diff --git a/src/main/java/auth_service/configuration/CacheConfig.java b/src/main/java/auth_service/configuration/CacheConfig.java index e5d16f0..e821a2e 100644 --- a/src/main/java/auth_service/configuration/CacheConfig.java +++ b/src/main/java/auth_service/configuration/CacheConfig.java @@ -15,6 +15,8 @@ public class CacheConfig { // After write, not after access: an entry another path changed without an evict still ages out. static final Duration DEFAULT_TTL = Duration.ofMinutes(10); + // The real limit after a block or delete: a concurrent read can put the old answer back after the evict. + static final Duration ACTIVE_USERS_TTL = Duration.ofSeconds(30); @Bean public CacheManager cacheManager() { @@ -23,6 +25,11 @@ public class CacheConfig { .maximumSize(500) .expireAfterWrite(DEFAULT_TTL) .recordStats()); + cacheManager.registerCustomCache("activeUsers", Caffeine.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(ACTIVE_USERS_TTL) + .recordStats() + .build()); return cacheManager; } } diff --git a/src/main/java/auth_service/configuration/security/JwtAuthenticationFilter.java b/src/main/java/auth_service/configuration/security/JwtAuthenticationFilter.java index 12245de..11db9a1 100644 --- a/src/main/java/auth_service/configuration/security/JwtAuthenticationFilter.java +++ b/src/main/java/auth_service/configuration/security/JwtAuthenticationFilter.java @@ -1,6 +1,7 @@ package auth_service.configuration.security; import auth_service.shared.api.AuthCookies; +import auth_service.shared.security.ActiveUserCheck; import auth_service.shared.security.Role; import auth_service.shared.security.UserPrincipal; import jakarta.servlet.FilterChain; @@ -24,7 +25,8 @@ import java.util.stream.Stream; /** * Takes the login token from the access cookie or from a Bearer header (API clients and tests), the first that - * verifies. A stale cookie next to a valid header must not block the request. + * verifies. A stale cookie next to a valid header must not block the request. A blocked or deleted user counts as + * not logged in. */ @Component @RequiredArgsConstructor @@ -33,6 +35,7 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { private static final String BEARER = "Bearer "; private final JwtService jwtService; + private final ActiveUserCheck activeUserCheck; @Override protected void doFilterInternal( @@ -45,6 +48,7 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { .flatMap(Optional::stream) .map(jwtService::parseLoginToken) .flatMap(Optional::stream) + .filter(each -> activeUserCheck.isActive(each.getIdAsLong())) .findFirst(); if (principal.isPresent() && SecurityContextHolder.getContext().getAuthentication() == null) { UsernamePasswordAuthenticationToken authToken = new UsernamePasswordAuthenticationToken( diff --git a/src/main/java/auth_service/modules/account/service/AccountService.java b/src/main/java/auth_service/modules/account/service/AccountService.java index 6d3a0e0..463f968 100644 --- a/src/main/java/auth_service/modules/account/service/AccountService.java +++ b/src/main/java/auth_service/modules/account/service/AccountService.java @@ -9,6 +9,7 @@ import auth_service.shared.security.RequiresRole; import auth_service.shared.security.Role; import lombok.extern.slf4j.Slf4j; import org.springframework.cache.annotation.CacheEvict; +import org.springframework.cache.annotation.Caching; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; @@ -41,7 +42,8 @@ public class AccountService implements AccountDeleter { * Flags the user as deleted and ends all logins; the nightly cleanup removes the user 30 days later. Core * hides the user at once; when Core is down, the nightly cleanup tells it. */ - @CacheEvict(value = {"users", "publicStats"}, allEntries = true) + @Caching(evict = {@CacheEvict(value = {"users", "activeUsers"}, key = "#userId"), + @CacheEvict(value = "publicStats", allEntries = true)}) public void deleteAccount(long userId, String password) { String key = String.valueOf(userId); if (!attemptLimiter.tryAcquire(key, MAX_WRONG_PASSWORDS)) { @@ -62,7 +64,8 @@ public class AccountService implements AccountDeleter { @Override @RequiresRole(Role.PLATFORM_ADMIN) - @CacheEvict(value = {"users", "publicStats"}, allEntries = true) + @Caching(evict = {@CacheEvict(value = {"users", "activeUsers"}, key = "#userId"), + @CacheEvict(value = "publicStats", allEntries = true)}) public boolean deleteByAdmin(long userId) { return flagAndTellCore(userId); } diff --git a/src/main/java/auth_service/modules/admin/service/AdminUserService.java b/src/main/java/auth_service/modules/admin/service/AdminUserService.java index 153611e..51d18e1 100644 --- a/src/main/java/auth_service/modules/admin/service/AdminUserService.java +++ b/src/main/java/auth_service/modules/admin/service/AdminUserService.java @@ -12,6 +12,7 @@ import auth_service.shared.security.Role; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.cache.annotation.CacheEvict; +import org.springframework.cache.annotation.Caching; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -39,7 +40,8 @@ public class AdminUserService { } @Transactional - @CacheEvict(value = {"users", "publicStats"}, allEntries = true) + @Caching(evict = {@CacheEvict(value = {"users", "activeUsers"}, key = "#targetUserId"), + @CacheEvict(value = "publicStats", allEntries = true)}) public void setBlocked(long actorUserId, long targetUserId, boolean blocked) { TargetState target = checkedTarget(actorUserId, targetUserId); if (target.deleted()) { diff --git a/src/main/java/auth_service/modules/login/service/ActiveUserService.java b/src/main/java/auth_service/modules/login/service/ActiveUserService.java new file mode 100644 index 0000000..07fe465 --- /dev/null +++ b/src/main/java/auth_service/modules/login/service/ActiveUserService.java @@ -0,0 +1,26 @@ +package auth_service.modules.login.service; + +import auth_service.modules.login.model.User; +import auth_service.modules.login.persistence.MinimalUserRepository; +import auth_service.shared.security.ActiveUserCheck; +import auth_service.shared.security.Public; +import lombok.RequiredArgsConstructor; +import org.springframework.cache.annotation.Cacheable; +import org.springframework.stereotype.Service; + +/** + * Runs on every request with a token, so the answer is cached. + */ +@Service +@Public +@RequiredArgsConstructor +public class ActiveUserService implements ActiveUserCheck { + + private final MinimalUserRepository minimalUserRepository; + + @Override + @Cacheable(value = "activeUsers", key = "#userId") + public boolean isActive(long userId) { + return minimalUserRepository.findById(userId).filter(User::canLogin).isPresent(); + } +} diff --git a/src/main/java/auth_service/shared/security/ActiveUserCheck.java b/src/main/java/auth_service/shared/security/ActiveUserCheck.java new file mode 100644 index 0000000..7a480a8 --- /dev/null +++ b/src/main/java/auth_service/shared/security/ActiveUserCheck.java @@ -0,0 +1,8 @@ +package auth_service.shared.security; + +/** + * Port for the filter: the access token of a blocked or deleted user stops working before it expires. + */ +public interface ActiveUserCheck { + boolean isActive(long userId); +} diff --git a/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java b/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java index 128a923..d6dd909 100644 --- a/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java +++ b/src/test/java/auth_service/configuration/security/RoleCheckFlowTest.java @@ -7,10 +7,12 @@ import auth_service.shared.security.TokenGenerator; import auth_service.shared.api.AuthCookies; import auth_service.shared.test.AbstractIntegrationTest; import jakarta.servlet.http.Cookie; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.context.annotation.Import; +import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.stereotype.Service; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.ResultActions; @@ -92,8 +94,21 @@ class RoleCheckFlowTest extends AbstractIntegrationTest { @Autowired private TokenGenerator tokenGenerator; + @Autowired + private JdbcTemplate jdbc; + + private long userId; + + // The filter accepts only the token of an existing, active user. + @BeforeEach + void seedUser() { + jdbc.update("DELETE FROM users WHERE email = 'role-check@example.com'"); + userId = jdbc.queryForObject("INSERT INTO users (email, password, enabled) VALUES ('role-check@example.com', 'x', true) " + + "RETURNING id", Long.class); + } + private String token(Role role) { - return tokenGenerator.generateToken("4711", null, role); + return tokenGenerator.generateToken(String.valueOf(userId), null, role); } private ResultActions call(String path, Role role) throws Exception { diff --git a/src/test/java/auth_service/modules/account/api/DeletedAccountFlowTest.java b/src/test/java/auth_service/modules/account/api/DeletedAccountFlowTest.java index 03392a9..727ddbc 100644 --- a/src/test/java/auth_service/modules/account/api/DeletedAccountFlowTest.java +++ b/src/test/java/auth_service/modules/account/api/DeletedAccountFlowTest.java @@ -156,6 +156,8 @@ class DeletedAccountFlowTest extends AbstractIntegrationTest { MockHttpServletResponse session = login(); String access = session.getCookie(AuthCookies.ACCESS_COOKIE).getValue(); String refresh = session.getCookie(AuthCookies.REFRESH_COOKIE).getValue(); + mockMvc.perform(get("/api/users/me").cookie(new Cookie(AuthCookies.ACCESS_COOKIE, access))) + .andExpect(status().isOk()); long usersBefore = totalUsers(); MockHttpServletResponse deleted = deleteAccount(access, PASSWORD).andExpect(status().isNoContent()) @@ -168,6 +170,8 @@ class DeletedAccountFlowTest extends AbstractIntegrationTest { .cookie(new Cookie(AuthCookies.REFRESH_COOKIE, refresh))) .andExpect(status().isUnauthorized()); assertThat(login().getStatus()).isEqualTo(401); + mockMvc.perform(get("/api/users/me").cookie(new Cookie(AuthCookies.ACCESS_COOKIE, access))) + .andExpect(status().isUnauthorized()); mockMvc.perform(get("/api/public/profiles/" + NAME)).andExpect(status().isNotFound()); assertThat(totalUsers()).isEqualTo(usersBefore - 1); } diff --git a/src/test/java/auth_service/modules/admin/api/AdminFlowTest.java b/src/test/java/auth_service/modules/admin/api/AdminFlowTest.java index 7c6f3b1..aab9924 100644 --- a/src/test/java/auth_service/modules/admin/api/AdminFlowTest.java +++ b/src/test/java/auth_service/modules/admin/api/AdminFlowTest.java @@ -169,9 +169,12 @@ class AdminFlowTest extends AbstractIntegrationTest { void block_EndsTheSessionsAndTheLoginTellsWhy() throws Exception { Session admin = Session.of(login(ADMIN)); Session player = Session.of(login(PLAYER)); + assertThat(me(player)).isEqualTo(200); block(admin, playerId, true).andExpect(status().isNoContent()); + assertThat(me(player)).isEqualTo(401); + assertThat(login(PLAYER).getContentAsString()).contains("ACCOUNT_BLOCKED"); assertThat(login(PLAYER).getStatus()).isEqualTo(403); assertThat(mockMvc.perform(post("/api/auth/refresh").header(ClientHeaderFilter.CLIENT_HEADER, "web") @@ -181,11 +184,32 @@ class AdminFlowTest extends AbstractIntegrationTest { block(admin, playerId, false).andExpect(status().isNoContent()); assertThat(login(PLAYER).getStatus()).isEqualTo(200); + assertThat(me(player)).isEqualTo(200); + } + + @Test + void blockedCookieNextToAValidBearer_UsesTheBearer() throws Exception { + Session admin = Session.of(login(ADMIN)); + Session player = Session.of(login(PLAYER)); + block(admin, playerId, true).andExpect(status().isNoContent()); + + mockMvc.perform(get("/api/users/me") + .cookie(new Cookie(AuthCookies.ACCESS_COOKIE, player.access())) + .header("Authorization", "Bearer " + admin.access())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.email").value(ADMIN)); + } + + private int me(Session session) throws Exception { + return mockMvc.perform(get("/api/users/me").cookie(new Cookie(AuthCookies.ACCESS_COOKIE, session.access()))) + .andReturn().getResponse().getStatus(); } @Test void delete_FlagsTheUser() throws Exception { Session admin = Session.of(login(ADMIN)); + Session player = Session.of(login(PLAYER)); + assertThat(me(player)).isEqualTo(200); mockMvc.perform(delete("/api/admin/users/" + playerId).header(ClientHeaderFilter.CLIENT_HEADER, "web") .cookie(new Cookie(AuthCookies.ACCESS_COOKIE, admin.access()))) @@ -193,6 +217,7 @@ class AdminFlowTest extends AbstractIntegrationTest { assertThat(jdbc.queryForObject("SELECT deleted_at FROM users WHERE id = ?", Object.class, playerId)).isNotNull(); assertThat(login(PLAYER).getStatus()).isEqualTo(401); + assertThat(me(player)).isEqualTo(401); assertThat(audits("DELETE", playerId)).isEqualTo(1); }