fix revokation bugs and add unit tests

src/main/java/com/ddf/vodsystem/controllers/UserController.java

@@ -80,20 +80,20 @@ public class UserController {
     }
 
     /**
-     * Exchanges a refresh token for a new refresh token and JWT, replacing both cookies.
+     * Exchanges the {@code refreshToken} cookie for a new refresh token and JWT, replacing both cookies.
      *
-     * @param token    the payload carrying the current refresh token
+     * @param refreshToken the refresh token from the {@code refreshToken} cookie, if present
-     * @param response the response the new cookies are written to
+     * @param request      the incoming request, used to decide the cookies' {@code Secure} flag
-     * @param request  the incoming request, used to decide the cookies' {@code Secure} flag
+     * @param response     the response the new cookies are written to
      * @return {@code 200 OK} with no data
-     * @throws NotAuthenticated if the refresh token is rejected
+     * @throws NotAuthenticated if the refresh token is missing or rejected
      */
     @PostMapping("/refresh")
     public ResponseEntity<APIResponse<Void>> refresh(
-            @RequestBody Token token,
+            @CookieValue(name = "refreshToken", required = false) String refreshToken,
-            HttpServletResponse response,
+            HttpServletRequest request,
-            HttpServletRequest request) {
+            HttpServletResponse response) {
-        TokenPackage tokenPackage = userService.refresh(token.token());
+        TokenPackage tokenPackage = userService.refresh(refreshToken);
         addCookies(request, response, tokenPackage.accessToken(), tokenPackage.refreshToken());
 
         return ResponseEntity.ok(
@@ -102,35 +102,46 @@ public class UserController {
     }
 
     /**
-     * Logs the user out by revoking the session tied to the {@code refreshToken} cookie.
+     * Logs the user out by revoking the session tied to the {@code refreshToken} cookie and clearing
-     * The auth cookies are not cleared.
+     * both auth cookies. Succeeds even if no refresh token cookie is present.
      *
      * @param refreshToken the refresh token from the {@code refreshToken} cookie, if present
+     * @param request      the incoming request, used to decide the cookies' {@code Secure} flag
+     * @param response     the response the expired cookies are written to
      * @return {@code 200 OK} confirming logout
      */
     @PostMapping("/logout")
     public ResponseEntity<APIResponse<Void>> logout(
-            @CookieValue(name = "refreshToken", required = false) String refreshToken) {
+            @CookieValue(name = "refreshToken") String refreshToken,
+            HttpServletRequest request,
+            HttpServletResponse response) {
         userService.logout(refreshToken);
+        writeCookies(request, response, "", Duration.ZERO, "", Duration.ZERO);
 
         return ResponseEntity.ok(
                 new APIResponse<>(SUCCESS, "Logged out successfully", null)
         );
     }
 
-    /**
-     * Writes the {@code accessToken} cookie ({@code SameSite=Lax}, path {@code /}) and the
-     * {@code refreshToken} cookie ({@code SameSite=Strict}, path {@code /api/v1/users}), both HttpOnly
-     * and {@code Secure} when the request is.
-     */
     private void addCookies(
             HttpServletRequest request,
             HttpServletResponse response,
             String accessToken,
             String refreshToken) {
+        writeCookies(request, response, accessToken, jwtExpiration, refreshToken, refreshTokenExpiration);
+    }
+
+
+    private void writeCookies(
+            HttpServletRequest request,
+            HttpServletResponse response,
+            String accessToken,
+            Duration accessTokenMaxAge,
+            String refreshToken,
+            Duration refreshTokenMaxAge) {
         ResponseCookie accessTokenCookie = ResponseCookie.from("accessToken", accessToken)
                 .httpOnly(true)
-                .maxAge(jwtExpiration.getSeconds())
+                .maxAge(accessTokenMaxAge)
                 .sameSite("Lax")
                 .secure(request.isSecure())
                 .path(ACCESS_COOKIE_PATH)
@@ -138,7 +149,7 @@ public class UserController {
 
         ResponseCookie refreshTokenCookie = ResponseCookie.from("refreshToken", refreshToken)
                 .httpOnly(true)
-                .maxAge(refreshTokenExpiration.getSeconds())
+                .maxAge(refreshTokenMaxAge)
                 .sameSite("Strict")
                 .secure(request.isSecure())
                 .path(REFRESH_COOKIE_PATH)

src/main/java/com/ddf/vodsystem/security/TokenService.java

@@ -158,13 +158,14 @@ public class TokenService {
 
     /**
      * Revokes a refresh token and its whole token family, ending the session. Does nothing if the
-     * token is unknown, and leaves existing revocation timestamps unchanged.
+     * token is missing or unknown, and leaves existing revocation timestamps unchanged.
      *
-     * @param hexToken the raw refresh token as a hex string
+     * @param hexToken the raw refresh token as a hex string, may be {@code null}
      * @throws NotAuthenticated if the token is not valid hex
      */
+    @Transactional
     public void revoke(String hexToken) {
-        Instant now = Instant.now(clock);
+        if (hexToken == null || hexToken.isBlank()) return;
 
         byte[] hash;
         try {
@@ -173,14 +174,22 @@ public class TokenService {
             throw new NotAuthenticated("Malformed refresh token");
         }
 
-        Optional<RefreshToken> found = refreshTokenRepository.findByHash(sha256(hash));
+        Optional<RefreshToken> found = refreshTokenRepository.findByHash(hash);
         if (found.isEmpty()) return;
 
+        Instant now = Instant.now(clock);
         RefreshToken refreshToken = found.get();
         TokenFamily family = refreshToken.getTokenFamily();
 
-        if (family.getRevokedAt() == null) family.setRevokedAt(now);
+        if (family.getRevokedAt() == null) {
-        if (refreshToken.getRevokedAt() == null) refreshToken.setRevokedAt(now);
+            family.setRevokedAt(now);
+            tokenFamilyRepository.save(family);
+        }
+
+        if (refreshToken.getRevokedAt() == null) {
+            refreshToken.setRevokedAt(now);
+            refreshTokenRepository.save(refreshToken);
+        }
     }
 
     /**

src/main/java/com/ddf/vodsystem/services/UserService.java

@@ -99,7 +99,7 @@ public class UserService {
      * Ends the session tied to a refresh token by revoking its token family.
      * Already-issued JWT access tokens stay valid until they expire.
      *
-     * @param refreshToken the raw refresh token as a hex string
+     * @param refreshToken the raw refresh token as a hex string, may be {@code null}
      * @throws NotAuthenticated if the token is not valid hex
      */
     public void logout(String refreshToken) {

src/test/java/com/ddf/vodsystem/security/TokenServiceTest.java

@@ -17,6 +17,7 @@ import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
+import java.security.MessageDigest;
 import java.time.*;
 import java.util.HexFormat;
 import java.util.Optional;
@@ -518,20 +519,110 @@ class TokenServiceTest {
             verify(refreshTokenRepository, never()).save(any());
             verifyNoInteractions(tokenFamilyRepository);
         }
+    }
 
-        private TokenFamily activeFamily() {
+    @Nested
-            TokenFamily tokenFamily = new TokenFamily();
+    @DisplayName("revoke()")
-            tokenFamily.setId(1L);
+    class revoke {
-            tokenFamily.setUser(createUser(1L, "user", "user"));
+        @Test
-            tokenFamily.setExpiresAt(clock.instant().plusSeconds(10));
+        void knownToken_revokesTokenAndFamily() {
-            return tokenFamily;
+            TokenFamily tokenFamily = activeFamily();
+            RefreshToken refreshToken = tokenIn(tokenFamily);
+            when(refreshTokenRepository.findByHash(any())).thenReturn(Optional.of(refreshToken));
+
+            tokenService.revoke(rawToken);
+
+            assertEquals(clock.instant(), tokenFamily.getRevokedAt());
+            assertEquals(clock.instant(), refreshToken.getRevokedAt());
         }
 
-        private RefreshToken tokenIn(TokenFamily tokenFamily) {
+        @Test
-            RefreshToken refreshToken = new RefreshToken();
+        void knownToken_savesTokenAndFamily() {
-            refreshToken.setTokenFamily(tokenFamily);
+            TokenFamily tokenFamily = activeFamily();
-            refreshToken.setExpiresAt(clock.instant().plusSeconds(10));
+            RefreshToken refreshToken = tokenIn(tokenFamily);
-            return refreshToken;
+            when(refreshTokenRepository.findByHash(any())).thenReturn(Optional.of(refreshToken));
+
+            tokenService.revoke(rawToken);
+
+            verify(tokenFamilyRepository).save(tokenFamily);
+            verify(refreshTokenRepository).save(refreshToken);
+        }
+
+        @Test
+        void knownToken_looksUpBySingleSha256Hash() throws Exception {
+            when(refreshTokenRepository.findByHash(any())).thenReturn(Optional.empty());
+
+            tokenService.revoke(rawToken);
+
+            byte[] expectedHash = MessageDigest.getInstance("SHA-256").digest(HEX.parseHex(rawToken));
+            verify(refreshTokenRepository).findByHash(expectedHash);
+        }
+
+        @Test
+        void alreadyRevoked_keepsOriginalTimestampsAndDoesNotSave() {
+            Instant revokedAt = clock.instant();
+            TokenFamily tokenFamily = activeFamily();
+            tokenFamily.setRevokedAt(revokedAt);
+            RefreshToken refreshToken = tokenIn(tokenFamily);
+            refreshToken.setRevokedAt(revokedAt);
+            when(refreshTokenRepository.findByHash(any())).thenReturn(Optional.of(refreshToken));
+
+            clock.advance(Duration.ofMinutes(1));
+            tokenService.revoke(rawToken);
+
+            assertEquals(revokedAt, tokenFamily.getRevokedAt());
+            assertEquals(revokedAt, refreshToken.getRevokedAt());
+            verify(tokenFamilyRepository, never()).save(any());
+            verify(refreshTokenRepository, never()).save(any());
+        }
+
+        @Test
+        void unknownToken_doesNothing() {
+            when(refreshTokenRepository.findByHash(any())).thenReturn(Optional.empty());
+
+            tokenService.revoke(rawToken);
+
+            verify(refreshTokenRepository, never()).save(any());
+            verifyNoInteractions(tokenFamilyRepository);
+        }
+
+        @Test
+        void nullToken_doesNothing() {
+            tokenService.revoke(null);
+
+            verifyNoInteractions(refreshTokenRepository, tokenFamilyRepository);
+        }
+
+        @Test
+        void blankToken_doesNothing() {
+            tokenService.revoke("");
+
+            verifyNoInteractions(refreshTokenRepository, tokenFamilyRepository);
+        }
+
+        @Test
+        void malformedToken_throwsNotAuthenticated() {
+            assertThrows(
+                    NotAuthenticated.class,
+                    () -> tokenService.revoke("not-hex")
+            );
+
+            verifyNoInteractions(refreshTokenRepository, tokenFamilyRepository);
         }
     }
+
+    private TokenFamily activeFamily() {
+        TokenFamily tokenFamily = new TokenFamily();
+        tokenFamily.setId(1L);
+        tokenFamily.setUser(createUser(1L, "user", "user"));
+        tokenFamily.setExpiresAt(clock.instant().plusSeconds(10));
+        return tokenFamily;
+    }
+
+    private RefreshToken tokenIn(TokenFamily tokenFamily) {
+        RefreshToken refreshToken = new RefreshToken();
+        refreshToken.setTokenFamily(tokenFamily);
+        refreshToken.setExpiresAt(clock.instant().plusSeconds(10));
+        return refreshToken;
+    }
 }