From 826308427dcdf13564fdfff357c6e8f3081fee09 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:18:06 +0200 Subject: [PATCH 01/28] test(harness): add full-context @SpringBootTest integration harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds HeliosIntegrationTest — the first full-context integration-test base — booting the whole application against a real embedded Postgres (zonky) with the real Flyway schema and MockMvc, so tests drive the actual servlet stack (interceptors, OSIV, repositories). This is the harness for the tenant-isolation guard tests in the Option-B refactor: a scoping test written here exercises the same request path production uses, so it passes both with the legacy gitRepositoryFilter and with explicit per-query filtering after the migration. Completes the test profile in application-test.yml (dummy values for every no-default @Value placeholder; NATS/schedulers/reconciliation/notifications/AI disabled) and mocks the GitHub clients so the context boots without credentials or network. ContextBootIT smoke-tests that the context loads and a public GET responds. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/main/resources/application-test.yml | 47 ++++++++++++++++++- .../de/tum/cit/aet/helios/ContextBootIT.java | 15 ++++++ .../cit/aet/helios/HeliosIntegrationTest.java | 46 ++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/ContextBootIT.java create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/HeliosIntegrationTest.java diff --git a/server/application-server/src/main/resources/application-test.yml b/server/application-server/src/main/resources/application-test.yml index dbc49bbc3..06355a793 100644 --- a/server/application-server/src/main/resources/application-test.yml +++ b/server/application-server/src/main/resources/application-test.yml @@ -1,10 +1,53 @@ +# Profile used by full-context integration tests (HeliosIntegrationTest). Supplies dummy values for +# every no-default @Value placeholder and disables all external integrations / schedulers so the +# context boots without credentials, network, NATS, or a running scheduler. Not used in any real +# deployment (prod/staging/dev activate their own profiles). spring: data: jpa: repositories: - bootstrap-mode: deferred + # deferred needs an async bootstrap executor not wired in the test context + bootstrap-mode: default security: oauth2: resourceserver: jwt: - issuer-uri: "url.invalid" \ No newline at end of file + issuer-uri: "url.invalid" + flyway: + enabled: true + +nats: + enabled: false + server: "nats://localhost:4222" + auth: + token: "dummy" + durableConsumerName: "test-consumer" + consumerAckWaitSeconds: 300 + consumerInactiveThresholdMinutes: 45 + timeframe: 1440 + +github: + organizationName: "test-org" + authToken: "dummy-token" + appName: "test-app" + clientId: "dummy-client" + privateKeyPath: "dummy-key.pem" + tokenExchangeClientId: "dummy-exchange-client" + tokenExchangeClientSecret: "dummy-exchange-secret" + +monitoring: + repositories: "" + timeframe: 14 + repository-sync-cron: "0 0 4 * * *" + runOnStartup: false + runOnStartupCooldownInMinutes: 1440 + +notification: + enabled: false + +reconciliation: + enabled: false + +helios: + ai: + enabled: false diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/ContextBootIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/ContextBootIT.java new file mode 100644 index 000000000..ddd2ddf03 --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/ContextBootIT.java @@ -0,0 +1,15 @@ +package de.tum.cit.aet.helios; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import org.junit.jupiter.api.Test; + +/** Smoke test: the full application context boots and the servlet stack answers a public GET. */ +class ContextBootIT extends HeliosIntegrationTest { + + @Test + void contextLoadsAndApiResponds() throws Exception { + mockMvc.perform(get("/api/repository")).andExpect(status().isOk()); + } +} diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/HeliosIntegrationTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/HeliosIntegrationTest.java new file mode 100644 index 000000000..901f18f55 --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/HeliosIntegrationTest.java @@ -0,0 +1,46 @@ +package de.tum.cit.aet.helios; + +import de.tum.cit.aet.helios.github.GitHubClientManager; +import de.tum.cit.aet.helios.github.GitHubFacade; +import de.tum.cit.aet.helios.github.GitHubService; +import io.zonky.test.db.AutoConfigureEmbeddedDatabase; +import javax.sql.DataSource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +/** + * Base class for full-context integration tests. Boots the whole application against a real embedded + * PostgreSQL (zonky, via Docker) with the real Flyway schema, and exposes {@link MockMvc} so tests + * drive the actual servlet stack — interceptors, Open-Session-In-View, and the repository layer. + * + *

This is the harness for the tenant-isolation guard tests: it exercises the same request path that + * production uses, so a scoping test written here passes both with the legacy Hibernate + * {@code gitRepositoryFilter} and with explicit per-query filtering after the migration. + * + *

