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.
This commit is contained in:
lobo committed 2026-09-30 13:41:57 +02:00
1 parent 509280da14
commit 1f94c87a20
10 files changed
+109 -12

No files matched your search

+10 -7
View File
@@ -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 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. 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 - **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 (`users.enabled`; blocking ends every login and, within 30 s, the access token at Auth; Core accepts the token
right password), delete (the same flag and nightly cleanup as "Delete my account"). No action on the own account 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`. 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 - **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. 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 "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`. 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 - **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 `users.deleted_at` and revokes every refresh token: from then on no login, no refresh, no access token, no public
link, not in the landing stats. Email and display name stay taken; the Steam ID is freed, so an undo needs a profile, no Steam link, not in the landing stats. Email and display name stay taken; the Steam ID is freed, so an
new Steam link. A nightly job (04:15, Europe/Berlin) removes 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 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. **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`. 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 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. 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; Three Caffeine caches in `CacheConfig`: `users` (by userId) and `publicStats` expire 10 minutes after the write,
entries also 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. `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. `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 `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->>A: users.deleted_at = now, revoke refresh tokens
A->>C: POST /internal/accounts/deleted (service token) A->>C: POST /internal/accounts/deleted (service token)
A-->>B: 204, both cookies cleared 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 Note over A,C: nightly 04:15
A->>C: PUT /internal/accounts/deleted (all flagged users) A->>C: PUT /internal/accounts/deleted (all flagged users)
A->>C: POST /internal/accounts/purge (flagged 30+ days) A->>C: POST /internal/accounts/purge (flagged 30+ days)
@@ -15,6 +15,8 @@ public class CacheConfig {
// After write, not after access: an entry another path changed without an evict still ages out. // After write, not after access: an entry another path changed without an evict still ages out.
static final Duration DEFAULT_TTL = Duration.ofMinutes(10); 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 @Bean
public CacheManager cacheManager() { public CacheManager cacheManager() {
@@ -23,6 +25,11 @@ public class CacheConfig {
.maximumSize(500) .maximumSize(500)
.expireAfterWrite(DEFAULT_TTL) .expireAfterWrite(DEFAULT_TTL)
.recordStats()); .recordStats());
cacheManager.registerCustomCache("activeUsers", Caffeine.newBuilder()
.maximumSize(10_000)
.expireAfterWrite(ACTIVE_USERS_TTL)
.recordStats()
.build());
return cacheManager; return cacheManager;
} }
} }
@@ -1,6 +1,7 @@
package auth_service.configuration.security; package auth_service.configuration.security;
import auth_service.shared.api.AuthCookies; import auth_service.shared.api.AuthCookies;
import auth_service.shared.security.ActiveUserCheck;
import auth_service.shared.security.Role; import auth_service.shared.security.Role;
import auth_service.shared.security.UserPrincipal; import auth_service.shared.security.UserPrincipal;
import jakarta.servlet.FilterChain; 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 * 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 @Component
@RequiredArgsConstructor @RequiredArgsConstructor
@@ -33,6 +35,7 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter {
private static final String BEARER = "Bearer "; private static final String BEARER = "Bearer ";
private final JwtService jwtService; private final JwtService jwtService;
private final ActiveUserCheck activeUserCheck;
@Override @Override
protected void doFilterInternal( protected void doFilterInternal(
@@ -45,6 +48,7 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter {
.flatMap(Optional::stream) .flatMap(Optional::stream)
.map(jwtService::parseLoginToken) .map(jwtService::parseLoginToken)
.flatMap(Optional::stream) .flatMap(Optional::stream)
.filter(each -> activeUserCheck.isActive(each.getIdAsLong()))
.findFirst(); .findFirst();
if (principal.isPresent() && SecurityContextHolder.getContext().getAuthentication() == null) { if (principal.isPresent() && SecurityContextHolder.getContext().getAuthentication() == null) {
UsernamePasswordAuthenticationToken authToken = new UsernamePasswordAuthenticationToken( UsernamePasswordAuthenticationToken authToken = new UsernamePasswordAuthenticationToken(
@@ -9,6 +9,7 @@ import auth_service.shared.security.RequiresRole;
import auth_service.shared.security.Role; import auth_service.shared.security.Role;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CacheEvict;
import org.springframework.cache.annotation.Caching;
import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service; 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 * 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. * 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) { public void deleteAccount(long userId, String password) {
String key = String.valueOf(userId); String key = String.valueOf(userId);
if (!attemptLimiter.tryAcquire(key, MAX_WRONG_PASSWORDS)) { if (!attemptLimiter.tryAcquire(key, MAX_WRONG_PASSWORDS)) {
@@ -62,7 +64,8 @@ public class AccountService implements AccountDeleter {
@Override @Override
@RequiresRole(Role.PLATFORM_ADMIN) @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) { public boolean deleteByAdmin(long userId) {
return flagAndTellCore(userId); return flagAndTellCore(userId);
} }
@@ -12,6 +12,7 @@ import auth_service.shared.security.Role;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.cache.annotation.CacheEvict; import org.springframework.cache.annotation.CacheEvict;
import org.springframework.cache.annotation.Caching;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@@ -39,7 +40,8 @@ public class AdminUserService {
} }
@Transactional @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) { public void setBlocked(long actorUserId, long targetUserId, boolean blocked) {
TargetState target = checkedTarget(actorUserId, targetUserId); TargetState target = checkedTarget(actorUserId, targetUserId);
if (target.deleted()) { if (target.deleted()) {
@@ -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();
}
}
@@ -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);
}
@@ -7,10 +7,12 @@ import auth_service.shared.security.TokenGenerator;
import auth_service.shared.api.AuthCookies; import auth_service.shared.api.AuthCookies;
import auth_service.shared.test.AbstractIntegrationTest; import auth_service.shared.test.AbstractIntegrationTest;
import jakarta.servlet.http.Cookie; import jakarta.servlet.http.Cookie;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc;
import org.springframework.context.annotation.Import; import org.springframework.context.annotation.Import;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.ResultActions; import org.springframework.test.web.servlet.ResultActions;
@@ -92,8 +94,21 @@ class RoleCheckFlowTest extends AbstractIntegrationTest {
@Autowired @Autowired
private TokenGenerator tokenGenerator; 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) { 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 { private ResultActions call(String path, Role role) throws Exception {
@@ -156,6 +156,8 @@ class DeletedAccountFlowTest extends AbstractIntegrationTest {
MockHttpServletResponse session = login(); MockHttpServletResponse session = login();
String access = session.getCookie(AuthCookies.ACCESS_COOKIE).getValue(); String access = session.getCookie(AuthCookies.ACCESS_COOKIE).getValue();
String refresh = session.getCookie(AuthCookies.REFRESH_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(); long usersBefore = totalUsers();
MockHttpServletResponse deleted = deleteAccount(access, PASSWORD).andExpect(status().isNoContent()) MockHttpServletResponse deleted = deleteAccount(access, PASSWORD).andExpect(status().isNoContent())
@@ -168,6 +170,8 @@ class DeletedAccountFlowTest extends AbstractIntegrationTest {
.cookie(new Cookie(AuthCookies.REFRESH_COOKIE, refresh))) .cookie(new Cookie(AuthCookies.REFRESH_COOKIE, refresh)))
.andExpect(status().isUnauthorized()); .andExpect(status().isUnauthorized());
assertThat(login().getStatus()).isEqualTo(401); 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()); mockMvc.perform(get("/api/public/profiles/" + NAME)).andExpect(status().isNotFound());
assertThat(totalUsers()).isEqualTo(usersBefore - 1); assertThat(totalUsers()).isEqualTo(usersBefore - 1);
} }
@@ -169,9 +169,12 @@ class AdminFlowTest extends AbstractIntegrationTest {
void block_EndsTheSessionsAndTheLoginTellsWhy() throws Exception { void block_EndsTheSessionsAndTheLoginTellsWhy() throws Exception {
Session admin = Session.of(login(ADMIN)); Session admin = Session.of(login(ADMIN));
Session player = Session.of(login(PLAYER)); Session player = Session.of(login(PLAYER));
assertThat(me(player)).isEqualTo(200);
block(admin, playerId, true).andExpect(status().isNoContent()); block(admin, playerId, true).andExpect(status().isNoContent());
assertThat(me(player)).isEqualTo(401);
assertThat(login(PLAYER).getContentAsString()).contains("ACCOUNT_BLOCKED"); assertThat(login(PLAYER).getContentAsString()).contains("ACCOUNT_BLOCKED");
assertThat(login(PLAYER).getStatus()).isEqualTo(403); assertThat(login(PLAYER).getStatus()).isEqualTo(403);
assertThat(mockMvc.perform(post("/api/auth/refresh").header(ClientHeaderFilter.CLIENT_HEADER, "web") 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()); block(admin, playerId, false).andExpect(status().isNoContent());
assertThat(login(PLAYER).getStatus()).isEqualTo(200); 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 @Test
void delete_FlagsTheUser() throws Exception { void delete_FlagsTheUser() throws Exception {
Session admin = Session.of(login(ADMIN)); 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") mockMvc.perform(delete("/api/admin/users/" + playerId).header(ClientHeaderFilter.CLIENT_HEADER, "web")
.cookie(new Cookie(AuthCookies.ACCESS_COOKIE, admin.access()))) .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(jdbc.queryForObject("SELECT deleted_at FROM users WHERE id = ?", Object.class, playerId)).isNotNull();
assertThat(login(PLAYER).getStatus()).isEqualTo(401); assertThat(login(PLAYER).getStatus()).isEqualTo(401);
assertThat(me(player)).isEqualTo(401);
assertThat(audits("DELETE", playerId)).isEqualTo(1); assertThat(audits("DELETE", playerId)).isEqualTo(1);
} }