From 3fa851e9c07a88a0c373fcb67d889f918c2d3ed8 Mon Sep 17 00:00:00 2001 From: Julia Shtal Date: Tue, 15 Sep 2026 14:59:36 +0200 Subject: [PATCH] fix(security): admit team members to their own memberships endpoint The /api/teams/** rule in SecurityConfig required MANAGER or ADMIN and runs before any controller, so TeamController#getMyMemberships never reached its method-level @PreAuthorize("isAuthenticated()"). Every DEVELOPER got 403, and the Sidebar query gating the Team nav entry left isMember permanently false. The exemption names the endpoint in full rather than the /me subtree: every sibling route binds a {teamId}, so a wildcard would hand nine manager-only handlers to MVC and leave Long conversion as the only thing stopping them. TeamMembershipsControllerTest ran with addFilters = false, disabling the filter chain the bug lived in, and so asserted 200 against code that returned 403. It now loads the real chain and pins that the neighbouring routes stay at 403. fix(http): bound the shared RestTemplate's connect and read timeouts The default factory waits forever. Its only consumers are JiraCollector and JiraProjectService, and discoverProjectsFromJira runs on the request thread, so an unreachable Jira host held a Tomcat worker until the client disconnected. The bounds are app.http.* properties, overridable like the rest of the config. Also drops an appended MappingJackson2HttpMessageConverter that never resolved: both consumers read String and parse with their own ObjectMapper, and RestTemplate registers a Jackson converter ahead of it by default. fix(scheduling): give scheduled jobs a thread pool and serialise metric writers All six @Scheduled jobs shared Spring Boot's default pool of one, so a long enrichment pass delayed the nightly metric, backfill and weekly AI summary jobs. metric_snapshots carries no unique key, so MetricSnapshotWriter reads before it writes: with more than one thread the three scheduled writers can overlap and each insert. MetricWriteGate makes them mutually exclusive. The daily jobs skip on contention, since the next run covers the gap; the weekly job gates only its metric refresh and still summarises stored snapshots, because skipping there would cost a user their brief for seven days. The gate is process-local and does not cover request threads, the collect pool or the attribution listener; its Javadoc says so. All six @Scheduled jobs shared Spring Boot's default pool of one, so a long enrichment pass delayed the nightly metric, backfill and weekly AI summary jobs. metric_snapshots carries no unique key, so MetricSnapshotWriter reads before it writes: with more than one thread the three scheduled writers can overlap and each insert. MetricWriteGate makes them mutually exclusive. The daily jobs skip on contention, since the next run covers the gap; the weekly job gates only its metric refresh and still summarises stored snapshots, because skipping there would cost a user their brief for seven days. The gate is process-local and does not cover request threads, the collect pool or the attribution listener; its Javadoc says so. --- .../ai/scheduler/MetricsSummaryScheduler.java | 10 ++- .../config/RestTemplateConfig.java | 20 ++++-- .../devanalytics/config/SecurityConfig.java | 5 ++ .../service/MetricBackfillScheduler.java | 14 +++- .../metrics/service/MetricWriteGate.java | 40 +++++++++++ .../metrics/service/MetricsScheduler.java | 8 +++ .../src/main/resources/application.yml | 10 +++ .../ai/MetricsSummarySchedulerTest.java | 24 ++++++- .../config/RestTemplateConfigTest.java | 38 ++++++++++ .../metrics/MetricBackfillSchedulerTest.java | 16 ++++- .../metrics/MetricWriteGateTest.java | 72 +++++++++++++++++++ .../metrics/MetricsSchedulerTest.java | 15 +++- .../user/TeamMembershipsControllerTest.java | 61 +++++++++++++++- 13 files changed, 316 insertions(+), 17 deletions(-) create mode 100644 dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricWriteGate.java create mode 100644 dev-analytics/src/test/java/com/juliashtal/devanalytics/config/RestTemplateConfigTest.java create mode 100644 dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricWriteGateTest.java diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/scheduler/MetricsSummaryScheduler.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/scheduler/MetricsSummaryScheduler.java index 314bcf2..5c8d215 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/scheduler/MetricsSummaryScheduler.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/ai/scheduler/MetricsSummaryScheduler.java @@ -3,6 +3,7 @@ import com.juliashtal.devanalytics.ai.model.MetricsSummaryDto; import com.juliashtal.devanalytics.ai.service.MetricsAiService; import com.juliashtal.devanalytics.config.SystemClock; +import com.juliashtal.devanalytics.metrics.service.MetricWriteGate; import com.juliashtal.devanalytics.metrics.service.MetricsService; import com.juliashtal.devanalytics.notification.NotificationDispatchService; import com.juliashtal.devanalytics.user.repository.UserRepository; @@ -28,6 +29,7 @@ public class MetricsSummaryScheduler { private final MetricsAiService metricsAiService; private final NotificationDispatchService notificationDispatch; private final SystemClock systemClock; + private final MetricWriteGate writeGate; @Scheduled(cron = "0 0 8 * * MON", zone = "UTC") public void generateWeeklySummaries() { @@ -39,7 +41,13 @@ public void generateWeeklySummaries() { userRepository.findAll().forEach(user -> { try { // Compute the week before summarising it: the nightly job may not have covered this ISO week. - metricsService.calculateDailyMetrics(user.getId(), from, to); + // Only the refresh is gated — a summary over stored snapshots still beats no summary + // for a week, which is what skipping the whole run would cost at this cadence. + boolean refreshed = writeGate.runExclusively( + () -> metricsService.calculateDailyMetrics(user.getId(), from, to)); + if (!refreshed) { + log.info("Metric refresh skipped for userId={}; summarising stored snapshots", user.getId()); + } MetricsSummaryDto summary = metricsAiService.generateSummary(user, from, to, null); log.debug("Generated weekly summary for userId={}", user.getId()); diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/RestTemplateConfig.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/RestTemplateConfig.java index a502994..1e94cf0 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/RestTemplateConfig.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/RestTemplateConfig.java @@ -1,22 +1,30 @@ package com.juliashtal.devanalytics.config; +import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; +import org.springframework.http.client.SimpleClientHttpRequestFactory; import org.springframework.web.client.RestTemplate; /** * Provides the shared RestTemplate used for outbound HTTP calls. + * + *

Timeouts bound each hop because the default factory waits forever, and + * {@code JiraProjectService.discoverProjectsFromJira} runs on the request thread — an unreachable + * Jira host would otherwise hold a Tomcat worker until the client gave up. They bound a single + * request, not a paged loop over many.

*/ @Configuration public class RestTemplateConfig { @Bean - public RestTemplate restTemplate() { - RestTemplate restTemplate = new RestTemplate(); - restTemplate.getMessageConverters().add(new MappingJackson2HttpMessageConverter()); - return restTemplate; + public RestTemplate restTemplate( + @Value("${app.http.connect-timeout-ms}") int connectTimeoutMs, + @Value("${app.http.read-timeout-ms}") int readTimeoutMs) { + SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory(); + factory.setConnectTimeout(connectTimeoutMs); + factory.setReadTimeout(readTimeoutMs); + return new RestTemplate(factory); } } - diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/SecurityConfig.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/SecurityConfig.java index 321b45f..a04be8f 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/SecurityConfig.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/config/SecurityConfig.java @@ -4,6 +4,7 @@ import com.juliashtal.devanalytics.security.service.CustomUserDetailsService; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.AuthenticationProvider; @@ -99,6 +100,10 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti .authorizeHttpRequests(auth -> auth .requestMatchers(PUBLIC_ENDPOINTS).permitAll() .requestMatchers("/api/admin/**").hasRole("ADMIN") + // Ordered before the team rule below: the first matching pattern wins. Named + // in full rather than as a subtree — "me" is a literal where every sibling + // route takes a {teamId}, so a wildcard here would shadow all of them. + .requestMatchers(HttpMethod.GET, "/api/teams/me/memberships").authenticated() .requestMatchers("/api/teams/**").hasAnyRole("MANAGER", "ADMIN") .anyRequest().authenticated() ) diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricBackfillScheduler.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricBackfillScheduler.java index c667a85..68c6810 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricBackfillScheduler.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricBackfillScheduler.java @@ -11,9 +11,9 @@ * Drives {@link MetricBackfillService} over every user once a night, filling days inside each * user's collected history that have never been calculated. * - *

Separate from {@link MetricsScheduler}, which only computes yesterday. Runs at 03:00 UTC; - * ordering against that job is not a correctness requirement, because both write through the - * same {@link MetricsService#calculateDailyMetrics} upsert guard.

+ *

Separate from {@link MetricsScheduler}, which only computes yesterday. Runs at 03:00 UTC and + * takes {@link MetricWriteGate} first: the upsert guard both jobs share reads before it writes, so + * it orders writes but does not make concurrent ones safe.

*/ @Component @RequiredArgsConstructor @@ -22,9 +22,17 @@ public class MetricBackfillScheduler { private final UserRepository userRepository; private final MetricBackfillService backfillService; + private final MetricWriteGate writeGate; @Scheduled(cron = "0 0 3 * * ?", zone = "UTC") public void backfillAll() { + boolean ran = writeGate.runExclusively(this::backfillAllUsers); + if (!ran) { + log.info("History backfill job skipped: another metric writer is running"); + } + } + + private void backfillAllUsers() { log.info("History backfill job started"); userRepository.findAll().forEach(user -> { diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricWriteGate.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricWriteGate.java new file mode 100644 index 0000000..ac74964 --- /dev/null +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricWriteGate.java @@ -0,0 +1,40 @@ +package com.juliashtal.devanalytics.metrics.service; + +import org.springframework.stereotype.Component; + +import java.util.concurrent.locks.ReentrantLock; + +/** + * Serialises the scheduled jobs that write {@code metric_snapshots}; request threads, the + * {@code collect-} pool and the attribution listener still write outside it. + * + *

The table carries no unique key, so the guard in {@code MetricSnapshotWriter} is a + * read-then-write: two writers computing the same window both miss {@code findExisting} and both + * insert. A process-local lock narrows that to the scheduled writers; a unique index over the + * {@code findExisting} columns is what would close it for every caller.

+ */ +@Component +public class MetricWriteGate { + + private final ReentrantLock lock = new ReentrantLock(); + + /** + * Runs {@code job} only while no other writer holds the gate. + * + *

Skips rather than queues: each caller recomputes from persisted state, so the next run + * covers whatever this one declined.

+ * + * @return false when the job was skipped because another writer was running + */ + public boolean runExclusively(Runnable job) { + if (!lock.tryLock()) { + return false; + } + try { + job.run(); + return true; + } finally { + lock.unlock(); + } + } +} diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricsScheduler.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricsScheduler.java index b6d350c..d2d132c 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricsScheduler.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/service/MetricsScheduler.java @@ -24,10 +24,18 @@ public class MetricsScheduler { private final MetricsService metricsService; private final UserRepository userRepository; private final SystemClock systemClock; + private final MetricWriteGate writeGate; /** Runs every day at 01:00 UTC. */ @Scheduled(cron = "0 0 1 * * ?", zone = "UTC") public void calculateYesterday() { + boolean ran = writeGate.runExclusively(this::calculateYesterdayForAllUsers); + if (!ran) { + log.info("Nightly metrics scheduler skipped: another metric writer is running"); + } + } + + private void calculateYesterdayForAllUsers() { LocalDate yesterday = systemClock.yesterday(); log.info("Nightly metrics scheduler started for {}", yesterday); diff --git a/dev-analytics/src/main/resources/application.yml b/dev-analytics/src/main/resources/application.yml index bffd35a..b63ea66 100644 --- a/dev-analytics/src/main/resources/application.yml +++ b/dev-analytics/src/main/resources/application.yml @@ -1,4 +1,10 @@ spring: + task: + scheduling: + pool: + # Enough for the 2-minute enrichment pass to overrun without delaying the three + # nightly crons, which fire an hour apart. The default of 1 queues them behind it. + size: ${SCHEDULING_POOL_SIZE:4} servlet: multipart: max-file-size: 2MB @@ -41,6 +47,10 @@ app: refresh-expiration: 604800000 # 7 days password-reset: expiration: 3600000 # 1 hour + http: + # Bounds one outbound hop on the shared RestTemplate (Jira discovery and collection). + connect-timeout-ms: ${HTTP_CONNECT_TIMEOUT_MS:5000} + read-timeout-ms: ${HTTP_READ_TIMEOUT_MS:30000} base-url: ${APP_BASE_URL:http://localhost:8080} frontend-url: ${APP_FRONTEND_URL:http://localhost:5173} encryption: diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/MetricsSummarySchedulerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/MetricsSummarySchedulerTest.java index 5268d25..695e454 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/MetricsSummarySchedulerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/ai/MetricsSummarySchedulerTest.java @@ -4,6 +4,7 @@ import com.juliashtal.devanalytics.ai.scheduler.MetricsSummaryScheduler; import com.juliashtal.devanalytics.ai.service.MetricsAiService; import com.juliashtal.devanalytics.config.SystemClock; +import com.juliashtal.devanalytics.metrics.service.MetricWriteGate; import com.juliashtal.devanalytics.metrics.service.MetricsService; import com.juliashtal.devanalytics.notification.NotificationDispatchService; import com.juliashtal.devanalytics.user.model.User; @@ -61,7 +62,7 @@ void restoreZone() { @BeforeEach void setUp() { scheduler = new MetricsSummaryScheduler(userRepository, metricsService, metricsAiService, - notificationDispatch, new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC))); + notificationDispatch, new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC)), new MetricWriteGate()); } @Test @@ -117,6 +118,27 @@ void generateWeeklySummaries_oneUsersCalculationFailing_doesNotStopTheRest() { verify(metricsAiService, org.mockito.Mockito.never()).generateSummary(eq(failing), any(), any(), any()); } + /** + * A weekly job that skipped entirely on contention would leave the user with no brief for + * seven days, so only the refresh is allowed to be declined. + */ + @Test + void generateWeeklySummaries_anotherWriterHoldsGate_stillSummarisesStoredSnapshots() { + User user = user(1L); + MetricWriteGate busyGate = org.mockito.Mockito.mock(MetricWriteGate.class); + when(busyGate.runExclusively(any())).thenReturn(false); + scheduler = new MetricsSummaryScheduler(userRepository, metricsService, metricsAiService, + notificationDispatch, new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC)), busyGate); + when(userRepository.findAll()).thenReturn(List.of(user)); + when(metricsAiService.generateSummary(any(), any(), any(), any())) + .thenReturn(MetricsSummaryDto.builder().headline("h").build()); + + scheduler.generateWeeklySummaries(); + + org.mockito.Mockito.verifyNoInteractions(metricsService); + verify(metricsAiService).generateSummary(user, FROM, TO, null); + } + /** * The job fires at 08:00 UTC, an hour at which Auckland has already entered the next day and * Los Angeles is still in the previous one; the summarised week must not move with either. diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/config/RestTemplateConfigTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/config/RestTemplateConfigTest.java new file mode 100644 index 0000000..6165ffa --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/config/RestTemplateConfigTest.java @@ -0,0 +1,38 @@ +package com.juliashtal.devanalytics.config; + +import org.junit.jupiter.api.Test; +import org.springframework.http.client.ClientHttpRequestFactory; +import org.springframework.http.client.SimpleClientHttpRequestFactory; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.web.client.RestTemplate; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Pins that the configured timeouts reach the request factory, so the shared RestTemplate cannot + * wait forever on an unresponsive host. + * + *

The factory exposes no getters, so its fields are the only reachable evidence; a rename + * there fails this test loudly rather than silently restoring the unlimited default.

+ */ +class RestTemplateConfigTest { + + @Test + void restTemplate_configuredTimeouts_reachTheRequestFactory() { + RestTemplate restTemplate = new RestTemplateConfig().restTemplate(1_500, 9_000); + + ClientHttpRequestFactory factory = restTemplate.getRequestFactory(); + assertThat(factory).isInstanceOf(SimpleClientHttpRequestFactory.class); + assertThat(ReflectionTestUtils.getField(factory, "connectTimeout")).isEqualTo(1_500); + assertThat(ReflectionTestUtils.getField(factory, "readTimeout")).isEqualTo(9_000); + } + + @Test + void restTemplate_applicationDefaults_areFiniteAndPositive() { + RestTemplate restTemplate = new RestTemplateConfig().restTemplate(5_000, 30_000); + + ClientHttpRequestFactory factory = restTemplate.getRequestFactory(); + assertThat((Integer) ReflectionTestUtils.getField(factory, "connectTimeout")).isPositive(); + assertThat((Integer) ReflectionTestUtils.getField(factory, "readTimeout")).isPositive(); + } +} diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricBackfillSchedulerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricBackfillSchedulerTest.java index 9b31a67..ddc7088 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricBackfillSchedulerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricBackfillSchedulerTest.java @@ -2,6 +2,7 @@ import com.juliashtal.devanalytics.metrics.model.BackfillResult; import com.juliashtal.devanalytics.metrics.service.MetricBackfillScheduler; +import com.juliashtal.devanalytics.metrics.service.MetricWriteGate; import com.juliashtal.devanalytics.metrics.service.MetricBackfillService; import com.juliashtal.devanalytics.user.model.User; import com.juliashtal.devanalytics.user.repository.UserRepository; @@ -33,12 +34,23 @@ void backfillAll_everyUser_isBackfilled() { when(backfillService.backfillUser(anyLong())) .thenReturn(new BackfillResult(0, 0, LocalDate.now(), LocalDate.now())); - new MetricBackfillScheduler(userRepository, backfillService).backfillAll(); + new MetricBackfillScheduler(userRepository, backfillService, new MetricWriteGate()).backfillAll(); verify(backfillService).backfillUser(1L); verify(backfillService).backfillUser(2L); } + /** Without this, removing the gate from the scheduler leaves every other test here green. */ + @Test + void backfillAll_anotherWriterHoldsGate_backfillsNothing() { + MetricWriteGate busyGate = mock(MetricWriteGate.class); + when(busyGate.runExclusively(any())).thenReturn(false); + + new MetricBackfillScheduler(userRepository, backfillService, busyGate).backfillAll(); + + verifyNoInteractions(backfillService, userRepository); + } + @Test void backfillAll_oneUserFails_othersStillProcessed() { when(userRepository.findAll()) @@ -49,7 +61,7 @@ void backfillAll_oneUserFails_othersStillProcessed() { when(backfillService.backfillUser(3L)) .thenReturn(new BackfillResult(0, 0, LocalDate.now(), LocalDate.now())); - new MetricBackfillScheduler(userRepository, backfillService).backfillAll(); + new MetricBackfillScheduler(userRepository, backfillService, new MetricWriteGate()).backfillAll(); verify(backfillService).backfillUser(1L); verify(backfillService).backfillUser(3L); diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricWriteGateTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricWriteGateTest.java new file mode 100644 index 0000000..3e1407a --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricWriteGateTest.java @@ -0,0 +1,72 @@ +package com.juliashtal.devanalytics.metrics; + +import com.juliashtal.devanalytics.metrics.service.MetricWriteGate; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Pins that only one metric writer runs at a time, and that a declined run does not execute. + * + *

Contention is produced with a real second thread rather than a re-entrant call: the gate + * holds a {@link java.util.concurrent.locks.ReentrantLock}, so a same-thread attempt would be + * admitted and the test would pass without proving anything.

+ */ +class MetricWriteGateTest { + + @Test + void runExclusively_noContention_runsJobAndReturnsTrue() { + MetricWriteGate gate = new MetricWriteGate(); + AtomicBoolean ran = new AtomicBoolean(false); + + boolean result = gate.runExclusively(() -> ran.set(true)); + + assertThat(result).isTrue(); + assertThat(ran).isTrue(); + } + + @Test + void runExclusively_otherWriterHolding_skipsJobAndReturnsFalse() throws Exception { + MetricWriteGate gate = new MetricWriteGate(); + CountDownLatch holding = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicBoolean secondJobRan = new AtomicBoolean(false); + + Thread holder = new Thread(() -> gate.runExclusively(() -> { + holding.countDown(); + try { + release.await(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + })); + holder.start(); + assertThat(holding.await(5, TimeUnit.SECONDS)).isTrue(); + + boolean result = gate.runExclusively(() -> secondJobRan.set(true)); + + release.countDown(); + holder.join(5_000); + + assertThat(result).isFalse(); + assertThat(secondJobRan).isFalse(); + } + + @Test + void runExclusively_jobThrows_releasesGateForTheNextRun() { + MetricWriteGate gate = new MetricWriteGate(); + + assertThatThrownBy(() -> gate.runExclusively(() -> { + throw new IllegalStateException("boom"); + })).isInstanceOf(IllegalStateException.class); + + AtomicBoolean ran = new AtomicBoolean(false); + assertThat(gate.runExclusively(() -> ran.set(true))).isTrue(); + assertThat(ran).isTrue(); + } +} diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricsSchedulerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricsSchedulerTest.java index 544ef6e..eda36e6 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricsSchedulerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/MetricsSchedulerTest.java @@ -1,6 +1,7 @@ package com.juliashtal.devanalytics.metrics; import com.juliashtal.devanalytics.config.SystemClock; +import com.juliashtal.devanalytics.metrics.service.MetricWriteGate; import com.juliashtal.devanalytics.metrics.service.MetricsScheduler; import com.juliashtal.devanalytics.metrics.service.MetricsService; import com.juliashtal.devanalytics.user.model.User; @@ -48,7 +49,7 @@ void restoreZone() { private MetricsScheduler scheduler() { return new MetricsScheduler(metricsService, userRepository, - new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC))); + new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC)), new MetricWriteGate()); } private User userWithId(Long id) { @@ -110,6 +111,18 @@ void calculateYesterday_computesExactlyYesterdayToYesterday_neverAWiderRange() { assertThat(toCaptor.getValue()).isEqualTo(YESTERDAY); } + /** Without this, removing the gate from the scheduler leaves every other test here green. */ + @Test + void calculateYesterday_anotherWriterHoldsGate_computesNothing() { + MetricWriteGate busyGate = mock(MetricWriteGate.class); + when(busyGate.runExclusively(any())).thenReturn(false); + + new MetricsScheduler(metricsService, userRepository, + new SystemClock(Clock.fixed(FIXED, ZoneOffset.UTC)), busyGate).calculateYesterday(); + + verifyNoInteractions(metricsService, userRepository); + } + /** * At the fixed instant the two zones sit on opposite sides of the date line, so a server-zone * reading would produce 2026-03-14 in Auckland and 2026-03-13 in Los Angeles. diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/user/TeamMembershipsControllerTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/user/TeamMembershipsControllerTest.java index c0bc18a..e7db809 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/user/TeamMembershipsControllerTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/user/TeamMembershipsControllerTest.java @@ -1,5 +1,7 @@ package com.juliashtal.devanalytics.user; +import com.juliashtal.devanalytics.config.SecurityConfig; +import com.juliashtal.devanalytics.security.JwtAuthFilter; import com.juliashtal.devanalytics.security.service.CustomUserDetailsService; import com.juliashtal.devanalytics.security.service.JwtService; import com.juliashtal.devanalytics.user.controller.TeamController; @@ -8,9 +10,10 @@ import com.juliashtal.devanalytics.user.service.UserService; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Import; +import org.springframework.http.MediaType; import org.springframework.security.test.context.support.WithMockUser; import org.springframework.test.web.servlet.MockMvc; @@ -18,18 +21,29 @@ import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +/** + * Pins that a member with no managing role can read their own team memberships while the rest of + * {@code /api/teams} stays manager-only. + * + *

{@link SecurityConfig} is imported because the {@code authorizeHttpRequests} path rules are + * evaluated before any controller, and a slice without them cannot observe a rejection.

+ */ @WebMvcTest(TeamController.class) -@AutoConfigureMockMvc(addFilters = false) +@Import({SecurityConfig.class, JwtAuthFilter.class}) class TeamMembershipsControllerTest { @Autowired MockMvc mvc; + @MockBean TeamService teamService; - @MockBean UserService userService; + // Required by SecurityConfig / JwtAuthFilter when filters are active @MockBean JwtService jwtService; @MockBean CustomUserDetailsService customUserDetailsService; + // Required by ActivityInterceptor (HandlerInterceptor picked up by @WebMvcTest) + @MockBean UserService userService; @Test @WithMockUser(roles = "DEVELOPER") @@ -52,4 +66,45 @@ void getMyMemberships_noTeams_returnsEmptyList() throws Exception { .andExpect(status().isOk()) .andExpect(jsonPath("$").isEmpty()); } + + @Test + @WithMockUser(roles = "MANAGER") + void getMyMemberships_asManager_returns200() throws Exception { + when(teamService.getMyMemberships()).thenReturn(List.of()); + + mvc.perform(get("/api/teams/me/memberships")) + .andExpect(status().isOk()); + } + + @Test + void getMyMemberships_unauthenticated_returns401() throws Exception { + mvc.perform(get("/api/teams/me/memberships")) + .andExpect(status().isUnauthorized()); + } + + /** The self-scoped exemption must not widen to the manager-only endpoints beside it. */ + @Test + @WithMockUser(roles = "DEVELOPER") + void getMyTeams_asDeveloper_returns403() throws Exception { + mvc.perform(get("/api/teams")) + .andExpect(status().isForbidden()); + } + + /** + * Every sibling route binds {@code me} to a {@code Long} {@code teamId}, so a subtree + * exemption would hand these to the controller and let type conversion decide the outcome. + */ + @Test + @WithMockUser(roles = "DEVELOPER") + void renameTeamNamedMe_asDeveloper_returns403NotAConversionError() throws Exception { + mvc.perform(put("/api/teams/me").contentType(MediaType.APPLICATION_JSON).content("{}")) + .andExpect(status().isForbidden()); + } + + @Test + @WithMockUser(roles = "DEVELOPER") + void exportTeamNamedMe_asDeveloper_returns403() throws Exception { + mvc.perform(get("/api/teams/me/export").param("from", "2026-01-01").param("to", "2026-01-31")) + .andExpect(status().isForbidden()); + } }