From d61513639316bcd910a460f0ec24e2a9087a5f64 Mon Sep 17 00:00:00 2001 From: MorganaFuture Date: Wed, 29 Apr 2026 16:43:34 +0300 Subject: [PATCH] add scheduled task to disable expired api keys expired keys were staying enabled=true in the db even after expiry. gateway rejected them correctly at request time, but the stored state was stale. added hourly sweep in management-service-core with shedlock. --- .../openframe/api/service/ApiKeyService.java | 15 -- openframe-management-service-core/pom.xml | 7 + .../scheduler/ApiKeyExpirationScheduler.java | 33 +++ .../service/ApiKeyExpirationService.java | 30 +++ .../service/ApiKeyExpirationServiceTest.java | 198 ++++++++++++++++++ 5 files changed, 268 insertions(+), 15 deletions(-) create mode 100644 openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ApiKeyExpirationScheduler.java create mode 100644 openframe-management-service-core/src/main/java/com/openframe/management/service/ApiKeyExpirationService.java create mode 100644 openframe-management-service-core/src/test/java/com/openframe/management/service/ApiKeyExpirationServiceTest.java diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/service/ApiKeyService.java b/openframe-api-service-core/src/main/java/com/openframe/api/service/ApiKeyService.java index 427872ae6..eb776ddf0 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/service/ApiKeyService.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/service/ApiKeyService.java @@ -101,21 +101,6 @@ public void deleteApiKey(String keyId, String userId) { log.info("Deleted API key and its statistics: {}", keyId); } - /** - * Disable expired API keys TODO (scheduled task) - */ - public void disableExpiredKeys() { - List expiredKeys = apiKeyRepository.findExpiredKeys(Instant.now()); - - for (ApiKey key : expiredKeys) { - key.setEnabled(false); - key.setUpdatedAt(Instant.now()); - apiKeyRepository.save(key); - log.info("Disabled expired API key: {}", key.getKeyId()); - } - } - - private String generateKeyId() { byte[] bytes = new byte[KEY_ID_LENGTH / 2]; secureRandom.nextBytes(bytes); diff --git a/openframe-management-service-core/pom.xml b/openframe-management-service-core/pom.xml index c4b61f07c..6a151d10a 100644 --- a/openframe-management-service-core/pom.xml +++ b/openframe-management-service-core/pom.xml @@ -88,6 +88,13 @@ shedlock-provider-redis-spring 5.10.2 + + + + org.springframework.boot + spring-boot-starter-test + test + diff --git a/openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ApiKeyExpirationScheduler.java b/openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ApiKeyExpirationScheduler.java new file mode 100644 index 000000000..3b1ee131a --- /dev/null +++ b/openframe-management-service-core/src/main/java/com/openframe/management/scheduler/ApiKeyExpirationScheduler.java @@ -0,0 +1,33 @@ +package com.openframe.management.scheduler; + +import com.openframe.management.service.ApiKeyExpirationService; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import net.javacrumbs.shedlock.spring.annotation.SchedulerLock; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.scheduling.annotation.Scheduled; +import org.springframework.stereotype.Component; + +@Component +@RequiredArgsConstructor +@Slf4j +@ConditionalOnProperty(name = "openframe.api-key-expiration.enabled", havingValue = "true", matchIfMissing = true) +public class ApiKeyExpirationScheduler { + + private final ApiKeyExpirationService expirationService; + + @Scheduled(fixedDelayString = "${openframe.api-key-expiration.interval:3600000}") + @SchedulerLock( + name = "apiKeyExpiration", + lockAtMostFor = "${openframe.api-key-expiration.lock-at-most-for:10m}", + lockAtLeastFor = "${openframe.api-key-expiration.lock-at-least-for:1m}" + ) + public void disableExpiredKeys() { + log.info("Starting API key expiration sweep"); + try { + expirationService.disableExpiredKeys(); + } catch (Exception e) { + log.error("API key expiration sweep failed", e); + } + } +} diff --git a/openframe-management-service-core/src/main/java/com/openframe/management/service/ApiKeyExpirationService.java b/openframe-management-service-core/src/main/java/com/openframe/management/service/ApiKeyExpirationService.java new file mode 100644 index 000000000..d1fde0aa3 --- /dev/null +++ b/openframe-management-service-core/src/main/java/com/openframe/management/service/ApiKeyExpirationService.java @@ -0,0 +1,30 @@ +package com.openframe.management.service; + +import com.openframe.data.document.apikey.ApiKey; +import com.openframe.data.repository.apikey.ApiKeyRepository; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Service; + +import java.time.Instant; +import java.util.List; + +@Service +@RequiredArgsConstructor +@Slf4j +public class ApiKeyExpirationService { + + private final ApiKeyRepository apiKeyRepository; + + public int disableExpiredKeys() { + List expiredKeys = apiKeyRepository.findExpiredKeys(Instant.now()); + for (ApiKey key : expiredKeys) { + key.setEnabled(false); + key.setUpdatedAt(Instant.now()); + apiKeyRepository.save(key); + log.info("Disabled expired API key: {}", key.getKeyId()); + } + log.info("API key expiration sweep complete. Disabled {} keys.", expiredKeys.size()); + return expiredKeys.size(); + } +} diff --git a/openframe-management-service-core/src/test/java/com/openframe/management/service/ApiKeyExpirationServiceTest.java b/openframe-management-service-core/src/test/java/com/openframe/management/service/ApiKeyExpirationServiceTest.java new file mode 100644 index 000000000..d58f3d9ee --- /dev/null +++ b/openframe-management-service-core/src/test/java/com/openframe/management/service/ApiKeyExpirationServiceTest.java @@ -0,0 +1,198 @@ +package com.openframe.management.service; + +import com.openframe.data.document.apikey.ApiKey; +import com.openframe.data.repository.apikey.ApiKeyRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +@ExtendWith(MockitoExtension.class) +class ApiKeyExpirationServiceTest { + + @Mock + private ApiKeyRepository apiKeyRepository; + + private ApiKeyExpirationService service; + + @BeforeEach + void setUp() { + service = new ApiKeyExpirationService(apiKeyRepository); + } + + // ----------------------------------------------------------------------- + // Proving the problem existed: expired key stays enabled=true without a sweep + // ----------------------------------------------------------------------- + + @Nested + @DisplayName("ApiKey model — expiration semantics (proving the original bug)") + class ExpirationSemantics { + + @Test + @DisplayName("isExpired() returns true when expiresAt is in the past") + void isExpiredWhenPast() { + ApiKey key = expiredKey(); + assertThat(key.isExpired()).isTrue(); + } + + @Test + @DisplayName("isActive() returns false for an expired key — gateway correctly rejects at request time") + void isNotActiveWhenExpired() { + ApiKey key = expiredKey(); + assertThat(key.isActive()).isFalse(); + } + + @Test + @DisplayName("enabled flag stays true without a sweep — proves the bug: stale data in DB") + void enabledFlagStaysTrueWithoutSweep() { + ApiKey key = expiredKey(); + // isActive() is false but enabled is still true — consumers reading `enabled` + // directly (e.g. audit queries) would see the key as enabled + assertThat(key.isExpired()).isTrue(); + assertThat(key.isEnabled()).isTrue(); + } + + @Test + @DisplayName("isExpired() returns false when expiresAt is in the future") + void isNotExpiredWhenFuture() { + ApiKey key = validKey(); + assertThat(key.isExpired()).isFalse(); + } + + @Test + @DisplayName("isActive() returns true for an enabled, non-expired key") + void isActiveWhenValid() { + ApiKey key = validKey(); + assertThat(key.isActive()).isTrue(); + } + + @Test + @DisplayName("isExpired() returns false when expiresAt is null — no expiry set") + void isNotExpiredWhenNoExpirySet() { + ApiKey key = keyWithNoExpiry(); + assertThat(key.isExpired()).isFalse(); + assertThat(key.isActive()).isTrue(); + } + } + + // ----------------------------------------------------------------------- + // Proving the fix works + // ----------------------------------------------------------------------- + + @Nested + @DisplayName("disableExpiredKeys — fix verification") + class DisableExpiredKeys { + + @Test + @DisplayName("sets enabled=false and saves each expired key") + void disablesExpiredKeys() { + ApiKey key1 = expiredKey("key-1"); + ApiKey key2 = expiredKey("key-2"); + when(apiKeyRepository.findExpiredKeys(any())).thenReturn(List.of(key1, key2)); + when(apiKeyRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + service.disableExpiredKeys(); + + ArgumentCaptor captor = ArgumentCaptor.forClass(ApiKey.class); + verify(apiKeyRepository, times(2)).save(captor.capture()); + + assertThat(captor.getAllValues()).allSatisfy(saved -> { + assertThat(saved.isEnabled()).isFalse(); + assertThat(saved.getUpdatedAt()).isNotNull(); + }); + } + + @Test + @DisplayName("returns the count of disabled keys") + void returnsCountOfDisabledKeys() { + when(apiKeyRepository.findExpiredKeys(any())) + .thenReturn(List.of(expiredKey("k1"), expiredKey("k2"), expiredKey("k3"))); + when(apiKeyRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + int count = service.disableExpiredKeys(); + + assertThat(count).isEqualTo(3); + } + + @Test + @DisplayName("does nothing and returns 0 when no expired keys exist") + void doesNothingWhenNoExpiredKeys() { + when(apiKeyRepository.findExpiredKeys(any())).thenReturn(List.of()); + + int count = service.disableExpiredKeys(); + + assertThat(count).isZero(); + verify(apiKeyRepository, never()).save(any()); + } + + @Test + @DisplayName("already-disabled keys are not returned by findExpiredKeys — no double-save") + void alreadyDisabledKeysAreNotReprocessed() { + // findExpiredKeys query filters: { expiresAt: {$lt: now}, enabled: true } + // so disabled keys are already excluded at the DB level + when(apiKeyRepository.findExpiredKeys(any())).thenReturn(List.of()); + + service.disableExpiredKeys(); + + verify(apiKeyRepository, never()).save(any()); + } + + @Test + @DisplayName("after sweep, key is both isExpired()=true and enabled=false") + void afterSweepKeyIsFullyDeactivated() { + ApiKey key = expiredKey("key-sweep"); + when(apiKeyRepository.findExpiredKeys(any())).thenReturn(List.of(key)); + when(apiKeyRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + service.disableExpiredKeys(); + + assertThat(key.isExpired()).isTrue(); + assertThat(key.isEnabled()).isFalse(); + assertThat(key.isActive()).isFalse(); + } + } + + // ----------------------------------------------------------------------- + // Helpers + // ----------------------------------------------------------------------- + + private ApiKey expiredKey() { + return expiredKey("key-expired"); + } + + private ApiKey expiredKey(String keyId) { + ApiKey key = new ApiKey(); + key.setKeyId(keyId); + key.setEnabled(true); + key.setExpiresAt(Instant.now().minus(1, ChronoUnit.HOURS)); + return key; + } + + private ApiKey validKey() { + ApiKey key = new ApiKey(); + key.setKeyId("key-valid"); + key.setEnabled(true); + key.setExpiresAt(Instant.now().plus(24, ChronoUnit.HOURS)); + return key; + } + + private ApiKey keyWithNoExpiry() { + ApiKey key = new ApiKey(); + key.setKeyId("key-no-expiry"); + key.setEnabled(true); + key.setExpiresAt(null); + return key; + } +}