External integrations that would otherwise reach out on startup are neutralised: NATS is disabled + * ({@code nats.enabled=false}), the schedulers/reconciliation/notifications/AI are turned off, and the + * GitHub clients are mocked so no credentials or network are required. + */ +@SpringBootTest +@ActiveProfiles("test") +@AutoConfigureMockMvc +@AutoConfigureEmbeddedDatabase( + type = AutoConfigureEmbeddedDatabase.DatabaseType.POSTGRES, + provider = AutoConfigureEmbeddedDatabase.DatabaseProvider.DOCKER) +public abstract class HeliosIntegrationTest { + + public static final String X_REPOSITORY_ID = "X-REPOSITORY-ID"; + + @Autowired protected MockMvc mockMvc; + @Autowired protected DataSource dataSource; + + // GitHub clients reach out to GitHub on construction/first use; mock them so the context boots + // without credentials or network. Read (GET) scoping tests do not exercise GitHub. + @MockitoBean protected GitHubClientManager gitHubClientManager; + @MockitoBean protected GitHubFacade gitHubFacade; + @MockitoBean protected GitHubService gitHubService; +} From d20958deff5fca199c6b5b0944bf41447b927e93 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:21:10 +0200 Subject: [PATCH 02/28] test(scoping): guard that GET /api/environments* is scoped per repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds EnvironmentScopingIT (full-context MockMvc): seeds two repositories and asserts the enabled / all environment list endpoints return only the current repository's environments (via the X-REPOSITORY-ID header). Passes on the current gitRepositoryFilter-based code — the pre-migration baseline for the Environments endpoints. (getEnvironmentById scoping is deliberately not asserted here: Hibernate's @Filter does not apply to findById/PK loads, so it currently leaks cross-repo; that assertion + fix land together in the explicit-filtering commit.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../environment/EnvironmentScopingIT.java | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java new file mode 100644 index 000000000..fa0e8359c --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java @@ -0,0 +1,87 @@ +package de.tum.cit.aet.helios.environment; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.everyItem; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-refactor guard: {@code GET /api/environments*} must return only the current repository's + * environments (scoped by the {@code X-REPOSITORY-ID} header). This passes with the legacy + * Hibernate {@code gitRepositoryFilter} and must keep passing after the switch to explicit + * per-query filtering — it exercises the real request path (interceptor → OSIV → repository). + */ +class EnvironmentScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long ENV_A1 = 11L; + private static final long ENV_A2 = 12L; + private static final long ENV_B1 = 21L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertEnabledEnv(jdbc, ENV_A1, REPO_A, "a-staging"); + insertEnabledEnv(jdbc, ENV_A2, REPO_A, "a-production"); + insertEnabledEnv(jdbc, ENV_B1, REPO_B, "b-production"); + } + + @Test + void enabledEnvironmentsAreScopedToRepositoryA() throws Exception { + mockMvc + .perform(get("/api/environments/enabled").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + } + + @Test + void enabledEnvironmentsAreScopedToRepositoryB() throws Exception { + mockMvc + .perform(get("/api/environments/enabled").header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); + } + + @Test + void allEnvironmentsAreScopedToCurrentRepository() throws Exception { + mockMvc + .perform(get("/api/environments").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + } + + // NOTE: getEnvironmentById scoping is added together with the Phase-B migration. The Hibernate + // @Filter does not apply to findById()/PK loads, so today getEnvironmentById(cross-repo-id) leaks + // the other repo's environment; the explicit-filtering commit fixes that (red→green). + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertEnabledEnv(JdbcTemplate jdbc, long id, long repositoryId, String name) { + jdbc.update( + "INSERT INTO environment (id, repository_id, enabled, locked, name) " + + "VALUES (?, ?, true, false, ?)", + id, + repositoryId, + name); + } +} From 16234a70287ae4a5c190dd97d39ae814dc8c7141 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:24:01 +0200 Subject: [PATCH 03/28] refactor(env): explicit per-repository filtering for environment reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces reliance on the ambient gitRepositoryFilter in EnvironmentService with explicit RepositoryContext-based scoping (conditional: scoped when a repo context is present, unscoped when absent — preserving today's header-conditional behavior). getAllEnvironments / getAllEnabledEnvironments now use new scoped finders; getEnvironmentById / getEnvironmentTypeById go through findScopedById, which also closes a latent cross-repo read (Hibernate @Filter never applied to findById/PK loads). EnvironmentScopingIT's list guards stay green (behavior preserved) and the re-added environmentByIdIsInvisibleFromAnotherRepository test now passes (leak fixed). Existing EnvironmentServiceTest still green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../environment/EnvironmentRepository.java | 7 +++++ .../environment/EnvironmentService.java | 31 ++++++++++++++++--- .../environment/EnvironmentScopingIT.java | 15 +++++++-- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java index 76ab78f89..c666778e1 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java @@ -24,6 +24,13 @@ public interface EnvironmentRepository extends JpaRepository List findByEnabledTrueOrderByNameAsc(); + // Explicit per-repository scoped finders (Option B: replaces the ambient gitRepositoryFilter). + List findByRepositoryRepositoryIdOrderByNameAsc(Long repositoryId); + + List findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc(Long repositoryId); + + Optional findByIdAndRepositoryRepositoryId(Long id, Long repositoryId); + @Query("SELECT DISTINCT e FROM Environment e " + "LEFT JOIN FETCH e.statusHistory es " + "WHERE (es is NULL OR es.checkTimestamp = " diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java index 4186a9f0a..46d33698f 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java @@ -77,15 +77,32 @@ public class EnvironmentService { private final NatsNotificationPublisherService notificationPublisherService; public Optional getEnvironmentById(Long id) { - return environmentRepository.findById(id).map(EnvironmentDto::fromEnvironment); + return findScopedById(id).map(EnvironmentDto::fromEnvironment); } public Optional getEnvironmentTypeById(Long id) { - return environmentRepository.findById(id).map(Environment::getType); + return findScopedById(id).map(Environment::getType); + } + + /** + * Loads an environment by id, scoped to the current repository when a repository context is + * present. Explicit replacement for the ambient gitRepositoryFilter, which never applied to + * findById/PK loads — so this also closes a latent cross-repository read. + */ + private Optional findScopedById(Long id) { + Long repositoryId = RepositoryContext.getRepositoryId(); + return repositoryId == null + ? environmentRepository.findById(id) + : environmentRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } public List getAllEnvironments() { - return environmentRepository.findAllByOrderByNameAsc().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + List environments = + repositoryId == null + ? environmentRepository.findAllByOrderByNameAsc() + : environmentRepository.findByRepositoryRepositoryIdOrderByNameAsc(repositoryId); + return environments.stream() .map( environment -> { LatestDeploymentUnion latest = findLatestDeployment(environment); @@ -98,7 +115,13 @@ public List getAllEnvironments() { } public List getAllEnabledEnvironments() { - return environmentRepository.findByEnabledTrueOrderByNameAsc().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + List environments = + repositoryId == null + ? environmentRepository.findByEnabledTrueOrderByNameAsc() + : environmentRepository.findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc( + repositoryId); + return environments.stream() .map( environment -> { LatestDeploymentUnion latest = findLatestDeployment(environment); diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java index fa0e8359c..aceea382a 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java @@ -63,9 +63,18 @@ void allEnvironmentsAreScopedToCurrentRepository() throws Exception { .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); } - // NOTE: getEnvironmentById scoping is added together with the Phase-B migration. The Hibernate - // @Filter does not apply to findById()/PK loads, so today getEnvironmentById(cross-repo-id) leaks - // the other repo's environment; the explicit-filtering commit fixes that (red→green). + @Test + void environmentByIdIsInvisibleFromAnotherRepository() throws Exception { + // ENV_B1 belongs to repo B: invisible when scoped to repo A, visible when scoped to repo B. + mockMvc + .perform( + get("/api/environments/{id}", ENV_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + mockMvc + .perform( + get("/api/environments/{id}", ENV_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()); + } private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { jdbc.update( From 1557a7c1d2181c7f857a396bc4d36d1c6e5a73fb Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:32:02 +0200 Subject: [PATCH 04/28] refactor(branch): explicit per-repository filtering for GET /api/branches BranchService.getAllBranches now scopes explicitly via RepositoryContext (conditional: scoped when a repo context is present, unscoped otherwise) using the existing findByRepositoryRepositoryId finder, instead of relying on the ambient gitRepositoryFilter over findAll(). Adds BranchScopingIT (green on the old filter-based code and after the migration); existing BranchServiceTest still green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../cit/aet/helios/branch/BranchService.java | 8 ++- .../aet/helios/branch/BranchScopingIT.java | 69 +++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java index 8e03f0de8..e8ce2fcee 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java @@ -2,6 +2,7 @@ import de.tum.cit.aet.helios.auth.AuthService; import de.tum.cit.aet.helios.commit.CommitRepository; +import de.tum.cit.aet.helios.filters.RepositoryContext; import de.tum.cit.aet.helios.releaseinfo.releasecandidate.ReleaseCandidate; import de.tum.cit.aet.helios.releaseinfo.releasecandidate.ReleaseCandidateRepository; import de.tum.cit.aet.helios.userpreference.UserPreference; @@ -32,7 +33,12 @@ public List getAllBranches(String sortField, String sortDirection ? userPreferenceRepository.findByUser(authService.getUserFromGithubId()) : Optional.empty(); Comparator comparator = buildBranchInfoDtoComparator(sortField, sortDirection); - return branchRepository.findAll().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + List branches = + repositoryId == null + ? branchRepository.findAll() + : branchRepository.findByRepositoryRepositoryId(repositoryId); + return branches.stream() .map((branch) -> BranchInfoDto.fromBranchAndUserPreference(branch, userPreference, commitRepository)) .sorted(comparator) diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java new file mode 100644 index 000000000..f1ec85d89 --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java @@ -0,0 +1,69 @@ +package de.tum.cit.aet.helios.branch; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.everyItem; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-refactor guard: {@code GET /api/branches} must return only the current repository's + * branches (scoped by {@code X-REPOSITORY-ID}). Passes with the legacy gitRepositoryFilter and must + * keep passing after the switch to explicit per-query filtering. + */ +class BranchScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertBranch(jdbc, REPO_A, "a-main"); + insertBranch(jdbc, REPO_A, "a-feature"); + insertBranch(jdbc, REPO_B, "b-main"); + } + + @Test + void branchesAreScopedToRepositoryA() throws Exception { + mockMvc + .perform(get("/api/branches").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + } + + @Test + void branchesAreScopedToRepositoryB() throws Exception { + mockMvc + .perform(get("/api/branches").header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertBranch(JdbcTemplate jdbc, long repositoryId, String name) { + jdbc.update( + "INSERT INTO branch (repository_id, name, ahead_by, behind_by, is_default, protection) " + + "VALUES (?, ?, 0, 0, false, false)", + repositoryId, + name); + } +} From e6ab21f7c73ca838989f7e0f2a9c5d3e0e20b7e0 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:41:04 +0200 Subject: [PATCH 05/28] =?UTF-8?q?refactor(env,branch):=20never=20findAll?= =?UTF-8?q?=20for=20tenant=20reads=20=E2=80=94=20empty=20on=20missing=20re?= =?UTF-8?q?po=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: findAll() loads every repository's rows and must never back a tenant-scoped read. Drops the findAll() fallback from EnvironmentService.getAll(Enabled)Environments and BranchService.getAllBranches; when no repository context is present these now return an empty list (the scoped finder is the only DB path). Adds no-context guard tests locking that behavior, and updates the service unit tests to the new contract (set a RepositoryContext, mock the scoped finder, clear in @AfterEach). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../cit/aet/helios/branch/BranchService.java | 9 ++++---- .../environment/EnvironmentService.java | 21 +++++++++---------- .../aet/helios/branch/BranchScopingIT.java | 9 ++++++++ .../aet/helios/branch/BranchServiceTest.java | 10 ++++++++- .../environment/EnvironmentScopingIT.java | 13 ++++++++++++ .../environment/EnvironmentServiceTest.java | 17 +++++++++++---- 6 files changed, 58 insertions(+), 21 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java index e8ce2fcee..26123d1e1 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java @@ -34,11 +34,10 @@ public List getAllBranches(String sortField, String sortDirection : Optional.empty(); Comparator comparator = buildBranchInfoDtoComparator(sortField, sortDirection); Long repositoryId = RepositoryContext.getRepositoryId(); - List branches = - repositoryId == null - ? branchRepository.findAll() - : branchRepository.findByRepositoryRepositoryId(repositoryId); - return branches.stream() + if (repositoryId == null) { + return List.of(); + } + return branchRepository.findByRepositoryRepositoryId(repositoryId).stream() .map((branch) -> BranchInfoDto.fromBranchAndUserPreference(branch, userPreference, commitRepository)) .sorted(comparator) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java index 46d33698f..5bfaa11e1 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java @@ -98,11 +98,10 @@ private Optional findScopedById(Long id) { public List getAllEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); - List environments = - repositoryId == null - ? environmentRepository.findAllByOrderByNameAsc() - : environmentRepository.findByRepositoryRepositoryIdOrderByNameAsc(repositoryId); - return environments.stream() + if (repositoryId == null) { + return List.of(); + } + return environmentRepository.findByRepositoryRepositoryIdOrderByNameAsc(repositoryId).stream() .map( environment -> { LatestDeploymentUnion latest = findLatestDeployment(environment); @@ -116,12 +115,12 @@ public List getAllEnvironments() { public List getAllEnabledEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); - List environments = - repositoryId == null - ? environmentRepository.findByEnabledTrueOrderByNameAsc() - : environmentRepository.findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc( - repositoryId); - return environments.stream() + if (repositoryId == null) { + return List.of(); + } + return environmentRepository + .findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc(repositoryId) + .stream() .map( environment -> { LatestDeploymentUnion latest = findLatestDeployment(environment); diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java index f1ec85d89..0ecd93777 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchScopingIT.java @@ -50,6 +50,15 @@ void branchesAreScopedToRepositoryB() throws Exception { .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); } + @Test + void withoutRepositoryContextReturnsEmpty() throws Exception { + // No X-REPOSITORY-ID header → a tenant-scoped read has no repository → empty (never findAll). + mockMvc + .perform(get("/api/branches")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + } + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { jdbc.update( "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchServiceTest.java index 8b208803a..cb552dc00 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/branch/BranchServiceTest.java @@ -6,6 +6,7 @@ import de.tum.cit.aet.helios.auth.AuthService; import de.tum.cit.aet.helios.commit.CommitRepository; +import de.tum.cit.aet.helios.filters.RepositoryContext; import de.tum.cit.aet.helios.gitrepo.GitRepository; import de.tum.cit.aet.helios.releaseinfo.releasecandidate.ReleaseCandidateRepository; import de.tum.cit.aet.helios.userpreference.UserPreference; @@ -14,6 +15,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -44,6 +46,7 @@ public class BranchServiceTest { @BeforeEach public void setUp() { + RepositoryContext.setRepositoryId("1"); GitRepository repo = new GitRepository(); repo.setRepositoryId(1L); @@ -63,10 +66,15 @@ public void setUp() { b3.setUpdatedAt(OffsetDateTime.parse("2025-03-15T10:00:00Z")); when(commitRepository.findByShaAndRepository(any(), any())).thenReturn(Optional.empty()); - when(branchRepository.findAll()).thenReturn(List.of(b1, b2, b3)); + when(branchRepository.findByRepositoryRepositoryId(1L)).thenReturn(List.of(b1, b2, b3)); when(authService.isLoggedIn()).thenReturn(false); } + @AfterEach + public void tearDown() { + RepositoryContext.clear(); + } + @Test public void testPinnedBranchesAreShownFirst() { final UserPreference userPreference = new UserPreference(); diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java index aceea382a..a624d80d8 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java @@ -76,6 +76,19 @@ void environmentByIdIsInvisibleFromAnotherRepository() throws Exception { .andExpect(status().isOk()); } + @Test + void withoutRepositoryContextReturnsEmpty() throws Exception { + // No X-REPOSITORY-ID header → a tenant-scoped read has no repository → empty (never findAll). + mockMvc + .perform(get("/api/environments/enabled")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + mockMvc + .perform(get("/api/environments")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + } + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { jdbc.update( "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java index d662f2a54..49ac8ff7f 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java @@ -36,6 +36,7 @@ import java.time.OffsetDateTime; import java.util.List; import java.util.Optional; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -86,6 +87,11 @@ public void setUp() { user.setId(1L); } + @AfterEach + public void tearDown() { + RepositoryContext.clear(); + } + @Test public void testGetEnvironmentById() { when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); @@ -109,7 +115,8 @@ public void testGetEnvironmentTypeById() { @Test public void testGetAllEnvironments() { - when(environmentRepository.findAllByOrderByNameAsc()) + RepositoryContext.setRepositoryId("1"); + when(environmentRepository.findByRepositoryRepositoryIdOrderByNameAsc(1L)) .thenReturn(List.of(environment, environment)); List result = environmentService.getAllEnvironments(); @@ -118,12 +125,13 @@ public void testGetAllEnvironments() { assertEquals(2, result.size()); assertEquals(List.of(dto, dto), result); - verify(environmentRepository, times(1)).findAllByOrderByNameAsc(); + verify(environmentRepository, times(1)).findByRepositoryRepositoryIdOrderByNameAsc(1L); } @Test public void testGetAllEnabledEnvironments() { - when(environmentRepository.findByEnabledTrueOrderByNameAsc()) + RepositoryContext.setRepositoryId("1"); + when(environmentRepository.findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc(1L)) .thenReturn(List.of(environment, environment)); List result = environmentService.getAllEnabledEnvironments(); @@ -132,7 +140,8 @@ public void testGetAllEnabledEnvironments() { assertEquals(2, result.size()); assertEquals(List.of(dto, dto), result); - verify(environmentRepository, times(1)).findByEnabledTrueOrderByNameAsc(); + verify(environmentRepository, times(1)) + .findByEnabledTrueAndRepositoryRepositoryIdOrderByNameAsc(1L); } @Test From 0738587c835ad4288aa539ba7ee4f536e22fe1b0 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 22:44:38 +0200 Subject: [PATCH 06/28] refactor(workflow): explicit per-repository filtering for workflow reads WorkflowService.getAllWorkflows / getWorkflowsByState now scope explicitly via RepositoryContext (empty when no repo context, never findAll) using new scoped finders; getWorkflowById goes through findScopedById (closing the findById/PK cross-repo leak). Adds WorkflowScopingIT covering the list, state, by-id, and no-context cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../helios/workflow/WorkflowRepository.java | 7 ++ .../aet/helios/workflow/WorkflowService.java | 29 +++++- .../helios/workflow/WorkflowScopingIT.java | 91 +++++++++++++++++++ 3 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowScopingIT.java diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRepository.java index cadf93d46..221608df1 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRepository.java @@ -3,6 +3,7 @@ import de.tum.cit.aet.helios.gitrepo.GitRepository; import de.tum.cit.aet.helios.workflow.Workflow.Label; import java.util.List; +import java.util.Optional; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.Query; import org.springframework.data.repository.query.Param; @@ -16,6 +17,12 @@ public interface WorkflowRepository extends JpaRepository { List findByStateOrderByCreatedAtDesc(Workflow.State state); + // Explicit per-repository scoped finders (Option B). + List findByStateAndRepositoryRepositoryIdOrderByCreatedAtDesc( + Workflow.State state, Long repositoryId); + + Optional findByIdAndRepositoryRepositoryId(Long id, Long repositoryId); + List findByLabelAndRepositoryRepositoryId(Label label, Long repositoryId); Workflow findFirstByLabelAndRepositoryRepositoryIdOrderByCreatedAtDesc( diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java index 69e4d708b..4955a03ae 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java @@ -1,5 +1,6 @@ package de.tum.cit.aet.helios.workflow; +import de.tum.cit.aet.helios.filters.RepositoryContext; import jakarta.transaction.Transactional; import java.util.List; import java.util.Optional; @@ -14,11 +15,27 @@ public class WorkflowService { private final WorkflowRepository workflowRepository; public Optional getWorkflowById(Long id) { - return workflowRepository.findById(id).map(WorkflowDto::fromWorkflow); + return findScopedById(id).map(WorkflowDto::fromWorkflow); + } + + /** + * Loads a workflow by id, scoped to the current repository when a repository context is present. + * Explicit replacement for the ambient gitRepositoryFilter, which never applied to findById/PK + * loads — so this also closes a latent cross-repository read. + */ + private Optional findScopedById(Long id) { + Long repositoryId = RepositoryContext.getRepositoryId(); + return repositoryId == null + ? workflowRepository.findById(id) + : workflowRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } public List getAllWorkflows() { - return workflowRepository.findAll().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return List.of(); + } + return workflowRepository.findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId).stream() .map(WorkflowDto::fromWorkflow) .collect(Collectors.toList()); } @@ -32,7 +49,13 @@ public List getWorkflowsByRepositoryId(Long repositoryId) { } public List getWorkflowsByState(Workflow.State state) { - return workflowRepository.findByStateOrderByCreatedAtDesc(state).stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return List.of(); + } + return workflowRepository + .findByStateAndRepositoryRepositoryIdOrderByCreatedAtDesc(state, repositoryId) + .stream() .map(WorkflowDto::fromWorkflow) .collect(Collectors.toList()); } diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowScopingIT.java new file mode 100644 index 000000000..00c249f7d --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowScopingIT.java @@ -0,0 +1,91 @@ +package de.tum.cit.aet.helios.workflow; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.everyItem; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-refactor guard: {@code GET /api/workflows*} must return only the current repository's + * workflows (scoped by {@code X-REPOSITORY-ID}). Passes with the legacy gitRepositoryFilter and + * after the switch to explicit per-query filtering; also asserts the {@code findById} path no longer + * leaks cross-repo and that a missing repository context yields an empty list (never findAll). + */ +class WorkflowScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long WF_A1 = 11L; + private static final long WF_A2 = 12L; + private static final long WF_B1 = 21L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertWorkflow(jdbc, WF_A1, REPO_A, "CI"); + insertWorkflow(jdbc, WF_A2, REPO_A, "Deploy"); + insertWorkflow(jdbc, WF_B1, REPO_B, "CI"); + } + + @Test + void workflowsAreScopedToRepositoryA() throws Exception { + mockMvc + .perform(get("/api/workflows").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + } + + @Test + void workflowsByStateAreScopedToRepositoryA() throws Exception { + mockMvc + .perform(get("/api/workflows/state/ACTIVE").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + } + + @Test + void workflowByIdIsInvisibleFromAnotherRepository() throws Exception { + mockMvc + .perform(get("/api/workflows/{id}", WF_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + mockMvc + .perform(get("/api/workflows/{id}", WF_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()); + } + + @Test + void withoutRepositoryContextReturnsEmpty() throws Exception { + mockMvc + .perform(get("/api/workflows")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertWorkflow(JdbcTemplate jdbc, long id, long repositoryId, String name) { + jdbc.update( + "INSERT INTO workflow (id, repository_id, name, state) VALUES (?, ?, ?, 'ACTIVE')", + id, + repositoryId, + name); + } +} From e11d967a00a6c20f922707b578695f8c067330be Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:30:10 +0200 Subject: [PATCH 07/28] refactor(deployment): explicit per-repository filtering for deployment reads DeploymentService.getAllDeployments (empty on no repo context, never findAll) and getDeploymentById (scoped findById, closing the PK cross-repo leak) now filter explicitly via RepositoryContext using new scoped finders. DeploymentServiceTest updated to the new contract. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../helios/deployment/DeploymentRepository.java | 5 +++++ .../aet/helios/deployment/DeploymentService.java | 14 ++++++++++++-- .../helios/deployment/DeploymentServiceTest.java | 8 +++++--- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentRepository.java index 1fadfc185..7cb2ebc7a 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentRepository.java @@ -18,6 +18,11 @@ public interface DeploymentRepository extends JpaRepository { List findByRepositoryRepositoryIdAndSha(Long repositoryId, String sha); + // Explicit per-repository scoped finders (Option B). + List findByRepositoryRepositoryIdOrderByCreatedAtDesc(Long repositoryId); + + Optional findByIdAndRepositoryRepositoryId(Long id, Long repositoryId); + List findByRepositoryRepositoryIdAndRefOrderByCreatedAtDesc( Long repositoryId, String ref); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java index 633ac47a7..3622addeb 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java @@ -60,11 +60,21 @@ public class DeploymentService { private final WorkflowRunRepository workflowRunRepository; public Optional getDeploymentById(Long id) { - return deploymentRepository.findById(id).map(DeploymentDto::fromDeployment); + Long repositoryId = RepositoryContext.getRepositoryId(); + Optional deployment = + repositoryId == null + ? deploymentRepository.findById(id) + : deploymentRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); + return deployment.map(DeploymentDto::fromDeployment); } public List getAllDeployments() { - return deploymentRepository.findAll().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return List.of(); + } + return deploymentRepository.findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) + .stream() .map(DeploymentDto::fromDeployment) .collect(Collectors.toList()); } diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java index f21c5581e..ec7047131 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java @@ -100,7 +100,8 @@ public void tearDown() { @Test public void testGetDeploymentById() { - when(deploymentRepository.findById(1L)).thenReturn(Optional.of(deployment)); + when(deploymentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(deployment)); Optional result = deploymentService.getDeploymentById(1L); @@ -109,13 +110,14 @@ public void testGetDeploymentById() { assertTrue(result.isPresent()); assertEquals(deploymentDto, result.get()); - verify(deploymentRepository, times(1)).findById(1L); + verify(deploymentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test public void testGetAllDeployments() { - when(deploymentRepository.findAll()).thenReturn(List.of(deployment, deployment)); + when(deploymentRepository.findByRepositoryRepositoryIdOrderByCreatedAtDesc(1L)) + .thenReturn(List.of(deployment, deployment)); List result = deploymentService.getAllDeployments(); From b9233f8c6331aed5ecc3b434f6c4c430435c1a32 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:35:33 +0200 Subject: [PATCH 08/28] refactor(workflow-run): remove dead unscoped getAllWorkflowRuns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getAllWorkflowRuns() (findAll, no repository scope) had no callers. The live workflow-run reads (getWorkflowRunById, getPaginatedWorkflowRuns, getLatestWorkflowRunsBy*) are already repository-scoped via RepositoryContext, so no migration is needed there — just delete the dead method. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../de/tum/cit/aet/helios/workflow/WorkflowRunService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java index b67b2af94..de01322c2 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java @@ -41,10 +41,6 @@ public class WorkflowRunService { private final GitRepoRepository gitRepoRepository; private final TestSuiteRepository testSuiteRepository; - public List getAllWorkflowRuns() { - return workflowRunRepository.findAll(); - } - private Stream getLatestWorkflowRuns(List runs) { return runs.stream() .collect(Collectors.groupingBy(WorkflowRun::getWorkflowId)) From 342ad2a2f3f50271233d52b03d9fb52c05f9f0de Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:35:33 +0200 Subject: [PATCH 09/28] refactor(pull-request): explicit per-repository filtering for pull-request reads getAllPullRequests (empty on no context, never findAll) and getPullRequestById (scoped findById, closing the PK cross-repo leak) now filter explicitly via RepositoryContext. getPaginatedPullRequests now adds an explicit repository predicate to the non-pinned Specification (it previously leaned on the ambient filter) and short-circuits to an empty page when there is no repo context. PullRequestServiceTest updated to set/clear a RepositoryContext and mock the scoped finder. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pullrequest/PullRequestRepository.java | 2 ++ .../pullrequest/PullRequestService.java | 25 ++++++++++++++++--- .../pullrequest/PullRequestServiceTest.java | 16 +++++++++++- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java index bcb7273dc..0755b888f 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java @@ -42,6 +42,8 @@ List findByRepositoryRepositoryIdAndStateOrderByUpdatedAtDesc( List findByRepositoryRepositoryIdOrderByUpdatedAtDesc(Long repositoryId); + Optional findByIdAndRepositoryRepositoryId(Long id, Long repositoryId); + Optional findByRepositoryRepositoryIdAndNumber(Long repositoryId, Integer number); @Query( diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java index 7c2d56563..fe41e1c82 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java @@ -48,7 +48,12 @@ public List getAllPullRequests() { authService.isLoggedIn() ? userPreferenceRepository.findByUser(authService.getUserFromGithubId()) : Optional.empty(); - return pullRequestRepository.findAllByOrderByUpdatedAtDesc().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return List.of(); + } + return pullRequestRepository.findByRepositoryRepositoryIdOrderByUpdatedAtDesc(repositoryId) + .stream() .map((pr) -> PullRequestBaseInfoDto.fromPullRequestAndUserPreference(pr, userPreference)) .sorted( (pr1, pr2) -> { @@ -89,6 +94,10 @@ public PaginatedPullRequestsResponse getPaginatedPullRequests( : Optional.empty(); Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return new PaginatedPullRequestsResponse( + List.of(), List.of(), pageRequest.getPage(), pageRequest.getSize(), 0, 0); + } /* ---------- pinned ---------- */ List pinnedDtos = @@ -103,7 +112,7 @@ public PaginatedPullRequestsResponse getPaginatedPullRequests( /* ---------- non-pinned ---------- */ Specification nonPinnedSpec = - buildNonPinnedPullRequestSpecification(pageRequest, currentUserId); + buildNonPinnedPullRequestSpecification(pageRequest, currentUserId, repositoryId); long totalNonPinned = pullRequestRepository.count(nonPinnedSpec); log.debug( @@ -137,11 +146,14 @@ public PaginatedPullRequestsResponse getPaginatedPullRequests( } private Specification buildNonPinnedPullRequestSpecification( - PullRequestPageRequest pageRequest, String currentUserId) { + PullRequestPageRequest pageRequest, String currentUserId, Long repositoryId) { return (root, query, cb) -> { query.distinct(true); List predicates = new ArrayList<>(); + // Scope to the current repository (explicit; replaces the ambient gitRepositoryFilter). + predicates.add(cb.equal(root.get("repository").get("repositoryId"), repositoryId)); + // Add predicate for pinned status if (currentUserId != null) { // Create a subquery to find pinned PRs @@ -302,7 +314,12 @@ private Sort resolveSort(PullRequestPageRequest pageRequest) { } public Optional getPullRequestById(Long id) { - return pullRequestRepository.findById(id).map(PullRequestInfoDto::fromPullRequest); + Long repositoryId = RepositoryContext.getRepositoryId(); + Optional pullRequest = + repositoryId == null + ? pullRequestRepository.findById(id) + : pullRequestRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); + return pullRequest.map(PullRequestInfoDto::fromPullRequest); } public List getPullRequestByRepositoryId(Long repositoryId) { diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestServiceTest.java index e3d4a3c8e..f1eaa11d2 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestServiceTest.java @@ -8,6 +8,7 @@ import static org.mockito.Mockito.when; import de.tum.cit.aet.helios.auth.AuthService; +import de.tum.cit.aet.helios.filters.RepositoryContext; import de.tum.cit.aet.helios.gitrepo.GitRepository; import de.tum.cit.aet.helios.gitrepo.GitRepository.Visibility; import de.tum.cit.aet.helios.label.Label; @@ -19,6 +20,8 @@ import java.util.List; import java.util.Optional; import java.util.Set; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; @@ -39,6 +42,16 @@ public class PullRequestServiceTest { @Mock private UserPreferenceRepository userPreferenceRepository; @Mock private AuthService authService; + @BeforeEach + public void setUp() { + RepositoryContext.setRepositoryId("1"); + } + + @AfterEach + public void tearDown() { + RepositoryContext.clear(); + } + @Test public void testPinnedBranchesAreShownFirst() { final GitRepository repo = new GitRepository(); @@ -59,7 +72,8 @@ public void testPinnedBranchesAreShownFirst() { final UserPreference userPreference = new UserPreference(); userPreference.setFavouritePullRequests(Set.of(pr2)); - when(pullRequestsRepository.findAllByOrderByUpdatedAtDesc()).thenReturn(prs); + when(pullRequestsRepository.findByRepositoryRepositoryIdOrderByUpdatedAtDesc(1L)) + .thenReturn(prs); when(authService.isLoggedIn()).thenReturn(true); when(authService.getUserFromGithubId()).thenReturn(null); when(userPreferenceRepository.findByUser(null)).thenReturn(Optional.of(userPreference)); From 9db96af5c15967e57bdd712de084932d82f8fcad Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:38:45 +0200 Subject: [PATCH 10/28] refactor(release-info): explicit per-repository filtering for getAllReleaseInfos Replaces the unscoped findAllByOrderByCreatedAtDesc with a repository-scoped finder via RepositoryContext (empty on no context, never findAll). Other ReleaseInfoService reads already scope explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../cit/aet/helios/releaseinfo/ReleaseInfoService.java | 8 +++++++- .../releasecandidate/ReleaseCandidateRepository.java | 2 ++ .../aet/helios/releaseinfo/ReleaseInfoServiceTest.java | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java index d7f177000..a67fc2ae3 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java @@ -65,7 +65,13 @@ public class ReleaseInfoService { private final GitHubReleaseSyncService gitHubReleaseSyncService; public List getAllReleaseInfos() { - return releaseCandidateRepository.findAllByOrderByCreatedAtDesc().stream() + Long repositoryId = RepositoryContext.getRepositoryId(); + if (repositoryId == null) { + return List.of(); + } + return releaseCandidateRepository + .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) + .stream() .map(ReleaseInfoListDto::fromReleaseCandidate) .toList(); } diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidateRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidateRepository.java index 25990f742..5b97a00a2 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidateRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidateRepository.java @@ -12,6 +12,8 @@ public interface ReleaseCandidateRepository extends JpaRepository findByRepository(GitRepository repository); + List findByRepositoryRepositoryIdOrderByCreatedAtDesc(Long repositoryId); + Optional findByRepositoryRepositoryIdAndName(Long repositoryId, String name); Optional findByRepositoryRepositoryIdAndReleaseId( diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoServiceTest.java index b70deb381..ceee40cb5 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoServiceTest.java @@ -459,7 +459,8 @@ void getAllReleaseInfos_returnsMappedList() { commit2.setSha("dummySha2"); rc2.setCommit(commit2); - when(releaseCandidateRepository.findAllByOrderByCreatedAtDesc()).thenReturn(List.of(rc2, rc1)); + when(releaseCandidateRepository.findByRepositoryRepositoryIdOrderByCreatedAtDesc(repoId)) + .thenReturn(List.of(rc2, rc1)); // Act List result = service.getAllReleaseInfos(); From 57341d69854f6c9da314cb3cdf80d1341b7997b8 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:46:30 +0200 Subject: [PATCH 11/28] =?UTF-8?q?refactor(tenancy):=20remove=20the=20Hiber?= =?UTF-8?q?nate=20gitRepositoryFilter=20=E2=80=94=20scoping=20is=20now=20f?= =?UTF-8?q?ully=20explicit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With every repository-scoped web read migrated to explicit RepositoryContext-based filtering, the ambient Hibernate filter is no longer relied upon anywhere, so remove it entirely: - drop @Filter from RepositoryFilterEntity and the six directly-annotated entities (Branch, Label, GitRepository, ReleaseCandidate, TestCaseStatistics, TestCaseFlakiness); - delete the @FilterDef (package-info.java) and RepositoryFilterTransactionConfig; - RepositoryInterceptor now only publishes/clears the repository id (from X-REPOSITORY-ID); it clears the context when no header is present (correct semantics + defends against a stale ThreadLocal on a reused worker thread), and SecurityConfig no longer needs the OSIV ordering; - delete the obsolete RepositoryTenantFilterIntegrationTest (the endpoint scoping ITs cover this); - add a missing RepositoryContext @AfterEach clear in WorkflowRunServiceTest (test hygiene). RepositoryContext + RepositoryFilterEntity (as the base holding the repository association) + the X-REPOSITORY-ID header remain. Full server test suite (432 tests) green with the filter gone. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../de/tum/cit/aet/helios/branch/Branch.java | 2 - .../cit/aet/helios/config/SecurityConfig.java | 5 +- .../filters/RepositoryFilterEntity.java | 2 - .../RepositoryFilterTransactionConfig.java | 64 -------- .../helios/filters/RepositoryInterceptor.java | 53 ++----- .../cit/aet/helios/gitrepo/GitRepository.java | 2 - .../de/tum/cit/aet/helios/label/Label.java | 2 - .../de/tum/cit/aet/helios/package-info.java | 5 - .../releasecandidate/ReleaseCandidate.java | 2 - .../aet/helios/tests/TestCaseFlakiness.java | 2 - .../aet/helios/tests/TestCaseStatistics.java | 2 - ...RepositoryTenantFilterIntegrationTest.java | 149 ------------------ .../workflow/WorkflowRunServiceTest.java | 6 + 13 files changed, 20 insertions(+), 276 deletions(-) delete mode 100644 server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterTransactionConfig.java delete mode 100644 server/application-server/src/main/java/de/tum/cit/aet/helios/package-info.java delete mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/filters/RepositoryTenantFilterIntegrationTest.java diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/Branch.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/Branch.java index ec60987a8..cc73d87bd 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/Branch.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/Branch.java @@ -14,7 +14,6 @@ import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; @Entity @IdClass(BranchId.class) @@ -22,7 +21,6 @@ @Setter @NoArgsConstructor @ToString(callSuper = true) -@Filter(name = "gitRepositoryFilter") public class Branch { @Id private String name; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/config/SecurityConfig.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/config/SecurityConfig.java index 11911572a..aea064f5a 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/config/SecurityConfig.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/config/SecurityConfig.java @@ -95,10 +95,7 @@ public WebMvcConfigurer corsConfigurer() { @Override public void addInterceptors(@NonNull InterceptorRegistry registry) { - // Order last (Integer.MAX_VALUE == Ordered.LOWEST_PRECEDENCE) so this runs AFTER Spring's - // Open-Session-In-View interceptor. That guarantees the request's EntityManager is already - // bound when RepositoryInterceptor enables the tenant filter on it. - registry.addWebRequestInterceptor(requestInterceptor).order(Integer.MAX_VALUE); + registry.addWebRequestInterceptor(requestInterceptor); } @Override diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterEntity.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterEntity.java index db56ed21f..5667644c6 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterEntity.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterEntity.java @@ -7,11 +7,9 @@ import jakarta.persistence.MappedSuperclass; import lombok.Data; import lombok.ToString; -import org.hibernate.annotations.Filter; @MappedSuperclass @Data -@Filter(name = "gitRepositoryFilter") public abstract class RepositoryFilterEntity { @ManyToOne(fetch = FetchType.LAZY) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterTransactionConfig.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterTransactionConfig.java deleted file mode 100644 index 661a9690e..000000000 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryFilterTransactionConfig.java +++ /dev/null @@ -1,64 +0,0 @@ -package de.tum.cit.aet.helios.filters; - -import org.hibernate.Session; -import org.springframework.beans.factory.config.BeanPostProcessor; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.orm.jpa.JpaTransactionManager; - -/** - * Enables Hibernate's {@code gitRepositoryFilter} on the transactional {@link - * jakarta.persistence.EntityManager} when a request-scoped repository ID is present in - * {@link RepositoryContext}, scoping all queries against tenant-filtered entities (those - * annotated with {@code @Filter(name = "gitRepositoryFilter")}) to that repository. - * - *

Wires up via {@link JpaTransactionManager#setEntityManagerInitializer} — Spring's - * purpose-built hook for "do X to each new transactional {@link - * jakarta.persistence.EntityManager}", added specifically for the Hibernate-filter use - * case. It fires inside {@code createEntityManagerForTransaction()} with a guaranteed - * non-null EM, before the JDBC connection is acquired and before the dialect begins the - * transaction. We customise Spring Boot's auto-configured {@link JpaTransactionManager} - * via a {@link BeanPostProcessor} rather than declaring a {@code @Primary} replacement, - * so we keep all of Boot's other tx-manager wiring intact. - * - *

Replaces the legacy {@code RepositoryFilterAspect} + {@code @EnableLoadTimeWeaving} - * setup, which targeted a Hibernate-internal class ({@code SessionBuilder.openSession}) - * and therefore required AspectJ load-time weaving plus the {@code aspectjweaver} and - * {@code spring-instrument} javaagents at JVM startup. The new mechanism is agent-free. - * - *

Semantic note vs the old aspect: the filter is enabled at transaction begin (per - * new transactional EM), not at every Hibernate session open. Tenant-filtered entity - * access outside a {@code @Transactional} boundary is therefore not filtered — the - * codebase consistently wraps such access in {@code @Transactional}, so this matches - * existing call patterns. - */ -@Configuration -public class RepositoryFilterTransactionConfig { - - /** - * Bean post-processor that catches Spring Boot's auto-configured {@link - * JpaTransactionManager} and installs the entity-manager initializer on it. Declared - * {@code static} so it can be created before regular singletons (the BPP contract). - */ - @Bean - public static BeanPostProcessor gitRepositoryFilterEnabler() { - return new BeanPostProcessor() { - @Override - public Object postProcessAfterInitialization(Object bean, String beanName) { - if (bean instanceof JpaTransactionManager jpaTxManager) { - jpaTxManager.setEntityManagerInitializer( - em -> { - Long repositoryId = RepositoryContext.getRepositoryId(); - if (repositoryId == null) { - return; - } - em.unwrap(Session.class) - .enableFilter("gitRepositoryFilter") - .setParameter("repository_id", repositoryId); - }); - } - return bean; - } - }; - } -} diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryInterceptor.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryInterceptor.java index 42c2c8b33..a070371d4 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryInterceptor.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/filters/RepositoryInterceptor.java @@ -1,66 +1,39 @@ package de.tum.cit.aet.helios.filters; -import jakarta.persistence.EntityManagerFactory; -import lombok.RequiredArgsConstructor; -import lombok.extern.log4j.Log4j2; -import org.hibernate.Session; import org.springframework.lang.NonNull; import org.springframework.lang.Nullable; -import org.springframework.orm.jpa.EntityManagerHolder; import org.springframework.stereotype.Component; -import org.springframework.transaction.support.TransactionSynchronizationManager; import org.springframework.ui.ModelMap; import org.springframework.web.context.request.WebRequest; import org.springframework.web.context.request.WebRequestInterceptor; +/** + * Publishes the current repository id (from the {@code X-REPOSITORY-ID} header) into + * {@link RepositoryContext} for the duration of the request. Repository-scoped services read it and + * filter their queries explicitly (there is no longer any ambient Hibernate filter). + */ @Component -@RequiredArgsConstructor -@Log4j2 public class RepositoryInterceptor implements WebRequestInterceptor { public static final String X_REPOSITORY_ID = "X-REPOSITORY-ID"; - private final EntityManagerFactory entityManagerFactory; - @Override public void preHandle(@NonNull WebRequest request) { final String tenantId = request.getHeader(X_REPOSITORY_ID); - if (tenantId == null) { - return; - } - RepositoryContext.setRepositoryId(tenantId); - final Long repositoryId = RepositoryContext.getRepositoryId(); - if (repositoryId == null) { - return; - } - // Enable Hibernate's gitRepositoryFilter on this request's EntityManager so tenant-filtered - // entities are scoped to the current repository. RepositoryFilterTransactionConfig only - // enables the filter on EntityManagers created by the transaction manager, which does NOT - // happen under Open-Session-In-View (the request's transaction reuses the EM opened by OSIV). - // Without this, web reads (pull requests, environments, ...) leak across all repositories. - // This interceptor is ordered after OSIV (see SecurityConfig#corsConfigurer) so the request's - // EntityManager is already bound here. - final EntityManagerHolder holder = - (EntityManagerHolder) TransactionSynchronizationManager.getResource(entityManagerFactory); - if (holder != null) { - holder - .getEntityManager() - .unwrap(Session.class) - .enableFilter("gitRepositoryFilter") - .setParameter("repository_id", repositoryId); + if (tenantId != null) { + RepositoryContext.setRepositoryId(tenantId); } else { - log.warn( - "gitRepositoryFilter not enabled for repository {}: no EntityManager bound at preHandle", - repositoryId); + // No repository header → no repository context (also defends against a stale ThreadLocal + // left on a reused worker thread). + RepositoryContext.clear(); } } @Override public void postHandle(@NonNull WebRequest request, @Nullable ModelMap model) { - // Intentionally empty — clearing happens in afterCompletion so the ThreadLocal is wiped - // even when the handler threw (postHandle is skipped on exception, afterCompletion runs - // unconditionally). Otherwise a Tomcat worker thread keeps a stale repositoryId across - // requests, which would silently apply the previous user's tenant filter to the next. + // Intentionally empty — clearing happens in afterCompletion so the ThreadLocal is wiped even + // when the handler threw (postHandle is skipped on exception, afterCompletion runs + // unconditionally). Otherwise a Tomcat worker thread keeps a stale repositoryId across requests. } @Override diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitrepo/GitRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitrepo/GitRepository.java index b0f69ffe4..6c569da51 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitrepo/GitRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitrepo/GitRepository.java @@ -20,7 +20,6 @@ import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; import org.springframework.lang.NonNull; @Entity @@ -29,7 +28,6 @@ @Setter @NoArgsConstructor @ToString(callSuper = true) -@Filter(name = "gitRepositoryFilter") public class GitRepository { @Id diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/label/Label.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/label/Label.java index cef6a3059..e12ab15d8 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/label/Label.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/label/Label.java @@ -15,14 +15,12 @@ import lombok.NonNull; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; @Entity @Table(name = "label") @Getter @Setter @NoArgsConstructor -@Filter(name = "gitRepositoryFilter") public class Label { @Id diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/package-info.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/package-info.java deleted file mode 100644 index 2286df394..000000000 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/package-info.java +++ /dev/null @@ -1,5 +0,0 @@ -@org.hibernate.annotations.FilterDef( - name = "gitRepositoryFilter", - parameters = @org.hibernate.annotations.ParamDef(name = "repository_id", type = Long.class), - defaultCondition = "repository_id = :repository_id") -package de.tum.cit.aet.helios; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidate.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidate.java index e7bf69004..a61b12dbf 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidate.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/releasecandidate/ReleaseCandidate.java @@ -24,7 +24,6 @@ import lombok.Getter; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; @Entity @Getter @@ -35,7 +34,6 @@ @UniqueConstraint(columnNames = {"repository_id", "name"}), @UniqueConstraint(columnNames = {"release_id"}) }) -@Filter(name = "gitRepositoryFilter") public class ReleaseCandidate { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseFlakiness.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseFlakiness.java index 994883c06..9fb9b85f2 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseFlakiness.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseFlakiness.java @@ -16,7 +16,6 @@ import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; /** * Precomputed flakiness metric for test cases, stored in a separate table for efficient retrieval. @@ -40,7 +39,6 @@ @Setter @NoArgsConstructor @ToString -@Filter(name = "gitRepositoryFilter") public class TestCaseFlakiness { @Id diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseStatistics.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseStatistics.java index 96f20579e..2fc76af35 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseStatistics.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestCaseStatistics.java @@ -16,7 +16,6 @@ import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; -import org.hibernate.annotations.Filter; /** * Entity representing statistics for a test case across multiple runs. Used for flaky test @@ -46,7 +45,6 @@ @Setter @NoArgsConstructor @ToString -@Filter(name = "gitRepositoryFilter") public class TestCaseStatistics { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/filters/RepositoryTenantFilterIntegrationTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/filters/RepositoryTenantFilterIntegrationTest.java deleted file mode 100644 index e9cf56d54..000000000 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/filters/RepositoryTenantFilterIntegrationTest.java +++ /dev/null @@ -1,149 +0,0 @@ -package de.tum.cit.aet.helios.filters; - -import static org.assertj.core.api.Assertions.assertThat; - -import de.tum.cit.aet.helios.environment.Environment; -import de.tum.cit.aet.helios.environment.EnvironmentRepository; -import io.zonky.test.db.AutoConfigureEmbeddedDatabase; -import jakarta.persistence.EntityManagerFactory; -import java.util.List; -import javax.sql.DataSource; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.data.jpa.test.autoconfigure.DataJpaTest; -import org.springframework.boot.jdbc.test.autoconfigure.AutoConfigureTestDatabase; -import org.springframework.boot.test.context.TestConfiguration; -import org.springframework.cache.CacheManager; -import org.springframework.cache.concurrent.ConcurrentMapCacheManager; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Import; -import org.springframework.jdbc.core.JdbcTemplate; -import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.web.context.request.ServletWebRequest; - -/** - * Verifies Helios's per-repository tenant isolation: the Hibernate {@code gitRepositoryFilter} - * must actually scope a web-request query to the current repository. - * - *

This guards the regression that shipped in 1.7.0 and was fixed in 1.7.2: after the - * {@code RepositoryFilterAspect -> RepositoryFilterTransactionConfig} refactor, the filter was no - * longer enabled for web reads (under Open-Session-In-View the request transaction reuses the - * pre-opened EntityManager, so the transaction-manager initializer never fired), and every tenant - * query leaked across all repositories. {@link RepositoryInterceptor} now enables the filter on - * the request's EntityManager; this test drives that exact path against a real embedded Postgres - * and the real Flyway schema, so a future change that stops the filter from being enabled — or - * breaks the {@code @Filter} definition — fails here instead of silently leaking data. - * - *

Scope note: the servlet-level ordering that guarantees the EntityManager is bound before the - * interceptor runs (i.e. that OSIV runs first) is a wiring concern verified in staging/prod, not - * reproduced here; this test binds the EntityManager via the surrounding {@code @DataJpaTest} - * transaction, exactly as OSIV would at request time. - */ -@DataJpaTest(properties = {"spring.flyway.enabled=true", "spring.jpa.hibernate.ddl-auto=none"}) -@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) -@AutoConfigureEmbeddedDatabase( - type = AutoConfigureEmbeddedDatabase.DatabaseType.POSTGRES, - provider = AutoConfigureEmbeddedDatabase.DatabaseProvider.DOCKER) -@Import(RepositoryTenantFilterIntegrationTest.CacheTestConfig.class) -class RepositoryTenantFilterIntegrationTest { - - /** The @DataJpaTest slice does not load cache auto-configuration; provide a simple one. */ - @TestConfiguration - static class CacheTestConfig { - @Bean - CacheManager cacheManager() { - return new ConcurrentMapCacheManager(); - } - } - - private static final long REPO_A = 1L; - private static final long REPO_B = 2L; - - @Autowired private EnvironmentRepository environmentRepository; - @Autowired private EntityManagerFactory entityManagerFactory; - @Autowired private DataSource dataSource; - - private RepositoryInterceptor interceptor; - - @BeforeEach - void seed() { - JdbcTemplate jdbc = new JdbcTemplate(dataSource); - insertRepository(jdbc, REPO_A, "ls1intum/repo-a"); - insertRepository(jdbc, REPO_B, "ls1intum/repo-b"); - // Two enabled environments in repo A, one in repo B. - insertEnabledEnvironment(jdbc, 1L, REPO_A, "repo-a-staging"); - insertEnabledEnvironment(jdbc, 2L, REPO_A, "repo-a-production"); - insertEnabledEnvironment(jdbc, 3L, REPO_B, "repo-b-production"); - interceptor = new RepositoryInterceptor(entityManagerFactory); - } - - @AfterEach - void clearContext() { - RepositoryContext.clear(); - } - - @Test - void scopesEnabledEnvironmentsToRepositoryA() { - enterWebRequestForRepository(REPO_A); - - List environments = environmentRepository.findByEnabledTrueOrderByNameAsc(); - - assertThat(environments) - .extracting(env -> env.getRepository().getRepositoryId()) - .as("only repo A's environments must be visible while scoped to repo A") - .containsExactly(REPO_A, REPO_A); - } - - @Test - void scopesEnabledEnvironmentsToRepositoryB() { - enterWebRequestForRepository(REPO_B); - - List environments = environmentRepository.findByEnabledTrueOrderByNameAsc(); - - assertThat(environments) - .extracting(env -> env.getRepository().getRepositoryId()) - .as("only repo B's environment must be visible while scoped to repo B") - .containsExactly(REPO_B); - } - - @Test - void withoutRepositoryContextTheFilterIsNotEnabled() { - // No X-REPOSITORY-ID header -> no repository context -> filter stays off, matching how - // global (non-repo-scoped) requests behave. Proves the filter — not some other mechanism — is - // what scopes the tenant queries; if this returned only one repo, scoping would be an - // accident of the data rather than the filter. - List environments = environmentRepository.findByEnabledTrueOrderByNameAsc(); - - assertThat(environments) - .extracting(env -> env.getRepository().getRepositoryId()) - .containsExactlyInAnyOrder(REPO_A, REPO_A, REPO_B); - } - - /** Reproduces what {@link RepositoryInterceptor} does at the start of a web request. */ - private void enterWebRequestForRepository(long repositoryId) { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.addHeader(RepositoryInterceptor.X_REPOSITORY_ID, String.valueOf(repositoryId)); - interceptor.preHandle(new ServletWebRequest(request)); - } - - private static void insertRepository(JdbcTemplate jdbc, long id, String nameWithOwner) { - jdbc.update( - "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " - + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " - + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", - id, - nameWithOwner); - } - - private static void insertEnabledEnvironment( - JdbcTemplate jdbc, long id, long repositoryId, String name) { - jdbc.update( - "INSERT INTO environment (id, repository_id, enabled, locked, name) " - + "VALUES (?, ?, true, false, ?)", - id, - repositoryId, - name); - } -} diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunServiceTest.java index f5718ac1c..dc92a335f 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunServiceTest.java @@ -33,6 +33,7 @@ import java.util.HashSet; import java.util.List; import java.util.Optional; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -64,6 +65,11 @@ public void setUp() { RepositoryContext.setRepositoryId("1"); } + @AfterEach + public void tearDown() { + RepositoryContext.clear(); + } + @Test public void getPaginatedWorkflowRuns_returnsMappedRunsAndPageMetadata() { WorkflowRun run1 = createWorkflowRun( From fc3f48466b6e7b6971881835beb8951ca557c269 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:49:21 +0200 Subject: [PATCH 12/28] fix(settings): stop GET /settings from persisting a settings row (write-on-GET) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/settings/{repositoryId}/settings called getOrCreateGitRepoSettingsByRepositoryId, which INSERTs a defaults row when none exists — a write on a read path. Add a read-only getGitRepoSettingsByRepositoryId that returns the persisted settings or transient defaults without writing, and point the GET at it. getOrCreate stays for the write callers (lock/reservation calc, scheduler). New test asserts the read path never calls save(). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../GitRepoSettingsController.java | 2 +- .../GitRepoSettingsService.java | 24 +++++++++++++++++++ .../GitRepoSettingsServiceTest.java | 12 ++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsController.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsController.java index 9f7640c0b..482f0ae52 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsController.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsController.java @@ -33,7 +33,7 @@ public class GitRepoSettingsController { @GetMapping("/settings") public ResponseEntity getGitRepoSettings(@PathVariable Long repositoryId) { GitRepoSettingsDto gitRepoSettingsDto = - gitRepoSettingsService.getOrCreateGitRepoSettingsByRepositoryId(repositoryId).orElseThrow(); + gitRepoSettingsService.getGitRepoSettingsByRepositoryId(repositoryId); return ResponseEntity.ok(gitRepoSettingsDto); } diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java index a7962580a..a77dd8974 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java @@ -45,6 +45,30 @@ public Optional getOrCreateGitRepoSettingsByRepositoryId(Lon })); } + /** + * Read-only lookup for the settings GET endpoint: returns the persisted settings, or transient + * defaults when none exist yet — WITHOUT writing. (getOrCreate persists a row, which a plain GET + * must not do; callers that need the row to exist keep using getOrCreate.) + */ + public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { + return gitRepoRepository + .findByRepositoryRepositoryId(repositoryId) + .map(GitRepoSettingsDto::fromGitRepoSettings) + .orElseGet( + () -> { + GitRepository gitRepo = + gitRepository + .findByRepositoryId(repositoryId) + .orElseThrow( + () -> + new IllegalArgumentException( + "Repository with id " + repositoryId + " not found")); + GitRepoSettings defaults = new GitRepoSettings(); + defaults.setRepository(gitRepo); + return GitRepoSettingsDto.fromGitRepoSettings(defaults); + }); + } + public Optional updateGitRepoSettings( @PathVariable Long repositoryId, @RequestBody GitRepoSettingsDto gitRepoSettingsDto) { GitRepoSettings gitRepoSettings = diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java index 00cbe0489..1e45924c5 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java @@ -112,6 +112,18 @@ public void testCreateNewGitRepoSettingsRepositoryNotFound() { verify(gitRepository, times(1)).findByRepositoryId(1L); } + @Test + public void getGitRepoSettingsByRepositoryId_returnsDefaultsWithoutWriting() { + when(gitRepoRepository.findByRepositoryRepositoryId(1L)).thenReturn(Optional.empty()); + when(gitRepository.findByRepositoryId(1L)).thenReturn(Optional.of(testGitRepository)); + + GitRepoSettingsDto result = gitRepoSettingsService.getGitRepoSettingsByRepositoryId(1L); + + assertNotNull(result); + // A plain GET must not persist a settings row. + verify(gitRepoRepository, times(0)).save(any(GitRepoSettings.class)); + } + @Test public void testUpdateGitRepoSettings() { when(gitRepoRepository.findByRepositoryRepositoryId(1L)) From 1dc0e045d5ad012d2cb0238061eed4e759fe4d01 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 3 Jul 2026 23:58:05 +0200 Subject: [PATCH 13/28] refactor(tx): drop class-level @Transactional from GitHubService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHubService is a pure GitHub-API gateway with zero JPA repositories injected — the class-level @Transactional made every GitHub network call hold an open DB transaction for its duration, an antipattern. Removing it (and the now-unused jakarta.transaction.Transactional import) has no effect on persistence and frees the connection during remote calls. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../main/java/de/tum/cit/aet/helios/github/GitHubService.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java index 4f9727347..2eaad7cc0 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/github/GitHubService.java @@ -14,7 +14,6 @@ import de.tum.cit.aet.helios.github.permissions.GitHubRepositoryRoleDto; import de.tum.cit.aet.helios.github.permissions.RepoPermissionType; import de.tum.cit.aet.helios.workflow.GitHubWorkflowContext; -import jakarta.transaction.Transactional; import java.io.BufferedReader; import java.io.FilterInputStream; import java.io.IOException; @@ -56,7 +55,6 @@ @Log4j2 @RequiredArgsConstructor @Service -@Transactional public class GitHubService { private static final String GITHUB_API_VERSION = "2026-03-10"; private static final int GITHUB_ENVIRONMENTS_PAGE_SIZE = 100; From c42405b4fdd6ff27471eae15531a9a4a7ef09e66 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 00:21:33 +0200 Subject: [PATCH 14/28] refactor(tx): remove class-level @Transactional from read-heavy services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the class-level @Transactional (which wrapped every read in a write-capable transaction) from the 9 read-heavy services. Reads now run under Open-Session-in-View only (no transaction). Writes are handled explicitly: - Single repository writes (save/saveAndFlush) keep no annotation — Spring Data's own per-call transaction commits them (e.g. WorkflowService.updateWorkflowLabel, UserService.updateUserSettings, BranchService.setBranchPinned..., ReleaseInfoService.create/evaluate/updateNotes/updateName, EnvironmentService.markStatusAsChanged). - Genuinely-atomic multi-write methods keep/gain a method-level @Transactional so they stay all-or-nothing: DeploymentService.deployToEnvironment/cancelDeployment, EnvironmentService.lock/extend/unlock/updateEnvironment/updateLockExpirationAndReservation/ syncRepositoryEnvironments, WorkflowRunService.reRunWorkflow/reRunFailedJobs, UserService.handleFirstLogin, NotificationPreferenceService.initialize/updatePreferences, PullRequestService.setPrPinned... (conditional create + collection mutation + save). - Derived delete queries, which REQUIRE an active transaction, keep it: BranchService.deleteBranchByNameAndRepositoryId, ReleaseInfoService.deleteReleaseCandidateByName. No behavior change for writes (atomicity preserved where it existed); reads simply stop opening a write transaction. Full server suite green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/de/tum/cit/aet/helios/branch/BranchService.java | 1 - .../tum/cit/aet/helios/deployment/DeploymentService.java | 3 ++- .../tum/cit/aet/helios/environment/EnvironmentService.java | 7 ++++++- .../helios/notification/NotificationPreferenceService.java | 5 ++++- .../tum/cit/aet/helios/pullrequest/PullRequestService.java | 3 ++- .../tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java | 5 ++++- .../main/java/de/tum/cit/aet/helios/user/UserService.java | 3 ++- .../de/tum/cit/aet/helios/workflow/WorkflowRunService.java | 5 ++++- .../de/tum/cit/aet/helios/workflow/WorkflowService.java | 2 -- 9 files changed, 24 insertions(+), 10 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java index 26123d1e1..84d20d943 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java @@ -18,7 +18,6 @@ import org.springframework.transaction.annotation.Transactional; @Service -@Transactional @RequiredArgsConstructor public class BranchService { diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java index 3622addeb..eec4c407d 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java @@ -40,7 +40,6 @@ @Log4j2 @Service -@Transactional @RequiredArgsConstructor public class DeploymentService { @@ -91,6 +90,8 @@ public Optional getLatestDeploymentByEnvironmentId(Long environme .map(DeploymentDto::fromDeployment); } + // Atomic: locks the environment, creates the HeliosDeployment, and dispatches — all or nothing. + @Transactional public void deployToEnvironment(DeployRequest deployRequest) { validateDeployRequest(deployRequest); validateEnvironmentAndPermissions(deployRequest.environmentId()); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java index 5bfaa11e1..d026a87ef 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java @@ -51,7 +51,6 @@ import org.springframework.stereotype.Service; @Service -@Transactional @Log4j2 @RequiredArgsConstructor public class EnvironmentService { @@ -538,6 +537,8 @@ public EnvironmentDto unlockEnvironment(Long id) { * no environment is found with the specified ID * @throws EnvironmentException if the environment is locked and cannot be disabled */ + // Atomic: many field mutations plus lock-expiry recalculation on one environment. + @Transactional public Optional updateEnvironment(Long id, EnvironmentDto environmentDto) throws EnvironmentException { return environmentRepository @@ -764,6 +765,8 @@ private EnvironmentDeploymentReadinessDto.WorkflowStatus mapWorkflowRunToReadine } // Called by GitRepoSettingsService when lock expiration threshold is updated + // Atomic: recompute and persist expiry for every locked environment together. + @Transactional public void updateLockExpirationAndReservation(Long repositoryId) { List lockedEnvironments = environmentRepository.findByRepositoryRepositoryIdAndLockedTrue(repositoryId); @@ -956,6 +959,8 @@ public boolean hasTeamReviewers() { /** * Synchronizes the environments of the current repository with the GitHub repository. */ + // Atomic: reconciles (creates/updates/deletes) the repository's environments as one unit. + @Transactional public void syncRepositoryEnvironments() throws IOException { Long repoId = RepositoryContext.getRepositoryId(); GitRepository repo = gitRepoRepository.findById(repoId).orElseThrow(); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java index f69cf5089..b5eedf1f8 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java @@ -9,12 +9,13 @@ @Service @RequiredArgsConstructor -@Transactional public class NotificationPreferenceService { private final NotificationPreferenceRepository repository; private final AuthService authService; + // Atomic: seed every default preference row for the user or none. + @Transactional public void initializeDefaultsForUser(User user) { for (NotificationPreference.Type type : NotificationPreference.Type.values()) { boolean exists = repository.findByUserAndType(user, type).isPresent(); @@ -31,6 +32,8 @@ public List getCurrentUserPreferences() { .toList(); } + // Atomic: apply all preference updates together. + @Transactional public void updatePreferencesForCurrentUser(List preferences) { User user = authService.getUserFromGithubId(); for (NotificationPreferenceDto dto : preferences) { diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java index fe41e1c82..85f95c797 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java @@ -35,7 +35,6 @@ @Log4j2 @Service -@Transactional @RequiredArgsConstructor public class PullRequestService { @@ -330,6 +329,8 @@ public List getPullRequestByRepositoryId(Long repositoryId) .collect(Collectors.toList()); } + // Atomic: create-if-absent preference, mutate its favourites collection, and save together. + @Transactional public void setPrPinnedByNumberAndUserId(Long prId, Boolean isPinned) { final UserPreference userPreference = userPreferenceRepository diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java index a67fc2ae3..a2ff230df 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java @@ -46,7 +46,6 @@ import org.springframework.stereotype.Service; @Service -@Transactional @Log4j2 @RequiredArgsConstructor public class ReleaseInfoService { @@ -275,6 +274,8 @@ public void evaluateReleaseCandidate(String name, boolean isWorking, String comm releaseCandidateEvaluationRepository.save(evaluation); } + // Derived delete query requires an active transaction. + @Transactional public ReleaseInfoListDto deleteReleaseCandidateByName(String name) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -290,6 +291,8 @@ public ReleaseInfoListDto deleteReleaseCandidateByName(String name) { return rc; } + // Atomic: publishing persists the Release and links it to the candidate via processRelease. + @Transactional public void publishReleaseDraft(String tagName) { Long repositoryId = RepositoryContext.getRepositoryId(); GitRepository repository = gitRepoRepository.findById(repositoryId).orElseThrow(); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java index f26acf25a..7bcd97111 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java @@ -9,7 +9,6 @@ import org.springframework.transaction.annotation.Transactional; @Service -@Transactional @RequiredArgsConstructor @Log4j2 public class UserService { @@ -22,6 +21,8 @@ public class UserService { * Handles user-related setup on their first login to Helios. * Sets hasLoggedIn = true and initializes notification email if applicable. */ + // Atomic: the user flag/email update and the default-preference rows must persist together. + @Transactional public void handleFirstLogin() { try { User loggedInUser = authService.isLoggedIn() ? authService.getUserFromGithubId() : null; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java index de01322c2..6741947c7 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java @@ -31,7 +31,6 @@ @Log4j2 @RequiredArgsConstructor @Service -@Transactional public class WorkflowRunService { private final WorkflowRunRepository workflowRunRepository; @@ -215,10 +214,14 @@ public void cancelWorkflowRun(Long runId) { executeWorkflowRunAction(runId, gitHubService::cancelWorkflowRun, "cancel", false); } + // Atomic: the test-suite delete and the workflow-run reset must commit together. + @Transactional public void reRunWorkflow(Long runId) { executeWorkflowRunAction(runId, gitHubService::reRunWorkflow, "re-run", true); } + // Atomic: the test-suite delete and the workflow-run reset must commit together. + @Transactional public void reRunFailedJobs(Long runId) { executeWorkflowRunAction(runId, gitHubService::reRunFailedJobs, "re-run failed jobs for", true); } diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java index 4955a03ae..606d62954 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java @@ -1,7 +1,6 @@ package de.tum.cit.aet.helios.workflow; import de.tum.cit.aet.helios.filters.RepositoryContext; -import jakarta.transaction.Transactional; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -9,7 +8,6 @@ import org.springframework.stereotype.Service; @Service -@Transactional @RequiredArgsConstructor public class WorkflowService { private final WorkflowRepository workflowRepository; From 5be962f6925f34071dc3bd85a7d0af9f88184d03 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 00:23:33 +0200 Subject: [PATCH 15/28] refactor(tx): remove class-level @Transactional from GitRepoSettingsService Last class-level @Transactional in the service layer. The read (getGitRepoSettingsByRepositoryId, used by GET /settings) and the single-save getOrCreate no longer run in a write transaction; updateGitRepoSettings keeps a method-level @Transactional because it persists the settings row AND cascades a cross-service locked-environment expiry recalculation that must be atomic. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../cit/aet/helios/gitreposettings/GitRepoSettingsService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java index a77dd8974..cde4a7992 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java @@ -13,7 +13,6 @@ @RequiredArgsConstructor @Service -@Transactional public class GitRepoSettingsService { private final GitRepoSettingsRepository gitRepoRepository; @@ -69,6 +68,8 @@ public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { }); } + // Atomic: persist the settings change and recalculate all locked-environment expiries together. + @Transactional public Optional updateGitRepoSettings( @PathVariable Long repositoryId, @RequestBody GitRepoSettingsDto gitRepoSettingsDto) { GitRepoSettings gitRepoSettings = From b21463a790ced7d428c995cb5a40373b7f38c1de Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 07:26:35 +0200 Subject: [PATCH 16/28] refactor: delete dead unscoped finders + empty IssueRepository Now that every tenant query filters by repository explicitly, these unscoped finders have zero callers (verified across main + test): BranchRepository.findByName, LabelRepository.findByName, PullRequestRepository.findAllByState, EnvironmentRepository.findAllByOrderByNameAsc / findByEnabledTrueOrderByNameAsc, ReleaseCandidateRepository.findAllByOrderByCreatedAtDesc, and the no-repository TestCaseStatisticsRepository.findByTestNameAndClassNameAndTestSuiteNameAndBranchName. Also removes the empty, never-injected IssueRepository. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tum/cit/aet/helios/branch/BranchRepository.java | 2 -- .../helios/environment/EnvironmentRepository.java | 4 ---- .../de/tum/cit/aet/helios/issue/IssueRepository.java | 5 ----- .../de/tum/cit/aet/helios/label/LabelRepository.java | 3 --- .../helios/pullrequest/PullRequestRepository.java | 2 -- .../releasecandidate/ReleaseCandidateRepository.java | 2 -- .../helios/tests/TestCaseStatisticsRepository.java | 12 ------------ 7 files changed, 30 deletions(-) delete mode 100644 server/application-server/src/main/java/de/tum/cit/aet/helios/issue/IssueRepository.java diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchRepository.java index 37c750be6..3c7cda781 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchRepository.java @@ -7,8 +7,6 @@ @Repository public interface BranchRepository extends JpaRepository { - Optional findByName(String name); - Optional findByNameAndRepositoryRepositoryId(String name, Long repositoryId); void deleteByNameAndRepositoryRepositoryId(String name, Long repositoryId); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java index c666778e1..d9cbfcb70 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentRepository.java @@ -10,8 +10,6 @@ @Repository public interface EnvironmentRepository extends JpaRepository { - List findAllByOrderByNameAsc(); - List findByRepository(GitRepository repository); @Query("SELECT e FROM Environment e " @@ -22,8 +20,6 @@ public interface EnvironmentRepository extends JpaRepository List findByRepositoryRepositoryIdOrderByCreatedAtDesc(Long repositoryId); - List findByEnabledTrueOrderByNameAsc(); - // Explicit per-repository scoped finders (Option B: replaces the ambient gitRepositoryFilter). List findByRepositoryRepositoryIdOrderByNameAsc(Long repositoryId); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/issue/IssueRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/issue/IssueRepository.java deleted file mode 100644 index b7612e5df..000000000 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/issue/IssueRepository.java +++ /dev/null @@ -1,5 +0,0 @@ -package de.tum.cit.aet.helios.issue; - -import org.springframework.data.jpa.repository.JpaRepository; - -public interface IssueRepository extends JpaRepository {} diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/label/LabelRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/label/LabelRepository.java index 4c385bf10..42ed389d8 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/label/LabelRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/label/LabelRepository.java @@ -1,7 +1,6 @@ package de.tum.cit.aet.helios.label; import java.util.Optional; -import lombok.NonNull; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; @@ -9,8 +8,6 @@ @Repository public interface LabelRepository extends JpaRepository { - Optional

An {@link Optional#empty()} result means "no {@code REQUIRED_REVIEWERS} rule on this env" — + *

An {@link Optional#empty()} result means "no {@code REQUIRED_REVIEWERS} rule here" — * the deployment is not gated on reviewers (it may still be gated on wait-timer or branch * policy, which GitHub handles itself). */ diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java index a7f62f77f..0096c230f 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java @@ -315,7 +315,7 @@ public Optional getPullRequestById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); Optional pullRequest = repositoryId == null - ? pullRequestRepository.findById(id) + ? Optional.empty() : pullRequestRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); return pullRequest.map(PullRequestInfoDto::fromPullRequest); } diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java index 7f92a9fc6..fb936fad4 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java @@ -192,7 +192,15 @@ public TestResultsDto getLatestTestResultsForBranch( * @return the grouped test results */ public TestResultsDto getLatestTestResultsForPr(Long pullRequestId, TestSearchCriteria criteria) { - var pullRequest = pullRequestRepository.findById(pullRequestId).orElseThrow(); + // Scope to the current repository (like the run/branch variants): a PR id from another + // repository must not surface its test results. Null context → not found. + var pullRequest = + pullRequestRepository + .findByIdAndRepositoryRepositoryId(pullRequestId, RepositoryContext.getRepositoryId()) + .orElseThrow( + () -> + new EntityNotFoundException( + "Pull request not found with id: " + pullRequestId)); var defaultContext = getDefaultBranchContext(pullRequest.getRepository()); var latestRuns = diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java index 606d62954..963c4805a 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java @@ -24,7 +24,7 @@ public Optional getWorkflowById(Long id) { private Optional findScopedById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); return repositoryId == null - ? workflowRepository.findById(id) + ? Optional.empty() : workflowRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } @@ -33,7 +33,9 @@ public List getAllWorkflows() { if (repositoryId == null) { return List.of(); } - return workflowRepository.findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId).stream() + return workflowRepository + .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) + .stream() .map(WorkflowDto::fromWorkflow) .collect(Collectors.toList()); } diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentScopingIT.java new file mode 100644 index 000000000..ab57adeef --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentScopingIT.java @@ -0,0 +1,158 @@ +package de.tum.cit.aet.helios.deployment; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.everyItem; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-repository isolation guard for {@code /api/deployments*}. Deployments are keyed by id, + * environment id, and pull-request id; the removed Hibernate {@code gitRepositoryFilter} used to + * scope the derived {@code Deployment} queries to the current repository, so these endpoints must + * now enforce it explicitly. Because {@code GET /api/**} is unauthenticated, the no-header case is + * exercised too (a missing {@code X-REPOSITORY-ID} must not fall back to an unscoped PK load). + */ +class DeploymentScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long ENV_A = 11L; + private static final long ENV_B = 21L; + private static final long DEPLOY_A = 101L; + private static final long DEPLOY_B = 201L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertEnv(jdbc, ENV_A, REPO_A, "a-staging"); + insertEnv(jdbc, ENV_B, REPO_B, "b-staging"); + insertDeployment(jdbc, DEPLOY_A, ENV_A, REPO_A); + insertDeployment(jdbc, DEPLOY_B, ENV_B, REPO_B); + } + + @Test + void deploymentsListIsScopedToCurrentRepository() throws Exception { + mockMvc + .perform(get("/api/deployments").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + mockMvc + .perform(get("/api/deployments").header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); + } + + @Test + void deploymentsListWithoutHeaderIsEmpty() throws Exception { + mockMvc + .perform(get("/api/deployments")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + } + + @Test + void deploymentByIdIsInvisibleFromAnotherRepository() throws Exception { + mockMvc + .perform( + get("/api/deployments/{id}", DEPLOY_B).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + mockMvc + .perform( + get("/api/deployments/{id}", DEPLOY_B).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()); + } + + @Test + void deploymentByIdWithoutHeaderIsNotFound() throws Exception { + // No header → null context → must not fall back to an unscoped findById (anonymous IDOR). + mockMvc + .perform(get("/api/deployments/{id}", DEPLOY_A)) + .andExpect(status().isNotFound()); + } + + @Test + void deploymentsByForeignEnvironmentAreEmpty() throws Exception { + // ENV_B belongs to repo B: scoping to repo A must not surface repo B's deployments. + mockMvc + .perform( + get("/api/deployments/environment/{envId}", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + mockMvc + .perform( + get("/api/deployments/environment/{envId}", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)); + } + + @Test + void latestDeploymentByForeignEnvironmentIsNotFound() throws Exception { + mockMvc + .perform( + get("/api/deployments/environment/{envId}/latest", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + mockMvc + .perform( + get("/api/deployments/environment/{envId}/latest", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()); + } + + @Test + void activityHistoryByForeignEnvironmentIsEmpty() throws Exception { + mockMvc + .perform( + get("/api/deployments/environment/{envId}/activity-history", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + mockMvc + .perform( + get("/api/deployments/environment/{envId}/activity-history", ENV_B) + .header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertEnv(JdbcTemplate jdbc, long id, long repositoryId, String name) { + jdbc.update( + "INSERT INTO environment (id, repository_id, enabled, locked, name) " + + "VALUES (?, ?, true, false, ?)", + id, + repositoryId, + name); + } + + private static void insertDeployment( + JdbcTemplate jdbc, long id, long environmentId, long repositoryId) { + jdbc.update( + "INSERT INTO deployment (id, environment_id, repository_id, created_at, state, ref, sha, " + + "url) VALUES (?, ?, ?, now(), 'SUCCESS', 'main', 'deadbeef', 'https://example/dep')", + id, + environmentId, + repositoryId); + } +} diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java index ec7047131..224bd1306 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/deployment/DeploymentServiceTest.java @@ -129,6 +129,8 @@ public void testGetAllDeployments() { @Test public void testGetDeploymentsByEnvironmentId() { + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(deploymentRepository.findByEnvironmentIdOrderByCreatedAtDesc(1L)) .thenReturn(List.of(deployment)); @@ -142,6 +144,8 @@ public void testGetDeploymentsByEnvironmentId() { @Test public void testGetLatestDeploymentByEnvironmentId() { + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(deploymentRepository.findFirstByEnvironmentIdOrderByCreatedAtDesc(1L)) .thenReturn(Optional.of(deployment)); @@ -153,6 +157,15 @@ public void testGetLatestDeploymentByEnvironmentId() { assertEquals(deploymentDto, result.get()); } + @Test + public void getDeploymentsByEnvironmentId_forEnvironmentInAnotherRepository_returnsEmpty() { + // Env id from another repo → gate fails → empty (no cross-repo deployment leak). + when(environmentRepository.findByIdAndRepositoryRepositoryId(99L, 1L)) + .thenReturn(Optional.empty()); + + assertTrue(deploymentService.getDeploymentsByEnvironmentId(99L).isEmpty()); + } + @Test public void testDeployWithInvalidDeployRequest() { DeployRequest deployRequest = mock(DeployRequest.class); @@ -333,6 +346,8 @@ public void testGetActivityHistoryByEnvironmentId() { lockHistory.setLockedAt(now.minusMinutes(3)); lockHistory.setUnlockedAt(now); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(deploymentRepository.findByEnvironmentIdOrderByCreatedAtDesc(1L)) .thenReturn(List.of(deployment)); when(lockHistoryRepository.findLockHistoriesByEnvironment(1L)).thenReturn(List.of(lockHistory)); @@ -362,7 +377,8 @@ public void testGetActivityHistoryByPullRequestId() { heliosDeployment.setEnvironment(environment); deployment.setCreatedAt(now.minusMinutes(2)); - when(pullRequestRepository.findById(1L)).thenReturn(Optional.of(mock())); + when(pullRequestRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(mock())); when(deploymentRepository.findByPullRequest_IdOrderByCreatedAtDesc(1L)) .thenReturn(List.of(deployment)); when(heliosDeploymentRepository @@ -444,7 +460,8 @@ public void testGetWorkflowJobStatusUsesWorkflowRunRepository() throws IOExcepti workflowRun.setId(42L); workflowRun.setRepository(gitRepository); - when(workflowRunRepository.findById(42L)).thenReturn(Optional.of(workflowRun)); + when(workflowRunRepository.findByIdAndRepositoryRepositoryId(42L, 1L)) + .thenReturn(Optional.of(workflowRun)); when(gitHubService.getWorkflowJobStatus("owner/repo", 42L)) .thenReturn("{\"total_count\":0,\"jobs\":[]}"); @@ -459,12 +476,13 @@ public void testGetWorkflowJobStatusUsesWorkflowRunRepository() throws IOExcepti @Test public void testGetWorkflowJobStatusFallsBackToHeliosDeploymentRepository() throws IOException { GitRepository otherRepository = new GitRepository(); - otherRepository.setRepositoryId(2L); + otherRepository.setRepositoryId(1L); otherRepository.setNameWithOwner("other/repo"); environment.setRepository(otherRepository); heliosDeployment.setWorkflowRunId(43L); - when(workflowRunRepository.findById(43L)).thenReturn(Optional.empty()); + when(workflowRunRepository.findByIdAndRepositoryRepositoryId(43L, 1L)) + .thenReturn(Optional.empty()); when(heliosDeploymentRepository.findByWorkflowRunId(43L)) .thenReturn(Optional.of(heliosDeployment)); when(gitHubService.getWorkflowJobStatus("other/repo", 43L)) diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java index a624d80d8..37aa34de6 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentScopingIT.java @@ -78,7 +78,7 @@ void environmentByIdIsInvisibleFromAnotherRepository() throws Exception { @Test void withoutRepositoryContextReturnsEmpty() throws Exception { - // No X-REPOSITORY-ID header → a tenant-scoped read has no repository → empty (never findAll). + // No X-REPOSITORY-ID header → tenant-scoped read has no repository → empty (never findAll). mockMvc .perform(get("/api/environments/enabled")) .andExpect(status().isOk()) @@ -87,6 +87,9 @@ void withoutRepositoryContextReturnsEmpty() throws Exception { .perform(get("/api/environments")) .andExpect(status().isOk()) .andExpect(jsonPath("$.length()").value(0)); + // Detail-by-id with no header must not fall back to an unscoped findById (anonymous IDOR): + // GET /api/** is permitAll, so a missing header must yield 404, not another repo's environment. + mockMvc.perform(get("/api/environments/{id}", ENV_A1)).andExpect(status().isNotFound()); } private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java index 49ac8ff7f..0f667e3a6 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/environment/EnvironmentServiceTest.java @@ -74,6 +74,8 @@ public class EnvironmentServiceTest { @BeforeEach public void setUp() { + RepositoryContext.setRepositoryId("1"); + gitRepository = new GitRepository(); gitRepository.setRepositoryId(1L); @@ -94,23 +96,46 @@ public void tearDown() { @Test public void testGetEnvironmentById() { - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Optional result = environmentService.getEnvironmentById(1L); assertTrue(result.isPresent()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); + } + + @Test + public void getLockHistoryByEnvironmentId_forEnvironmentInAnotherRepository_returnsEmpty() { + // Env id from another repo → gate fails → empty (lock history is keyed only by env id). + when(environmentRepository.findByIdAndRepositoryRepositoryId(99L, 1L)) + .thenReturn(Optional.empty()); + + assertTrue(environmentService.getLockHistoryByEnvironmentId(99L).isEmpty()); + verify(lockHistoryRepository, never()).findLockHistoriesByEnvironment(99L); + } + + @Test + public void lockEnvironment_forEnvironmentInAnotherRepository_throwsNotFound() { + // Cross-repo write must not reach a foreign environment by id. + when(environmentRepository.findByIdAndRepositoryRepositoryId(99L, 1L)) + .thenReturn(Optional.empty()); + + assertThrows( + jakarta.persistence.EntityNotFoundException.class, + () -> environmentService.lockEnvironment(99L)); } @Test public void testGetEnvironmentTypeById() { - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Optional result = environmentService.getEnvironmentTypeById(1L); assertTrue(result.isPresent()); assertEquals(Environment.Type.TEST, result.get()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -147,7 +172,8 @@ public void testGetAllEnabledEnvironments() { @Test public void testLockEnvironment() { when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(lockHistoryRepository.saveAndFlush(any(EnvironmentLockHistory.class))) .thenReturn(new EnvironmentLockHistory()); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); @@ -156,7 +182,7 @@ public void testLockEnvironment() { assertTrue(result.isPresent()); assertTrue(result.get().isLocked()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); verify(environmentRepository, times(1)).save(any(Environment.class)); } @@ -167,7 +193,8 @@ public void testLockEnvironmentAlreadyLocked() { environment.setLockedBy(user); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Optional result = environmentService.lockEnvironment(1L); @@ -175,7 +202,7 @@ public void testLockEnvironmentAlreadyLocked() { assertTrue(result.get().isLocked()); // Should not modify lockedAt assertTrue(result.get().getLockedAt().isEqual(environment.getLockedAt())); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -186,7 +213,8 @@ public void testLockEnvironmentAlreadyLockedByOtherUser() { environment.setLockedBy(otherUser); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -196,7 +224,7 @@ public void testLockEnvironmentAlreadyLockedByOtherUser() { }); assertTrue(exception.getMessage().contains("Environment is locked by another user")); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -211,7 +239,8 @@ public void testExtendEnvironmentLock() { environment.setLockReservationThreshold(lockReservationExpirationThreshold); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); Optional result = environmentService.extendEnvironmentLock(1L); @@ -227,14 +256,15 @@ public void testExtendEnvironmentLock() { .get() .getLockReservationExpiresAt() .isAfter(dateTimeNow.plusMinutes(lockReservationExpirationThreshold))); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); verify(environmentRepository, times(1)).save(any(Environment.class)); } @Test public void testExtendEnvironmentLockNotLockedFails() { when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -244,7 +274,7 @@ public void testExtendEnvironmentLockNotLockedFails() { }); assertEquals("Environment is not locked. Cannot extend lock.", exception.getMessage()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -252,7 +282,8 @@ public void testExtendProductionEnvironmentLockFails() { environment.setType(Environment.Type.PRODUCTION); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -262,7 +293,7 @@ public void testExtendProductionEnvironmentLockFails() { }); assertEquals("Only TEST environments can have their locks extended", exception.getMessage()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -270,7 +301,8 @@ public void testExtendDisabledEnvironmentLockFails() { environment.setEnabled(false); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -280,7 +312,7 @@ public void testExtendDisabledEnvironmentLockFails() { }); assertEquals("Environment is disabled", exception.getMessage()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -290,7 +322,8 @@ public void testUnlockEnvironment() { environment.setLockedAt(OffsetDateTime.now()); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); EnvironmentDto result = environmentService.unlockEnvironment(1L); @@ -302,7 +335,7 @@ public void testUnlockEnvironment() { assertNull(result.lockedAt()); assertNull(result.lockWillExpireAt()); assertNull(result.lockReservationWillExpireAt()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); verify(environmentRepository, times(1)).save(any(Environment.class)); } @@ -315,7 +348,8 @@ public void testLockOwnerCanNotUnlockWithActiveDeployment() { createHeliosDeployment(HeliosDeployment.Status.IN_PROGRESS, OffsetDateTime.now()); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(heliosDeploymentRepository.findTopByEnvironmentOrderByCreatedAtDesc(environment)) .thenReturn(Optional.of(deployment)); @@ -339,7 +373,8 @@ public void testMaintainerCanNotUnlockWithActiveDeploymentBeforeTimeout() { when(authService.getUserFromGithubId()).thenReturn(user); when(authService.isAtLeastMaintainer()).thenReturn(true); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(heliosDeploymentRepository.findTopByEnvironmentOrderByCreatedAtDesc(environment)) .thenReturn(Optional.of(deployment)); @@ -364,7 +399,8 @@ public void testMaintainerCanUnlockWithStaleActiveDeployment() { when(authService.getUserFromGithubId()).thenReturn(user); when(authService.isAtLeastMaintainer()).thenReturn(true); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); when(heliosDeploymentRepository.findTopByEnvironmentOrderByCreatedAtDesc(environment)) .thenReturn(Optional.of(deployment)); @@ -388,7 +424,8 @@ public void testLockOwnerCanUnlockWithStaleActiveDeployment() { HeliosDeployment.Status.IN_PROGRESS, OffsetDateTime.now().minusMinutes(21)); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); when(heliosDeploymentRepository.findTopByEnvironmentOrderByCreatedAtDesc(environment)) .thenReturn(Optional.of(deployment)); @@ -417,7 +454,8 @@ public void testUnlockEnvironmentWithTerminalDeployments() { HeliosDeployment deployment = createHeliosDeployment(status, OffsetDateTime.now()); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(environmentRepository.save(any(Environment.class))).thenReturn(environment); when(heliosDeploymentRepository.findTopByEnvironmentOrderByCreatedAtDesc(environment)) .thenReturn(Optional.of(deployment)); @@ -433,7 +471,8 @@ public void testUnlockEnvironmentWithTerminalDeployments() { @Test public void testUnlockEnvironmentNotLocked() { when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -443,7 +482,7 @@ public void testUnlockEnvironmentNotLocked() { }); assertEquals("Environment is not locked", exception.getMessage()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -455,7 +494,8 @@ public void testMaintainerCanUnlockOtherUsersLock() { when(authService.isAtLeastMaintainer()).thenReturn(true); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); EnvironmentDto result = environmentService.unlockEnvironment(1L); @@ -466,7 +506,7 @@ public void testMaintainerCanUnlockOtherUsersLock() { assertNull(result.lockedAt()); assertNull(result.lockWillExpireAt()); assertNull(result.lockReservationWillExpireAt()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); verify(environmentRepository, times(1)).save(any(Environment.class)); } @@ -479,7 +519,8 @@ public void testUserCanNotUnlockOtherUsersLock() { when(authService.isAtLeastMaintainer()).thenReturn(false); when(authService.getUserFromGithubId()).thenReturn(user); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); Exception exception = assertThrows( @@ -490,7 +531,7 @@ public void testUserCanNotUnlockOtherUsersLock() { assertTrue( exception.getMessage().contains("You do not have permission to unlock this environment")); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -514,7 +555,8 @@ public void testUpdateLockExpirationAndReservation() { public void testSetLockedEnvironmentAsDisabled() { environment.setLocked(true); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); environment.setEnabled(false); @@ -526,7 +568,7 @@ public void testSetLockedEnvironmentAsDisabled() { }); assertTrue(exception.getMessage().contains("Environment is locked and can not be disabled")); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); } @Test @@ -536,7 +578,8 @@ public void testUpdateEnvironment() { wf.setRepository(gitRepository); final Long threshold = 10L; - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(workflowRepository.findById(1L)).thenReturn(Optional.of(wf)); environment.setEnabled(false); @@ -563,7 +606,7 @@ public void testUpdateEnvironment() { assertEquals(threshold, environmentDto.get().lockReservationThreshold()); assertFalse(environmentDto.get().enabled()); - verify(environmentRepository, times(1)).findById(1L); + verify(environmentRepository, times(1)).findByIdAndRepositoryRepositoryId(1L, 1L); verify(workflowRepository, times(1)).findById(1L); } @@ -600,7 +643,8 @@ public void testUpdateEnvironmentWithRequiredPreDeploymentWorkflows() { requiredWorkflow.setName("Build"); requiredWorkflow.setRepository(gitRepository); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(workflowRepository.findById(1L)).thenReturn(Optional.of(deploymentWorkflow)); when(workflowRepository.findById(2L)).thenReturn(Optional.of(requiredWorkflow)); @@ -624,7 +668,8 @@ public void testUpdateEnvironmentRejectsDeploymentWorkflowAsRequiredPreDeploymen deploymentWorkflow.setName("Deploy"); deploymentWorkflow.setRepository(gitRepository); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when(workflowRepository.findById(1L)).thenReturn(Optional.of(deploymentWorkflow)); environment.setDeploymentWorkflow(deploymentWorkflow); @@ -655,7 +700,8 @@ public void testGetDeploymentReadinessReturnsReadyForSuccessfulRequiredWorkflow( workflowRun.setStatus(WorkflowRun.Status.COMPLETED); workflowRun.setConclusion(Optional.of(WorkflowRun.Conclusion.SUCCESS)); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when( workflowRunRepository .findLatestForDeploymentReadiness( @@ -685,7 +731,8 @@ public void testGetDeploymentReadinessReturnsWaitingForRunningRequiredWorkflow() workflowRun.setHtmlUrl("https://example.com/run/100"); workflowRun.setStatus(WorkflowRun.Status.IN_PROGRESS); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when( workflowRunRepository .findLatestForDeploymentReadiness( @@ -709,7 +756,8 @@ public void testGetDeploymentReadinessReturnsMissingRunWhenNoMatchingWorkflowRun requiredWorkflow.setRepository(gitRepository); environment.setRequiredPreDeploymentWorkflows(List.of(requiredWorkflow)); - when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); + when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(environment)); when( workflowRunRepository .findLatestForDeploymentReadiness( diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultServiceTest.java index fa13dff83..8c295e5e9 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultServiceTest.java @@ -189,7 +189,8 @@ void getLatestTestResultsForPr_shouldReturnResults() { pullRequest.setHeadSha("prSha"); pullRequest.setRepository(gitRepository); - when(pullRequestRepository.findById(1L)).thenReturn(Optional.of(pullRequest)); + when(pullRequestRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) + .thenReturn(Optional.of(pullRequest)); when(branchRepository.findFirstByRepositoryRepositoryIdAndIsDefaultTrue(anyLong())) .thenReturn(Optional.of(defaultBranch)); when(workflowRunRepository.findByHeadBranchAndHeadShaAndRepositoryRepositoryId( @@ -236,6 +237,20 @@ void getLatestTestResultsForPr_shouldReturnResults() { assertEquals("test1", testTypeResults.testSuites().get(0).testCases().get(0).name()); } + @Test + void getLatestTestResultsForPr_forAnotherRepository_throwsNotFound() { + // A PR id from another repository must not surface its test results (scoped, like the + // run/branch variants). Null/foreign context → not found, not a leak. + when(pullRequestRepository.findByIdAndRepositoryRepositoryId(99L, 1L)) + .thenReturn(java.util.Optional.empty()); + + org.junit.jupiter.api.Assertions.assertThrows( + jakarta.persistence.EntityNotFoundException.class, + () -> + testResultService.getLatestTestResultsForPr( + 99L, new TestResultService.TestSearchCriteria(0, 10, null, false))); + } + @Test void getLatestTestResultsForWorkflowRun_shouldReturnResults() { final TestResultService.TestSearchCriteria criteria = From c9de35070a3dd744de19cb2531dcf4ca183a718a Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 08:29:35 +0200 Subject: [PATCH 19/28] fix(settings): PUT /settings creates the row on absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /settings no longer persists a settings row (write-on-GET fix), so updateGitRepoSettings could throw IllegalArgumentException → HTTP 400 when Save ran for a repo whose row was never lazily created (new repo, no locked env, no workflow group). It now creates-on-absent; the orElseThrow only fires when the repository itself does not exist. New test covers the create-on-absent path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../GitRepoSettingsService.java | 22 ++++++++++++++----- .../GitRepoSettingsServiceTest.java | 20 +++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java index 7849c8daa..32807f573 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java @@ -45,7 +45,7 @@ public Optional getOrCreateGitRepoSettingsByRepositoryId(Lon /** * Read-only lookup for the settings GET endpoint: returns the persisted settings, or transient - * defaults when none exist yet — WITHOUT writing. (getOrCreate persists a row, which a plain GET + * defaults when none exist yet — WITHOUT writing. (getOrCreate persists a row; a plain GET * must not do; callers that need the row to exist keep using getOrCreate.) */ public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { @@ -69,13 +69,25 @@ public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { public Optional updateGitRepoSettings( @PathVariable Long repositoryId, @RequestBody GitRepoSettingsDto gitRepoSettingsDto) { + // Create-on-absent: GET /settings no longer persists a row as a side effect, so Save must be + // robust for a repo whose settings row was never lazily created (new repo, no locked env, no + // workflow group). The orElseThrow now only fires when the repository itself does not exist. GitRepoSettings gitRepoSettings = gitRepoRepository .findByRepositoryRepositoryId(repositoryId) - .orElseThrow( - () -> - new IllegalArgumentException( - "GitRepoSettings not found for repository with id " + repositoryId)); + .orElseGet( + () -> { + GitRepository gitRepo = + gitRepository + .findByRepositoryId(repositoryId) + .orElseThrow( + () -> + new IllegalArgumentException( + "Repository with id " + repositoryId + " not found")); + GitRepoSettings created = new GitRepoSettings(); + created.setRepository(gitRepo); + return created; + }); if (gitRepoSettingsDto.lockExpirationThreshold() != null) { gitRepoSettings.setLockExpirationThreshold(gitRepoSettingsDto.lockExpirationThreshold()); diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java index 1e45924c5..553c8006f 100644 --- a/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsServiceTest.java @@ -144,6 +144,26 @@ public void testUpdateGitRepoSettings() { verify(environmentService, times(1)).updateLockExpirationAndReservation(1L); } + @Test + public void updateGitRepoSettings_createsRowWhenAbsent() { + // GET /settings no longer persists a row, so Save must create-on-absent instead of 400ing. + when(gitRepoRepository.findByRepositoryRepositoryId(1L)).thenReturn(Optional.empty()); + when(gitRepository.findByRepositoryId(1L)).thenReturn(Optional.of(testGitRepository)); + when(gitRepoRepository.save(any(GitRepoSettings.class))) + .thenAnswer(invocation -> invocation.getArgument(0)); + + GitRepoSettingsDto updateDto = new GitRepoSettingsDto(null, 120L, 60L, "com.example.created"); + + Optional result = + gitRepoSettingsService.updateGitRepoSettings(1L, updateDto); + + assertTrue(result.isPresent()); + assertEquals(120L, result.get().lockExpirationThreshold()); + assertEquals("com.example.created", result.get().packageName()); + verify(gitRepoRepository, times(1)).save(any(GitRepoSettings.class)); + verify(environmentService, times(1)).updateLockExpirationAndReservation(1L); + } + @Test public void testUpdateGitRepoSettingsNotFound() { when(gitRepoRepository.findByRepositoryRepositoryId(1L)).thenReturn(Optional.empty()); From ca5220e21f9bb3d6f205dce0eb7397f50fa80bf4 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 08:29:35 +0200 Subject: [PATCH 20/28] refactor: remove dead PullRequestRepository.findAllByOrderByUpdatedAtDesc No callers remain after getPullRequests switched to the repository-scoped finder. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tum/cit/aet/helios/pullrequest/PullRequestRepository.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java index adf120916..41a81f5f6 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestRepository.java @@ -36,8 +36,6 @@ Optional findByRepositoryRepositoryIdAndHeadRefNameAndState( List findByRepositoryRepositoryIdAndStateOrderByUpdatedAtDesc( Long repositoryId, Issue.State state); - List findAllByOrderByUpdatedAtDesc(); - List findByRepositoryRepositoryIdOrderByUpdatedAtDesc(Long repositoryId); Optional findByIdAndRepositoryRepositoryId(Long id, Long repositoryId); From fb41e477d6d9a61420a55e5eb63754e096d7dc8f Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 08:58:39 +0200 Subject: [PATCH 21/28] =?UTF-8?q?fix(tx):=20run=20read=20methods=20@Transa?= =?UTF-8?q?ctional(readOnly=20=3D=20true)=20=E2=80=94=20Postgres=20Large?= =?UTF-8?q?=20Objects=20need=20a=20tx?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Staging smoke test caught a regression: GET /api/environments (and other reads) returned HTTP 500 with `org.postgresql.util.PSQLException: Large Objects may not be used in auto-commit mode`. The PR body (@Lob Issue.body) and the release/release-candidate `body` oid columns are PostgreSQL Large Objects, which can only be read inside a transaction. Removing @Transactional from the read methods made them run in auto-commit; Open-Session-in-View keeps the EntityManager open but does NOT start a transaction, so it doesn't help. Fix: mark the pure-read methods @Transactional(readOnly = true) across the service layer (swapping the jakarta→Spring import where needed). This is a mechanical requirement (like the derived-delete transactions), not the write-transaction antipattern — it's read-only, holds no write lock, and spans no network call. Write methods remain transaction-free (except the two derived deletes), preserving the intended @Transactional cleanup. Verified: full server suite green; staging redeploy + smoke test confirm the reads recover. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../de/tum/cit/aet/helios/branch/BranchService.java | 2 ++ .../aet/helios/deployment/DeploymentService.java | 10 ++++++++++ .../aet/helios/environment/EnvironmentService.java | 13 +++++++++++++ .../gitreposettings/GitRepoSettingsService.java | 2 ++ .../notification/NotificationPreferenceService.java | 2 ++ .../aet/helios/pullrequest/PullRequestService.java | 7 +++++++ .../aet/helios/releaseinfo/ReleaseInfoService.java | 6 +++++- .../de/tum/cit/aet/helios/user/UserService.java | 2 ++ .../cit/aet/helios/workflow/WorkflowRunService.java | 5 +++++ .../cit/aet/helios/workflow/WorkflowService.java | 7 +++++++ 10 files changed, 55 insertions(+), 1 deletion(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java index 84d20d943..a8847a883 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java @@ -27,6 +27,7 @@ public class BranchService { private final AuthService authService; private final CommitRepository commitRepository; + @Transactional(readOnly = true) public List getAllBranches(String sortField, String sortDirection) { final Optional userPreference = authService.isLoggedIn() ? userPreferenceRepository.findByUser(authService.getUserFromGithubId()) @@ -62,6 +63,7 @@ public void deleteBranchByNameAndRepositoryId(String name, Long repositoryId) { branchRepository.deleteByNameAndRepositoryRepositoryId(name, repositoryId); } + @Transactional(readOnly = true) public Optional getBranchByRepositoryIdAndName(Long repositoryId, String name) { return branchRepository .findByRepositoryRepositoryIdAndName(repositoryId, name) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java index 5f6a98887..f35779847 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java @@ -36,6 +36,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.log4j.Log4j2; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Log4j2 @Service @@ -57,6 +58,7 @@ public class DeploymentService { private final HeliosDeploymentWorkflowRunSyncService heliosDeploymentWorkflowRunSyncService; private final WorkflowRunRepository workflowRunRepository; + @Transactional(readOnly = true) public Optional getDeploymentById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); Optional deployment = @@ -66,6 +68,7 @@ public Optional getDeploymentById(Long id) { return deployment.map(DeploymentDto::fromDeployment); } + @Transactional(readOnly = true) public List getAllDeployments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -88,6 +91,7 @@ private boolean environmentInCurrentRepository(Long environmentId) { .isPresent(); } + @Transactional(readOnly = true) public List getDeploymentsByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return List.of(); @@ -97,6 +101,7 @@ public List getDeploymentsByEnvironmentId(Long environmentId) { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public Optional getLatestDeploymentByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return Optional.empty(); @@ -199,6 +204,7 @@ private Map createWorkflowParams( return workflowParams; } + @Transactional(readOnly = true) public boolean canDeployToEnvironment(Environment.Type environmentType) { if (null != environmentType) { switch (environmentType) { @@ -322,6 +328,7 @@ private boolean canRedeploy(Environment environment, long timeoutMinutes) { *

  • Requires separate environment lookup to verify deployment recency * */ + @Transactional(readOnly = true) public List getActivityHistoryByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return List.of(); @@ -383,6 +390,7 @@ public List getActivityHistoryByEnvironmentId(Long environme * descending * @throws EntityNotFoundException if the pull request does not exist */ + @Transactional(readOnly = true) public List getActivityHistoryByPullRequestId(Long pullRequestId) { if (pullRequestRepository .findByIdAndRepositoryRepositoryId(pullRequestId, RepositoryContext.getRepositoryId()) @@ -420,6 +428,7 @@ public List getActivityHistoryByPullRequestId(Long pullReque * @return Combined list of {@link ActivityHistoryDto} for the branch, sorted by timestamp * descending */ + @Transactional(readOnly = true) public List getActivityHistoryByRepositoryIdAndBranchName( Long repositoryId, String branchName) { List combined = new ArrayList<>(); @@ -523,6 +532,7 @@ private boolean syncTerminalWorkflowRunStateAfterFailedCancellation(Long workflo * @return List of workflow jobs with their steps * @throws DeploymentException if there's an error fetching the workflow job status */ + @Transactional(readOnly = true) public WorkflowJobsResponse getWorkflowJobStatus(Long workflowRunId) { try { String repoNameWithOwner = resolveWorkflowRunRepositoryName(workflowRunId); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java index cc8781d3b..615f2192a 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java @@ -48,6 +48,7 @@ import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.PageRequest; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @Log4j2 @@ -74,10 +75,12 @@ public class EnvironmentService { private final GitHubService gitHubService; private final NatsNotificationPublisherService notificationPublisherService; + @Transactional(readOnly = true) public Optional getEnvironmentById(Long id) { return findScopedById(id).map(EnvironmentDto::fromEnvironment); } + @Transactional(readOnly = true) public Optional getEnvironmentTypeById(Long id) { return findScopedById(id).map(Environment::getType); } @@ -97,6 +100,7 @@ private Optional findScopedById(Long id) { : environmentRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } + @Transactional(readOnly = true) public List getAllEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -114,6 +118,7 @@ public List getAllEnvironments() { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public List getAllEnabledEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -133,6 +138,7 @@ public List getAllEnabledEnvironments() { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public List getEnvironmentsByRepositoryId(Long repositoryId) { return environmentRepository .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) @@ -179,6 +185,7 @@ private DeploymentDurationEstimate computeEstimate(Environment environment) { * @return A wrapper object containing the latest Deployment or HeliosDeployment, or an empty * result if no deployments exist. */ + @Transactional(readOnly = true) public LatestDeploymentUnion findLatestDeployment(Environment env) { // Retrieve the latest HeliosDeployment Optional latestHeliosOpt = @@ -658,6 +665,7 @@ private void updateRequiredPreDeploymentWorkflows( environment.setRequiredPreDeploymentWorkflows(new ArrayList<>(workflows)); } + @Transactional(readOnly = true) public EnvironmentDeploymentReadinessDto getDeploymentReadiness( Long environmentId, String branch, String sha) { Environment environment = @@ -768,6 +776,7 @@ public void updateLockExpirationAndReservation(Long repositoryId) { } } + @Transactional(readOnly = true) public EnvironmentLockHistoryDto getUsersCurrentLock() { final User currentUser = authService.getUserFromGithubId(); Optional lockHistory = @@ -781,6 +790,7 @@ public EnvironmentLockHistoryDto getUsersCurrentLock() { .orElse(null); } + @Transactional(readOnly = true) public List getLockHistoryByEnvironmentId(Long environmentId) { // Scope to the current repository (lock history is keyed only by environment id, so a foreign // id would otherwise leak its lock/unlock history). @@ -829,6 +839,7 @@ private boolean isActiveDeploymentStatus(HeliosDeployment.Status status) { || status == HeliosDeployment.Status.IN_PROGRESS; } + @Transactional(readOnly = true) public Optional getEnvironmentReviewers(Long environmentId) { // Scope to the current repository — protection rules are keyed only by environment id. if (findScopedById(environmentId).isEmpty()) { @@ -889,6 +900,7 @@ public Optional getEnvironmentReviewers(Long environmen * the deployment is not gated on reviewers (it may still be gated on wait-timer or branch * policy, which GitHub handles itself). */ + @Transactional(readOnly = true) public Optional resolveReviewers(Long environmentId) { return protectionRuleRepository .findByEnvironmentIdAndRuleType(environmentId, ProtectionRule.RuleType.REQUIRED_REVIEWERS) @@ -935,6 +947,7 @@ public Optional resolveReviewers(Long environmentId) { * Returns {@code false} if there's no rule, or only Team-type reviewers (which Helios doesn't yet * expand to members). */ + @Transactional(readOnly = true) public boolean isUserRequiredReviewer(Long environmentId, String userLogin) { if (userLogin == null) { return false; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java index 32807f573..7008e9597 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java @@ -7,6 +7,7 @@ import lombok.RequiredArgsConstructor; import org.springframework.context.annotation.Lazy; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; @@ -48,6 +49,7 @@ public Optional getOrCreateGitRepoSettingsByRepositoryId(Lon * defaults when none exist yet — WITHOUT writing. (getOrCreate persists a row; a plain GET * must not do; callers that need the row to exist keep using getOrCreate.) */ + @Transactional(readOnly = true) public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { return gitRepoRepository .findByRepositoryRepositoryId(repositoryId) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java index df9fdccc8..fa22c357d 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java @@ -5,6 +5,7 @@ import java.util.List; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -22,6 +23,7 @@ public void initializeDefaultsForUser(User user) { } } + @Transactional(readOnly = true) public List getCurrentUserPreferences() { User user = authService.getUserFromGithubId(); return repository.findByUser(user).stream() diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java index 0096c230f..a2abd77b6 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java @@ -30,6 +30,7 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; import org.springframework.util.StringUtils; @Log4j2 @@ -41,6 +42,7 @@ public class PullRequestService { private final UserPreferenceRepository userPreferenceRepository; private final AuthService authService; + @Transactional(readOnly = true) public List getAllPullRequests() { final Optional userPreference = authService.isLoggedIn() @@ -66,6 +68,7 @@ public List getAllPullRequests() { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public PaginatedPullRequestsResponse getPaginatedPullRequests( PullRequestPageRequest pageRequest) { log.debug( @@ -311,6 +314,7 @@ private Sort resolveSort(PullRequestPageRequest pageRequest) { return Sort.by(direction, property); } + @Transactional(readOnly = true) public Optional getPullRequestById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); Optional pullRequest = @@ -320,6 +324,7 @@ public Optional getPullRequestById(Long id) { return pullRequest.map(PullRequestInfoDto::fromPullRequest); } + @Transactional(readOnly = true) public List getPullRequestByRepositoryId(Long repositoryId) { return pullRequestRepository .findByRepositoryRepositoryIdOrderByUpdatedAtDesc(repositoryId) @@ -353,6 +358,7 @@ public void setPrPinnedByNumberAndUserId(Long prId, Boolean isPinned) { userPreferenceRepository.save(userPreference); } + @Transactional(readOnly = true) public Optional getPullRequestByRepositoryIdAndNumber( Long repoId, Integer number) { return pullRequestRepository @@ -360,6 +366,7 @@ public Optional getPullRequestByRepositoryIdAndNumber( .map(PullRequestInfoDto::fromPullRequest); } + @Transactional(readOnly = true) public PullRequestFilterOptionsDto getPullRequestFilterOptionsByRepositoryId(Long repositoryId) { List authors = pullRequestRepository.findDistinctAuthorsByRepositoryId(repositoryId).stream() diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java index a3700bc5a..3c15d5388 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java @@ -30,7 +30,6 @@ import de.tum.cit.aet.helios.user.User; import de.tum.cit.aet.helios.user.UserInfoDto; import de.tum.cit.aet.helios.user.UserRepository; -import jakarta.transaction.Transactional; import java.io.IOException; import java.time.OffsetDateTime; import java.util.ArrayList; @@ -44,6 +43,7 @@ import org.kohsuke.github.GHRelease; import org.kohsuke.github.GHRepository; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @Log4j2 @@ -63,6 +63,7 @@ public class ReleaseInfoService { private final GitHubDataSyncOrchestrator gitHubDataSyncOrchestrator; private final GitHubReleaseSyncService gitHubReleaseSyncService; + @Transactional(readOnly = true) public List getAllReleaseInfos() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -122,6 +123,7 @@ private List getCandidateDeployments(final ReleaseCandida return new ArrayList<>(deploymentsByEnvironment.values()); } + @Transactional(readOnly = true) public ReleaseInfoDetailsDto getReleaseInfoByName(String name) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -154,6 +156,7 @@ public ReleaseInfoDetailsDto getReleaseInfoByName(String name) { .orElseThrow(() -> new ReleaseCandidateException("ReleaseCandidate not found")); } + @Transactional(readOnly = true) public CommitsSinceReleaseCandidateDto getCommitsFromBranchSinceLastReleaseCandidate( String branchName) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -332,6 +335,7 @@ public void publishReleaseDraft(String tagName) { } } + @Transactional(readOnly = true) public String generateReleaseNotes(String tagName) { final Long repositoryId = RepositoryContext.getRepositoryId(); final GitRepository repository = diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java index 3e3fbb89b..a624a9c86 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java @@ -6,6 +6,7 @@ import lombok.extern.log4j.Log4j2; import org.apache.commons.lang3.StringUtils; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -50,6 +51,7 @@ public void handleFirstLogin() { /** * Retrieves the current user's settings (email and global notifications toggle). */ + @Transactional(readOnly = true) public UserSettingsDto getCurrentUserSettings() { User user = authService.getUserFromGithubId(); return new UserSettingsDto(user.getNotificationEmail(), user.isNotificationsEnabled()); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java index 0276266f9..f3c36fa78 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java @@ -25,6 +25,7 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; import org.springframework.util.CollectionUtils; @Log4j2 @@ -49,6 +50,7 @@ private Stream getLatestWorkflowRuns(List runs) { workflowRuns.stream().max(Comparator.comparing(WorkflowRun::getRunNumber)).get()); } + @Transactional(readOnly = true) public List getLatestWorkflowRunsByPullRequestIdAndHeadCommit( Long pullRequestId) { @@ -61,6 +63,7 @@ public List getLatestWorkflowRunsByPullRequestIdAndHeadCommit( return getLatestWorkflowRunsByBranchAndHeadCommitSha(pullRequest.getHeadRefName()); } + @Transactional(readOnly = true) public List getLatestWorkflowRunsByBranchAndHeadCommitSha(String branchName) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -78,6 +81,7 @@ public List getLatestWorkflowRunsByBranchAndHeadCommitSha(String return latestRuns.map(WorkflowRunDto::fromWorkflowRun).toList(); } + @Transactional(readOnly = true) public PaginatedWorkflowRunsResponse getPaginatedWorkflowRuns(WorkflowRunPageRequest request) { Long repositoryId = RepositoryContext.getRepositoryId(); @@ -201,6 +205,7 @@ private Sort resolveSort(WorkflowRunPageRequest request) { return Sort.by(direction, property).and(defaultSort); } + @Transactional(readOnly = true) public WorkflowRunDto getWorkflowRunById(Long runId) { Long repositoryId = RepositoryContext.getRepositoryId(); return getWorkflowRunForCurrentRepository(runId, repositoryId, false) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java index 963c4805a..6bcdaf400 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java @@ -6,12 +6,14 @@ import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor public class WorkflowService { private final WorkflowRepository workflowRepository; + @Transactional(readOnly = true) public Optional getWorkflowById(Long id) { return findScopedById(id).map(WorkflowDto::fromWorkflow); } @@ -28,6 +30,7 @@ private Optional findScopedById(Long id) { : workflowRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } + @Transactional(readOnly = true) public List getAllWorkflows() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -40,6 +43,7 @@ public List getAllWorkflows() { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public List getWorkflowsByRepositoryId(Long repositoryId) { return workflowRepository .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) @@ -48,6 +52,7 @@ public List getWorkflowsByRepositoryId(Long repositoryId) { .collect(Collectors.toList()); } + @Transactional(readOnly = true) public List getWorkflowsByState(Workflow.State state) { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -69,11 +74,13 @@ public void updateWorkflowLabel(Long workflowId, Workflow.Label label) { workflowRepository.save(workflow); } + @Transactional(readOnly = true) public List getDeploymentWorkflowsForAllEnv(Long repositoryId) { return workflowRepository.findDeploymentWorkflowsForEnabledEnvironmentsByRepositoryId( repositoryId); } + @Transactional(readOnly = true) public List getTestWorkflows(Long repositoryId) { return workflowRepository.findByLabelAndRepositoryRepositoryId( Workflow.Label.TEST, repositoryId); From 9258efd261fff6e8a454c8c990aa6c5862c3f703 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 09:07:24 +0200 Subject: [PATCH 22/28] fix(tx): run TestResultService reads @Transactional(readOnly = true) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Staging smoke test: GET /api/tests/pr/{id} for a valid same-repo PR returned 500 ("Large Objects may not be used in auto-commit mode") — the test-result graph reads PR/release Large Objects, which require a transaction. getLatestTestResultsForPr/ForBranch/ForWorkflowRun now run readOnly. Verified no real writes in the service (the flakiness/status setters target @Transient fields, so readOnly is safe). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/de/tum/cit/aet/helios/tests/TestResultService.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java index fb936fad4..355929393 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java @@ -24,6 +24,7 @@ import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -149,6 +150,9 @@ private TestRunContext getDefaultBranchContext(GitRepository repository) { * @param branchName the branch name * @return the grouped test results */ + // readOnly tx: the test-result graph reads Large Objects (PR/release bodies), which Postgres + // forbids in auto-commit. No real writes here (setters target @Transient fields). + @Transactional(readOnly = true) public TestResultsDto getLatestTestResultsForBranch( String branchName, TestSearchCriteria criteria) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -191,6 +195,7 @@ public TestResultsDto getLatestTestResultsForBranch( * @param pullRequestId the pull request ID * @return the grouped test results */ + @Transactional(readOnly = true) public TestResultsDto getLatestTestResultsForPr(Long pullRequestId, TestSearchCriteria criteria) { // Scope to the current repository (like the run/branch variants): a PR id from another // repository must not surface its test results. Null context → not found. @@ -234,6 +239,7 @@ public TestResultsDto getLatestTestResultsForPr(Long pullRequestId, TestSearchCr * @param criteria search and pagination criteria * @return the grouped test results */ + @Transactional(readOnly = true) public TestResultsDto getTestResultsForWorkflowRun( Long workflowRunId, TestSearchCriteria criteria) { final Long repositoryId = RepositoryContext.getRepositoryId(); From db87b2809e8db11f5ebf820480637051b90324ff Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 13:27:07 +0200 Subject: [PATCH 23/28] refactor(tx): disable JDBC auto-commit instead of annotating reads @Transactional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn off Hikari auto-commit (spring.datasource.hikari.auto-commit=false) and tell Hibernate the pool already disabled it (hibernate.connection.provider_disables_autocommit=true). Connections now start inside a transaction, so reads that run outside an explicit @Transactional (under Open-Session-in-View) can still access PostgreSQL Large Objects (@Lob PR body, oid release/RC bodies) — the exact failure that 500'd on staging — without needing @Transactional(readOnly=true) sprinkled across every read. This is also a Hibernate-recommended setting (skips per-transaction auto-commit toggling). So the readOnly annotations added as the interim fix are removed again: reads are annotation-free, writes stay transaction-free except the two derived deletes (Branch/ReleaseInfo) that Spring requires a transaction for. Test profile override: zonky's embedded-postgres DataSource ignores hikari.auto-commit and hands out auto-commit=true connections, so provider_disables_autocommit=true would break tx rollback there (Cannot rollback when autoCommit is enabled). application-test.yml sets it back to false; prod/staging (real Hikari, auto-commit=false) keep the base true. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tum/cit/aet/helios/branch/BranchService.java | 2 -- .../aet/helios/deployment/DeploymentService.java | 10 ---------- .../aet/helios/environment/EnvironmentService.java | 13 ------------- .../gitreposettings/GitRepoSettingsService.java | 2 -- .../NotificationPreferenceService.java | 2 -- .../aet/helios/pullrequest/PullRequestService.java | 7 ------- .../aet/helios/releaseinfo/ReleaseInfoService.java | 4 ---- .../cit/aet/helios/tests/TestResultService.java | 6 ------ .../de/tum/cit/aet/helios/user/UserService.java | 2 -- .../aet/helios/workflow/WorkflowRunService.java | 5 ----- .../cit/aet/helios/workflow/WorkflowService.java | 7 ------- .../src/main/resources/application-test.yml | 10 ++++++++++ .../src/main/resources/application.yml | 14 ++++++++++++++ 13 files changed, 24 insertions(+), 60 deletions(-) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java index a8847a883..84d20d943 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/branch/BranchService.java @@ -27,7 +27,6 @@ public class BranchService { private final AuthService authService; private final CommitRepository commitRepository; - @Transactional(readOnly = true) public List getAllBranches(String sortField, String sortDirection) { final Optional userPreference = authService.isLoggedIn() ? userPreferenceRepository.findByUser(authService.getUserFromGithubId()) @@ -63,7 +62,6 @@ public void deleteBranchByNameAndRepositoryId(String name, Long repositoryId) { branchRepository.deleteByNameAndRepositoryRepositoryId(name, repositoryId); } - @Transactional(readOnly = true) public Optional getBranchByRepositoryIdAndName(Long repositoryId, String name) { return branchRepository .findByRepositoryRepositoryIdAndName(repositoryId, name) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java index f35779847..5f6a98887 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/deployment/DeploymentService.java @@ -36,7 +36,6 @@ import lombok.RequiredArgsConstructor; import lombok.extern.log4j.Log4j2; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Log4j2 @Service @@ -58,7 +57,6 @@ public class DeploymentService { private final HeliosDeploymentWorkflowRunSyncService heliosDeploymentWorkflowRunSyncService; private final WorkflowRunRepository workflowRunRepository; - @Transactional(readOnly = true) public Optional getDeploymentById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); Optional deployment = @@ -68,7 +66,6 @@ public Optional getDeploymentById(Long id) { return deployment.map(DeploymentDto::fromDeployment); } - @Transactional(readOnly = true) public List getAllDeployments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -91,7 +88,6 @@ private boolean environmentInCurrentRepository(Long environmentId) { .isPresent(); } - @Transactional(readOnly = true) public List getDeploymentsByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return List.of(); @@ -101,7 +97,6 @@ public List getDeploymentsByEnvironmentId(Long environmentId) { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public Optional getLatestDeploymentByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return Optional.empty(); @@ -204,7 +199,6 @@ private Map createWorkflowParams( return workflowParams; } - @Transactional(readOnly = true) public boolean canDeployToEnvironment(Environment.Type environmentType) { if (null != environmentType) { switch (environmentType) { @@ -328,7 +322,6 @@ private boolean canRedeploy(Environment environment, long timeoutMinutes) { *
  • Requires separate environment lookup to verify deployment recency * */ - @Transactional(readOnly = true) public List getActivityHistoryByEnvironmentId(Long environmentId) { if (!environmentInCurrentRepository(environmentId)) { return List.of(); @@ -390,7 +383,6 @@ public List getActivityHistoryByEnvironmentId(Long environme * descending * @throws EntityNotFoundException if the pull request does not exist */ - @Transactional(readOnly = true) public List getActivityHistoryByPullRequestId(Long pullRequestId) { if (pullRequestRepository .findByIdAndRepositoryRepositoryId(pullRequestId, RepositoryContext.getRepositoryId()) @@ -428,7 +420,6 @@ public List getActivityHistoryByPullRequestId(Long pullReque * @return Combined list of {@link ActivityHistoryDto} for the branch, sorted by timestamp * descending */ - @Transactional(readOnly = true) public List getActivityHistoryByRepositoryIdAndBranchName( Long repositoryId, String branchName) { List combined = new ArrayList<>(); @@ -532,7 +523,6 @@ private boolean syncTerminalWorkflowRunStateAfterFailedCancellation(Long workflo * @return List of workflow jobs with their steps * @throws DeploymentException if there's an error fetching the workflow job status */ - @Transactional(readOnly = true) public WorkflowJobsResponse getWorkflowJobStatus(Long workflowRunId) { try { String repoNameWithOwner = resolveWorkflowRunRepositoryName(workflowRunId); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java index 615f2192a..cc8781d3b 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/environment/EnvironmentService.java @@ -48,7 +48,6 @@ import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.PageRequest; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service @Log4j2 @@ -75,12 +74,10 @@ public class EnvironmentService { private final GitHubService gitHubService; private final NatsNotificationPublisherService notificationPublisherService; - @Transactional(readOnly = true) public Optional getEnvironmentById(Long id) { return findScopedById(id).map(EnvironmentDto::fromEnvironment); } - @Transactional(readOnly = true) public Optional getEnvironmentTypeById(Long id) { return findScopedById(id).map(Environment::getType); } @@ -100,7 +97,6 @@ private Optional findScopedById(Long id) { : environmentRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } - @Transactional(readOnly = true) public List getAllEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -118,7 +114,6 @@ public List getAllEnvironments() { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public List getAllEnabledEnvironments() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -138,7 +133,6 @@ public List getAllEnabledEnvironments() { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public List getEnvironmentsByRepositoryId(Long repositoryId) { return environmentRepository .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) @@ -185,7 +179,6 @@ private DeploymentDurationEstimate computeEstimate(Environment environment) { * @return A wrapper object containing the latest Deployment or HeliosDeployment, or an empty * result if no deployments exist. */ - @Transactional(readOnly = true) public LatestDeploymentUnion findLatestDeployment(Environment env) { // Retrieve the latest HeliosDeployment Optional latestHeliosOpt = @@ -665,7 +658,6 @@ private void updateRequiredPreDeploymentWorkflows( environment.setRequiredPreDeploymentWorkflows(new ArrayList<>(workflows)); } - @Transactional(readOnly = true) public EnvironmentDeploymentReadinessDto getDeploymentReadiness( Long environmentId, String branch, String sha) { Environment environment = @@ -776,7 +768,6 @@ public void updateLockExpirationAndReservation(Long repositoryId) { } } - @Transactional(readOnly = true) public EnvironmentLockHistoryDto getUsersCurrentLock() { final User currentUser = authService.getUserFromGithubId(); Optional lockHistory = @@ -790,7 +781,6 @@ public EnvironmentLockHistoryDto getUsersCurrentLock() { .orElse(null); } - @Transactional(readOnly = true) public List getLockHistoryByEnvironmentId(Long environmentId) { // Scope to the current repository (lock history is keyed only by environment id, so a foreign // id would otherwise leak its lock/unlock history). @@ -839,7 +829,6 @@ private boolean isActiveDeploymentStatus(HeliosDeployment.Status status) { || status == HeliosDeployment.Status.IN_PROGRESS; } - @Transactional(readOnly = true) public Optional getEnvironmentReviewers(Long environmentId) { // Scope to the current repository — protection rules are keyed only by environment id. if (findScopedById(environmentId).isEmpty()) { @@ -900,7 +889,6 @@ public Optional getEnvironmentReviewers(Long environmen * the deployment is not gated on reviewers (it may still be gated on wait-timer or branch * policy, which GitHub handles itself). */ - @Transactional(readOnly = true) public Optional resolveReviewers(Long environmentId) { return protectionRuleRepository .findByEnvironmentIdAndRuleType(environmentId, ProtectionRule.RuleType.REQUIRED_REVIEWERS) @@ -947,7 +935,6 @@ public Optional resolveReviewers(Long environmentId) { * Returns {@code false} if there's no rule, or only Team-type reviewers (which Helios doesn't yet * expand to members). */ - @Transactional(readOnly = true) public boolean isUserRequiredReviewer(Long environmentId, String userLogin) { if (userLogin == null) { return false; diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java index 7008e9597..32807f573 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsService.java @@ -7,7 +7,6 @@ import lombok.RequiredArgsConstructor; import org.springframework.context.annotation.Lazy; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; @@ -49,7 +48,6 @@ public Optional getOrCreateGitRepoSettingsByRepositoryId(Lon * defaults when none exist yet — WITHOUT writing. (getOrCreate persists a row; a plain GET * must not do; callers that need the row to exist keep using getOrCreate.) */ - @Transactional(readOnly = true) public GitRepoSettingsDto getGitRepoSettingsByRepositoryId(Long repositoryId) { return gitRepoRepository .findByRepositoryRepositoryId(repositoryId) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java index fa22c357d..df9fdccc8 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/notification/NotificationPreferenceService.java @@ -5,7 +5,6 @@ import java.util.List; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -23,7 +22,6 @@ public void initializeDefaultsForUser(User user) { } } - @Transactional(readOnly = true) public List getCurrentUserPreferences() { User user = authService.getUserFromGithubId(); return repository.findByUser(user).stream() diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java index a2abd77b6..0096c230f 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/pullrequest/PullRequestService.java @@ -30,7 +30,6 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; import org.springframework.util.StringUtils; @Log4j2 @@ -42,7 +41,6 @@ public class PullRequestService { private final UserPreferenceRepository userPreferenceRepository; private final AuthService authService; - @Transactional(readOnly = true) public List getAllPullRequests() { final Optional userPreference = authService.isLoggedIn() @@ -68,7 +66,6 @@ public List getAllPullRequests() { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public PaginatedPullRequestsResponse getPaginatedPullRequests( PullRequestPageRequest pageRequest) { log.debug( @@ -314,7 +311,6 @@ private Sort resolveSort(PullRequestPageRequest pageRequest) { return Sort.by(direction, property); } - @Transactional(readOnly = true) public Optional getPullRequestById(Long id) { Long repositoryId = RepositoryContext.getRepositoryId(); Optional pullRequest = @@ -324,7 +320,6 @@ public Optional getPullRequestById(Long id) { return pullRequest.map(PullRequestInfoDto::fromPullRequest); } - @Transactional(readOnly = true) public List getPullRequestByRepositoryId(Long repositoryId) { return pullRequestRepository .findByRepositoryRepositoryIdOrderByUpdatedAtDesc(repositoryId) @@ -358,7 +353,6 @@ public void setPrPinnedByNumberAndUserId(Long prId, Boolean isPinned) { userPreferenceRepository.save(userPreference); } - @Transactional(readOnly = true) public Optional getPullRequestByRepositoryIdAndNumber( Long repoId, Integer number) { return pullRequestRepository @@ -366,7 +360,6 @@ public Optional getPullRequestByRepositoryIdAndNumber( .map(PullRequestInfoDto::fromPullRequest); } - @Transactional(readOnly = true) public PullRequestFilterOptionsDto getPullRequestFilterOptionsByRepositoryId(Long repositoryId) { List authors = pullRequestRepository.findDistinctAuthorsByRepositoryId(repositoryId).stream() diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java index 3c15d5388..c65542f6b 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoService.java @@ -63,7 +63,6 @@ public class ReleaseInfoService { private final GitHubDataSyncOrchestrator gitHubDataSyncOrchestrator; private final GitHubReleaseSyncService gitHubReleaseSyncService; - @Transactional(readOnly = true) public List getAllReleaseInfos() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -123,7 +122,6 @@ private List getCandidateDeployments(final ReleaseCandida return new ArrayList<>(deploymentsByEnvironment.values()); } - @Transactional(readOnly = true) public ReleaseInfoDetailsDto getReleaseInfoByName(String name) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -156,7 +154,6 @@ public ReleaseInfoDetailsDto getReleaseInfoByName(String name) { .orElseThrow(() -> new ReleaseCandidateException("ReleaseCandidate not found")); } - @Transactional(readOnly = true) public CommitsSinceReleaseCandidateDto getCommitsFromBranchSinceLastReleaseCandidate( String branchName) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -335,7 +332,6 @@ public void publishReleaseDraft(String tagName) { } } - @Transactional(readOnly = true) public String generateReleaseNotes(String tagName) { final Long repositoryId = RepositoryContext.getRepositoryId(); final GitRepository repository = diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java index 355929393..fb936fad4 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/tests/TestResultService.java @@ -24,7 +24,6 @@ import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -150,9 +149,6 @@ private TestRunContext getDefaultBranchContext(GitRepository repository) { * @param branchName the branch name * @return the grouped test results */ - // readOnly tx: the test-result graph reads Large Objects (PR/release bodies), which Postgres - // forbids in auto-commit. No real writes here (setters target @Transient fields). - @Transactional(readOnly = true) public TestResultsDto getLatestTestResultsForBranch( String branchName, TestSearchCriteria criteria) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -195,7 +191,6 @@ public TestResultsDto getLatestTestResultsForBranch( * @param pullRequestId the pull request ID * @return the grouped test results */ - @Transactional(readOnly = true) public TestResultsDto getLatestTestResultsForPr(Long pullRequestId, TestSearchCriteria criteria) { // Scope to the current repository (like the run/branch variants): a PR id from another // repository must not surface its test results. Null context → not found. @@ -239,7 +234,6 @@ public TestResultsDto getLatestTestResultsForPr(Long pullRequestId, TestSearchCr * @param criteria search and pagination criteria * @return the grouped test results */ - @Transactional(readOnly = true) public TestResultsDto getTestResultsForWorkflowRun( Long workflowRunId, TestSearchCriteria criteria) { final Long repositoryId = RepositoryContext.getRepositoryId(); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java index a624a9c86..3e3fbb89b 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/user/UserService.java @@ -6,7 +6,6 @@ import lombok.extern.log4j.Log4j2; import org.apache.commons.lang3.StringUtils; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor @@ -51,7 +50,6 @@ public void handleFirstLogin() { /** * Retrieves the current user's settings (email and global notifications toggle). */ - @Transactional(readOnly = true) public UserSettingsDto getCurrentUserSettings() { User user = authService.getUserFromGithubId(); return new UserSettingsDto(user.getNotificationEmail(), user.isNotificationsEnabled()); diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java index f3c36fa78..0276266f9 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowRunService.java @@ -25,7 +25,6 @@ import org.springframework.data.domain.Sort; import org.springframework.data.jpa.domain.Specification; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; import org.springframework.util.CollectionUtils; @Log4j2 @@ -50,7 +49,6 @@ private Stream getLatestWorkflowRuns(List runs) { workflowRuns.stream().max(Comparator.comparing(WorkflowRun::getRunNumber)).get()); } - @Transactional(readOnly = true) public List getLatestWorkflowRunsByPullRequestIdAndHeadCommit( Long pullRequestId) { @@ -63,7 +61,6 @@ public List getLatestWorkflowRunsByPullRequestIdAndHeadCommit( return getLatestWorkflowRunsByBranchAndHeadCommitSha(pullRequest.getHeadRefName()); } - @Transactional(readOnly = true) public List getLatestWorkflowRunsByBranchAndHeadCommitSha(String branchName) { final Long repositoryId = RepositoryContext.getRepositoryId(); @@ -81,7 +78,6 @@ public List getLatestWorkflowRunsByBranchAndHeadCommitSha(String return latestRuns.map(WorkflowRunDto::fromWorkflowRun).toList(); } - @Transactional(readOnly = true) public PaginatedWorkflowRunsResponse getPaginatedWorkflowRuns(WorkflowRunPageRequest request) { Long repositoryId = RepositoryContext.getRepositoryId(); @@ -205,7 +201,6 @@ private Sort resolveSort(WorkflowRunPageRequest request) { return Sort.by(direction, property).and(defaultSort); } - @Transactional(readOnly = true) public WorkflowRunDto getWorkflowRunById(Long runId) { Long repositoryId = RepositoryContext.getRepositoryId(); return getWorkflowRunForCurrentRepository(runId, repositoryId, false) diff --git a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java index 6bcdaf400..963c4805a 100644 --- a/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java +++ b/server/application-server/src/main/java/de/tum/cit/aet/helios/workflow/WorkflowService.java @@ -6,14 +6,12 @@ import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor public class WorkflowService { private final WorkflowRepository workflowRepository; - @Transactional(readOnly = true) public Optional getWorkflowById(Long id) { return findScopedById(id).map(WorkflowDto::fromWorkflow); } @@ -30,7 +28,6 @@ private Optional findScopedById(Long id) { : workflowRepository.findByIdAndRepositoryRepositoryId(id, repositoryId); } - @Transactional(readOnly = true) public List getAllWorkflows() { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -43,7 +40,6 @@ public List getAllWorkflows() { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public List getWorkflowsByRepositoryId(Long repositoryId) { return workflowRepository .findByRepositoryRepositoryIdOrderByCreatedAtDesc(repositoryId) @@ -52,7 +48,6 @@ public List getWorkflowsByRepositoryId(Long repositoryId) { .collect(Collectors.toList()); } - @Transactional(readOnly = true) public List getWorkflowsByState(Workflow.State state) { Long repositoryId = RepositoryContext.getRepositoryId(); if (repositoryId == null) { @@ -74,13 +69,11 @@ public void updateWorkflowLabel(Long workflowId, Workflow.Label label) { workflowRepository.save(workflow); } - @Transactional(readOnly = true) public List getDeploymentWorkflowsForAllEnv(Long repositoryId) { return workflowRepository.findDeploymentWorkflowsForEnabledEnvironmentsByRepositoryId( repositoryId); } - @Transactional(readOnly = true) public List getTestWorkflows(Long repositoryId) { return workflowRepository.findByLabelAndRepositoryRepositoryId( Workflow.Label.TEST, repositoryId); diff --git a/server/application-server/src/main/resources/application-test.yml b/server/application-server/src/main/resources/application-test.yml index 06355a793..c3cd27092 100644 --- a/server/application-server/src/main/resources/application-test.yml +++ b/server/application-server/src/main/resources/application-test.yml @@ -15,6 +15,16 @@ spring: issuer-uri: "url.invalid" flyway: enabled: true + jpa: + properties: + hibernate: + connection: + # zonky's embedded-postgres DataSource hands out auto-commit=true connections and + # ignores spring.datasource.hikari.auto-commit, so the base + # provider_disables_autocommit=true would be a lie here and break tx rollback. + # Let Hibernate manage auto-commit in tests; prod/staging keep the base true (real + # Hikari with auto-commit=false) so Large-Object reads work without @Transactional. + provider_disables_autocommit: false nats: enabled: false diff --git a/server/application-server/src/main/resources/application.yml b/server/application-server/src/main/resources/application.yml index 1124feb02..9f2f2a61d 100644 --- a/server/application-server/src/main/resources/application.yml +++ b/server/application-server/src/main/resources/application.yml @@ -20,6 +20,20 @@ spring: pool: # This sets the maximum number of threads for scheduled tasks. size: 10 + datasource: + hikari: + # Hand out pooled connections with auto-commit OFF so reads that run outside an explicit + # @Transactional (under Open-Session-in-View) still execute inside a DB transaction. + # Required for PostgreSQL Large Objects (the @Lob PR body + oid release/RC bodies), which + # cannot be accessed in auto-commit mode — and lets read methods stay annotation-free. + auto-commit: false + jpa: + properties: + hibernate: + connection: + # The pool already disables auto-commit, so Hibernate skips toggling it per + # transaction (saves a round-trip each way) and delays connection acquisition. + provider_disables_autocommit: true springdoc: default-produces-media-type: application/json From 677cd3ed75bda3f22dfc76604fb4d59799b419a4 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 14:01:12 +0200 Subject: [PATCH 24/28] test(tenancy): add PullRequestScopingIT (real-path cross-repo guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-context integration test for /api/pullrequests* over two seeded repositories: - GET /{id}: own repo → 200 (asserts id + repository.id), cross-repo → 404, no header → 404. - GET /repository/{id}: returns only that repo's PRs (count + every repository.id). - GET /api/pullrequests (paginated): scoped by totalNonPinned + the returned PR ids (repo A sees 31,32; repo B sees exactly 41); no header → empty page, totalNonPinned 0. 5 tests, 20+ assertions. Seeds PRs as single-table issue rows (issue_type=PULL_REQUEST) with all primitive/@NonNull columns populated so entity load + DTO mapping exercise the real request path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pullrequest/PullRequestScopingIT.java | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestScopingIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestScopingIT.java new file mode 100644 index 000000000..d7809d6ec --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/pullrequest/PullRequestScopingIT.java @@ -0,0 +1,131 @@ +package de.tum.cit.aet.helios.pullrequest; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.everyItem; +import static org.hamcrest.Matchers.hasItems; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-repository isolation guard for {@code /api/pullrequests*}. Detail-by-id scopes explicitly; + * with {@code GET /api/**} unauthenticated, a missing {@code X-REPOSITORY-ID} must 404, not leak. + */ +class PullRequestScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long PR_A1 = 31L; + private static final long PR_A2 = 32L; + private static final long PR_B1 = 41L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertPr(jdbc, PR_A1, REPO_A, 1, "A-first"); + insertPr(jdbc, PR_A2, REPO_A, 2, "A-second"); + insertPr(jdbc, PR_B1, REPO_B, 1, "B-first"); + } + + @Test + void pullRequestByIdIsInvisibleFromAnotherRepository() throws Exception { + // PR_B1 belongs to repo B. + mockMvc + .perform( + get("/api/pullrequests/{id}", PR_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.id").value((int) PR_B1)) + .andExpect(jsonPath("$.repository.id").value((int) REPO_B)); + mockMvc + .perform( + get("/api/pullrequests/{id}", PR_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + } + + @Test + void pullRequestByIdWithoutHeaderIsNotFound() throws Exception { + // No header → null context → must not fall back to an unscoped findById (anonymous IDOR). + mockMvc.perform(get("/api/pullrequests/{id}", PR_A1)).andExpect(status().isNotFound()); + } + + @Test + void pullRequestsByRepositoryIdReturnOnlyThatRepository() throws Exception { + mockMvc + .perform(get("/api/pullrequests/repository/{id}", REPO_A)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); + mockMvc + .perform(get("/api/pullrequests/repository/{id}", REPO_B)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].id").value((int) PR_B1)) + .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); + } + + @Test + void paginatedPullRequestsAreScopedToCurrentRepository() throws Exception { + mockMvc + .perform( + get("/api/pullrequests") + .param("page", "1") + .param("size", "50") + .header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalNonPinned").value(2)) + .andExpect(jsonPath("$.page.length()").value(2)) + .andExpect(jsonPath("$.page[*].id", hasItems((int) PR_A1, (int) PR_A2))); + mockMvc + .perform( + get("/api/pullrequests") + .param("page", "1") + .param("size", "50") + .header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalNonPinned").value(1)) + .andExpect(jsonPath("$.page.length()").value(1)) + .andExpect(jsonPath("$.page[0].id").value((int) PR_B1)); + } + + @Test + void paginatedPullRequestsWithoutHeaderAreEmpty() throws Exception { + mockMvc + .perform(get("/api/pullrequests").param("page", "1").param("size", "50")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalNonPinned").value(0)) + .andExpect(jsonPath("$.page.length()").value(0)); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertPr(JdbcTemplate jdbc, long id, long repositoryId, int number, + String title) { + jdbc.update( + "INSERT INTO issue (id, repository_id, issue_type, number, additions, deletions, " + + "changed_files, commits, comments_count, is_locked, is_draft, is_merged, " + + "is_mergeable, maintainer_can_modify, state, title, html_url, created_at, " + + "updated_at) VALUES " + + "(?, ?, 'PULL_REQUEST', ?, 0, 0, 0, 0, 0, false, false, false, false, false, " + + "'OPEN', ?, ?, now(), now())", + id, + repositoryId, + number, + title, + "https://example/pr/" + id); + } +} From 628d45d2cc455f116b2b6429b9477fcad4029601 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 14:04:49 +0200 Subject: [PATCH 25/28] test(tenancy): add WorkflowRunScopingIT (real-path cross-repo guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-context integration test for /api/workflows/runs* over two seeded repositories: - GET /runs/{id}: own repo → 200 (asserts id), cross-repo → 404, no header → 404. - GET /runs (paginated): scoped by totalElements + the returned run ids (repo A sees 61,62; repo B sees exactly 71); no header → empty runs, totalElements 0. 4 tests. Seeds workflow + workflow_run rows with all primitive/@NonNull columns populated so entity load + WorkflowRunDto mapping exercise the real request path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../helios/workflow/WorkflowRunScopingIT.java | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunScopingIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunScopingIT.java new file mode 100644 index 000000000..d99b3a65d --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/workflow/WorkflowRunScopingIT.java @@ -0,0 +1,117 @@ +package de.tum.cit.aet.helios.workflow; + +import static org.hamcrest.Matchers.hasItems; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-repository isolation guard for {@code /api/workflows/runs*}. Detail-by-id scopes with no + * findById fallback and the paginated query carries a repository predicate; a missing + * {@code X-REPOSITORY-ID} ({@code GET /api/**} is unauthenticated) must 404 / return empty. + */ +class WorkflowRunScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long WF_A = 51L; + private static final long WF_B = 52L; + private static final long RUN_A1 = 61L; + private static final long RUN_A2 = 62L; + private static final long RUN_B1 = 71L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertWorkflow(jdbc, WF_A, REPO_A); + insertWorkflow(jdbc, WF_B, REPO_B); + insertRun(jdbc, RUN_A1, REPO_A, WF_A); + insertRun(jdbc, RUN_A2, REPO_A, WF_A); + insertRun(jdbc, RUN_B1, REPO_B, WF_B); + } + + @Test + void workflowRunByIdIsInvisibleFromAnotherRepository() throws Exception { + mockMvc + .perform(get("/api/workflows/runs/{id}", RUN_B1).header(X_REPOSITORY_ID, str(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.id").value((int) RUN_B1)); + mockMvc + .perform(get("/api/workflows/runs/{id}", RUN_B1).header(X_REPOSITORY_ID, str(REPO_A))) + .andExpect(status().isNotFound()); + } + + @Test + void workflowRunByIdWithoutHeaderIsNotFound() throws Exception { + // No header → null context → must not fall back to an unscoped findById (anonymous IDOR). + mockMvc.perform(get("/api/workflows/runs/{id}", RUN_A1)).andExpect(status().isNotFound()); + } + + @Test + void paginatedWorkflowRunsAreScopedToCurrentRepository() throws Exception { + mockMvc + .perform(get("/api/workflows/runs").param("page", "1").param("size", "50") + .header(X_REPOSITORY_ID, str(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalElements").value(2)) + .andExpect(jsonPath("$.runs.length()").value(2)) + .andExpect(jsonPath("$.runs[*].id", hasItems((int) RUN_A1, (int) RUN_A2))); + mockMvc + .perform(get("/api/workflows/runs").param("page", "1").param("size", "50") + .header(X_REPOSITORY_ID, str(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalElements").value(1)) + .andExpect(jsonPath("$.runs.length()").value(1)) + .andExpect(jsonPath("$.runs[0].id").value((int) RUN_B1)); + } + + @Test + void paginatedWorkflowRunsWithoutHeaderAreEmpty() throws Exception { + mockMvc + .perform(get("/api/workflows/runs").param("page", "1").param("size", "50")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.totalElements").value(0)) + .andExpect(jsonPath("$.runs.length()").value(0)); + } + + private static String str(long id) { + return String.valueOf(id); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertWorkflow(JdbcTemplate jdbc, long id, long repositoryId) { + jdbc.update( + "INSERT INTO workflow (id, repository_id, state, label, name) " + + "VALUES (?, ?, 'ACTIVE', 'NONE', ?)", + id, + repositoryId, + "wf-" + id); + } + + private static void insertRun(JdbcTemplate jdbc, long id, long repositoryId, long workflowId) { + jdbc.update( + "INSERT INTO workflow_run (id, repository_id, workflow_id, run_attempt, run_number, " + + "status, head_branch, head_sha, created_at, updated_at) " + + "VALUES (?, ?, ?, 1, ?, 'COMPLETED', 'main', 'deadbeef', now(), now())", + id, + repositoryId, + workflowId, + id); + } +} From a667ee54d5a43bed9735bbadd570eb4d4dc46d49 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 14:09:39 +0200 Subject: [PATCH 26/28] test(tenancy): add ReleaseInfoScopingIT (real-path cross-repo guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-context integration test for GET /api/release-info over two seeded repositories: - header A → only repo A's release candidates (count 2, names rc-a1/rc-a2). - header B → only repo B's (count 1, name rc-b1). - no header → empty list (never a cross-repo or unscoped read). Seeds commit + release_candidate rows (release_candidate.commit is a required composite-key FK), so getAllReleaseInfos + ReleaseInfoListDto.fromReleaseCandidate map on the real request path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../releaseinfo/ReleaseInfoScopingIT.java | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoScopingIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoScopingIT.java new file mode 100644 index 000000000..d74eed30c --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/releaseinfo/ReleaseInfoScopingIT.java @@ -0,0 +1,86 @@ +package de.tum.cit.aet.helios.releaseinfo; + +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-repository isolation guard for {@code GET /api/release-info}. Release candidates are scoped + * by the current repository; with {@code GET /api/**} unauthenticated, a missing + * {@code X-REPOSITORY-ID} must return an empty list (never a cross-repo or unscoped read). + */ +class ReleaseInfoScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertCommit(jdbc, "shaA", REPO_A); + insertCommit(jdbc, "shaB", REPO_B); + insertReleaseCandidate(jdbc, 81L, REPO_A, "rc-a1", "shaA"); + insertReleaseCandidate(jdbc, 82L, REPO_A, "rc-a2", "shaA"); + insertReleaseCandidate(jdbc, 91L, REPO_B, "rc-b1", "shaB"); + } + + @Test + void releaseInfosAreScopedToRepositoryA() throws Exception { + mockMvc + .perform(get("/api/release-info").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(2)) + .andExpect(jsonPath("$[*].name", containsInAnyOrder("rc-a1", "rc-a2"))); + } + + @Test + void releaseInfosAreScopedToRepositoryB() throws Exception { + mockMvc + .perform(get("/api/release-info").header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(1)) + .andExpect(jsonPath("$[0].name").value("rc-b1")); + } + + @Test + void releaseInfosWithoutHeaderAreEmpty() throws Exception { + mockMvc + .perform(get("/api/release-info")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.length()").value(0)); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertCommit(JdbcTemplate jdbc, String sha, long repositoryId) { + jdbc.update("INSERT INTO commit (sha, repository_id) VALUES (?, ?)", sha, repositoryId); + } + + private static void insertReleaseCandidate( + JdbcTemplate jdbc, long id, long repositoryId, String name, String commitSha) { + jdbc.update( + "INSERT INTO release_candidate (id, repository_id, name, created_at, commit_repository_id, " + + "commit_sha) VALUES (?, ?, ?, now(), ?, ?)", + id, + repositoryId, + name, + repositoryId, + commitSha); + } +} From 2d8d2900dfc80518841bc05fab843658555b807d Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 14:12:31 +0200 Subject: [PATCH 27/28] test(tenancy): add TestResultScopingIT (real-path cross-repo guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-context integration test for /api/tests/pr/{id} and /api/tests/run/{id} — the endpoints that a cross-repo leak previously exposed (a foreign PR/run id surfaced another repo's test results): - own repo → 200 with a well-formed empty result set, - cross-repo → 404, - no header (unauthenticated) → 404. Seeds a PR, workflow, workflow_run and a default branch in repo B so the own-repo path maps on the real request path while the scoped entry lookups reject foreign/no-context ids. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../aet/helios/tests/TestResultScopingIT.java | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultScopingIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultScopingIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultScopingIT.java new file mode 100644 index 000000000..2d98cb62b --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/tests/TestResultScopingIT.java @@ -0,0 +1,115 @@ +package de.tum.cit.aet.helios.tests; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Cross-repository isolation guard for {@code /api/tests/*}. Test results are reached via a pull + * request or workflow-run id; a foreign id must not surface another repository's test results. The + * entry lookups are scoped, so cross-repo and no-header (unauthenticated) requests must 404. + */ +class TestResultScopingIT extends HeliosIntegrationTest { + + private static final long REPO_A = 1L; + private static final long REPO_B = 2L; + private static final long PR_B = 41L; + private static final long WF_B = 52L; + private static final long RUN_B = 71L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); + insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); + insertDefaultBranch(jdbc, REPO_B, "main"); + insertPr(jdbc, PR_B, REPO_B, 1, "b-pr"); + insertWorkflow(jdbc, WF_B, REPO_B); + insertRun(jdbc, RUN_B, REPO_B, WF_B); + } + + @Test + void testResultsForPullRequestAreScopedByRepository() throws Exception { + // Own repo → 200 (well-formed, empty result set — no linked runs). + mockMvc + .perform(get("/api/tests/pr/{id}", PR_B).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.testResults.length()").value(0)); + // A PR id from another repository must not surface its test results. + mockMvc + .perform(get("/api/tests/pr/{id}", PR_B).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + // No header → null context → 404 (anonymous, since GET /api/** is permitAll). + mockMvc.perform(get("/api/tests/pr/{id}", PR_B)).andExpect(status().isNotFound()); + } + + @Test + void testResultsForWorkflowRunAreScopedByRepository() throws Exception { + mockMvc + .perform(get("/api/tests/run/{id}", RUN_B).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.testResults.length()").value(0)); + mockMvc + .perform(get("/api/tests/run/{id}", RUN_B).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) + .andExpect(status().isNotFound()); + mockMvc.perform(get("/api/tests/run/{id}", RUN_B)).andExpect(status().isNotFound()); + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } + + private static void insertDefaultBranch(JdbcTemplate jdbc, long repositoryId, String name) { + jdbc.update( + "INSERT INTO branch (name, repository_id, ahead_by, behind_by, is_default, protection) " + + "VALUES (?, ?, 0, 0, true, false)", + name, + repositoryId); + } + + private static void insertPr(JdbcTemplate jdbc, long id, long repositoryId, int number, + String title) { + jdbc.update( + "INSERT INTO issue (id, repository_id, issue_type, number, additions, deletions, " + + "changed_files, commits, comments_count, is_locked, is_draft, is_merged, " + + "is_mergeable, maintainer_can_modify, state, title, html_url, head_sha, created_at, " + + "updated_at) VALUES (?, ?, 'PULL_REQUEST', ?, 0, 0, 0, 0, 0, false, false, false, " + + "false, false, 'OPEN', ?, ?, 'prsha', now(), now())", + id, + repositoryId, + number, + title, + "https://example/pr/" + id); + } + + private static void insertWorkflow(JdbcTemplate jdbc, long id, long repositoryId) { + jdbc.update( + "INSERT INTO workflow (id, repository_id, state, label, name) " + + "VALUES (?, ?, 'ACTIVE', 'NONE', ?)", + id, + repositoryId, + "wf-" + id); + } + + private static void insertRun(JdbcTemplate jdbc, long id, long repositoryId, long workflowId) { + jdbc.update( + "INSERT INTO workflow_run (id, repository_id, workflow_id, run_attempt, run_number, " + + "status, head_branch, head_sha, created_at, updated_at) " + + "VALUES (?, ?, ?, 1, ?, 'COMPLETED', 'main', 'deadbeef', now(), now())", + id, + repositoryId, + workflowId, + id); + } +} From 663d403e7fcf09d0388499c46f6f2fc4726924a6 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 4 Jul 2026 14:15:46 +0200 Subject: [PATCH 28/28] test(settings): add GitRepoSettingsIT (write-on-GET guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full-context integration test proving GET /api/settings/{id}/settings is a pure read: for a repo with no settings row it returns the transient defaults (lockExpirationThreshold 60, lockReservationThreshold 30) and creates no repository_settings row — asserted by querying the table before and after (and after a second GET). Guards the write-on-GET regression fixed earlier. (PUT create-on-absent needs repo-scoped maintainer auth, only mockable in @WebMvcTest slices, so it stays covered by GitRepoSettingsServiceTest.updateGitRepoSettings_createsRowWhenAbsent.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../gitreposettings/GitRepoSettingsIT.java | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsIT.java diff --git a/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsIT.java b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsIT.java new file mode 100644 index 000000000..f075bd272 --- /dev/null +++ b/server/application-server/src/test/java/de/tum/cit/aet/helios/gitreposettings/GitRepoSettingsIT.java @@ -0,0 +1,67 @@ +package de.tum.cit.aet.helios.gitreposettings; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import de.tum.cit.aet.helios.HeliosIntegrationTest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.jdbc.core.JdbcTemplate; + +/** + * Guards the write-on-GET fix: {@code GET /api/settings/{id}/settings} must be a pure read. For a + * repository with no settings row it returns transient defaults and persists nothing. Previously a + * GET INSERTed a row as a side effect, and a plain GET must never write. + */ +class GitRepoSettingsIT extends HeliosIntegrationTest { + + private static final long REPO = 1L; + + @BeforeEach + void seed() { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + jdbc.execute("TRUNCATE TABLE repository CASCADE"); + insertRepo(jdbc, REPO, "ls1intum/repo-a"); + } + + @Test + void getSettingsReturnsDefaultsAndDoesNotPersistARow() throws Exception { + JdbcTemplate jdbc = new JdbcTemplate(dataSource); + assertSettingsRowCount(jdbc, 0); + + mockMvc + .perform(get("/api/settings/{id}/settings", REPO)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.lockExpirationThreshold").value(60)) + .andExpect(jsonPath("$.lockReservationThreshold").value(30)); + + // The GET must not have created a settings row (write-on-GET regression). + assertSettingsRowCount(jdbc, 0); + + // A second GET is still a pure read. + mockMvc.perform(get("/api/settings/{id}/settings", REPO)).andExpect(status().isOk()); + assertSettingsRowCount(jdbc, 0); + } + + private static void assertSettingsRowCount(JdbcTemplate jdbc, int expected) { + Integer count = + jdbc.queryForObject( + "SELECT count(*) FROM repository_settings WHERE repository_id = ?", + Integer.class, + REPO); + if (count == null || count != expected) { + throw new AssertionError( + "expected " + expected + " repository_settings rows but found " + count); + } + } + + private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { + jdbc.update( + "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " + + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " + + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", + id, + nameWithOwner); + } +}