From 0051d327105e507c567cdd083d4660f804bf9ab9 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 08:48:19 +0200 Subject: [PATCH 01/25] refactor: move TrustedPublishingAPI to the trustedpublishing package The controller lived in the root org.eclipse.openvsx package while the rest of the feature (service, config, providers) is in org.eclipse.openvsx.trustedpublishing. Endpoint paths are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../openvsx/{ => trustedpublishing}/TrustedPublishingAPI.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename server/src/main/java/org/eclipse/openvsx/{ => trustedpublishing}/TrustedPublishingAPI.java (98%) diff --git a/server/src/main/java/org/eclipse/openvsx/TrustedPublishingAPI.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java similarity index 98% rename from server/src/main/java/org/eclipse/openvsx/TrustedPublishingAPI.java rename to server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java index 22ac3406d..0b3d0534b 100644 --- a/server/src/main/java/org/eclipse/openvsx/TrustedPublishingAPI.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java @@ -10,7 +10,7 @@ * * SPDX-License-Identifier: EPL-2.0 *****************************************************************************/ -package org.eclipse.openvsx; +package org.eclipse.openvsx.trustedpublishing; import java.util.Objects; @@ -25,6 +25,7 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.server.ResponseStatusException; +import org.eclipse.openvsx.UserService; import org.eclipse.openvsx.eclipse.EclipseService; import org.eclipse.openvsx.entities.TrustedPublisher; import org.eclipse.openvsx.json.AccessTokenJson; @@ -35,7 +36,6 @@ import org.eclipse.openvsx.json.TrustedPublisherStatusJson; import org.eclipse.openvsx.json.TrustedPublisherTokenRequestJson; import org.eclipse.openvsx.settings.MutatingOperation; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingService; import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.NotFoundException; From 078e5d2fd127842b28c103450c1e0086ecb4828e Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 08:48:30 +0200 Subject: [PATCH 02/25] test: add endpoint tests for TrustedPublishingAPI Covers all five endpoints in a @WebMvcTest slice with the service, the Eclipse service and the user service mocked: request validation, status codes, response shape and the mapping of NotFoundException and ErrorResultException. Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingAPITest.java | 540 ++++++++++++++++++ 1 file changed, 540 insertions(+) create mode 100644 server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java new file mode 100644 index 000000000..38146d337 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java @@ -0,0 +1,540 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.trustedpublishing; + +import java.time.LocalDateTime; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import io.micrometer.core.instrument.MeterRegistry; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.security.oauth2.client.autoconfigure.servlet.OAuth2ClientWebSecurityAutoConfiguration; +import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; +import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import tools.jackson.databind.json.JsonMapper; + +import org.eclipse.openvsx.UserService; +import org.eclipse.openvsx.eclipse.EclipseService; +import org.eclipse.openvsx.entities.Extension; +import org.eclipse.openvsx.entities.Namespace; +import org.eclipse.openvsx.entities.TrustedPublisher; +import org.eclipse.openvsx.entities.UserData; +import org.eclipse.openvsx.json.AccessTokenJson; +import org.eclipse.openvsx.json.ResultJson; +import org.eclipse.openvsx.json.TrustedPublisherInputJson; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingService.TrustedPublishers; +import org.eclipse.openvsx.util.ErrorResultException; +import org.eclipse.openvsx.util.NotFoundException; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@WebMvcTest( + value = TrustedPublishingAPI.class, + excludeAutoConfiguration = { OAuth2ClientWebSecurityAutoConfiguration.class } +) +@AutoConfigureMockMvc(addFilters = false) +class TrustedPublishingAPITest { + + private static final String NAMESPACE = "foo"; + + private static final String EXTENSION = "bar"; + + private static final String PROVIDER = "github"; + + private static final Map REGISTRATION = Map + .of("owner", "foo-org", "repository", "bar-repo", "workflow", "publish.yml"); + + @Autowired + MockMvc mockMvc; + + @MockitoBean + MeterRegistry meterRegistry; + + @MockitoBean + UserService users; + + @MockitoBean + EclipseService eclipseService; + + @MockitoBean + TrustedPublishingService trustedPublishing; + + // --------------------------------------------------------------------------------------- + // POST /user/namespace/{namespace}/trusted-publishing/create + // --------------------------------------------------------------------------------------- + + @Test + void createTrustedPublisher_returns403_whenNotLoggedIn() throws Exception { + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isForbidden()); + + verifyNoInteractions(trustedPublishing); + } + + @Test + void createTrustedPublisher_returns400_whenMandatoryFieldsAreMissing() throws Exception { + mockLoggedInUser(); + + var missingProvider = registrationBody(NAMESPACE, EXTENSION, null, REGISTRATION); + var missingExtension = registrationBody(NAMESPACE, null, PROVIDER, REGISTRATION); + var missingNamespace = registrationBody(null, EXTENSION, PROVIDER, REGISTRATION); + var missingRegistration = registrationBody(NAMESPACE, EXTENSION, PROVIDER, Map.of()); + + for (var body : List.of(missingProvider, missingExtension, missingNamespace, missingRegistration)) { + mockMvc.perform(createRequest(NAMESPACE, body)) + .andExpect(status().isBadRequest()) + .andExpect( + jsonPath("$.error") + .value( + "The fields provider, namespace, extension and registration are mandatory.")); + } + + verify(trustedPublishing, never()) + .registerTrustedPublisher(any(), anyString(), anyString(), anyString(), any()); + } + + @Test + void createTrustedPublisher_returns400_whenNamespaceDoesNotMatchPath() throws Exception { + mockLoggedInUser(); + + mockMvc.perform(createRequest("other", registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.error").value("The namespace in the path and in the request body must match.")); + + verify(trustedPublishing, never()) + .registerTrustedPublisher(any(), anyString(), anyString(), anyString(), any()); + } + + @Test + void createTrustedPublisher_returns201_andRegisteredPublisher() throws Exception { + var user = mockLoggedInUser(); + var publisher = trustedPublisher(7L); + when(trustedPublishing.registerTrustedPublisher(user, NAMESPACE, EXTENSION, PROVIDER, REGISTRATION)) + .thenReturn(publisher); + + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.id").value(7)) + .andExpect(jsonPath("$.namespace").value(NAMESPACE)) + .andExpect(jsonPath("$.extension").value(EXTENSION)) + .andExpect(jsonPath("$.provider").value(PROVIDER)) + .andExpect(jsonPath("$.registration.owner").value("foo-org")) + .andExpect(jsonPath("$.registration.repository").value("bar-repo")) + .andExpect(jsonPath("$.registration.workflow").value("publish.yml")) + .andExpect(jsonPath("$.createdTimestamp").value("2026-01-02T03:04:05Z")) + .andExpect(jsonPath("$.error").doesNotExist()); + + verify(eclipseService).checkPublisherAgreement(user); + } + + @Test + void createTrustedPublisher_returns404_whenNamespaceIsUnknown() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.registerTrustedPublisher(user, NAMESPACE, EXTENSION, PROVIDER, REGISTRATION)) + .thenThrow(new NotFoundException()); + + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.error").value("Namespace not found: " + NAMESPACE)); + } + + @Test + void createTrustedPublisher_returns403_whenUserDoesNotOwnNamespace() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.registerTrustedPublisher(user, NAMESPACE, EXTENSION, PROVIDER, REGISTRATION)) + .thenThrow(new ErrorResultException("You must be an owner of this namespace.", HttpStatus.FORBIDDEN)); + + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.error").value("You must be an owner of this namespace.")); + } + + @Test + void createTrustedPublisher_returns400_whenRegistrationIsRejected() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.registerTrustedPublisher(user, NAMESPACE, EXTENSION, PROVIDER, REGISTRATION)) + .thenThrow(new ErrorResultException("An equivalent trusted publisher is already registered.")); + + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.error").value("An equivalent trusted publisher is already registered.")); + } + + // --------------------------------------------------------------------------------------- + // GET /user/namespace/{namespace}/trusted-publishing + // --------------------------------------------------------------------------------------- + + @Test + void getTrustedPublishers_returns403_whenNotLoggedIn() throws Exception { + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isForbidden()); + + verifyNoInteractions(trustedPublishing); + } + + @Test + void getTrustedPublishers_returnsPublishersAndRegistrableExtensions() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.getTrustedPublishers(user, NAMESPACE)) + .thenReturn(new TrustedPublishers(List.of(trustedPublisher(7L)), List.of("baz", "qux"))); + + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.trustedPublishers.length()").value(1)) + .andExpect(jsonPath("$.trustedPublishers[0].id").value(7)) + .andExpect(jsonPath("$.trustedPublishers[0].namespace").value(NAMESPACE)) + .andExpect(jsonPath("$.trustedPublishers[0].extension").value(EXTENSION)) + .andExpect(jsonPath("$.trustedPublishers[0].provider").value(PROVIDER)) + .andExpect(jsonPath("$.registrableExtensions.length()").value(2)) + .andExpect(jsonPath("$.registrableExtensions[0]").value("baz")) + .andExpect(jsonPath("$.registrableExtensions[1]").value("qux")) + .andExpect(jsonPath("$.error").doesNotExist()); + } + + @Test + void getTrustedPublishers_returnsEmptyLists_whenNothingIsRegistered() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.getTrustedPublishers(user, NAMESPACE)) + .thenReturn(new TrustedPublishers(List.of(), List.of())); + + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.trustedPublishers.length()").value(0)) + .andExpect(jsonPath("$.registrableExtensions.length()").value(0)); + } + + @Test + void getTrustedPublishers_returns404_whenNamespaceIsUnknown() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.getTrustedPublishers(user, NAMESPACE)).thenThrow(new NotFoundException()); + + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.error").value("Namespace not found: " + NAMESPACE)); + } + + @Test + void getTrustedPublishers_returns404_whenFeatureIsDisabled() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.getTrustedPublishers(user, NAMESPACE)) + .thenThrow(new ErrorResultException("Trusted publishing is not enabled.", HttpStatus.NOT_FOUND)); + + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.error").value("Trusted publishing is not enabled.")); + } + + @Test + void getTrustedPublishers_returns403_whenUserDoesNotOwnNamespace() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.getTrustedPublishers(user, NAMESPACE)) + .thenThrow(new ErrorResultException("You must be an owner of this namespace.", HttpStatus.FORBIDDEN)); + + mockMvc.perform(get("/user/namespace/{namespace}/trusted-publishing", NAMESPACE)) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.error").value("You must be an owner of this namespace.")); + } + + // --------------------------------------------------------------------------------------- + // POST /user/namespace/{namespace}/trusted-publishing/delete/{id} + // --------------------------------------------------------------------------------------- + + @Test + void deleteTrustedPublisher_returns403_whenNotLoggedIn() throws Exception { + mockMvc.perform(post("/user/namespace/{namespace}/trusted-publishing/delete/{id}", NAMESPACE, 7)) + .andExpect(status().isForbidden()); + + verifyNoInteractions(trustedPublishing); + } + + @Test + void deleteTrustedPublisher_returns200_andSuccessMessage() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.deleteTrustedPublisher(user, NAMESPACE, 7L)) + .thenReturn(ResultJson.success("Deleted trusted publisher for namespace " + NAMESPACE + ".")); + + mockMvc.perform(post("/user/namespace/{namespace}/trusted-publishing/delete/{id}", NAMESPACE, 7)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.success").value("Deleted trusted publisher for namespace " + NAMESPACE + ".")) + .andExpect(jsonPath("$.error").doesNotExist()); + + verify(trustedPublishing).deleteTrustedPublisher(user, NAMESPACE, 7L); + } + + @Test + void deleteTrustedPublisher_returns404_whenPublisherIsUnknown() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.deleteTrustedPublisher(user, NAMESPACE, 7L)).thenThrow(new NotFoundException()); + + mockMvc.perform(post("/user/namespace/{namespace}/trusted-publishing/delete/{id}", NAMESPACE, 7)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.error").value("Trusted publisher does not exist.")); + } + + @Test + void deleteTrustedPublisher_returns403_whenUserDoesNotOwnNamespace() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.deleteTrustedPublisher(user, NAMESPACE, 7L)) + .thenThrow(new ErrorResultException("You must be an owner of this namespace.", HttpStatus.FORBIDDEN)); + + mockMvc.perform(post("/user/namespace/{namespace}/trusted-publishing/delete/{id}", NAMESPACE, 7)) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.error").value("You must be an owner of this namespace.")); + } + + // --------------------------------------------------------------------------------------- + // POST /api/-/trusted-publishing/token + // --------------------------------------------------------------------------------------- + + @Test + void requestPublishToken_returns400_whenMandatoryFieldsAreMissing() throws Exception { + var missingNamespace = tokenRequestBody(null, EXTENSION, "the-token"); + var missingExtension = tokenRequestBody(NAMESPACE, null, "the-token"); + var missingToken = tokenRequestBody(NAMESPACE, EXTENSION, null); + + for (var body : List.of(missingNamespace, missingExtension, missingToken)) { + mockMvc.perform(tokenRequest(body)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.error").value("The fields namespace, extension and token are mandatory.")); + } + + verify(trustedPublishing, never()).requestPublishToken(anyString(), anyString(), anyString()); + } + + @Test + void requestPublishToken_returns201_andAccessToken() throws Exception { + // the exchange authenticates through the presented OIDC token, so no logged-in user is needed + var accessToken = new AccessTokenJson(); + accessToken.setId(42L); + accessToken.setValue("the-access-token"); + accessToken.setDescription("Trusted publishing (github)"); + when(trustedPublishing.requestPublishToken(NAMESPACE, EXTENSION, "the-token")).thenReturn(accessToken); + + mockMvc.perform(tokenRequest(tokenRequestBody(NAMESPACE, EXTENSION, "the-token"))) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.id").value(42)) + .andExpect(jsonPath("$.value").value("the-access-token")) + .andExpect(jsonPath("$.description").value("Trusted publishing (github)")) + .andExpect(jsonPath("$.error").doesNotExist()); + + verifyNoInteractions(users); + } + + @Test + void requestPublishToken_returns403_whenNoTrustedPublisherMatches() throws Exception { + when(trustedPublishing.requestPublishToken(NAMESPACE, EXTENSION, "the-token")).thenThrow( + new ErrorResultException( + "No trusted publisher matches the presented token.", + HttpStatus.FORBIDDEN)); + + mockMvc.perform(tokenRequest(tokenRequestBody(NAMESPACE, EXTENSION, "the-token"))) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.error").value("No trusted publisher matches the presented token.")); + } + + @Test + void requestPublishToken_returns400_whenTokenIsNotAcceptable() throws Exception { + when(trustedPublishing.requestPublishToken(NAMESPACE, EXTENSION, "the-token")) + .thenThrow(new ErrorResultException("Unsupported token issuer.")); + + mockMvc.perform(tokenRequest(tokenRequestBody(NAMESPACE, EXTENSION, "the-token"))) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.error").value("Unsupported token issuer.")); + } + + // --------------------------------------------------------------------------------------- + // GET /api/-/trusted-publishing/status + // --------------------------------------------------------------------------------------- + + @Test + void getTrustedPublishingStatus_returns403_whenNotLoggedIn() throws Exception { + mockMvc.perform(get("/api/-/trusted-publishing/status")).andExpect(status().isForbidden()); + + verifyNoInteractions(trustedPublishing); + } + + @Test + void getTrustedPublishingStatus_reportsDisabledFeature() throws Exception { + mockLoggedInUser(); + when(trustedPublishing.isEnabled()).thenReturn(false); + + mockMvc.perform(get("/api/-/trusted-publishing/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.enabled").value(false)) + .andExpect(jsonPath("$.allowed").value(false)) + .andExpect(jsonPath("$.trustedPublisherProviders").doesNotExist()); + + verify(trustedPublishing, never()).getTrustedPublisherProviders(); + } + + @Test + void getTrustedPublishingStatus_hidesProviders_whenPublisherAgreementIsMissing() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.isEnabled()).thenReturn(true); + when(eclipseService.hasPublisherAgreement(user)).thenReturn(false); + + mockMvc.perform(get("/api/-/trusted-publishing/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.enabled").value(true)) + .andExpect(jsonPath("$.allowed").value(false)) + .andExpect(jsonPath("$.trustedPublisherProviders").doesNotExist()); + + verify(trustedPublishing, never()).getTrustedPublisherProviders(); + } + + @Test + void getTrustedPublishingStatus_listsActiveProviders() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.isEnabled()).thenReturn(true); + when(eclipseService.hasPublisherAgreement(user)).thenReturn(true); + + // LinkedHashMap so the order of the listed providers is the one asserted below + var providers = new LinkedHashMap(); + providers.put( + PROVIDER, + provider( + PROVIDER, + "GitHub Actions", + "https://github.com", + List.of( + TrustedPublisherInputJson.create("owner", "The owner of the repository", false), + TrustedPublisherInputJson.create("environment", "The environment", true)))); + providers.put("gitlab", provider("gitlab", "GitLab CI/CD", "https://gitlab.com", List.of())); + when(trustedPublishing.getTrustedPublisherProviders()).thenReturn(providers); + + mockMvc.perform(get("/api/-/trusted-publishing/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.enabled").value(true)) + .andExpect(jsonPath("$.allowed").value(true)) + .andExpect(jsonPath("$.trustedPublisherProviders.length()").value(2)) + .andExpect(jsonPath("$.trustedPublisherProviders[0].id").value(PROVIDER)) + .andExpect(jsonPath("$.trustedPublisherProviders[0].name").value("GitHub Actions")) + .andExpect(jsonPath("$.trustedPublisherProviders[0].url").value("https://github.com")) + .andExpect(jsonPath("$.trustedPublisherProviders[0].registrationInputs.length()").value(2)) + .andExpect(jsonPath("$.trustedPublisherProviders[0].registrationInputs[0].key").value("owner")) + .andExpect( + jsonPath("$.trustedPublisherProviders[0].registrationInputs[0].description") + .value("The owner of the repository")) + .andExpect(jsonPath("$.trustedPublisherProviders[0].registrationInputs[0].optional").value(false)) + .andExpect(jsonPath("$.trustedPublisherProviders[0].registrationInputs[1].key").value("environment")) + .andExpect(jsonPath("$.trustedPublisherProviders[0].registrationInputs[1].optional").value(true)) + .andExpect(jsonPath("$.trustedPublisherProviders[1].id").value("gitlab")) + .andExpect(jsonPath("$.trustedPublisherProviders[1].name").value("GitLab CI/CD")); + } + + @Test + void getTrustedPublishingStatus_returns404_whenProviderLookupFindsFeatureDisabled() throws Exception { + var user = mockLoggedInUser(); + when(trustedPublishing.isEnabled()).thenReturn(true); + when(eclipseService.hasPublisherAgreement(user)).thenReturn(true); + when(trustedPublishing.getTrustedPublisherProviders()) + .thenThrow(new ErrorResultException("Trusted publishing is not enabled.", HttpStatus.NOT_FOUND)); + + mockMvc.perform(get("/api/-/trusted-publishing/status")) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.error").value("Trusted publishing is not enabled.")); + } + + // --------------------------------------------------------------------------------------- + // helpers + // --------------------------------------------------------------------------------------- + + private UserData mockLoggedInUser() { + var user = new UserData(); + user.setId(1L); + user.setLoginName("test_user"); + when(users.findLoggedInUser()).thenReturn(user); + return user; + } + + private static TrustedPublisher trustedPublisher(long id) { + var namespace = new Namespace(); + namespace.setId(1L); + namespace.setName(NAMESPACE); + var extension = new Extension(); + extension.setId(2L); + extension.setName(EXTENSION); + extension.setNamespace(namespace); + + var publisher = new TrustedPublisher(); + publisher.setId(id); + publisher.setExtension(extension); + publisher.setProvider(PROVIDER); + publisher.setRegistration(REGISTRATION); + publisher.setCreatedTimestamp(LocalDateTime.of(2026, 1, 2, 3, 4, 5)); + return publisher; + } + + private static TrustedPublishingProviderSupport provider( + String id, + String name, + String url, + List registrationInputs + ) { + var provider = mock(TrustedPublishingProviderSupport.class); + when(provider.getProviderId()).thenReturn(id); + when(provider.getProviderName()).thenReturn(name); + when(provider.getProviderUrl()).thenReturn(url); + when(provider.getRegistrationInputs()).thenReturn(registrationInputs); + return provider; + } + + private static MockHttpServletRequestBuilder createRequest(String namespace, String body) { + return post("/user/namespace/{namespace}/trusted-publishing/create", namespace) + .contentType(MediaType.APPLICATION_JSON) + .content(body); + } + + private static MockHttpServletRequestBuilder tokenRequest(String body) { + return post("/api/-/trusted-publishing/token").contentType(MediaType.APPLICATION_JSON).content(body); + } + + private static String registrationBody( + String namespace, + String extension, + String provider, + Map registration + ) { + var body = new LinkedHashMap(); + body.put("namespace", namespace); + body.put("extension", extension); + body.put("provider", provider); + body.put("registration", registration); + return JsonMapper.shared().writeValueAsString(body); + } + + private static String tokenRequestBody(String namespace, String extension, String token) { + var body = new LinkedHashMap(); + body.put("namespace", namespace); + body.put("extension", extension); + body.put("token", token); + return JsonMapper.shared().writeValueAsString(body); + } +} From 63c03568b995cf4df6df512f92d80e5eb21e3f57 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 09:11:51 +0200 Subject: [PATCH 03/25] feat: make GitLab trusted publishing instances configurable Every GitLab instance behaves identically and differs only in id, name, URL and OIDC issuer, so EclipseGitLabTrustedPublishingProvider carried nothing but four constants. Instances are now configuration: ovsx.trusted-publishing.gitlab..name/url/issuer with the public and the Eclipse instance configured by default, so existing deployments are unaffected. Whether an instance can be used is still decided by ovsx.trusted-publishing.active-providers. GitLabTrustedPublishingProviderSupport and its two subclasses collapse into a single concrete GitLabTrustedPublishingProvider. The ci_config_ref_uri claim is now derived from the host *and* the path of the instance URL, so an instance served under a relative URL root registers a matchable claim. Startup rejects an instance without a name or URL, with a malformed URL, or taking the GitHub provider id, and warns about active provider ids that no provider is configured for. Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingConfig.java | 58 ++++++- .../TrustedPublishingProperties.java | 141 +++++++++++++++ .../TrustedPublishingService.java | 43 ++++- ...clipseGitLabTrustedPublishingProvider.java | 28 --- .../GitLabTrustedPublishingProvider.java | 160 +++++++++++++++++- ...itLabTrustedPublishingProviderSupport.java | 147 ---------------- .../openvsx/LocalRegistryServiceTest.java | 3 +- .../org/eclipse/openvsx/RegistryAPITest.java | 3 +- .../java/org/eclipse/openvsx/UserAPITest.java | 3 +- .../eclipse/openvsx/admin/AdminAPITest.java | 3 +- .../TrustedPublishingPropertiesTest.java | 150 ++++++++++++++++ .../GitHubTrustedPublishingProviderTest.java | 3 +- .../GitLabTrustedPublishingProviderTest.java | 53 +++++- 13 files changed, 598 insertions(+), 197 deletions(-) create mode 100644 server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java delete mode 100644 server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/EclipseGitLabTrustedPublishingProvider.java delete mode 100644 server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderSupport.java create mode 100644 server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java index a068ba85e..8f5b1a69f 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java @@ -12,15 +12,30 @@ *****************************************************************************/ package org.eclipse.openvsx.trustedpublishing; +import java.net.URI; import java.util.List; +import java.util.Map; import jakarta.annotation.PostConstruct; import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Configuration; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance; +import org.eclipse.openvsx.trustedpublishing.github.GitHubTrustedPublishingProvider; + @Configuration +@EnableConfigurationProperties(TrustedPublishingProperties.class) public class TrustedPublishingConfig { + + private final TrustedPublishingProperties properties; + + public TrustedPublishingConfig(TrustedPublishingProperties properties) { + this.properties = properties; + } + /** * Whether trusted publishing is enabled at all. */ @@ -41,7 +56,8 @@ public class TrustedPublishingConfig { private List forbiddenJwtHeaders; /** - * The comma separated list of active trusted publishing providers. + * The comma separated list of active trusted publishing providers. An id listed here must be + * {@code github} or one of the configured GitLab instances, see {@link TrustedPublishingProperties}. * Default: {@code github}. */ @Value("${ovsx.trusted-publishing.active-providers:github}") @@ -66,6 +82,15 @@ public List getActiveProviders() { return activeProviders; } + /** + * The configured GitLab instances, keyed by provider id. Whether an instance can actually be used + * is decided by {@link #getActiveProviders()}. + */ + @NonNull + public Map getGitLabInstances() { + return properties.getGitlab(); + } + @PostConstruct public void validate() { if (enabled) { @@ -80,6 +105,37 @@ public void validate() { throw new IllegalStateException( "Trusted publishing is enabled, but there are no active providers configured"); } + validateGitLabInstances(); + } + } + + private void validateGitLabInstances() { + for (var entry : getGitLabInstances().entrySet()) { + var id = entry.getKey(); + var instance = entry.getValue(); + if (GitHubTrustedPublishingProvider.PROVIDER_ID.equals(id)) { + throw new IllegalStateException( + "GitLab instance '" + id + "' uses the provider id of the GitHub provider"); + } + // a configured instance replaces a default one as a whole, so it must carry every field itself + if (instance.getName() == null || instance.getName().isBlank() + || instance.getUrl() == null || instance.getUrl().isBlank()) { + throw new IllegalStateException("GitLab instance '" + id + "' has no name or no URL configured"); + } + for (var url : List.of(instance.getUrl(), instance.getIssuer())) { + if (hostOf(url) == null) { + throw new IllegalStateException("GitLab instance '" + id + "' has a malformed URL: " + url); + } + } + } + } + + @Nullable + private static String hostOf(String url) { + try { + return URI.create(url).getHost(); + } catch (IllegalArgumentException exc) { + return null; } } } diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java new file mode 100644 index 000000000..ad80188e7 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java @@ -0,0 +1,141 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.trustedpublishing; + +import java.util.LinkedHashMap; +import java.util.Map; + +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotBlank; +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; + +import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProvider; + +/** + * The trusted publishing provider instances that are known to this registry. + *

+ * Every GitLab instance behaves the same way, it only differs in its id, name, URL and OIDC issuer, + * so instances are configured rather than coded. Configured instances are merged into the defaults + * below, and become usable once their id is listed in {@code ovsx.trusted-publishing.active-providers}: + * + *

+ * ovsx:
+ *   trusted-publishing:
+ *     active-providers: github,eclipse-gitlab,acme-gitlab
+ *     gitlab:
+ *       acme-gitlab:
+ *         name: ACME GitLab
+ *         url: https://gitlab.acme.example
+ *         issuer: https://gitlab.acme.example   # optional, defaults to the URL
+ * 
+ * + * The id is persisted with every registration, so renaming it hides the registrations made for it. + *

+ * Configuring an id that is already a default replaces that instance as a whole rather than patching + * single fields, so such an entry has to carry the name and the URL itself. + */ +@ConfigurationProperties(prefix = "ovsx.trusted-publishing") +@Validated +public class TrustedPublishingProperties { + + /** + * The id of the Eclipse Foundation GitLab instance, configured out of the box. + */ + public static final String ECLIPSE_GITLAB_PROVIDER_ID = "eclipse-gitlab"; + + @Valid + private Map gitlab = defaultGitLabInstances(); + + /** + * The known GitLab instances, keyed by provider id. Configured instances are merged into the + * defaults, so the public and the Eclipse instance stay available unless their id is redefined. + */ + @NonNull + public Map getGitlab() { + return gitlab; + } + + public void setGitlab(Map gitlab) { + this.gitlab = gitlab; + } + + private static Map defaultGitLabInstances() { + var instances = new LinkedHashMap(); + instances.put( + GitLabTrustedPublishingProvider.PROVIDER_ID, + new GitLabInstance("GitLab", GitLabTrustedPublishingProvider.PROVIDER_URL)); + instances.put(ECLIPSE_GITLAB_PROVIDER_ID, new GitLabInstance("Eclipse GitLab", "https://gitlab.eclipse.org")); + return instances; + } + + /** + * A single GitLab instance. + */ + public static class GitLabInstance { + + @NotBlank + private String name; + + @NotBlank + private String url; + + @Nullable + private String issuer; + + public GitLabInstance() { + // for configuration property binding + } + + public GitLabInstance(String name, String url) { + this.name = name; + this.url = url; + } + + /** + * The instance name, for human consumption. + */ + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + /** + * The base URL of the instance; the API and the {@code ci_config_ref_uri} claim are derived from it. + */ + public String getUrl() { + return url; + } + + public void setUrl(String url) { + this.url = url; + } + + /** + * The issuer to expect in the {@code iss} claim of issued OIDC ID tokens. GitLab issues tokens + * under its own base URL, so this defaults to {@link #getUrl()}. + */ + public String getIssuer() { + return issuer == null || issuer.isBlank() ? url : issuer; + } + + public void setIssuer(@Nullable String issuer) { + this.issuer = issuer; + } + } +} diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index bcbbde234..cb248ecec 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -38,9 +38,7 @@ import org.eclipse.openvsx.json.ResultJson; import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.trustedpublishing.github.GitHubTrustedPublishingProvider; -import org.eclipse.openvsx.trustedpublishing.gitlab.EclipseGitLabTrustedPublishingProvider; import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProvider; -import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProviderSupport; import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.NotFoundException; import org.eclipse.openvsx.util.TimeUtil; @@ -71,18 +69,45 @@ public TrustedPublishingService( this.entityManager = requireNonNull(entityManager); if (config.isEnabled()) { - this.providers = Map.of( - GitHubTrustedPublishingProvider.PROVIDER_ID, - new GitHubTrustedPublishingProvider(config), - GitLabTrustedPublishingProvider.PROVIDER_ID, - new GitLabTrustedPublishingProvider(config), - EclipseGitLabTrustedPublishingProvider.PROVIDER_ID, - new EclipseGitLabTrustedPublishingProvider(config)); + this.providers = createProviders(config); + warnAboutUnknownActiveProviders(config); } else { this.providers = Map.of(); } } + /** + * GitHub is a single, hard-wired provider; every configured GitLab instance becomes one of its own. + */ + private static Map createProviders(TrustedPublishingConfig config) { + var providers = new HashMap(); + providers.put(GitHubTrustedPublishingProvider.PROVIDER_ID, new GitHubTrustedPublishingProvider(config)); + config.getGitLabInstances() + .forEach( + (providerId, instance) -> providers.put( + providerId, + new GitLabTrustedPublishingProvider( + config, + providerId, + instance.getName(), + instance.getUrl(), + instance.getIssuer()))); + return Map.copyOf(providers); + } + + /** + * An active provider without a matching definition is silently unusable, which is hard to tell apart + * from a working setup, so say so at startup. + */ + private void warnAboutUnknownActiveProviders(TrustedPublishingConfig config) { + var unknown = config.getActiveProviders().stream().filter(id -> !providers.containsKey(id)).toList(); + if (!unknown.isEmpty()) { + logger.warn( + "Trusted publishing lists active providers that are not configured and stay unusable: {}", + unknown); + } + } + public boolean isEnabled() { return config.isEnabled(); } diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/EclipseGitLabTrustedPublishingProvider.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/EclipseGitLabTrustedPublishingProvider.java deleted file mode 100644 index c24787fe5..000000000 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/EclipseGitLabTrustedPublishingProvider.java +++ /dev/null @@ -1,28 +0,0 @@ -/****************************************************************************** - * Copyright (c) 2026 Contributors to the Eclipse Foundation. - * - * See the NOTICE file(s) distributed with this work for additional - * information regarding copyright ownership. - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License 2.0 which is available at - * https://www.eclipse.org/legal/epl-2.0. - * - * SPDX-License-Identifier: EPL-2.0 - *****************************************************************************/ -package org.eclipse.openvsx.trustedpublishing.gitlab; - -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; - -/** - * GitLab provider for Eclipse GitLab Instance. - */ -public class EclipseGitLabTrustedPublishingProvider extends GitLabTrustedPublishingProviderSupport { - public static final String PROVIDER_ID = "eclipse-gitlab"; - public static final String PROVIDER_URL = "https://gitlab.eclipse.org"; - private static final String OIDC_ISSUER = "https://gitlab.eclipse.org"; - - public EclipseGitLabTrustedPublishingProvider(TrustedPublishingConfig config) { - super(config, PROVIDER_ID, "Eclipse GitLab", PROVIDER_URL, OIDC_ISSUER); - } -} diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java index 1fee0449d..2658df8f3 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java @@ -12,17 +12,167 @@ *****************************************************************************/ package org.eclipse.openvsx.trustedpublishing.gitlab; +import java.net.URI; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.jspecify.annotations.NonNull; +import org.springframework.core.ParameterizedTypeReference; +import org.springframework.http.MediaType; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.security.oauth2.jwt.JwtClaimNames; +import org.springframework.web.client.RestClientException; + +import org.eclipse.openvsx.json.TrustedPublisherInputJson; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProviderSupport; +import org.eclipse.openvsx.util.ErrorResultException; + +import static java.util.Objects.requireNonNull; /** - * GitLab provider for GitLab Public Instance. + * A GitLab instance as a trusted publishing provider. + *

+ * Every instance - the public one, the Eclipse Foundation one, any self-hosted one - is served by this + * class: they only differ in id, name, URL and OIDC issuer, which are configured through + * {@link org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance}. + * + * @see GitLab OpenID Connect */ -public class GitLabTrustedPublishingProvider extends GitLabTrustedPublishingProviderSupport { +public class GitLabTrustedPublishingProvider extends TrustedPublishingProviderSupport { + /** + * The provider id of the public GitLab instance, configured out of the box. + */ public static final String PROVIDER_ID = "gitlab"; + + /** + * The URL of the public GitLab instance. + */ public static final String PROVIDER_URL = "https://gitlab.com"; - private static final String OIDC_ISSUER = "https://gitlab.com"; - public GitLabTrustedPublishingProvider(TrustedPublishingConfig config) { - super(config, PROVIDER_ID, "GitLab", PROVIDER_URL, OIDC_ISSUER); + private static final String CLAIM_NAMESPACE_ID = "namespace_id"; // "72" + private static final String CLAIM_NAMESPACE_PATH = "namespace_path"; // "my-group" + private static final String CLAIM_PROJECT_ID = "project_id"; // "20" + private static final String CLAIM_PROJECT_PATH = "project_path"; // "my-group/my-project" + private static final String CLAIM_ENVIRONMENT = "environment"; // "prod"; optional + private static final String CLAIM_RUNNER_ENVIRONMENT = "runner_environment"; // "gitlab-hosted" + private static final String CLAIM_CI_CONFIG_REF_URI = "ci_config_ref_uri"; // "gitlab.example.com/my-group/my-project//.gitlab-ci.yml@refs/heads/main" + + private static final String API_RESOLVE_REQUEST = "/api/v4/projects/{path}"; + + private static final String REG_NAMESPACE = "namespace"; + private static final String REG_PROJECT = "project"; + private static final String REG_WORKFLOW = "workflow"; + private static final String REG_ENVIRONMENT = "environment"; + private static final List REGISTRATION_INPUTS = List.of( + TrustedPublisherInputJson.create(REG_NAMESPACE, "Namespace", false), + TrustedPublisherInputJson.create(REG_PROJECT, "Project name", false), + TrustedPublisherInputJson.create(REG_WORKFLOW, "Top-level CI filename", false), + TrustedPublisherInputJson.create(REG_ENVIRONMENT, "Environment name (optional)", true)); + + /** + * The instance part of the {@code ci_config_ref_uri} claim: the host of the instance, plus the path + * when the instance is served under a relative URL root. + */ + private final String instanceLocation; + + public GitLabTrustedPublishingProvider( + TrustedPublishingConfig config, + String providerId, + String providerName, + String providerUrl, + String oidcIssuer + ) { + super(config, providerId, providerName, providerUrl, oidcIssuer, REGISTRATION_INPUTS); + this.instanceLocation = instanceLocation(providerUrl); + } + + private static String instanceLocation(String providerUrl) { + URI uri = URI.create(providerUrl); + String host = uri.getHost(); + if (host == null) { + throw new IllegalArgumentException("Not a valid GitLab instance URL: " + providerUrl); + } + String path = uri.getPath(); + while (path != null && path.endsWith("/")) { + path = path.substring(0, path.length() - 1); + } + return path == null || path.isEmpty() ? host : host + path; + } + + @NonNull + @Override + protected Map extractClaims(Jwt jwt) { + requireNonNull(jwt); + HashMap result = new HashMap<>(7); + mustClaim(jwt, JwtClaimNames.SUB, result); + mustClaim(jwt, CLAIM_NAMESPACE_ID, result); + mustClaim(jwt, CLAIM_NAMESPACE_PATH, result); + mustClaim(jwt, CLAIM_PROJECT_ID, result); + mustClaim(jwt, CLAIM_PROJECT_PATH, result); + mayClaim(jwt, CLAIM_ENVIRONMENT, result); + mustClaim(jwt, CLAIM_RUNNER_ENVIRONMENT, result); + mustClaim(jwt, CLAIM_CI_CONFIG_REF_URI, result); + return result; + } + + @NonNull + @Override + protected Map extractRequest(Map registration) throws ErrorResultException { + requireNonNull(registration); + + final String namespace = mustRegister(registration, REG_NAMESPACE); + final String project = mustRegister(registration, REG_PROJECT); + final String workflow = mustRegister(registration, REG_WORKFLOW); + final String environment = registration.get(REG_ENVIRONMENT); + final String projectPath = namespace + "/" + project; + + Map response = resolve(projectPath); + if (response == null || !(response.get("id") instanceof Number projectId) + || !(response.get("namespace") instanceof Map namespaceMap) + || !(namespaceMap.get("id") instanceof Number namespaceId)) { + throw new ErrorResultException("Unexpected GitLab response for project " + projectPath); + } + + HashMap result = new HashMap<>(); + result.put(CLAIM_NAMESPACE_ID, String.valueOf(namespaceId.longValue())); + result.put(CLAIM_NAMESPACE_PATH, namespace); + result.put(CLAIM_PROJECT_ID, String.valueOf(projectId.longValue())); + result.put(CLAIM_PROJECT_PATH, projectPath); + // registered without the "@" part: publishing is trusted regardless of branch or tag + result.put(CLAIM_CI_CONFIG_REF_URI, instanceLocation + "/" + projectPath + "//" + workflow); + if (environment != null) { + result.put(CLAIM_ENVIRONMENT, environment); + } + return result; + } + + /** + * Pulled out for testability; is mocked in UT to prevent real remote access. + */ + protected Map resolve(String projectPath) throws ErrorResultException { + try { + // the {path} template variable is URL-encoded by RestClient, turning "/" into "%2F" as GitLab expects + return restClient.get() + .uri(providerUrl + API_RESOLVE_REQUEST, projectPath) + .accept(MediaType.APPLICATION_JSON) + .retrieve() + .body(new ParameterizedTypeReference<>() { + }); + } catch (RestClientException e) { + throw new ErrorResultException("Could not resolve GitLab project " + projectPath, e); + } + } + + @Override + public boolean matches(@NonNull Map registered, @NonNull Map token) { + requireNonNull(registered); + requireNonNull(token); + return claimEquals(CLAIM_PROJECT_ID, registered, token) + && claimEquals(CLAIM_NAMESPACE_ID, registered, token) + && registered.get(CLAIM_CI_CONFIG_REF_URI) != null + && registered.get(CLAIM_CI_CONFIG_REF_URI).equals(stripRef(token.get(CLAIM_CI_CONFIG_REF_URI))) + && pinnedClaimMatches(CLAIM_ENVIRONMENT, registered, token); } } diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderSupport.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderSupport.java deleted file mode 100644 index 930b05dac..000000000 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderSupport.java +++ /dev/null @@ -1,147 +0,0 @@ -/****************************************************************************** - * Copyright (c) 2026 Contributors to the Eclipse Foundation. - * - * See the NOTICE file(s) distributed with this work for additional - * information regarding copyright ownership. - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License 2.0 which is available at - * https://www.eclipse.org/legal/epl-2.0. - * - * SPDX-License-Identifier: EPL-2.0 - *****************************************************************************/ -package org.eclipse.openvsx.trustedpublishing.gitlab; - -import java.net.URI; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import org.jspecify.annotations.NonNull; -import org.springframework.core.ParameterizedTypeReference; -import org.springframework.http.MediaType; -import org.springframework.security.oauth2.jwt.Jwt; -import org.springframework.security.oauth2.jwt.JwtClaimNames; -import org.springframework.web.client.RestClientException; - -import org.eclipse.openvsx.json.TrustedPublisherInputJson; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProviderSupport; -import org.eclipse.openvsx.util.ErrorResultException; - -import static java.util.Objects.requireNonNull; - -/** - * GitLab specific support. - * - * @see GitLab OpenID Connect - */ -public abstract class GitLabTrustedPublishingProviderSupport extends TrustedPublishingProviderSupport { - private static final String CLAIM_NAMESPACE_ID = "namespace_id"; // "72" - private static final String CLAIM_NAMESPACE_PATH = "namespace_path"; // "my-group" - private static final String CLAIM_PROJECT_ID = "project_id"; // "20" - private static final String CLAIM_PROJECT_PATH = "project_path"; // "my-group/my-project" - private static final String CLAIM_ENVIRONMENT = "environment"; // "prod"; optional - private static final String CLAIM_RUNNER_ENVIRONMENT = "runner_environment"; // "gitlab-hosted" - private static final String CLAIM_CI_CONFIG_REF_URI = "ci_config_ref_uri"; // "gitlab.example.com/my-group/my-project//.gitlab-ci.yml@refs/heads/main" - - private static final String API_RESOLVE_REQUEST = "/api/v4/projects/{path}"; - - private static final String REG_NAMESPACE = "namespace"; - private static final String REG_PROJECT = "project"; - private static final String REG_WORKFLOW = "workflow"; - private static final String REG_ENVIRONMENT = "environment"; - private static final List REGISTRATION_INPUTS = List.of( - TrustedPublisherInputJson.create(REG_NAMESPACE, "Namespace", false), - TrustedPublisherInputJson.create(REG_PROJECT, "Project name", false), - TrustedPublisherInputJson.create(REG_WORKFLOW, "Top-level CI filename", false), - TrustedPublisherInputJson.create(REG_ENVIRONMENT, "Environment name (optional)", true)); - - protected GitLabTrustedPublishingProviderSupport( - TrustedPublishingConfig config, - String providerId, - String providerName, - String providerUrl, - String oidcIssuer - ) { - super(config, providerId, providerName, providerUrl, oidcIssuer, REGISTRATION_INPUTS); - } - - @NonNull - @Override - protected Map extractClaims(Jwt jwt) { - requireNonNull(jwt); - HashMap result = new HashMap<>(7); - mustClaim(jwt, JwtClaimNames.SUB, result); - mustClaim(jwt, CLAIM_NAMESPACE_ID, result); - mustClaim(jwt, CLAIM_NAMESPACE_PATH, result); - mustClaim(jwt, CLAIM_PROJECT_ID, result); - mustClaim(jwt, CLAIM_PROJECT_PATH, result); - mayClaim(jwt, CLAIM_ENVIRONMENT, result); - mustClaim(jwt, CLAIM_RUNNER_ENVIRONMENT, result); - mustClaim(jwt, CLAIM_CI_CONFIG_REF_URI, result); - return result; - } - - @NonNull - @Override - protected Map extractRequest(Map registration) throws ErrorResultException { - requireNonNull(registration); - - final String namespace = mustRegister(registration, REG_NAMESPACE); - final String project = mustRegister(registration, REG_PROJECT); - final String workflow = mustRegister(registration, REG_WORKFLOW); - final String environment = registration.get(REG_ENVIRONMENT); - final String projectPath = namespace + "/" + project; - - Map response = resolve(projectPath); - if (response == null || !(response.get("id") instanceof Number projectId) - || !(response.get("namespace") instanceof Map namespaceMap) - || !(namespaceMap.get("id") instanceof Number namespaceId)) { - throw new ErrorResultException("Unexpected GitLab response for project " + projectPath); - } - - HashMap result = new HashMap<>(); - result.put(CLAIM_NAMESPACE_ID, String.valueOf(namespaceId.longValue())); - result.put(CLAIM_NAMESPACE_PATH, namespace); - result.put(CLAIM_PROJECT_ID, String.valueOf(projectId.longValue())); - result.put(CLAIM_PROJECT_PATH, projectPath); - // registered without the "@" part: publishing is trusted regardless of branch or tag - result.put( - CLAIM_CI_CONFIG_REF_URI, - URI.create(providerUrl).getHost() + "/" + projectPath - + "//" + workflow); - if (environment != null) { - result.put(CLAIM_ENVIRONMENT, environment); - } - return result; - } - - /** - * Pulled out for testability; is mocked in UT to prevent real remote access. - */ - protected Map resolve(String projectPath) throws ErrorResultException { - try { - // the {path} template variable is URL-encoded by RestClient, turning "/" into "%2F" as GitLab expects - return restClient.get() - .uri(providerUrl + API_RESOLVE_REQUEST, projectPath) - .accept(MediaType.APPLICATION_JSON) - .retrieve() - .body(new ParameterizedTypeReference<>() { - }); - } catch (RestClientException e) { - throw new ErrorResultException("Could not resolve GitLab project " + projectPath, e); - } - } - - @Override - public boolean matches(@NonNull Map registered, @NonNull Map token) { - requireNonNull(registered); - requireNonNull(token); - return claimEquals(CLAIM_PROJECT_ID, registered, token) - && claimEquals(CLAIM_NAMESPACE_ID, registered, token) - && registered.get(CLAIM_CI_CONFIG_REF_URI) != null - && registered.get(CLAIM_CI_CONFIG_REF_URI).equals(stripRef(token.get(CLAIM_CI_CONFIG_REF_URI))) - && pinnedClaimMatches(CLAIM_ENVIRONMENT, registered, token); - } -} diff --git a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java index 995edafdb..10792e4d9 100644 --- a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java @@ -51,6 +51,7 @@ import org.eclipse.openvsx.search.SimilarityCheckService; import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.TempFile; import org.eclipse.openvsx.util.VersionService; @@ -128,7 +129,7 @@ void setUp() { integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(), + new TrustedPublishingConfig(new TrustedPublishingProperties()), Duration.ofSeconds(30)); // A permissive default for a void method rather than a per-test expectation: the tests of diff --git a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java index 85fc0d516..345446d36 100644 --- a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java @@ -80,6 +80,7 @@ import org.eclipse.openvsx.storage.*; import org.eclipse.openvsx.storage.log.DownloadCountService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.ChangesCursor; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.NamingUtil; @@ -3747,7 +3748,7 @@ PublishingConfig publishingConfig() { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(); + return new TrustedPublishingConfig(new TrustedPublishingProperties()); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index 67c351315..bfad69387 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -63,6 +63,7 @@ import org.eclipse.openvsx.security.SecurityConfig; import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.TargetPlatform; import org.eclipse.openvsx.util.TargetPlatformVersion; @@ -1242,7 +1243,7 @@ LocalRegistryService localRegistryService( integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(), + new TrustedPublishingConfig(new TrustedPublishingProperties()), Duration.ofSeconds(30)); } diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index 51ffbb538..abf730ce4 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -102,6 +102,7 @@ import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.storage.log.DownloadCountService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.TargetPlatform; import org.eclipse.openvsx.util.TargetPlatformVersion; @@ -2638,7 +2639,7 @@ LocalRegistryService localRegistryService( integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(), + new TrustedPublishingConfig(new TrustedPublishingProperties()), Duration.ofSeconds(30)); } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java new file mode 100644 index 000000000..842f619b1 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java @@ -0,0 +1,150 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.trustedpublishing; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; +import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.boot.context.properties.source.MapConfigurationPropertySource; +import org.springframework.test.util.ReflectionTestUtils; + +import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProvider; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; + +/** + * The GitLab instances are configuration rather than code, so what matters is that the defaults are + * there, that configured instances are merged into them, and that a broken instance is caught at startup. + */ +class TrustedPublishingPropertiesTest { + + @Test + void defaultsCoverThePublicAndTheEclipseInstance() { + var instances = new TrustedPublishingProperties().getGitlab(); + + assertThat(instances) + .containsOnlyKeys( + GitLabTrustedPublishingProvider.PROVIDER_ID, + TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); + var eclipse = instances.get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); + assertThat(eclipse.getName()).isEqualTo("Eclipse GitLab"); + assertThat(eclipse.getUrl()).isEqualTo("https://gitlab.eclipse.org"); + // GitLab issues its tokens under its own base URL + assertThat(eclipse.getIssuer()).isEqualTo("https://gitlab.eclipse.org"); + } + + @Test + void configuredInstanceIsAddedToTheDefaults() { + var instances = bind( + Map.of( + "ovsx.trusted-publishing.gitlab.acme-gitlab.name", + "ACME GitLab", + "ovsx.trusted-publishing.gitlab.acme-gitlab.url", + "https://gitlab.acme.example")) + .getGitlab(); + + assertThat(instances) + .containsOnlyKeys( + GitLabTrustedPublishingProvider.PROVIDER_ID, + TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID, + "acme-gitlab"); + var acme = instances.get("acme-gitlab"); + assertThat(acme.getName()).isEqualTo("ACME GitLab"); + assertThat(acme.getIssuer()).isEqualTo("https://gitlab.acme.example"); + } + + @Test + void configuredInstanceCanRedefineADefault() { + var instances = bind( + Map.of( + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.name", + "Eclipse GitLab (staging)", + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.url", + "https://gitlab.staging.eclipse.org", + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.issuer", + "https://issuer.staging.eclipse.org")) + .getGitlab(); + + var eclipse = instances.get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); + assertThat(eclipse.getName()).isEqualTo("Eclipse GitLab (staging)"); + assertThat(eclipse.getUrl()).isEqualTo("https://gitlab.staging.eclipse.org"); + assertThat(eclipse.getIssuer()).isEqualTo("https://issuer.staging.eclipse.org"); + } + + @Test + void redefiningADefaultInstanceReplacesItAsAWhole() { + // only the URL is given, so the default name is gone rather than kept - and startup says so + var properties = bind( + Map.of( + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.url", + "https://gitlab.staging.eclipse.org")); + + assertThat(properties.getGitlab().get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID).getName()) + .isNull(); + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + .withMessageContaining("no name or no URL"); + } + + @Test + void enabledConfigAcceptsTheDefaultInstances() { + assertThatCode(() -> enabledConfig(new TrustedPublishingProperties()).validate()).doesNotThrowAnyException(); + } + + @Test + void enabledConfigRejectsAnInstanceTakingTheGitHubProviderId() { + var properties = bind( + Map.of( + "ovsx.trusted-publishing.gitlab.github.name", + "Not GitHub", + "ovsx.trusted-publishing.gitlab.github.url", + "https://gitlab.acme.example")); + + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + .withMessageContaining("provider id of the GitHub provider"); + } + + @Test + void enabledConfigRejectsAMalformedInstanceUrl() { + var properties = bind( + Map.of( + "ovsx.trusted-publishing.gitlab.acme-gitlab.name", + "ACME GitLab", + "ovsx.trusted-publishing.gitlab.acme-gitlab.url", + "gitlab.acme.example")); + + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + .withMessageContaining("malformed URL"); + } + + private static TrustedPublishingProperties bind(Map properties) { + return new Binder(new MapConfigurationPropertySource(properties)) + .bind( + "ovsx.trusted-publishing", + Bindable.ofInstance(new TrustedPublishingProperties())) + .orElseGet(TrustedPublishingProperties::new); + } + + private static TrustedPublishingConfig enabledConfig(TrustedPublishingProperties properties) { + var config = new TrustedPublishingConfig(properties); + ReflectionTestUtils.setField(config, "enabled", true); + ReflectionTestUtils.setField(config, "audience", "https://open-vsx.org"); + ReflectionTestUtils.setField(config, "forbiddenJwtHeaders", List.of("x5u")); + ReflectionTestUtils.setField(config, "activeProviders", List.of("github")); + return config; + } +} diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java index bebb7a85f..5c0a3af50 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java @@ -23,6 +23,7 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -155,7 +156,7 @@ void pinnedEnvironment() { static class TestConfig { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(); + return new TrustedPublishingConfig(new TrustedPublishingProperties()); } } } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java index 992c380e8..a837ff4f9 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java @@ -23,9 +23,11 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @ExtendWith(SpringExtension.class) @@ -57,7 +59,18 @@ private static Map registeredClaims() { } private static GitLabTrustedPublishingProvider newProvider(TrustedPublishingConfig config) { - return new GitLabTrustedPublishingProvider(config) { + return newProvider( + config, + GitLabTrustedPublishingProvider.PROVIDER_ID, + GitLabTrustedPublishingProvider.PROVIDER_URL); + } + + private static GitLabTrustedPublishingProvider newProvider( + TrustedPublishingConfig config, + String providerId, + String providerUrl + ) { + return new GitLabTrustedPublishingProvider(config, providerId, "GitLab", providerUrl, providerUrl) { @Override protected Map resolve(String projectPath) { return Map.of( @@ -151,11 +164,47 @@ void pinnedEnvironment() { assertTrue(gl.matches(registeredClaims(), token)); } + @Test + void selfHostedInstanceIsAddressedByItsOwnUrl() throws Exception { + GitLabTrustedPublishingProvider gl = newProvider(config, "acme-gitlab", "https://gitlab.acme.example"); + Map data = gl.extractRequest( + Map.of( + "namespace", + "gitlab-org", + "project", + "gitlab", + "workflow", + ".gitlab-ci.yml")); + assertEquals("gitlab.acme.example/gitlab-org/gitlab//.gitlab-ci.yml", data.get("ci_config_ref_uri")); + assertEquals("acme-gitlab", gl.getProviderId()); + } + + @Test + void instanceServedUnderARelativeUrlRootKeepsItsPath() throws Exception { + GitLabTrustedPublishingProvider gl = newProvider(config, "acme-gitlab", "https://acme.example/gitlab/"); + Map data = gl.extractRequest( + Map.of( + "namespace", + "gitlab-org", + "project", + "gitlab", + "workflow", + ".gitlab-ci.yml")); + assertEquals("acme.example/gitlab/gitlab-org/gitlab//.gitlab-ci.yml", data.get("ci_config_ref_uri")); + } + + @Test + void malformedInstanceUrlIsRejected() { + assertThrows( + IllegalArgumentException.class, + () -> newProvider(config, "acme-gitlab", "not-a-url")); + } + @TestConfiguration static class TestConfig { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(); + return new TrustedPublishingConfig(new TrustedPublishingProperties()); } } } From 4a1fdb862c9f208632d2a44f391e457f446bfaa2 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 09:23:31 +0200 Subject: [PATCH 04/25] feat: do not configure the Eclipse GitLab instance by default An instance the upstream registry does not itself use has no place in the built-in defaults; only the public GitLab instance is configured out of the box now. Deployments that want the Eclipse Foundation instance add it the same way as any other one: ovsx: trusted-publishing: active-providers: github,eclipse-gitlab gitlab: eclipse-gitlab: name: Eclipse GitLab url: https://gitlab.eclipse.org Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingProperties.java | 29 +++----- .../TrustedPublishingPropertiesTest.java | 74 ++++++++----------- 2 files changed, 44 insertions(+), 59 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java index ad80188e7..dc4cad9e7 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java @@ -28,40 +28,36 @@ * The trusted publishing provider instances that are known to this registry. *

* Every GitLab instance behaves the same way, it only differs in its id, name, URL and OIDC issuer, - * so instances are configured rather than coded. Configured instances are merged into the defaults - * below, and become usable once their id is listed in {@code ovsx.trusted-publishing.active-providers}: + * so instances are configured rather than coded. Only the public instance is configured out of the box; + * any other one - the Eclipse Foundation instance included - is added by configuration, and becomes + * usable once its id is listed in {@code ovsx.trusted-publishing.active-providers}: * *

  * ovsx:
  *   trusted-publishing:
- *     active-providers: github,eclipse-gitlab,acme-gitlab
+ *     active-providers: github,eclipse-gitlab
  *     gitlab:
- *       acme-gitlab:
- *         name: ACME GitLab
- *         url: https://gitlab.acme.example
- *         issuer: https://gitlab.acme.example   # optional, defaults to the URL
+ *       eclipse-gitlab:
+ *         name: Eclipse GitLab
+ *         url: https://gitlab.eclipse.org
+ *         issuer: https://gitlab.eclipse.org   # optional, defaults to the URL
  * 
* * The id is persisted with every registration, so renaming it hides the registrations made for it. *

- * Configuring an id that is already a default replaces that instance as a whole rather than patching - * single fields, so such an entry has to carry the name and the URL itself. + * Configuring the id of the default instance replaces it as a whole rather than patching single fields, + * so such an entry has to carry the name and the URL itself. */ @ConfigurationProperties(prefix = "ovsx.trusted-publishing") @Validated public class TrustedPublishingProperties { - /** - * The id of the Eclipse Foundation GitLab instance, configured out of the box. - */ - public static final String ECLIPSE_GITLAB_PROVIDER_ID = "eclipse-gitlab"; - @Valid private Map gitlab = defaultGitLabInstances(); /** - * The known GitLab instances, keyed by provider id. Configured instances are merged into the - * defaults, so the public and the Eclipse instance stay available unless their id is redefined. + * The known GitLab instances, keyed by provider id. Configured instances are added to the public + * instance, which stays available unless its id is redefined. */ @NonNull public Map getGitlab() { @@ -77,7 +73,6 @@ private static Map defaultGitLabInstances() { instances.put( GitLabTrustedPublishingProvider.PROVIDER_ID, new GitLabInstance("GitLab", GitLabTrustedPublishingProvider.PROVIDER_URL)); - instances.put(ECLIPSE_GITLAB_PROVIDER_ID, new GitLabInstance("Eclipse GitLab", "https://gitlab.eclipse.org")); return instances; } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java index 842f619b1..fe89ceae0 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java @@ -28,80 +28,70 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** - * The GitLab instances are configuration rather than code, so what matters is that the defaults are - * there, that configured instances are merged into them, and that a broken instance is caught at startup. + * The GitLab instances are configuration rather than code, so what matters is that the public instance + * is there, that configured instances are added to it, and that a broken instance is caught at startup. */ class TrustedPublishingPropertiesTest { @Test - void defaultsCoverThePublicAndTheEclipseInstance() { + void onlyThePublicInstanceIsConfiguredByDefault() { var instances = new TrustedPublishingProperties().getGitlab(); - assertThat(instances) - .containsOnlyKeys( - GitLabTrustedPublishingProvider.PROVIDER_ID, - TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); - var eclipse = instances.get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); - assertThat(eclipse.getName()).isEqualTo("Eclipse GitLab"); - assertThat(eclipse.getUrl()).isEqualTo("https://gitlab.eclipse.org"); + assertThat(instances).containsOnlyKeys(GitLabTrustedPublishingProvider.PROVIDER_ID); + var gitlab = instances.get(GitLabTrustedPublishingProvider.PROVIDER_ID); + assertThat(gitlab.getName()).isEqualTo("GitLab"); + assertThat(gitlab.getUrl()).isEqualTo(GitLabTrustedPublishingProvider.PROVIDER_URL); // GitLab issues its tokens under its own base URL - assertThat(eclipse.getIssuer()).isEqualTo("https://gitlab.eclipse.org"); + assertThat(gitlab.getIssuer()).isEqualTo(GitLabTrustedPublishingProvider.PROVIDER_URL); } @Test - void configuredInstanceIsAddedToTheDefaults() { + void configuredInstanceIsAddedToThePublicOne() { var instances = bind( Map.of( - "ovsx.trusted-publishing.gitlab.acme-gitlab.name", - "ACME GitLab", - "ovsx.trusted-publishing.gitlab.acme-gitlab.url", - "https://gitlab.acme.example")) + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.name", + "Eclipse GitLab", + "ovsx.trusted-publishing.gitlab.eclipse-gitlab.url", + "https://gitlab.eclipse.org")) .getGitlab(); - assertThat(instances) - .containsOnlyKeys( - GitLabTrustedPublishingProvider.PROVIDER_ID, - TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID, - "acme-gitlab"); - var acme = instances.get("acme-gitlab"); - assertThat(acme.getName()).isEqualTo("ACME GitLab"); - assertThat(acme.getIssuer()).isEqualTo("https://gitlab.acme.example"); + assertThat(instances).containsOnlyKeys(GitLabTrustedPublishingProvider.PROVIDER_ID, "eclipse-gitlab"); + var eclipse = instances.get("eclipse-gitlab"); + assertThat(eclipse.getName()).isEqualTo("Eclipse GitLab"); + assertThat(eclipse.getIssuer()).isEqualTo("https://gitlab.eclipse.org"); } @Test - void configuredInstanceCanRedefineADefault() { + void configuredInstanceCanRedefineThePublicOne() { var instances = bind( Map.of( - "ovsx.trusted-publishing.gitlab.eclipse-gitlab.name", - "Eclipse GitLab (staging)", - "ovsx.trusted-publishing.gitlab.eclipse-gitlab.url", - "https://gitlab.staging.eclipse.org", - "ovsx.trusted-publishing.gitlab.eclipse-gitlab.issuer", - "https://issuer.staging.eclipse.org")) + "ovsx.trusted-publishing.gitlab.gitlab.name", + "GitLab (staging)", + "ovsx.trusted-publishing.gitlab.gitlab.url", + "https://gitlab.staging.example", + "ovsx.trusted-publishing.gitlab.gitlab.issuer", + "https://issuer.staging.example")) .getGitlab(); - var eclipse = instances.get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID); - assertThat(eclipse.getName()).isEqualTo("Eclipse GitLab (staging)"); - assertThat(eclipse.getUrl()).isEqualTo("https://gitlab.staging.eclipse.org"); - assertThat(eclipse.getIssuer()).isEqualTo("https://issuer.staging.eclipse.org"); + var gitlab = instances.get(GitLabTrustedPublishingProvider.PROVIDER_ID); + assertThat(gitlab.getName()).isEqualTo("GitLab (staging)"); + assertThat(gitlab.getUrl()).isEqualTo("https://gitlab.staging.example"); + assertThat(gitlab.getIssuer()).isEqualTo("https://issuer.staging.example"); } @Test - void redefiningADefaultInstanceReplacesItAsAWhole() { + void redefiningTheDefaultInstanceReplacesItAsAWhole() { // only the URL is given, so the default name is gone rather than kept - and startup says so var properties = bind( - Map.of( - "ovsx.trusted-publishing.gitlab.eclipse-gitlab.url", - "https://gitlab.staging.eclipse.org")); + Map.of("ovsx.trusted-publishing.gitlab.gitlab.url", "https://gitlab.staging.example")); - assertThat(properties.getGitlab().get(TrustedPublishingProperties.ECLIPSE_GITLAB_PROVIDER_ID).getName()) - .isNull(); + assertThat(properties.getGitlab().get(GitLabTrustedPublishingProvider.PROVIDER_ID).getName()).isNull(); assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) .withMessageContaining("no name or no URL"); } @Test - void enabledConfigAcceptsTheDefaultInstances() { + void enabledConfigAcceptsTheDefaultInstance() { assertThatCode(() -> enabledConfig(new TrustedPublishingProperties()).validate()).doesNotThrowAnyException(); } From 39416689d2ef80dc247b223dd31793092ed30265 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 09:32:09 +0200 Subject: [PATCH 05/25] fix: return 403 instead of 500 when the publisher agreement is missing createTrustedPublisher called eclipseService.checkPublisherAgreement() outside its try/catch. Nothing else handles ErrorResultException - no @ControllerAdvice covers it - so a user without a signed publisher agreement got a 500 with an empty body instead of the 403 and the {"error": "..."} message the exception carries. Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingAPI.java | 27 ++++++++++--------- .../TrustedPublishingAPITest.java | 22 +++++++++++++++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java index 0b3d0534b..af4ccf731 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPI.java @@ -69,20 +69,21 @@ public ResponseEntity createTrustedPublisher( if (user == null) { throw new ResponseStatusException(HttpStatus.FORBIDDEN); } - eclipseService.checkPublisherAgreement(user); - if (!StringUtils.hasText(request.getProvider()) - || !StringUtils.hasText(request.getNamespace()) || !StringUtils.hasText(request.getExtension()) - || request.getRegistration() == null || request.getRegistration().isEmpty()) { - var json = TrustedPublisherJson - .error("The fields provider, namespace, extension and registration are mandatory."); - return new ResponseEntity<>(json, HttpStatus.BAD_REQUEST); - } - if (!Objects.equals(namespace, request.getNamespace())) { - var json = TrustedPublisherJson.error("The namespace in the path and in the request body must match."); - return new ResponseEntity<>(json, HttpStatus.BAD_REQUEST); - } - try { + // throws when the user has no signed publisher agreement, and so has to be caught below + eclipseService.checkPublisherAgreement(user); + if (!StringUtils.hasText(request.getProvider()) + || !StringUtils.hasText(request.getNamespace()) || !StringUtils.hasText(request.getExtension()) + || request.getRegistration() == null || request.getRegistration().isEmpty()) { + var json = TrustedPublisherJson + .error("The fields provider, namespace, extension and registration are mandatory."); + return new ResponseEntity<>(json, HttpStatus.BAD_REQUEST); + } + if (!Objects.equals(namespace, request.getNamespace())) { + var json = TrustedPublisherJson.error("The namespace in the path and in the request body must match."); + return new ResponseEntity<>(json, HttpStatus.BAD_REQUEST); + } + var publisher = trustedPublishing.registerTrustedPublisher( user, request.getNamespace(), diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java index 38146d337..0bae8874c 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingAPITest.java @@ -45,6 +45,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -154,6 +155,27 @@ void createTrustedPublisher_returns201_andRegisteredPublisher() throws Exception verify(eclipseService).checkPublisherAgreement(user); } + @Test + void createTrustedPublisher_returns403_whenPublisherAgreementIsMissing() throws Exception { + var user = mockLoggedInUser(); + doThrow( + new ErrorResultException( + "You must sign a Publisher Agreement with the Eclipse Foundation before publishing any extension.", + HttpStatus.FORBIDDEN)) + .when(eclipseService) + .checkPublisherAgreement(user); + + mockMvc.perform(createRequest(NAMESPACE, registrationBody(NAMESPACE, EXTENSION, PROVIDER, REGISTRATION))) + .andExpect(status().isForbidden()) + .andExpect( + jsonPath("$.error") + .value( + "You must sign a Publisher Agreement with the Eclipse Foundation before publishing any extension.")); + + verify(trustedPublishing, never()) + .registerTrustedPublisher(any(), anyString(), anyString(), anyString(), any()); + } + @Test void createTrustedPublisher_returns404_whenNamespaceIsUnknown() throws Exception { var user = mockLoggedInUser(); From a5e5a71e716afc42ed10847bb6dfb15a09f5b98a Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 09:46:33 +0200 Subject: [PATCH 06/25] refactor: rename ExtensionJson.trustedPublisher to publishedWithTrustedPublishing The field says whether a version was published through trusted publishing, not which trusted publisher it belongs to, and the old name reads like the latter. Nothing consumes it yet - not the webui, not the CLI - and it has not been in a release, so the rename breaks no client. Co-Authored-By: Claude Opus 5 (1M context) --- .../eclipse/openvsx/entities/ExtensionVersion.java | 3 ++- .../java/org/eclipse/openvsx/json/ExtensionJson.java | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java index 09a467def..e4a7ac52e 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java @@ -209,7 +209,8 @@ public ExtensionJson toExtensionJson() { if (this.getPublishedBy() != null) { json.setPublishedBy(this.getPublishedBy().toUserJson()); } - json.setTrustedPublisher(getPublishedWithTt() != null && getPublishedWithTt() == PersonalAccessTokenType.TPT); + json.setPublishedWithTrustedPublishing( + getPublishedWithTt() != null && getPublishedWithTt() == PersonalAccessTokenType.TPT); if (this.getDependencies() != null) { json.setDependencies(toExtensionReferenceJson(this.getDependencies())); } diff --git a/server/src/main/java/org/eclipse/openvsx/json/ExtensionJson.java b/server/src/main/java/org/eclipse/openvsx/json/ExtensionJson.java index 3bb3ae4df..0c7638bef 100644 --- a/server/src/main/java/org/eclipse/openvsx/json/ExtensionJson.java +++ b/server/src/main/java/org/eclipse/openvsx/json/ExtensionJson.java @@ -86,9 +86,9 @@ public static ExtensionJson error(String message) { @NotNull private UserJson publishedBy; - @Schema(description = "Indicates whether this version was published using trusted publisher") + @Schema(description = "Indicates whether this version was published using trusted publishing") @NotNull - private Boolean trustedPublisher; + private Boolean publishedWithTrustedPublishing; @Schema(hidden = true) private Boolean active; @@ -307,12 +307,12 @@ public void setPublishedBy(UserJson publishedBy) { this.publishedBy = publishedBy; } - public Boolean getTrustedPublisher() { - return trustedPublisher; + public Boolean getPublishedWithTrustedPublishing() { + return publishedWithTrustedPublishing; } - public void setTrustedPublisher(Boolean trustedPublisher) { - this.trustedPublisher = trustedPublisher; + public void setPublishedWithTrustedPublishing(Boolean publishedWithTrustedPublishing) { + this.publishedWithTrustedPublishing = publishedWithTrustedPublishing; } public Boolean getActive() { From fa015667d5d3bb4e6557f5ad73ab4db29cd8db4b Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 10:18:30 +0200 Subject: [PATCH 07/25] feat: mark trusted-published versions on the extension detail page A rocket icon next to "Published by" says the version came from a trusted publishing workflow rather than a personal access token, driven by the new publishedWithTrustedPublishing field. It links to the deployment's trusted publishing documentation when one is configured, and nothing is rendered for the ordinary case. The rocket is the same one that stands for trusted publishing in the user settings; a shield would collide with the verified-publisher shield sitting in the same row. Co-Authored-By: Claude Opus 5 (1M context) --- webui/CHANGELOG.md | 1 + webui/src/extension-registry-types.ts | 2 + .../extension-detail/extension-detail.tsx | 5 ++ .../trusted-publishing-icon.tsx | 61 +++++++++++++++++++ .../trusted-publishing-icon.spec.tsx | 53 ++++++++++++++++ 5 files changed, 122 insertions(+) create mode 100644 webui/src/pages/extension-detail/trusted-publishing-icon.tsx create mode 100644 webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx diff --git a/webui/CHANGELOG.md b/webui/CHANGELOG.md index c18d9f6b6..c0c2c17f4 100644 --- a/webui/CHANGELOG.md +++ b/webui/CHANGELOG.md @@ -11,6 +11,7 @@ This change log covers only the frontend library (webui) of Open VSX. - Add a "Data Consistency" page to the admin dashboard (#1622): a live overview of every registered consistency check's finding count, with actions to refresh it and to fix findings one at a time or all at once - Show a "Namespace not verified" state on an extension card when it can't be activated because its namespace already exists in a referenced external gallery and hasn't been verified, in both the "My Extensions" and namespace member extension lists. The card keeps its colour and takes a warning-toned frame and icon, since this is the publisher's to fix rather than an extension that is simply switched off - Show a warning notice with a claim action wherever an unverified namespace is holding something back — the extension settings page when the extension has a namespace ownership conflict, and the namespace settings page for any unverified namespace — making clear the namespace must be claimed (verified) first. The action is the deployment's configured `elements.claimNamespace`, falling back to the namespace access documentation when none is configured. The admin dashboard's extension and namespace views show the same explanation without the claim action, since claiming is the publisher's action to take, not an admin's on someone else's behalf +- Mark a version that was published through a trusted publishing workflow with a rocket icon next to "Published by" on the extension detail page, linking to the deployment's trusted publishing documentation when one is configured ### Changed diff --git a/webui/src/extension-registry-types.ts b/webui/src/extension-registry-types.ts index c03a7bcec..377cc70df 100644 --- a/webui/src/extension-registry-types.ts +++ b/webui/src/extension-registry-types.ts @@ -75,6 +75,8 @@ export interface Extension { targetPlatform: string; preRelease?: boolean; publishedBy: UserData; + /** True when this version was published through a trusted publishing workflow. */ + publishedWithTrustedPublishing?: boolean; verified: boolean; // key: version, value: url allVersions: { [version: string]: UrlString }; diff --git a/webui/src/pages/extension-detail/extension-detail.tsx b/webui/src/pages/extension-detail/extension-detail.tsx index a53eed728..39d1ad3dc 100644 --- a/webui/src/pages/extension-detail/extension-detail.tsx +++ b/webui/src/pages/extension-detail/extension-detail.tsx @@ -38,6 +38,7 @@ import { ExtensionDetailChanges } from './extension-detail-changes'; import { ExtensionDetailReviews } from './extension-detail-reviews'; import { ExtensionDetailRoutes } from './extension-detail-routes'; +import { TrustedPublishingIcon } from './trusted-publishing-icon'; import { useExtensionDetail } from './use-extension-details'; import { KbdKey } from '../../components/kbd-key'; import { useShortcut } from '../../hooks/use-shortcut'; @@ -248,6 +249,10 @@ const ExtensionHeaderInfo: FunctionComponent<{ Published by  + diff --git a/webui/src/pages/extension-detail/trusted-publishing-icon.tsx b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx new file mode 100644 index 000000000..a88f35600 --- /dev/null +++ b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx @@ -0,0 +1,61 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ + +import { FunctionComponent, useContext } from 'react'; +import { Link } from '@mui/material'; +import { styled } from '@mui/material/styles'; +import RocketLaunchIcon from '@mui/icons-material/RocketLaunch'; +import { MainContext } from '../../context'; + +const IconLink = styled(Link)(({ theme }) => ({ + display: 'flex', + alignItems: 'center', + marginLeft: theme.spacing(0.5) +})); + +const IconBadge = styled('span')(({ theme }) => ({ + display: 'flex', + alignItems: 'center', + marginLeft: theme.spacing(0.5) +})); + +/** + * Marks a version that was published from a trusted publishing workflow instead of with a + * personal access token. Nothing is rendered for the ordinary case, so the icon's presence + * carries the whole signal. The same rocket stands for trusted publishing in the user settings. + */ +export const TrustedPublishingIcon: FunctionComponent<{ + publishedWithTrustedPublishing?: boolean; + color: string; +}> = ({ publishedWithTrustedPublishing, color }) => { + const { pageSettings } = useContext(MainContext); + + if (!publishedWithTrustedPublishing) { + return null; + } + + const title = 'Published via trusted publishing'; + const url = pageSettings.urls.trustedPublishing; + const icon = ; + + // a plain badge when this instance configures no documentation URL to link to + return url ? ( + + {icon} + + ) : ( + + {icon} + + ); +}; diff --git a/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx b/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx new file mode 100644 index 000000000..e9c2eeecf --- /dev/null +++ b/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx @@ -0,0 +1,53 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ + +import { describe, it, expect } from 'vitest'; +import { screen } from '@testing-library/react'; +import { TrustedPublishingIcon } from '../../../../src/pages/extension-detail/trusted-publishing-icon'; +import { PageSettings } from '../../../../src/page-settings'; +import { renderWithProviders } from '../../support/test-providers'; + +const TITLE = 'Published via trusted publishing'; + +const pageSettings = (trustedPublishing?: string) => ({ elements: {}, urls: { trustedPublishing } }) as PageSettings; + +describe('TrustedPublishingIcon', () => { + it('renders nothing for a version published with an access token', () => { + renderWithProviders(, { + mainContext: { pageSettings: pageSettings('https://example.com/trusted-publishing') } + }); + + expect(screen.queryByLabelText(TITLE)).not.toBeInTheDocument(); + }); + + it('links to the documentation when the instance configures a URL for it', () => { + renderWithProviders(, { + mainContext: { pageSettings: pageSettings('https://example.com/trusted-publishing') } + }); + + const link = screen.getByLabelText(TITLE); + expect(link.tagName).toBe('A'); + expect(link).toHaveAttribute('href', 'https://example.com/trusted-publishing'); + expect(link).toHaveAttribute('target', '_blank'); + }); + + it('still shows the icon when no documentation URL is configured', () => { + renderWithProviders(, { + mainContext: { pageSettings: pageSettings() } + }); + + const icon = screen.getByLabelText(TITLE); + expect(icon).toBeInTheDocument(); + expect(icon.tagName).not.toBe('A'); + }); +}); From 97a5e28e36ebacac6686049e8efc5cdd6225e1ac Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 10:32:05 +0200 Subject: [PATCH 08/25] update trusted publishing icon and add a divider --- .../src/pages/extension-detail/extension-detail.tsx | 13 +++++++++---- .../extension-detail/trusted-publishing-icon.tsx | 10 +++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/webui/src/pages/extension-detail/extension-detail.tsx b/webui/src/pages/extension-detail/extension-detail.tsx index 39d1ad3dc..661509b0d 100644 --- a/webui/src/pages/extension-detail/extension-detail.tsx +++ b/webui/src/pages/extension-detail/extension-detail.tsx @@ -249,10 +249,15 @@ const ExtensionHeaderInfo: FunctionComponent<{ Published by  - + {extension.publishedWithTrustedPublishing && ( + <> + + + + )} diff --git a/webui/src/pages/extension-detail/trusted-publishing-icon.tsx b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx index a88f35600..5cd61d65e 100644 --- a/webui/src/pages/extension-detail/trusted-publishing-icon.tsx +++ b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx @@ -14,8 +14,8 @@ import { FunctionComponent, useContext } from 'react'; import { Link } from '@mui/material'; import { styled } from '@mui/material/styles'; -import RocketLaunchIcon from '@mui/icons-material/RocketLaunch'; import { MainContext } from '../../context'; +import VerifiedIcon from '@mui/icons-material/Verified'; const IconLink = styled(Link)(({ theme }) => ({ display: 'flex', @@ -37,16 +37,12 @@ const IconBadge = styled('span')(({ theme }) => ({ export const TrustedPublishingIcon: FunctionComponent<{ publishedWithTrustedPublishing?: boolean; color: string; -}> = ({ publishedWithTrustedPublishing, color }) => { +}> = ({ color }) => { const { pageSettings } = useContext(MainContext); - if (!publishedWithTrustedPublishing) { - return null; - } - const title = 'Published via trusted publishing'; const url = pageSettings.urls.trustedPublishing; - const icon = ; + const icon = ; // a plain badge when this instance configures no documentation URL to link to return url ? ( From b3f2a7b101f509a2b62b19fdee3b32fff86ffec9 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 10:42:56 +0200 Subject: [PATCH 09/25] fix: point the trusted publishing link at the wiki and cover the icon guard The default deployment linked to the OpenSSF guidance; it now links to the Trusted Publishing wiki page, which documents how the feature works here and refers onward to OpenSSF. Whether a version is marked is decided by the extension header, which pairs the icon with a divider, so TrustedPublishingIcon loses the prop it no longer reads and the condition is tested where it lives: ExtensionHeaderInfo is exported and covered for a marked version, an unmarked one, and a response that omits the field. Co-Authored-By: Claude Opus 5 (1M context) --- webui/CHANGELOG.md | 2 +- webui/src/default/page-settings.tsx | 2 +- .../extension-detail/extension-detail.tsx | 7 +- .../trusted-publishing-icon.tsx | 5 +- .../extension-detail.spec.tsx | 70 +++++++++++++++++++ .../trusted-publishing-icon.spec.tsx | 12 +--- 6 files changed, 78 insertions(+), 20 deletions(-) create mode 100644 webui/test/unit/pages/extension-detail/extension-detail.spec.tsx diff --git a/webui/CHANGELOG.md b/webui/CHANGELOG.md index c0c2c17f4..bc96f33a2 100644 --- a/webui/CHANGELOG.md +++ b/webui/CHANGELOG.md @@ -11,7 +11,7 @@ This change log covers only the frontend library (webui) of Open VSX. - Add a "Data Consistency" page to the admin dashboard (#1622): a live overview of every registered consistency check's finding count, with actions to refresh it and to fix findings one at a time or all at once - Show a "Namespace not verified" state on an extension card when it can't be activated because its namespace already exists in a referenced external gallery and hasn't been verified, in both the "My Extensions" and namespace member extension lists. The card keeps its colour and takes a warning-toned frame and icon, since this is the publisher's to fix rather than an extension that is simply switched off - Show a warning notice with a claim action wherever an unverified namespace is holding something back — the extension settings page when the extension has a namespace ownership conflict, and the namespace settings page for any unverified namespace — making clear the namespace must be claimed (verified) first. The action is the deployment's configured `elements.claimNamespace`, falling back to the namespace access documentation when none is configured. The admin dashboard's extension and namespace views show the same explanation without the claim action, since claiming is the publisher's action to take, not an admin's on someone else's behalf -- Mark a version that was published through a trusted publishing workflow with a rocket icon next to "Published by" on the extension detail page, linking to the deployment's trusted publishing documentation when one is configured +- Mark a version that was published through a trusted publishing workflow with an icon next to "Published by" on the extension detail page, linking to the deployment's trusted publishing documentation. The default deployment points that link at the [Trusted Publishing](https://github.com/eclipse-openvsx/openvsx/wiki/Trusted-Publishing) wiki page ### Changed diff --git a/webui/src/default/page-settings.tsx b/webui/src/default/page-settings.tsx index 44f667991..7115edbc1 100644 --- a/webui/src/default/page-settings.tsx +++ b/webui/src/default/page-settings.tsx @@ -206,7 +206,7 @@ export default function createPageSettings(prefersDarkMode: boolean, serverUrl: extensionDefaultIcon: '/default-icon.png', namespaceAccessInfo: 'https://github.com/eclipse/openvsx/wiki/Namespace-Access', publisherAgreement: createAbsoluteURL([serverUrl, 'documents', 'publisher-agreement.md']), - trustedPublishing: 'https://repos.openssf.org/trusted-publishers-for-all-package-repositories' + trustedPublishing: 'https://github.com/eclipse-openvsx/openvsx/wiki/Trusted-Publishing' } }; } diff --git a/webui/src/pages/extension-detail/extension-detail.tsx b/webui/src/pages/extension-detail/extension-detail.tsx index 661509b0d..41155370c 100644 --- a/webui/src/pages/extension-detail/extension-detail.tsx +++ b/webui/src/pages/extension-detail/extension-detail.tsx @@ -188,7 +188,7 @@ const LicenseLink: FunctionComponent<{ return <>{extension.license || 'Unlicensed'}; }; -const ExtensionHeaderInfo: FunctionComponent<{ +export const ExtensionHeaderInfo: FunctionComponent<{ extension: Extension; headerTextColor: string; }> = ({ extension, headerTextColor }) => { @@ -252,10 +252,7 @@ const ExtensionHeaderInfo: FunctionComponent<{ {extension.publishedWithTrustedPublishing && ( <> - + )} diff --git a/webui/src/pages/extension-detail/trusted-publishing-icon.tsx b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx index 5cd61d65e..64ff5414d 100644 --- a/webui/src/pages/extension-detail/trusted-publishing-icon.tsx +++ b/webui/src/pages/extension-detail/trusted-publishing-icon.tsx @@ -31,11 +31,10 @@ const IconBadge = styled('span')(({ theme }) => ({ /** * Marks a version that was published from a trusted publishing workflow instead of with a - * personal access token. Nothing is rendered for the ordinary case, so the icon's presence - * carries the whole signal. The same rocket stands for trusted publishing in the user settings. + * personal access token. Whether a version qualifies is the caller's to decide, since the + * header pairs the icon with a divider. */ export const TrustedPublishingIcon: FunctionComponent<{ - publishedWithTrustedPublishing?: boolean; color: string; }> = ({ color }) => { const { pageSettings } = useContext(MainContext); diff --git a/webui/test/unit/pages/extension-detail/extension-detail.spec.tsx b/webui/test/unit/pages/extension-detail/extension-detail.spec.tsx new file mode 100644 index 000000000..75a9e4053 --- /dev/null +++ b/webui/test/unit/pages/extension-detail/extension-detail.spec.tsx @@ -0,0 +1,70 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ + +import { describe, it, expect } from 'vitest'; +import { screen } from '@testing-library/react'; +import { ExtensionHeaderInfo } from '../../../../src/pages/extension-detail/extension-detail'; +import { Extension } from '../../../../src/extension-registry-types'; +import { PageSettings } from '../../../../src/page-settings'; +import { renderWithProviders } from '../../support/test-providers'; + +const TRUSTED_PUBLISHING_TITLE = 'Published via trusted publishing'; + +const extension = (overrides: Partial = {}): Extension => + ({ + name: 'bar', + namespace: 'foo', + namespaceDisplayName: 'Foo', + version: '1.0.0', + displayName: 'Bar Tools', + files: {}, + downloadCount: 0, + reviewCount: 0, + deprecated: false, + verified: true, + publishedBy: { loginName: 'test_user', homepage: 'https://example.com/test_user' }, + ...overrides + }) as unknown as Extension; + +const renderHeaderInfo = (overrides: Partial = {}) => + renderWithProviders(, { + mainContext: { + pageSettings: { + elements: {}, + urls: { + namespaceAccessInfo: 'https://example.com/namespace-access', + trustedPublishing: 'https://example.com/trusted-publishing' + } + } as PageSettings + } + }); + +describe('ExtensionHeaderInfo', () => { + it('marks a version published through trusted publishing', () => { + renderHeaderInfo({ publishedWithTrustedPublishing: true }); + + expect(screen.getByLabelText(TRUSTED_PUBLISHING_TITLE)).toBeInTheDocument(); + }); + + it('leaves a version published with an access token unmarked', () => { + renderHeaderInfo({ publishedWithTrustedPublishing: false }); + + expect(screen.queryByLabelText(TRUSTED_PUBLISHING_TITLE)).not.toBeInTheDocument(); + }); + + it('leaves the mark off when the registry does not report how a version was published', () => { + renderHeaderInfo(); + + expect(screen.queryByLabelText(TRUSTED_PUBLISHING_TITLE)).not.toBeInTheDocument(); + }); +}); diff --git a/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx b/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx index e9c2eeecf..21a4fbb31 100644 --- a/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx +++ b/webui/test/unit/pages/extension-detail/trusted-publishing-icon.spec.tsx @@ -22,16 +22,8 @@ const TITLE = 'Published via trusted publishing'; const pageSettings = (trustedPublishing?: string) => ({ elements: {}, urls: { trustedPublishing } }) as PageSettings; describe('TrustedPublishingIcon', () => { - it('renders nothing for a version published with an access token', () => { - renderWithProviders(, { - mainContext: { pageSettings: pageSettings('https://example.com/trusted-publishing') } - }); - - expect(screen.queryByLabelText(TITLE)).not.toBeInTheDocument(); - }); - it('links to the documentation when the instance configures a URL for it', () => { - renderWithProviders(, { + renderWithProviders(, { mainContext: { pageSettings: pageSettings('https://example.com/trusted-publishing') } }); @@ -42,7 +34,7 @@ describe('TrustedPublishingIcon', () => { }); it('still shows the icon when no documentation URL is configured', () => { - renderWithProviders(, { + renderWithProviders(, { mainContext: { pageSettings: pageSettings() } }); From 86d9bcb7e4affb30165b5941128c3595258c7084 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 12:36:03 +0200 Subject: [PATCH 10/25] fix: delete trusted publishers when their author stops being an owner Only a namespace owner may register a trusted publisher, but nothing removed the registration once that ownership ended: it stayed listed and kept issuing publishing tokens. Publishing itself was still refused, since the upload re-checks namespace membership, but re-adding the user as a plain contributor silently restored CI publishing they no longer had. UserService now revokes the registrations a user created in a namespace whenever they stop being an owner of it - on removal and on demotion to contributor - which also invalidates the tokens issued under them. AdminService.revokePublisherContributions deleted the membership rows in bulk and so went around that; it now removes them one by one through UserService, the way forgetUser already did. That also evicts the namespace details cache, which the bulk delete skipped. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/org/eclipse/openvsx/UserService.java | 23 +++++ .../eclipse/openvsx/admin/AdminService.java | 10 +- .../repositories/RepositoryService.java | 7 ++ .../TrustedPublisherRepository.java | 4 + .../java/org/eclipse/openvsx/UserAPITest.java | 9 ++ .../org/eclipse/openvsx/UserServiceTest.java | 95 +++++++++++++++++++ .../eclipse/openvsx/admin/AdminAPITest.java | 12 +++ .../openvsx/admin/AdminServiceTest.java | 30 ++++++ .../RepositoryServiceSmokeTest.java | 1 + 9 files changed, 188 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/UserService.java b/server/src/main/java/org/eclipse/openvsx/UserService.java index 209451722..f0009b3fe 100644 --- a/server/src/main/java/org/eclipse/openvsx/UserService.java +++ b/server/src/main/java/org/eclipse/openvsx/UserService.java @@ -162,6 +162,7 @@ public ResultJson removeNamespaceMember(Namespace namespace, UserData user) thro "User " + user.getLoginName() + " is not a member of " + namespace.getName() + "."); } entityManager.remove(membership); + revokeTrustedPublishers(namespace, user); return ResultJson.success("Removed " + user.getLoginName() + " from namespace " + namespace.getName() + "."); } @@ -177,7 +178,11 @@ public ResultJson addNamespaceMember(Namespace namespace, UserData user, String if (role.equals(membership.getRole())) { throw new ErrorResultException("User " + user.getLoginName() + " already has the role " + role + "."); } + var wasOwner = NamespaceMembership.ROLE_OWNER.equals(membership.getRole()); membership.setRole(role); + if (wasOwner) { + revokeTrustedPublishers(namespace, user); + } return ResultJson.success( "Changed role of " + user.getLoginName() + " in " + namespace.getName() + " to " + role + "."); } @@ -189,6 +194,24 @@ public ResultJson addNamespaceMember(Namespace namespace, UserData user, String return ResultJson.success("Added " + user.getLoginName() + " as " + role + " of " + namespace.getName() + "."); } + /** + * Only a namespace owner may register a trusted publisher, so a registration must not outlive the ownership + * it was made under: whoever stops being an owner - by demotion or by leaving the namespace - loses the + * registrations they created there. Deleting them also invalidates the publishing tokens already issued + * under them, which a mere loss of publishing rights would not. + */ + private void revokeTrustedPublishers(Namespace namespace, UserData user) { + var trustedPublishers = repositories.findTrustedPublishersByNamespaceAndCreatedBy(namespace, user).toList(); + trustedPublishers.forEach(repositories::deleteTrustedPublisher); + if (!trustedPublishers.isEmpty()) { + logger.info( + "Deleted {} trusted publisher(s) of {} in namespace {}, who is no longer an owner of it", + trustedPublishers.size(), + user.getLoginName(), + namespace.getName()); + } + } + @Transactional(rollbackOn = { ErrorResultException.class, NotFoundException.class }) @CacheEvict(value = { CACHE_NAMESPACE_DETAILS_JSON }, key = "#details.name") public ResultJson updateNamespaceDetails(NamespaceDetailsJson details, UserData user) { diff --git a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java index 020aed491..a2b3c50bb 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java @@ -536,13 +536,17 @@ public ResultJson revokePublisherContributions(String provider, String loginName extensions.updateExtension(extension); } - // revoke namespace memberships + // Revoke namespace memberships one by one through UserService rather than deleting them in bulk: + // that is what deletes the trusted publishers registered under an ownership being revoked here, and + // what evicts the namespace details cache - same as when an owner removes a member themselves. var namespaceMemberships = repositories.findMemberships(user); var numberOfNamespaceMemberships = 0L; // add a null check due to tests using mocks which return null if (namespaceMemberships != null) { - numberOfNamespaceMemberships = namespaceMemberships.stream().count(); - repositories.deleteMemberships(user); + for (var membership : namespaceMemberships.toList()) { + users.removeNamespaceMember(membership.getNamespace(), user); + numberOfNamespaceMemberships++; + } } var message = "Deactivated " + deactivatedTokenCount + " tokens, " diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index 4f8cc866f..9b87062c1 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -195,6 +195,13 @@ public Streamable findTrustedPublishersByExtension(Extension e return trustedPublisherRepo.findTrustedPublishersByExtension(extension); } + public Streamable findTrustedPublishersByNamespaceAndCreatedBy( + Namespace namespace, + UserData createdBy + ) { + return trustedPublisherRepo.findByExtension_NamespaceAndCreatedBy(namespace, createdBy); + } + public TrustedPublisher findTrustedPublisher(long id) { return trustedPublisherRepo.findById(id); } diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/TrustedPublisherRepository.java b/server/src/main/java/org/eclipse/openvsx/repositories/TrustedPublisherRepository.java index 22e9882e4..e51f6fd67 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/TrustedPublisherRepository.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/TrustedPublisherRepository.java @@ -16,11 +16,15 @@ import org.springframework.data.util.Streamable; import org.eclipse.openvsx.entities.Extension; +import org.eclipse.openvsx.entities.Namespace; import org.eclipse.openvsx.entities.TrustedPublisher; +import org.eclipse.openvsx.entities.UserData; public interface TrustedPublisherRepository extends Repository { Streamable findTrustedPublishersByExtension(Extension extension); + Streamable findByExtension_NamespaceAndCreatedBy(Namespace namespace, UserData createdBy); + TrustedPublisher findById(long id); void delete(TrustedPublisher trustedPublisher); diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index bfad69387..5f01052a4 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -19,6 +19,7 @@ import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; @@ -103,6 +104,14 @@ ) class UserAPITest { + @BeforeEach + void noTrustedPublishersByDefault() { + // the real repository hands back an empty Streamable rather than null; only the trusted + // publishing tests care what it actually holds + Mockito.when(repositories.findTrustedPublishersByNamespaceAndCreatedBy(any(), any())) + .thenReturn(Streamable.empty()); + } + @MockitoSpyBean UserService users; diff --git a/server/src/test/java/org/eclipse/openvsx/UserServiceTest.java b/server/src/test/java/org/eclipse/openvsx/UserServiceTest.java index f48f6254c..8c6764304 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserServiceTest.java @@ -21,13 +21,17 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.data.util.Streamable; import org.springframework.security.authentication.AuthenticationServiceException; import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; import org.springframework.test.context.bean.override.mockito.MockitoBean; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.eclipse.openvsx.cache.CacheService; +import org.eclipse.openvsx.entities.Extension; import org.eclipse.openvsx.entities.Namespace; +import org.eclipse.openvsx.entities.NamespaceMembership; +import org.eclipse.openvsx.entities.TrustedPublisher; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.json.NamespaceDetailsJson; import org.eclipse.openvsx.repositories.RepositoryService; @@ -155,6 +159,97 @@ void shouldSkipCollisionCheckWhenDisplayNameUnchanged() { verify(repositories, never()).findConflictingNamespaces(anyString(), any(Namespace.class)); } + // A trusted publisher may only be registered by a namespace owner, so it must not survive the + // ownership it was created under - the publishing tokens issued under it would outlive it otherwise. + + @Test + void shouldDeleteTrustedPublishersWhenAnOwnerLeavesTheNamespace() { + var user = mockUser("auth"); + var namespace = mockNamespace(); + var membership = mockMembership(user, namespace, NamespaceMembership.ROLE_OWNER); + var trustedPublisher = mockTrustedPublisher(namespace); + Mockito.when(repositories.findMembership(user, namespace)).thenReturn(membership); + Mockito.when(repositories.findTrustedPublishersByNamespaceAndCreatedBy(namespace, user)) + .thenReturn(Streamable.of(trustedPublisher)); + + users.removeNamespaceMember(namespace, user); + + verify(repositories).deleteTrustedPublisher(trustedPublisher); + } + + @Test + void shouldDeleteTrustedPublishersWhenAnOwnerIsDemotedToContributor() { + var user = mockUser("auth"); + var namespace = mockNamespace(); + var membership = mockMembership(user, namespace, NamespaceMembership.ROLE_OWNER); + var trustedPublisher = mockTrustedPublisher(namespace); + Mockito.when(repositories.findMembership(user, namespace)).thenReturn(membership); + Mockito.when(repositories.findTrustedPublishersByNamespaceAndCreatedBy(namespace, user)) + .thenReturn(Streamable.of(trustedPublisher)); + + users.addNamespaceMember(namespace, user, NamespaceMembership.ROLE_CONTRIBUTOR); + + assertEquals(NamespaceMembership.ROLE_CONTRIBUTOR, membership.getRole()); + verify(repositories).deleteTrustedPublisher(trustedPublisher); + } + + @Test + void shouldKeepTrustedPublishersWhenAContributorIsPromotedToOwner() { + var user = mockUser("auth"); + var namespace = mockNamespace(); + var membership = mockMembership(user, namespace, NamespaceMembership.ROLE_CONTRIBUTOR); + Mockito.when(repositories.findMembership(user, namespace)).thenReturn(membership); + + users.addNamespaceMember(namespace, user, NamespaceMembership.ROLE_OWNER); + + assertEquals(NamespaceMembership.ROLE_OWNER, membership.getRole()); + verify(repositories, never()).findTrustedPublishersByNamespaceAndCreatedBy(any(), any()); + verify(repositories, never()).deleteTrustedPublisher(any()); + } + + @Test + void shouldKeepTrustedPublishersOfEveryOtherOwner() { + var leaving = mockUser(1, "leaving_user", "auth-1"); + var namespace = mockNamespace(); + var membership = mockMembership(leaving, namespace, NamespaceMembership.ROLE_OWNER); + Mockito.when(repositories.findMembership(leaving, namespace)).thenReturn(membership); + // only the registrations this user created are looked up, so the other owners' ones stay + Mockito.when(repositories.findTrustedPublishersByNamespaceAndCreatedBy(namespace, leaving)) + .thenReturn(Streamable.empty()); + + users.removeNamespaceMember(namespace, leaving); + + verify(repositories, never()).deleteTrustedPublisher(any()); + } + + private Namespace mockNamespace() { + var namespace = new Namespace(); + namespace.setId(1); + namespace.setName("my-ns"); + return namespace; + } + + private NamespaceMembership mockMembership(UserData user, Namespace namespace, String role) { + var membership = new NamespaceMembership(); + membership.setUser(user); + membership.setNamespace(namespace); + membership.setRole(role); + return membership; + } + + private TrustedPublisher mockTrustedPublisher(Namespace namespace) { + var extension = new Extension(); + extension.setId(2); + extension.setName("my-ext"); + extension.setNamespace(namespace); + + var trustedPublisher = new TrustedPublisher(); + trustedPublisher.setId(3); + trustedPublisher.setExtension(extension); + trustedPublisher.setProvider("github"); + return trustedPublisher; + } + private UserData mockUser(String authId) { return mockUser(1, "test_user", authId); } diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index abf730ce4..6cd42588d 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -22,6 +22,7 @@ import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; @@ -152,6 +153,14 @@ ) class AdminAPITest { + @BeforeEach + void noTrustedPublishersByDefault() { + // the real repository hands back an empty Streamable rather than null; only the trusted + // publishing tests care what it actually holds + Mockito.when(repositories.findTrustedPublishersByNamespaceAndCreatedBy(any(), any())) + .thenReturn(Streamable.empty()); + } + @MockitoSpyBean UserService users; @@ -2080,6 +2089,9 @@ void testRevokeBulkPublishers() throws Exception { membership2.setUser(user2); when(repositories.findMemberships(user2)) .thenReturn(Streamable.of(membership2)); + // revoking goes through UserService.removeNamespaceMember, which looks the membership up again + when(repositories.findMembership(user, namespace)).thenReturn(membership); + when(repositories.findMembership(user2, namespace)).thenReturn(membership2); var baseRequest = """ { diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java index cb90a06f5..61d07093e 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java @@ -20,12 +20,14 @@ import org.springframework.data.util.Streamable; import org.eclipse.openvsx.ExtensionService; +import org.eclipse.openvsx.UserService; import org.eclipse.openvsx.eclipse.EclipseService; import org.eclipse.openvsx.entities.Extension; import org.eclipse.openvsx.entities.ExtensionVersion; import org.eclipse.openvsx.entities.ExtensionVersionChange; import org.eclipse.openvsx.entities.ExtensionVersionState; import org.eclipse.openvsx.entities.Namespace; +import org.eclipse.openvsx.entities.NamespaceMembership; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.util.LogService; @@ -64,6 +66,9 @@ class AdminServiceTest { @Mock LogService logs; + @Mock + UserService users; + @Mock EntityManager entityManager; @@ -97,6 +102,31 @@ private void mockNoReferences(Extension extension) { when(repositories.findDependenciesReference(extension)).thenReturn(Streamable.empty()); } + @Test + void revokingPublisherContributionsRemovesEachMembershipThroughUserService() { + var user = new UserData(); + user.setLoginName("amy"); + var namespace = new Namespace(); + namespace.setName(NAMESPACE); + var membership = new NamespaceMembership(); + membership.setUser(user); + membership.setNamespace(namespace); + membership.setRole(NamespaceMembership.ROLE_OWNER); + + when(repositories.findUserByLoginName("github", "amy")).thenReturn(user); + when(repositories.findPersonalAccessTokens(user)).thenReturn(Streamable.empty()); + when(repositories.findVersionsByUser(user, true)).thenReturn(Streamable.empty()); + when(repositories.findMemberships(user)).thenReturn(Streamable.of(membership)); + + var result = adminService.revokePublisherContributions("github", "amy", admin); + + // going through UserService is what deletes the trusted publishers the revoked owner registered; + // deleting the membership rows in bulk would leave those registrations behind + verify(users).removeNamespaceMember(namespace, user); + verify(repositories, never()).deleteMemberships(user); + assertThat(result.getSuccess()).contains("removed 1 namespace memberships"); + } + @Test void revokingPublisherContributionsRecordsWhenTheVersionsStoppedBeingVisible() { var user = new UserData(); diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index 4f224fe05..e99e03f42 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -490,6 +490,7 @@ void testExecuteQueries() { () -> repositories.findUnprocessedDaysForDailyUsage(customer), () -> repositories.saveDailyUsageStats(dailyUsageStats), () -> repositories.findTrustedPublishersByExtension(extension), + () -> repositories.findTrustedPublishersByNamespaceAndCreatedBy(namespace, userData), () -> repositories.findTrustedPublisher(1L), () -> repositories.deleteTrustedPublisher(trustedPublisher), () -> repositories.deleteTier(tier), From b956168333fa629fa503ddfa76ebd45c971036ec Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 12:36:15 +0200 Subject: [PATCH 11/25] fix: cascade trusted publisher and scoped token rows on delete trusted_publisher.extension_id had no ON DELETE clause, so purging an extension that ever had a registration was refused outright by the database. personal_access_token.scope_extension_id had the same gap, which kept the purge broken for any registration that had actually been used: the exchange leaves a token scoped to the extension behind, and expiry only deactivates that row. All of these now cascade. Cascade rather than SET NULL on the token, because AccessTokenService.getScope reads a token with neither scope column set as unrestricted - nulling a scope would widen a scoped token instead of retiring it. The trusted_publisher_id reference is cascaded too: a token issued under a registration may only publish the one extension it was made for, so it has nothing left to authorize once the registration is gone. Note for deployments that already ran V1_72: editing the migration changes its Flyway checksum. The feature has never been in a release, so only development databases are affected; they need a repair or a fresh database. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 4 +- .../db/migration/V1_72__Trusted_Publisher.sql | 22 +- .../TrustedPublisherCascadeTest.java | 190 ++++++++++++++++++ 3 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublisherCascadeTest.java diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index ec2257ef6..d1f2b3fab 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -211,7 +211,9 @@ public AccessTokenAuthentication useAccessToken(String tokenValue, AccessTokenAc token.setActive(false); return null; } - // TPT without TP => registration was deleted + // Deleting a registration takes its tokens with it, so this should not be reachable; kept as a + // guard, because a TPT that lost its registration may only ever publish an extension it can no + // longer be checked against. if (token.getType() == PersonalAccessTokenType.TPT && token.getTrustedPublisher() == null) { token.setActive(false); return null; diff --git a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql index 7c8d2f079..e3dd5eadf 100644 --- a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql +++ b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql @@ -5,7 +5,10 @@ CREATE SEQUENCE IF NOT EXISTS trusted_publisher_seq START WITH 1 INCREMENT BY 1; CREATE TABLE IF NOT EXISTS public.trusted_publisher ( id BIGINT NOT NULL PRIMARY KEY DEFAULT nextval('trusted_publisher_seq'), - extension_id BIGINT NOT NULL REFERENCES public.extension(id), + -- A registration only means anything for the extension it points at, so it goes when that extension + -- is purged. Cascading rather than restricting, because purging is an administrative action that has + -- to succeed: without this, an extension that ever had a trusted publisher could never be purged. + extension_id BIGINT NOT NULL REFERENCES public.extension(id) ON DELETE CASCADE, provider CHARACTER VARYING(32) NOT NULL, registration JSONB NOT NULL, claims JSONB NOT NULL, @@ -24,12 +27,17 @@ ALTER TABLE ONLY public.personal_access_token ADD COLUMN IF NOT EXISTS version SMALLINT, -- the type column LLT, OTT or TPT ADD COLUMN IF NOT EXISTS type CHARACTER VARYING(32), - -- optional; the extension that the token is scoped to - ADD COLUMN IF NOT EXISTS scope_extension_id BIGINT REFERENCES public.extension(id), - -- optional; the namespace that the token is scoped to - ADD COLUMN IF NOT EXISTS scope_namespace_id BIGINT REFERENCES public.namespace(id), - -- optional; the trusted publisher that the token was created for (token must remain; registration may be deleted) - ADD COLUMN IF NOT EXISTS trusted_publisher_id BIGINT REFERENCES public.trusted_publisher(id) ON DELETE SET NULL; + -- optional; the extension that the token is scoped to. Deleted with it rather than detached from + -- it: AccessTokenService.getScope treats a token with neither scope set as unrestricted, so setting + -- this to NULL would silently widen a scoped token instead of retiring it. A token scoped to an + -- extension that no longer exists can authorize nothing anyway. + ADD COLUMN IF NOT EXISTS scope_extension_id BIGINT REFERENCES public.extension(id) ON DELETE CASCADE, + -- optional; the namespace that the token is scoped to. Cascaded for the same reason. + ADD COLUMN IF NOT EXISTS scope_namespace_id BIGINT REFERENCES public.namespace(id) ON DELETE CASCADE, + -- optional; the trusted publisher that the token was created for. Deleting a registration retires the + -- tokens issued under it: they may only ever publish the one extension it was made for, so once it is + -- gone they can authorize nothing. + ADD COLUMN IF NOT EXISTS trusted_publisher_id BIGINT REFERENCES public.trusted_publisher(id) ON DELETE CASCADE; -- set OTT based on description (is hardwired in codebase) UPDATE public.personal_access_token diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublisherCascadeTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublisherCascadeTest.java new file mode 100644 index 000000000..053239c14 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublisherCascadeTest.java @@ -0,0 +1,190 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + *****************************************************************************/ +package org.eclipse.openvsx.trustedpublishing; + +import java.util.Map; + +import jakarta.persistence.EntityManager; +import org.jobrunr.scheduling.JobRequestScheduler; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.support.TransactionTemplate; + +import org.eclipse.openvsx.AbstractPostgresContainerTest; +import org.eclipse.openvsx.ExtensionService; +import org.eclipse.openvsx.entities.Extension; +import org.eclipse.openvsx.entities.Namespace; +import org.eclipse.openvsx.entities.PersonalAccessToken; +import org.eclipse.openvsx.entities.PersonalAccessTokenType; +import org.eclipse.openvsx.entities.TrustedPublisher; +import org.eclipse.openvsx.entities.UserData; +import org.eclipse.openvsx.repositories.RepositoryService; +import org.eclipse.openvsx.search.SearchUtilService; +import org.eclipse.openvsx.util.TimeUtil; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + +/** + * A trusted publisher points at an extension, and the publishing token issued under it points at both, so + * deleting either end has to take what depends on it along. Only the database can be asserted on: none of + * these references has an inverse mapping on {@link Extension} or {@link TrustedPublisher}, so JPA cascades + * nothing and the foreign key constraints alone decide what happens. + */ +@SpringBootTest +class TrustedPublisherCascadeTest extends AbstractPostgresContainerTest { + + @Autowired + ExtensionService extensions; + + @Autowired + RepositoryService repositories; + + @Autowired + EntityManager em; + + @Autowired + PlatformTransactionManager txManager; + + @MockitoBean + SearchUtilService search; + + @MockitoBean + JobRequestScheduler scheduler; + + @Test + void purgingAnExtensionTakesItsTrustedPublisherAndTokensWithIt() { + var ids = persistRegistrationWithToken("purge"); + + // without the constraints deleting the registration and the token, this fails outright on a foreign + // key and the extension can never be purged at all + assertThatCode(() -> transaction(status -> { + var user = em.find(UserData.class, ids.userId()); + var extension = em.find(Extension.class, ids.extensionId()); + extensions.purgeExtension(user, extension, false); + })).doesNotThrowAnyException(); + + transaction(status -> { + assertThat(em.find(Extension.class, ids.extensionId())).isNull(); + assertThat(em.find(TrustedPublisher.class, ids.trustedPublisherId())).isNull(); + // retiring the token rather than detaching it: one left behind with neither scope set would + // read as unrestricted + assertThat(em.find(PersonalAccessToken.class, ids.tokenId())).isNull(); + }); + + cleanUp(ids, "purge"); + } + + @Test + void deletingATrustedPublisherTakesTheTokensIssuedUnderItWithIt() { + var ids = persistRegistrationWithToken("revoke"); + + transaction( + status -> repositories + .deleteTrustedPublisher(em.find(TrustedPublisher.class, ids.trustedPublisherId()))); + + transaction(status -> { + assertThat(em.find(TrustedPublisher.class, ids.trustedPublisherId())).isNull(); + // a token issued under a registration may only publish the extension it was made for, so it + // has nothing left to authorize once the registration is gone + assertThat(em.find(PersonalAccessToken.class, ids.tokenId())).isNull(); + // the extension itself is untouched by this + assertThat(em.find(Extension.class, ids.extensionId())).isNotNull(); + }); + + cleanUp(ids, "revoke"); + } + + private record Ids(long userId, long namespaceId, long extensionId, long trustedPublisherId, long tokenId) {} + + /** + * A registration that has actually been used: the exchange leaves a token scoped to the same extension + * behind, and expiry only deactivates that row, so it outlives the exchange it was issued for. + */ + private Ids persistRegistrationWithToken(String prefix) { + return new TransactionTemplate(txManager).execute(status -> { + var user = new UserData(); + user.setLoginName(prefix + "-tp-owner"); + user.setProvider("github"); + em.persist(user); + + var namespace = new Namespace(); + namespace.setName(prefix + "-tp-testns"); + em.persist(namespace); + + var extension = new Extension(); + extension.setName(prefix + "-tp-testext"); + extension.setNamespace(namespace); + extension.setActive(true); + em.persist(extension); + + var trustedPublisher = new TrustedPublisher(); + trustedPublisher.setExtension(extension); + trustedPublisher.setProvider("github"); + trustedPublisher.setRegistration(Map.of("owner", "octo-org", "repo", "octo-repo")); + trustedPublisher.setClaims(Map.of("repository_id", "74")); + trustedPublisher.setCreatedBy(user); + trustedPublisher.setCreatedTimestamp(TimeUtil.getCurrentUTC()); + em.persist(trustedPublisher); + + var token = new PersonalAccessToken(); + token.setUser(user); + token.setValue(prefix + "-tp-token-value"); + token.setActive(false); + token.setCreatedTimestamp(TimeUtil.getCurrentUTC()); + token.setVersion(1); + token.setType(PersonalAccessTokenType.TPT); + token.setDescription("Trusted publishing (github)"); + token.setTrustedPublisher(trustedPublisher); + token.setScopeExtension(extension); + em.persist(token); + em.flush(); + + return new Ids( + user.getId(), + namespace.getId(), + extension.getId(), + trustedPublisher.getId(), + token.getId()); + }); + } + + private void transaction(java.util.function.Consumer work) { + new TransactionTemplate(txManager).executeWithoutResult(work::accept); + } + + /** The container is shared with every other test, so leave nothing of this one behind. */ + private void cleanUp(Ids ids, String prefix) { + transaction(status -> { + em.createQuery("delete from PersonalAccessToken t where t.user.id = :id") + .setParameter("id", ids.userId()) + .executeUpdate(); + em.createQuery("delete from TrustedPublisher p where p.createdBy.id = :id") + .setParameter("id", ids.userId()) + .executeUpdate(); + em.createQuery("delete from Extension e where e.namespace.id = :id") + .setParameter("id", ids.namespaceId()) + .executeUpdate(); + em.createQuery("delete from PersistedLog l where l.user.id = :id") + .setParameter("id", ids.userId()) + .executeUpdate(); + em.createQuery("delete from Namespace n where n.id = :id") + .setParameter("id", ids.namespaceId()) + .executeUpdate(); + em.createQuery("delete from UserData u where u.id = :id") + .setParameter("id", ids.userId()) + .executeUpdate(); + }); + } +} From c541bb368c28ba864a797887b945feaf10e50ab2 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 12:41:17 +0200 Subject: [PATCH 12/25] chore: remove the unused ott-expiration setting Nothing has read ovsx.access-token.ott-expiration since one-time general access tokens stopped being issued in 1.2.0 - PersonalAccessTokenType.OTT is deprecated and no code creates one - so the property, its two accessors and its startup validation only suggested a knob that does nothing. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenConfig.java | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java index 1de3b3b8e..1a52c0e1c 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java @@ -34,18 +34,6 @@ public class AccessTokenConfig { @Value("#{'${ovsx.access-token.prefix:${ovsx.token-prefix:}}'}") private String prefix; - /** - * The expiration period for one time personal access tokens. The one time - * access token must be used in this period. - *

- * If {@code 0} is provided, the one time access tokens do not expire. - *

- * Property: {@code ovsx.access-token.ott-expiration} - * Default: {@code PT5M}, expires in 5 minutes - */ - @Value("${ovsx.access-token.ott-expiration:PT5M}") - private Duration ottExpiration; - /** * The expiration period for one time trusted publishing tokens. The one time * access token must be used in this period. @@ -155,14 +143,6 @@ public boolean isTokenExpiryEnabled() { return this.expiration; } - public boolean isOttTokenExpiryEnabled() { - return this.ottExpiration.isPositive(); - } - - public @NonNull Duration getOttExpiration() { - return ottExpiration; - } - public boolean isTptTokenExpiryEnabled() { return this.tptExpiration.isPositive(); } @@ -222,10 +202,6 @@ public void validate() { throw new IllegalArgumentException( "ovsx.access-token.expiration must be a non-negative duration, got: " + expiration); } - if (ottExpiration.isNegative()) { - throw new IllegalArgumentException( - "ovsx.access-token.ott-expiration must be a non-negative duration, got: " + ottExpiration); - } if (tptExpiration.isNegative()) { throw new IllegalArgumentException( "ovsx.access-token.tpt-expiration must be a non-negative duration, got: " + tptExpiration); From 980eb5b2bbeebbffbf566b1f77b8772626087f95 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 13:28:43 +0200 Subject: [PATCH 13/25] refactor: let trusted publishing own the lifetime of the tokens it issues How long a publishing token lives is a trusted publishing decision, but it was configured as ovsx.access-token.tpt-expiration and read by AccessTokenService, so the token layer had to know about a feature that sits above it. The setting moves to ovsx.trusted-publishing.token-expiration, and the value is passed to createTrustedPublishingAccessToken instead of looked up there. It now has to be positive. A token that never expires is a long-lived credential, which is the thing trusted publishing exists to avoid, so the "0 disables expiry" escape hatch that ordinary access tokens offer is not carried over. The value is validated whether or not the feature is switched on, so a typo cannot lie in wait until someone enables it. It lives on TrustedPublishingProperties rather than as another @Value on TrustedPublishingConfig: @Value cannot convert a Duration in the bare SpringExtension contexts the provider tests use, while the configuration properties binder handles it natively. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenConfig.java | 24 ---------- .../accesstoken/AccessTokenService.java | 15 ++++-- .../TrustedPublishingConfig.java | 15 ++++++ .../TrustedPublishingProperties.java | 17 +++++++ .../TrustedPublishingService.java | 3 +- .../accesstoken/AccessTokenServiceTest.java | 46 +++++++++++++++++++ .../TrustedPublishingPropertiesTest.java | 35 ++++++++++++++ 7 files changed, 125 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java index 1a52c0e1c..6a18940a1 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenConfig.java @@ -34,18 +34,6 @@ public class AccessTokenConfig { @Value("#{'${ovsx.access-token.prefix:${ovsx.token-prefix:}}'}") private String prefix; - /** - * The expiration period for one time trusted publishing tokens. The one time - * access token must be used in this period. - *

- * If {@code 0} is provided, the one time trusted publishing tokens do not expire. - *

- * Property: {@code ovsx.access-token.tpt-expiration} - * Default: {@code PT5M}, expires in 5 minutes - */ - @Value("${ovsx.access-token.tpt-expiration:PT5M}") - private Duration tptExpiration; - /** * The expiration period for long-lived personal access tokens. *

@@ -143,14 +131,6 @@ public boolean isTokenExpiryEnabled() { return this.expiration; } - public boolean isTptTokenExpiryEnabled() { - return this.tptExpiration.isPositive(); - } - - public @NonNull Duration getTptExpiration() { - return tptExpiration; - } - public boolean isTokenExpiryNotificationEnabled() { return this.notification.isPositive(); } @@ -202,10 +182,6 @@ public void validate() { throw new IllegalArgumentException( "ovsx.access-token.expiration must be a non-negative duration, got: " + expiration); } - if (tptExpiration.isNegative()) { - throw new IllegalArgumentException( - "ovsx.access-token.tpt-expiration must be a non-negative duration, got: " + tptExpiration); - } if (notification.isNegative()) { throw new IllegalArgumentException( diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index d1f2b3fab..64637cae8 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -15,6 +15,7 @@ import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.time.Duration; import java.time.LocalDateTime; import jakarta.persistence.EntityManager; @@ -85,14 +86,18 @@ public AccessTokenJson createLongLivedAccessToken(UserData user, String descript /** * Creates a trusted publishing token for a trusted publisher. The token is scoped to given trusted publisher - * associated extension only. Depending on configuration, the token expiration may be set as well. + * associated extension only. How long it lives is the trusted publishing configuration's to decide, so the + * caller passes it in rather than this service reading it. */ @Transactional - public AccessTokenJson createTrustedPublishingAccessToken(TrustedPublisher trustedPublisher, String description) { + public AccessTokenJson createTrustedPublishingAccessToken( + TrustedPublisher trustedPublisher, + String description, + Duration expiration + ) { requireNonNull(trustedPublisher); - final LocalDateTime expiresTimestamp = config.isTptTokenExpiryEnabled() - ? TimeUtil.getCurrentUTC().plus(config.getTptExpiration()) - : null; + requireNonNull(expiration); + final LocalDateTime expiresTimestamp = TimeUtil.getCurrentUTC().plus(expiration); return createAccessToken( trustedPublisher.getCreatedBy(), description, diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java index 8f5b1a69f..8de6df49d 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java @@ -13,6 +13,7 @@ package org.eclipse.openvsx.trustedpublishing; import java.net.URI; +import java.time.Duration; import java.util.List; import java.util.Map; @@ -91,8 +92,22 @@ public Map getGitLabInstances() { return properties.getGitlab(); } + /** + * How long an issued publishing token is valid, {@code ovsx.trusted-publishing.token-expiration}. + */ + @NonNull + public Duration getTokenExpiration() { + return properties.getTokenExpiration(); + } + @PostConstruct public void validate() { + // checked whether or not the feature is on, so a typo does not lie in wait until it is turned on + var tokenExpiration = getTokenExpiration(); + if (tokenExpiration == null || !tokenExpiration.isPositive()) { + throw new IllegalStateException( + "ovsx.trusted-publishing.token-expiration must be a positive duration, got: " + tokenExpiration); + } if (enabled) { if (audience == null || audience.isBlank()) { throw new IllegalStateException("Trusted publishing is enabled, but audience is not configured"); diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java index dc4cad9e7..871545244 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java @@ -12,6 +12,7 @@ *****************************************************************************/ package org.eclipse.openvsx.trustedpublishing; +import java.time.Duration; import java.util.LinkedHashMap; import java.util.Map; @@ -52,9 +53,25 @@ @Validated public class TrustedPublishingProperties { + /** + * How long an issued publishing token is valid. Must be positive: a token that does not expire is a + * long-lived credential, which is the very thing trusted publishing exists to avoid. The lifetime of + * ordinary personal access tokens is {@code ovsx.access-token.expiration} instead. + */ + private Duration tokenExpiration = Duration.ofMinutes(5); + @Valid private Map gitlab = defaultGitLabInstances(); + @NonNull + public Duration getTokenExpiration() { + return tokenExpiration; + } + + public void setTokenExpiration(Duration tokenExpiration) { + this.tokenExpiration = tokenExpiration; + } + /** * The known GitLab instances, keyed by provider id. Configured instances are added to the public * instance, which stays available unless its id is redefined. diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index cb248ecec..f2b7925a2 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -314,7 +314,8 @@ public AccessTokenJson requestPublishToken(String namespaceName, String extensio // The issued token is TPT personal access token of the registering user scoped for selected namespace return tokens.createTrustedPublishingAccessToken( match, - TOKEN_DESCRIPTION_TEMPLATE.formatted(provider.getProviderId())); + TOKEN_DESCRIPTION_TEMPLATE.formatted(provider.getProviderId()), + config.getTokenExpiration()); } private Namespace requireOwnedNamespace(UserData user, String namespaceName) { diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index bbc2912ae..d6dfda614 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -12,16 +12,26 @@ *****************************************************************************/ package org.eclipse.openvsx.accesstoken; +import java.time.Duration; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Map; +import java.util.UUID; + import jakarta.persistence.EntityManager; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.eclipse.openvsx.entities.Extension; +import org.eclipse.openvsx.entities.Namespace; import org.eclipse.openvsx.entities.PersonalAccessToken; import org.eclipse.openvsx.entities.PersonalAccessTokenType; +import org.eclipse.openvsx.entities.TrustedPublisher; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.mail.MailService; import org.eclipse.openvsx.repositories.RepositoryService; @@ -29,6 +39,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -95,4 +106,39 @@ void rejectsATokenWithNoUser() { assertThat(tau).isNull(); } + + // How long a publishing token lives is the trusted publishing configuration's to decide, so this + // service applies whatever it is handed instead of reading a setting of its own. + @Test + void appliesTheExpirationItIsGivenToATrustedPublishingToken() { + var user = new UserData(); + var namespace = new Namespace(); + namespace.setName("foo"); + var extension = new Extension(); + extension.setName("bar"); + extension.setNamespace(namespace); + var trustedPublisher = new TrustedPublisher(); + trustedPublisher.setExtension(extension); + trustedPublisher.setCreatedBy(user); + trustedPublisher.setRegistration(Map.of()); + when(config.getPrefix()).thenReturn("ovsxat_"); + when(uuidService.generateRandom()).thenReturn(UUID.randomUUID()); + + var before = LocalDateTime.now(ZoneId.of("UTC")); + var json = accessTokenService + .createTrustedPublishingAccessToken( + trustedPublisher, + "Trusted publishing (github)", + Duration.ofMinutes(7)); + var after = LocalDateTime.now(ZoneId.of("UTC")); + + var persisted = ArgumentCaptor.forClass(PersonalAccessToken.class); + verify(entityManager).persist(persisted.capture()); + assertThat(persisted.getValue().getType()).isEqualTo(PersonalAccessTokenType.TPT); + assertThat(persisted.getValue().getExpiresTimestamp()) + .isBetween(before.plusMinutes(7), after.plusMinutes(7)); + // the token is scoped to the registration's extension, whatever its lifetime + assertThat(persisted.getValue().getScopeExtension()).isSameAs(extension); + assertThat(json.getValue()).isNotNull(); + } } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java index fe89ceae0..3c4cf84b2 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java @@ -12,6 +12,7 @@ *****************************************************************************/ package org.eclipse.openvsx.trustedpublishing; +import java.time.Duration; import java.util.List; import java.util.Map; @@ -90,6 +91,40 @@ void redefiningTheDefaultInstanceReplacesItAsAWhole() { .withMessageContaining("no name or no URL"); } + @Test + void issuedTokensExpireAfterFiveMinutesByDefault() { + assertThat(new TrustedPublishingProperties().getTokenExpiration()).isEqualTo(Duration.ofMinutes(5)); + } + + @Test + void tokenExpirationIsConfigurable() { + var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT30S")); + + assertThat(properties.getTokenExpiration()).isEqualTo(Duration.ofSeconds(30)); + } + + // A token that never expires is a long-lived credential, which is what trusted publishing exists to + // avoid, so "0 means no expiry" is not on offer here the way it is for personal access tokens. + @Test + void configRejectsANonPositiveTokenExpiration() { + for (var value : List.of("PT0S", "PT-5M")) { + var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", value)); + + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + .withMessageContaining("token-expiration must be a positive duration"); + } + } + + // ... and it is checked whether or not the feature is switched on, so a typo cannot lie in wait + @Test + void aBrokenTokenExpirationIsRejectedEvenWhileDisabled() { + var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT0S")); + var config = new TrustedPublishingConfig(properties); + + assertThatIllegalStateException().isThrownBy(config::validate) + .withMessageContaining("token-expiration must be a positive duration"); + } + @Test void enabledConfigAcceptsTheDefaultInstance() { assertThatCode(() -> enabledConfig(new TrustedPublishingProperties()).validate()).doesNotThrowAnyException(); From b5b832e8c3eaf8400395583dfb22a826671fb921 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 13:57:35 +0200 Subject: [PATCH 14/25] fix: offer trusted publishing providers in a stable order The provider map was built as a HashMap and returned through Map.copyOf, and the status endpoint collected the active ones into a HashMap again. Java's immutable maps salt their iteration order per JVM run, so the provider list the registration dialog offers was reshuffled on every restart. Both are insertion-ordered now: GitHub first, then the GitLab instances in configuration order. Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingService.java | 11 ++++++---- .../TrustedPublishingServiceTest.java | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index f2b7925a2..8e2c892e8 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -14,7 +14,8 @@ import java.text.ParseException; import java.util.ArrayList; -import java.util.HashMap; +import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -80,7 +81,9 @@ public TrustedPublishingService( * GitHub is a single, hard-wired provider; every configured GitLab instance becomes one of its own. */ private static Map createProviders(TrustedPublishingConfig config) { - var providers = new HashMap(); + // insertion-ordered, so the providers are always offered in the same order: GitHub first, then the + // GitLab instances as configured. An unordered map would reshuffle the list on every restart. + var providers = new LinkedHashMap(); providers.put(GitHubTrustedPublishingProvider.PROVIDER_ID, new GitHubTrustedPublishingProvider(config)); config.getGitLabInstances() .forEach( @@ -92,7 +95,7 @@ private static Map createProviders(Tru instance.getName(), instance.getUrl(), instance.getIssuer()))); - return Map.copyOf(providers); + return Collections.unmodifiableMap(providers); } /** @@ -242,7 +245,7 @@ public Map getTrustedPublisherProvider ensureEnabled(); return providers.entrySet().stream() .filter(e -> e.getValue().isActive()) - .collect(HashMap::new, (m, e) -> m.put(e.getKey(), e.getValue()), HashMap::putAll); + .collect(LinkedHashMap::new, (m, e) -> m.put(e.getKey(), e.getValue()), LinkedHashMap::putAll); } /** diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java index d39d749a9..d33721842 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java @@ -12,6 +12,7 @@ *****************************************************************************/ package org.eclipse.openvsx.trustedpublishing; +import java.util.LinkedHashMap; import java.util.List; import jakarta.persistence.EntityManager; @@ -28,6 +29,7 @@ import org.eclipse.openvsx.entities.TrustedPublisher; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.repositories.RepositoryService; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.when; @@ -91,6 +93,25 @@ void listsEveryRegistrationButOffersOnlyUntakenActiveExtensions() { assertThat(result.registrableExtensions()).containsExactly("free"); } + // The status endpoint hands this list straight to the client, so an unordered map would reshuffle + // the providers offered in the registration dialog on every restart. + @Test + void offersProvidersInAStableOrderGitHubFirst() { + when(config.isEnabled()).thenReturn(true); + when(config.getActiveProviders()).thenReturn(List.of("github", "gitlab", "eclipse-gitlab")); + var instances = new LinkedHashMap(); + instances.put("gitlab", new GitLabInstance("GitLab", "https://gitlab.com")); + instances.put("eclipse-gitlab", new GitLabInstance("Eclipse GitLab", "https://gitlab.eclipse.org")); + when(config.getGitLabInstances()).thenReturn(instances); + + var service = new TrustedPublishingService(config, repositories, tokens, entityManager); + + assertThat(service.getTrustedPublisherProviders().keySet()) + .containsExactly("github", "gitlab", "eclipse-gitlab"); + assertThat(service.getAllTrustedPublisherProviders().keySet()) + .containsExactly("github", "gitlab", "eclipse-gitlab"); + } + private Extension extension(long id, String name) { var extension = new Extension(); extension.setId(id); From 0c09a23fa48f727e24a9604f21af3a7f7e21a5af Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 14:26:51 +0200 Subject: [PATCH 15/25] fix: stop probing the database for a token value that cannot be there generateTokenValue regenerated until repositories.hasPersonalAccessToken came back false, but that looks the raw value up in personal_access_token. value - a column holding salted hashes since tokens started being stored hashed. For a current token the comparison can never match, so the loop never guarded anything and only cost a query per token created. Uniqueness belongs to the UNIQUE (value) constraint that has been on the table since the base migration. It is also the only check that can work: it applies to the hash that is actually stored, and it holds across every pod writing to the database, which a check-then-insert cannot. RepositoryService.hasPersonalAccessToken had no other caller and goes with it. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 11 +++++---- .../repositories/RepositoryService.java | 4 ---- .../accesstoken/AccessTokenServiceTest.java | 23 +++++++++++++++++-- .../RepositoryServiceSmokeTest.java | 1 - 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 64637cae8..f74ae98bb 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -155,13 +155,14 @@ private AccessTokenJson createAccessToken( return json; } + /** + * Uniqueness is the {@code UNIQUE (value)} constraint's to enforce, not this method's. It is the only + * check that can work: it applies to the hash that actually gets stored, and it holds across every pod + * writing to the database, which a check-then-insert here could not. + */ // public to be accessible from tests public String generateTokenValue() { - String value; - do { - value = config.getPrefix() + uuidService.generateRandom(); - } while (repositories.hasPersonalAccessToken(value)); - return value; + return config.getPrefix() + uuidService.generateRandom(); } @Transactional diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index 9b87062c1..baea431b8 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -598,10 +598,6 @@ public int updateExpiresTimeForLegacyPersonalAccessTokens(LocalDateTime timestam return personalAccessTokenRepo.updateExpiresTimeForLegacyAccessTokens(timestamp, type); } - public boolean hasPersonalAccessToken(String value) { - return personalAccessTokenRepo.findByValue(value) != null; - } - public PersonalAccessToken findPersonalAccessToken(UserData user, String description) { return personalAccessTokenRepo.findByUserAndDescriptionAndActiveTrue(user, description); } diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index d6dfda614..0efbdd4b0 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -39,7 +39,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -65,8 +67,9 @@ class AccessTokenServiceTest { @BeforeEach void setUp() { - when(config.getTokenHashAlgorithm()).thenReturn("SHA-256"); - when(config.getTokenHashSalt()).thenReturn("salt"); + // lenient: generateTokenValue does not hash anything, so it needs neither of these + lenient().when(config.getTokenHashAlgorithm()).thenReturn("SHA-256"); + lenient().when(config.getTokenHashSalt()).thenReturn("salt"); } private PersonalAccessToken activeUnrestrictedToken() { @@ -107,6 +110,22 @@ void rejectsATokenWithNoUser() { assertThat(tau).isNull(); } + // The old implementation regenerated until repositories.hasPersonalAccessToken(value) came back + // false, comparing a raw value against a column that stores salted hashes - it could never match for + // a current token, so it only cost a query per token created. Uniqueness is UNIQUE (value)'s job, and + // only it can work across pods anyway. + @Test + void generatesATokenValueWithoutAskingTheDatabase() { + var uuid = UUID.randomUUID(); + when(config.getPrefix()).thenReturn("ovsxat_"); + when(uuidService.generateRandom()).thenReturn(uuid); + + var value = accessTokenService.generateTokenValue(); + + assertThat(value).isEqualTo("ovsxat_" + uuid); + verifyNoInteractions(repositories); + } + // How long a publishing token lives is the trusted publishing configuration's to decide, so this // service applies whatever it is handed instead of reading a setting of its own. @Test diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index e99e03f42..6f81e5e6f 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -351,7 +351,6 @@ void testExecuteQueries() { () -> repositories .findFirstUnresolvedDependency(List.of(new ExtensionId("namespaceName", "extensionName"))), () -> repositories.findAllPersonalAccessTokens(), - () -> repositories.hasPersonalAccessToken("tokenValue"), () -> repositories .findSignatureKeyPairPublicId("namespaceName", "extensionName", "targetPlatform", "version"), () -> repositories.findFirstMembership("namespaceName"), From fea0cba196fb1401b11ab25bcbdfdabe50934031 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 14:44:09 +0200 Subject: [PATCH 16/25] fix: stop a token write from clobbering columns it never touched The paths that write a personal_access_token row each touch a different column: the upgrade job rewrites value and version, using a token writes accessed_timestamp, revoking one writes active. Hibernate's default full-row UPDATE carries all of them back from whatever the writing transaction happened to load, so an upgrade or a token use overlapping a revoke put active back to true and handed a revoked token to its holder. @DynamicUpdate, for the same reason and with the same effect as on Extension, which carries it already. Co-Authored-By: Claude Opus 5 (1M context) --- .../openvsx/entities/PersonalAccessToken.java | 7 + .../AccessTokenConcurrentWriteTest.java | 123 ++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java diff --git a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java index 0333c4f6e..3d0cd2749 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java @@ -15,13 +15,20 @@ import java.util.Objects; import jakarta.persistence.*; +import org.hibernate.annotations.DynamicUpdate; import org.eclipse.openvsx.json.AccessTokenJson; import org.eclipse.openvsx.util.TimeUtil; import static java.util.Objects.requireNonNull; +// Same reasoning as on Extension: the paths that write a token row each touch a different column - +// the upgrade job rewrites `value` and `version`, using a token writes `accessed_timestamp`, revoking +// one writes `active`. Without @DynamicUpdate, Hibernate's full-row UPDATE would rewrite all of them +// from whatever the writing transaction happened to load, so an upgrade or a token use running +// alongside a revoke could silently put `active` back to true. @Entity +@DynamicUpdate @Table(name = "personal_access_token") public class PersonalAccessToken implements Serializable { diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java new file mode 100644 index 000000000..334c1c673 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java @@ -0,0 +1,123 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.accesstoken; + +import jakarta.persistence.EntityManager; +import org.jobrunr.scheduling.JobRequestScheduler; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.support.TransactionTemplate; + +import org.eclipse.openvsx.AbstractPostgresContainerTest; +import org.eclipse.openvsx.entities.PersonalAccessToken; +import org.eclipse.openvsx.entities.PersonalAccessTokenType; +import org.eclipse.openvsx.entities.UserData; +import org.eclipse.openvsx.search.SearchUtilService; +import org.eclipse.openvsx.util.TimeUtil; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The paths that write a token row each touch a different column, so one must not carry the others back + * to whatever it happened to load. Only a real database shows this: it is Hibernate's generated UPDATE + * that decides, and with the default full-row update the loser's stale columns win. + */ +@SpringBootTest +class AccessTokenConcurrentWriteTest extends AbstractPostgresContainerTest { + + @Autowired + EntityManager em; + + @Autowired + PlatformTransactionManager txManager; + + @MockitoBean + SearchUtilService search; + + @MockitoBean + JobRequestScheduler scheduler; + + @Test + void upgradingATokenDoesNotResurrectOneRevokedMeanwhile() { + var tokenId = persistLegacyToken(); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + // what the upgrade job holds: a snapshot taken before the revoke below, so its copy still + // says active = true + var token = em.find(PersonalAccessToken.class, tokenId); + assertThat(token.isActive()).isTrue(); + + revokeInAnotherTransaction(tokenId); + + // and now it writes the two columns it actually cares about + token.setValue("hashed-" + token.getValue()); + token.setVersion(1); + }); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + var token = em.find(PersonalAccessToken.class, tokenId); + assertThat(token.getVersion()).isEqualTo(1); + assertThat(token.getValue()).startsWith("hashed-"); + // the revoke has to survive: without @DynamicUpdate the upgrade's full-row UPDATE writes + // active = true back over it, handing a revoked token back to its holder + assertThat(token.isActive()).isFalse(); + }); + + cleanUp(tokenId); + } + + private long persistLegacyToken() { + return new TransactionTemplate(txManager).execute(status -> { + var user = new UserData(); + user.setLoginName("concurrent-write-user"); + user.setProvider("github"); + em.persist(user); + + var token = new PersonalAccessToken(); + token.setUser(user); + token.setValue("concurrent-write-token-value"); + token.setActive(true); + token.setCreatedTimestamp(TimeUtil.getCurrentUTC()); + token.setVersion(0); + token.setType(PersonalAccessTokenType.LLT); + token.setDescription("legacy token"); + em.persist(token); + em.flush(); + return token.getId(); + }); + } + + private void revokeInAnotherTransaction(long tokenId) { + var requiresNew = new TransactionTemplate(txManager); + requiresNew.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + requiresNew.executeWithoutResult( + status -> em.createNativeQuery("UPDATE personal_access_token SET active = false WHERE id = :id") + .setParameter("id", tokenId) + .executeUpdate()); + } + + /** The container is shared with every other test, so leave nothing of this one behind. */ + private void cleanUp(long tokenId) { + new TransactionTemplate(txManager).executeWithoutResult(status -> { + var token = em.find(PersonalAccessToken.class, tokenId); + var userId = token.getUser().getId(); + em.remove(token); + em.flush(); + em.createQuery("delete from UserData u where u.id = :id").setParameter("id", userId).executeUpdate(); + }); + } +} From 49df3931410de4bab9b0629e72077804c6504639 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 14:51:00 +0200 Subject: [PATCH 17/25] fix: let one instance run the token upgrade, not every pod The upgrade job is enqueued from ApplicationStartedEvent, which fires in every instance's own JVM, and scheduler.enqueue mints a new job id per call - so a rolling update has each pod scan and rewrite the whole set of legacy tokens against the same rows. The work is idempotent, since each row is hashed from the raw value it still holds and stops matching once upgraded, so this is wasted write load rather than corruption. upgradeTokens now takes the same transaction-scoped Postgres advisory lock that guards startup scan recovery, under its own key, and returns early when another instance holds it. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 42 ++++++++++++++++++- .../org/eclipse/openvsx/RegistryAPITest.java | 7 +++- .../java/org/eclipse/openvsx/UserAPITest.java | 7 +++- .../accesstoken/AccessTokenServiceTest.java | 33 +++++++++++++++ .../eclipse/openvsx/admin/AdminAPITest.java | 7 +++- 5 files changed, 89 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index f74ae98bb..04165c053 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -22,7 +22,11 @@ import jakarta.transaction.Transactional; import org.apache.commons.codec.binary.Hex; import org.apache.commons.codec.digest.DigestUtils; +import org.jooq.DSLContext; +import org.jooq.impl.DSL; import org.jspecify.annotations.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; import org.eclipse.openvsx.entities.Extension; @@ -47,6 +51,15 @@ @Service public class AccessTokenService { + /** + * Arbitrary, fixed key for the Postgres advisory lock guarding the token upgrade below. Must stay + * distinct from every other advisory lock key this application uses; see + * {@code ExtensionScanJobRecoveryService.RECOVERY_LOCK_KEY} for the other one. + */ + private static final long UPGRADE_LOCK_KEY = 891_234_567_890_124L; + + private static final Logger logger = LoggerFactory.getLogger(AccessTokenService.class); + private static final int TOKEN_VERSION_0 = 0; private static final int TOKEN_VERSION_1 = 1; private static final int[] ALL_TOKEN_VERSIONS = { TOKEN_VERSION_0, TOKEN_VERSION_1 }; @@ -57,19 +70,22 @@ public class AccessTokenService { private final EntityManager entityManager; private final RepositoryService repositories; private final MailService mail; + private final DSLContext dsl; public AccessTokenService( AccessTokenConfig config, UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, - MailService mail + MailService mail, + DSLContext dsl ) { this.config = config; this.uuidService = uuidService; this.entityManager = entityManager; this.repositories = repositories; this.mail = mail; + this.dsl = dsl; } /** @@ -280,8 +296,24 @@ public int setExpirationTimeForLegacyAccessTokens(LocalDateTime expirationTime) return repositories.updateExpiresTimeForLegacyPersonalAccessTokens(expirationTime, PersonalAccessTokenType.LLT); } + /** + * Upgrades every token still stored in a legacy format. + *

+ * The job that calls this is enqueued from {@code ApplicationStartedEvent}, which fires in every + * instance's own JVM: during a rolling update several pods run this against the same rows. The work + * itself is idempotent - each row is hashed from the raw value it still holds, and the row stops + * matching once upgraded - but there is no point in every pod scanning and rewriting the whole set, + * so a transaction-scoped advisory lock lets one of them do it. {@code pg_try_advisory_xact_lock} + * returns immediately rather than blocking, and Postgres drops the lock when this method's + * transaction ends, so there is no unlock to forget and none can leak onto a pooled connection. + */ @Transactional public int upgradeTokens() { + if (!tryAcquireUpgradeLock()) { + logger.debug("Another instance already holds the token upgrade lock, skipping"); + return 0; + } + int upgradedCount = 0; for (int version : ALL_TOKEN_VERSIONS) { if (version == TOKEN_CURRENT_VERSION) { @@ -297,6 +329,14 @@ public int upgradeTokens() { return upgradedCount; } + // Package-private so a test can stub it via a spy without needing two real database connections. + boolean tryAcquireUpgradeLock() { + return Boolean.TRUE.equals( + dsl.fetchValue( + DSL.select( + DSL.function("pg_try_advisory_xact_lock", Boolean.class, DSL.val(UPGRADE_LOCK_KEY))))); + } + private boolean upgradeToken(PersonalAccessToken token) { int version = token.getVersion(); if (version == TOKEN_VERSION_0) { diff --git a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java index 345446d36..d0306b7b9 100644 --- a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java @@ -27,6 +27,7 @@ import jakarta.persistence.EntityManager; import org.apache.commons.lang3.ArrayUtils; import org.jobrunr.scheduling.JobRequestScheduler; +import org.jooq.DSLContext; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -110,6 +111,7 @@ @WebMvcTest(RegistryAPI.class) @MockitoBean( types = { + DSLContext.class, ClientRegistrationRepository.class, UpstreamRegistryService.class, GoogleCloudStorageService.class, @@ -3679,9 +3681,10 @@ AccessTokenService tokenService( UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, - MailService mailService + MailService mailService, + DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService); + return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index 5f01052a4..c0b7299db 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -19,6 +19,7 @@ import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; +import org.jooq.DSLContext; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -85,6 +86,7 @@ @WebMvcTest(UserAPI.class) @MockitoBean( types = { + DSLContext.class, EclipseService.class, ClientRegistrationRepository.class, StorageUtilService.class, @@ -1191,9 +1193,10 @@ AccessTokenService accessTokenService( UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, - MailService mailService + MailService mailService, + DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService); + return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index 0efbdd4b0..d26b36e48 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -19,13 +19,16 @@ import java.util.UUID; import jakarta.persistence.EntityManager; +import org.jooq.DSLContext; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.util.Streamable; import org.eclipse.openvsx.entities.Extension; import org.eclipse.openvsx.entities.Namespace; @@ -62,6 +65,9 @@ class AccessTokenServiceTest { @Mock MailService mail; + @Mock + DSLContext dsl; + @InjectMocks AccessTokenService accessTokenService; @@ -110,6 +116,33 @@ void rejectsATokenWithNoUser() { assertThat(tau).isNull(); } + // The upgrade job is enqueued from every pod's own ApplicationStartedEvent, so the advisory lock is + // what keeps one rolling update from having each of them scan and rewrite the same rows. + @Test + void skipsTheTokenUpgradeWhenAnotherInstanceHoldsTheLock() { + var service = Mockito.spy(accessTokenService); + Mockito.doReturn(false).when(service).tryAcquireUpgradeLock(); + + assertThat(service.upgradeTokens()).isZero(); + + verifyNoInteractions(repositories); + } + + @Test + void upgradesTokensWhenItWinsTheLock() { + var service = Mockito.spy(accessTokenService); + Mockito.doReturn(true).when(service).tryAcquireUpgradeLock(); + var legacy = new PersonalAccessToken(); + legacy.setVersion(0); + legacy.setValue("raw"); + when(repositories.findAllPersonalAccessTokensByVersion(0)).thenReturn(Streamable.of(legacy)); + + assertThat(service.upgradeTokens()).isEqualTo(1); + + assertThat(legacy.getVersion()).isEqualTo(1); + assertThat(legacy.getValue()).isNotEqualTo("raw"); + } + // The old implementation regenerated until repositories.hasPersonalAccessToken(value) came back // false, comparing a raw value against a column that stores salted hashes - it could never match for // a current token, so it only cost a query per token created. Uniqueness is UNIQUE (value)'s job, and diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index 6cd42588d..b87ba9a1c 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -22,6 +22,7 @@ import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; +import org.jooq.DSLContext; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -129,6 +130,7 @@ @WebMvcTest(AdminAPI.class) @MockitoBean( types = { + DSLContext.class, ClientRegistrationRepository.class, UpstreamRegistryService.class, GoogleCloudStorageService.class, @@ -2563,9 +2565,10 @@ AccessTokenService tokenService( UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, - MailService mailService + MailService mailService, + DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService); + return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); } @Bean From 4ff5393fa107bbe8fe16333b31daab76c574cf98 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 15:30:29 +0200 Subject: [PATCH 18/25] fix: keep one bad membership row from aborting a publisher revoke revokePublisherContributions walks a user's memberships and removed each through removeNamespaceMember, which looks the row up again and throws when it finds nothing. The whole method is @Transactional(rollbackOn = ErrorResultException.class), so one stale or duplicated row - and namespace_membership has no unique constraint on (user, namespace) - rolled back the entire revoke, leaving the tokens and versions it had just deactivated active again. UserService now separates the lookup from the work: removeNamespaceMembership takes the row the caller already holds, so the callers that walk a user's memberships have nothing left to fail on. forgetUser uses it too. RepositoryService.deleteMemberships goes with this: it has no callers left, and a bulk delete bypassing the trusted publisher revocation is a trap for the next one. Co-Authored-By: Claude Opus 5 (1M context) --- .../main/java/org/eclipse/openvsx/UserService.java | 14 ++++++++++++++ .../org/eclipse/openvsx/admin/AdminService.java | 8 +++++--- .../openvsx/repositories/RepositoryService.java | 4 ---- .../eclipse/openvsx/admin/AdminServiceTest.java | 7 +++---- .../repositories/RepositoryServiceSmokeTest.java | 1 - ...sTest.java => TrustedPublishingConfigTest.java} | 0 6 files changed, 22 insertions(+), 12 deletions(-) rename server/src/test/java/org/eclipse/openvsx/trustedpublishing/{TrustedPublishingPropertiesTest.java => TrustedPublishingConfigTest.java} (100%) diff --git a/server/src/main/java/org/eclipse/openvsx/UserService.java b/server/src/main/java/org/eclipse/openvsx/UserService.java index f0009b3fe..c2b526b42 100644 --- a/server/src/main/java/org/eclipse/openvsx/UserService.java +++ b/server/src/main/java/org/eclipse/openvsx/UserService.java @@ -161,6 +161,20 @@ public ResultJson removeNamespaceMember(Namespace namespace, UserData user) thro throw new ErrorResultException( "User " + user.getLoginName() + " is not a member of " + namespace.getName() + "."); } + return removeNamespaceMembership(membership); + } + + /** + * Removes a membership that the caller already holds, for callers walking a user's memberships rather + * than naming one. Nothing here can fail on a row that has since gone or been duplicated, which matters + * for {@code AdminService#revokePublisherContributions}: it revokes a whole publisher in one + * transaction, and one unremovable membership must not take the token and version deactivations with it. + */ + @Transactional(rollbackOn = ErrorResultException.class) + @CacheEvict(value = { CACHE_NAMESPACE_DETAILS_JSON }, key = "#membership.namespace.name") + public ResultJson removeNamespaceMembership(NamespaceMembership membership) { + var namespace = membership.getNamespace(); + var user = membership.getUser(); entityManager.remove(membership); revokeTrustedPublishers(namespace, user); return ResultJson.success("Removed " + user.getLoginName() + " from namespace " + namespace.getName() + "."); diff --git a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java index a2b3c50bb..8e1b95bd3 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java @@ -538,13 +538,15 @@ public ResultJson revokePublisherContributions(String provider, String loginName // Revoke namespace memberships one by one through UserService rather than deleting them in bulk: // that is what deletes the trusted publishers registered under an ownership being revoked here, and - // what evicts the namespace details cache - same as when an owner removes a member themselves. + // what evicts the namespace details cache - same as when an owner removes a member themselves. Each + // membership is handed over as the row we already hold, so no second lookup can fail and abort the + // whole revoke. var namespaceMemberships = repositories.findMemberships(user); var numberOfNamespaceMemberships = 0L; // add a null check due to tests using mocks which return null if (namespaceMemberships != null) { for (var membership : namespaceMemberships.toList()) { - users.removeNamespaceMember(membership.getNamespace(), user); + users.removeNamespaceMembership(membership); numberOfNamespaceMemberships++; } } @@ -610,7 +612,7 @@ public ResultJson forgetUser(String provider, String username, UserData admin) { var removedMembershipCount = 0; for (var membership : repositories.findMemberships(user)) { var namespace = membership.getNamespace(); - users.removeNamespaceMember(namespace, user); + users.removeNamespaceMembership(membership); removedMembershipCount++; search.updateSearchEntries(repositories.findActiveExtensions(namespace).toList()); } diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index baea431b8..fba943fbc 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -486,10 +486,6 @@ public NamespaceMembership findMembership(UserData user, Namespace namespace) { return membershipRepo.findByUserAndNamespace(user, namespace); } - public void deleteMemberships(UserData user) { - membershipRepo.deleteByUser(user); - } - public boolean hasMembership(UserData user, Namespace namespace) { return membershipJooqRepo.hasMembership(user, namespace); } diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java index 61d07093e..e94cf9e12 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminServiceTest.java @@ -120,10 +120,9 @@ void revokingPublisherContributionsRemovesEachMembershipThroughUserService() { var result = adminService.revokePublisherContributions("github", "amy", admin); - // going through UserService is what deletes the trusted publishers the revoked owner registered; - // deleting the membership rows in bulk would leave those registrations behind - verify(users).removeNamespaceMember(namespace, user); - verify(repositories, never()).deleteMemberships(user); + // handing over the row we already hold is what keeps one unremovable membership from aborting the + // whole revoke, and going through UserService is what deletes the trusted publishers with it + verify(users).removeNamespaceMembership(membership); assertThat(result.getSuccess()).contains("removed 1 namespace memberships"); } diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index 6f81e5e6f..3376845ac 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -240,7 +240,6 @@ void testExecuteQueries() { () -> repositories.findMembership(userData, namespace), () -> repositories.findMemberships(namespace), () -> repositories.findMemberships(namespace, "role"), - () -> repositories.deleteMemberships(userData), () -> repositories.findNamespace("name"), () -> repositories.lockNamespace(namespace), () -> repositories.findConflictingNamespaces("displayName", namespace), diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java similarity index 100% rename from server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingPropertiesTest.java rename to server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java From cbdee5b498440183d95a54cdb3bac1f24b17f0eb Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 15:30:44 +0200 Subject: [PATCH 19/25] fix: delete a trusted publishing token that lost its registration It was only deactivated. The row can never become valid again - the token may publish exactly one extension, and the registration it would be checked against is gone - so it is removed outright, the same as a one-time token once used. Co-Authored-By: Claude Opus 5 (1M context) --- .../org/eclipse/openvsx/accesstoken/AccessTokenService.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 04165c053..06cc3389a 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -235,9 +235,10 @@ public AccessTokenAuthentication useAccessToken(String tokenValue, AccessTokenAc } // Deleting a registration takes its tokens with it, so this should not be reachable; kept as a // guard, because a TPT that lost its registration may only ever publish an extension it can no - // longer be checked against. + // longer be checked against. Removed rather than deactivated: it can never become valid again, + // and nothing reads the row afterwards - the same reasoning as for a one-time token below. if (token.getType() == PersonalAccessTokenType.TPT && token.getTrustedPublisher() == null) { - token.setActive(false); + entityManager.remove(token); return null; } // scope From 1d36a0f3edb4833ced43b3211dd59590ec399379 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 15:30:54 +0200 Subject: [PATCH 20/25] refactor: one trusted publishing configuration class, not two TrustedPublishingProperties existed only because @Value cannot convert a Duration in the bare SpringExtension contexts the provider tests use, which did not justify a second class reached through pass-through getters. Both now live on TrustedPublishingConfig: the scalars stay on @Value, and only the two typed settings have setters, so the configuration properties binder touches exactly those. That keeps the audience fallback to ovsx.webui.url expressible as a placeholder default, which the binder cannot do. Also removes getAllTrustedPublisherProviders, which had no production caller and was kept alive by an assertion in its own test, replaces a verbose three-arg collect with Collectors.toMap, and states the created_by foreign key the way its siblings in the migration now read: a registration cannot outlive its author any more than it outlives their ownership. Co-Authored-By: Claude Opus 5 (1M context) --- .../TrustedPublishingConfig.java | 142 ++++++++++++++-- .../TrustedPublishingProperties.java | 153 ------------------ .../TrustedPublishingService.java | 14 +- .../GitLabTrustedPublishingProvider.java | 2 +- .../db/migration/V1_72__Trusted_Publisher.sql | 6 +- .../openvsx/LocalRegistryServiceTest.java | 3 +- .../org/eclipse/openvsx/RegistryAPITest.java | 3 +- .../java/org/eclipse/openvsx/UserAPITest.java | 3 +- .../eclipse/openvsx/admin/AdminAPITest.java | 3 +- .../TrustedPublishingConfigTest.java | 45 +++--- .../TrustedPublishingServiceTest.java | 6 +- .../GitHubTrustedPublishingProviderTest.java | 3 +- .../GitLabTrustedPublishingProviderTest.java | 3 +- 13 files changed, 167 insertions(+), 219 deletions(-) delete mode 100644 server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java index 8de6df49d..f121c3a35 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfig.java @@ -14,29 +14,55 @@ import java.net.URI; import java.time.Duration; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import jakarta.annotation.PostConstruct; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotBlank; import org.jspecify.annotations.NonNull; import org.jspecify.annotations.Nullable; import org.springframework.beans.factory.annotation.Value; -import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.context.annotation.Configuration; +import org.springframework.validation.annotation.Validated; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance; import org.eclipse.openvsx.trustedpublishing.github.GitHubTrustedPublishingProvider; +import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProvider; +/** + * The trusted publishing configuration. + *

+ * The scalar settings are read with {@code @Value}; the two that need a typed value - the token lifetime + * and the GitLab instance map - are bound by {@code @ConfigurationProperties}, whose binder converts them + * natively. Only fields with a setter are bound, so the two sets do not overlap. + *

+ * Every GitLab instance behaves the same way, differing only in id, name, URL and OIDC issuer, so instances + * are configured rather than coded. Only the public instance is configured out of the box; any other one - + * the Eclipse Foundation instance included - is added by configuration, and becomes usable once its id is + * listed in {@code ovsx.trusted-publishing.active-providers}: + * + *

+ * ovsx:
+ *   trusted-publishing:
+ *     active-providers: github,eclipse-gitlab
+ *     gitlab:
+ *       eclipse-gitlab:
+ *         name: Eclipse GitLab
+ *         url: https://gitlab.eclipse.org
+ *         issuer: https://gitlab.eclipse.org   # optional, defaults to the URL
+ * 
+ * + * The id is persisted with every registration, so renaming it hides the registrations made for it. + * Configuring the id of the default instance replaces it as a whole rather than patching single fields, + * so such an entry has to carry the name and the URL itself. + */ @Configuration -@EnableConfigurationProperties(TrustedPublishingProperties.class) +@ConfigurationProperties(prefix = "ovsx.trusted-publishing") +@Validated public class TrustedPublishingConfig { - private final TrustedPublishingProperties properties; - - public TrustedPublishingConfig(TrustedPublishingProperties properties) { - this.properties = properties; - } - /** * Whether trusted publishing is enabled at all. */ @@ -58,12 +84,26 @@ public TrustedPublishingConfig(TrustedPublishingProperties properties) { /** * The comma separated list of active trusted publishing providers. An id listed here must be - * {@code github} or one of the configured GitLab instances, see {@link TrustedPublishingProperties}. + * {@code github} or one of the configured GitLab instances. * Default: {@code github}. */ @Value("${ovsx.trusted-publishing.active-providers:github}") private List activeProviders; + /** + * How long an issued publishing token is valid. Must be positive: a token that does not expire is a + * long-lived credential, which is the very thing trusted publishing exists to avoid. The lifetime of + * ordinary personal access tokens is {@code ovsx.access-token.expiration} instead. + */ + private Duration tokenExpiration = Duration.ofMinutes(5); + + /** + * The known GitLab instances, keyed by provider id. Configured instances are added to the public + * instance, which stays available unless its id is redefined. + */ + @Valid + private Map gitlab = defaultGitLabInstances(); + public boolean isEnabled() { return enabled; } @@ -88,8 +128,12 @@ public List getActiveProviders() { * is decided by {@link #getActiveProviders()}. */ @NonNull - public Map getGitLabInstances() { - return properties.getGitlab(); + public Map getGitlab() { + return gitlab; + } + + public void setGitlab(Map gitlab) { + this.gitlab = gitlab; } /** @@ -97,7 +141,19 @@ public Map getGitLabInstances() { */ @NonNull public Duration getTokenExpiration() { - return properties.getTokenExpiration(); + return tokenExpiration; + } + + public void setTokenExpiration(Duration tokenExpiration) { + this.tokenExpiration = tokenExpiration; + } + + private static Map defaultGitLabInstances() { + var instances = new LinkedHashMap(); + instances.put( + GitLabTrustedPublishingProvider.PROVIDER_ID, + new GitLabInstance("GitLab", GitLabTrustedPublishingProvider.PROVIDER_URL)); + return instances; } @PostConstruct @@ -125,7 +181,7 @@ public void validate() { } private void validateGitLabInstances() { - for (var entry : getGitLabInstances().entrySet()) { + for (var entry : getGitlab().entrySet()) { var id = entry.getKey(); var instance = entry.getValue(); if (GitHubTrustedPublishingProvider.PROVIDER_ID.equals(id)) { @@ -153,4 +209,62 @@ private static String hostOf(String url) { return null; } } + + /** + * A single GitLab instance. + */ + public static class GitLabInstance { + + @NotBlank + private String name; + + @NotBlank + private String url; + + @Nullable + private String issuer; + + public GitLabInstance() { + // for configuration property binding + } + + public GitLabInstance(String name, String url) { + this.name = name; + this.url = url; + } + + /** + * The instance name, for human consumption. + */ + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + /** + * The base URL of the instance; the API and the {@code ci_config_ref_uri} claim are derived from it. + */ + public String getUrl() { + return url; + } + + public void setUrl(String url) { + this.url = url; + } + + /** + * The issuer to expect in the {@code iss} claim of issued OIDC ID tokens. GitLab issues tokens + * under its own base URL, so this defaults to {@link #getUrl()}. + */ + public String getIssuer() { + return issuer == null || issuer.isBlank() ? url : issuer; + } + + public void setIssuer(@Nullable String issuer) { + this.issuer = issuer; + } + } } diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java deleted file mode 100644 index 871545244..000000000 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingProperties.java +++ /dev/null @@ -1,153 +0,0 @@ -/****************************************************************************** - * Copyright (c) 2026 Contributors to the Eclipse Foundation. - * - * See the NOTICE file(s) distributed with this work for additional - * information regarding copyright ownership. - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License 2.0 which is available at - * https://www.eclipse.org/legal/epl-2.0. - * - * SPDX-License-Identifier: EPL-2.0 - *****************************************************************************/ -package org.eclipse.openvsx.trustedpublishing; - -import java.time.Duration; -import java.util.LinkedHashMap; -import java.util.Map; - -import jakarta.validation.Valid; -import jakarta.validation.constraints.NotBlank; -import org.jspecify.annotations.NonNull; -import org.jspecify.annotations.Nullable; -import org.springframework.boot.context.properties.ConfigurationProperties; -import org.springframework.validation.annotation.Validated; - -import org.eclipse.openvsx.trustedpublishing.gitlab.GitLabTrustedPublishingProvider; - -/** - * The trusted publishing provider instances that are known to this registry. - *

- * Every GitLab instance behaves the same way, it only differs in its id, name, URL and OIDC issuer, - * so instances are configured rather than coded. Only the public instance is configured out of the box; - * any other one - the Eclipse Foundation instance included - is added by configuration, and becomes - * usable once its id is listed in {@code ovsx.trusted-publishing.active-providers}: - * - *

- * ovsx:
- *   trusted-publishing:
- *     active-providers: github,eclipse-gitlab
- *     gitlab:
- *       eclipse-gitlab:
- *         name: Eclipse GitLab
- *         url: https://gitlab.eclipse.org
- *         issuer: https://gitlab.eclipse.org   # optional, defaults to the URL
- * 
- * - * The id is persisted with every registration, so renaming it hides the registrations made for it. - *

- * Configuring the id of the default instance replaces it as a whole rather than patching single fields, - * so such an entry has to carry the name and the URL itself. - */ -@ConfigurationProperties(prefix = "ovsx.trusted-publishing") -@Validated -public class TrustedPublishingProperties { - - /** - * How long an issued publishing token is valid. Must be positive: a token that does not expire is a - * long-lived credential, which is the very thing trusted publishing exists to avoid. The lifetime of - * ordinary personal access tokens is {@code ovsx.access-token.expiration} instead. - */ - private Duration tokenExpiration = Duration.ofMinutes(5); - - @Valid - private Map gitlab = defaultGitLabInstances(); - - @NonNull - public Duration getTokenExpiration() { - return tokenExpiration; - } - - public void setTokenExpiration(Duration tokenExpiration) { - this.tokenExpiration = tokenExpiration; - } - - /** - * The known GitLab instances, keyed by provider id. Configured instances are added to the public - * instance, which stays available unless its id is redefined. - */ - @NonNull - public Map getGitlab() { - return gitlab; - } - - public void setGitlab(Map gitlab) { - this.gitlab = gitlab; - } - - private static Map defaultGitLabInstances() { - var instances = new LinkedHashMap(); - instances.put( - GitLabTrustedPublishingProvider.PROVIDER_ID, - new GitLabInstance("GitLab", GitLabTrustedPublishingProvider.PROVIDER_URL)); - return instances; - } - - /** - * A single GitLab instance. - */ - public static class GitLabInstance { - - @NotBlank - private String name; - - @NotBlank - private String url; - - @Nullable - private String issuer; - - public GitLabInstance() { - // for configuration property binding - } - - public GitLabInstance(String name, String url) { - this.name = name; - this.url = url; - } - - /** - * The instance name, for human consumption. - */ - public String getName() { - return name; - } - - public void setName(String name) { - this.name = name; - } - - /** - * The base URL of the instance; the API and the {@code ci_config_ref_uri} claim are derived from it. - */ - public String getUrl() { - return url; - } - - public void setUrl(String url) { - this.url = url; - } - - /** - * The issuer to expect in the {@code iss} claim of issued OIDC ID tokens. GitLab issues tokens - * under its own base URL, so this defaults to {@link #getUrl()}. - */ - public String getIssuer() { - return issuer == null || issuer.isBlank() ? url : issuer; - } - - public void setIssuer(@Nullable String issuer) { - this.issuer = issuer; - } - } -} diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index 8e2c892e8..2fbfae8de 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; import com.nimbusds.jwt.JWTParser; import jakarta.persistence.EntityManager; @@ -85,7 +86,7 @@ private static Map createProviders(Tru // GitLab instances as configured. An unordered map would reshuffle the list on every restart. var providers = new LinkedHashMap(); providers.put(GitHubTrustedPublishingProvider.PROVIDER_ID, new GitHubTrustedPublishingProvider(config)); - config.getGitLabInstances() + config.getGitlab() .forEach( (providerId, instance) -> providers.put( providerId, @@ -245,15 +246,8 @@ public Map getTrustedPublisherProvider ensureEnabled(); return providers.entrySet().stream() .filter(e -> e.getValue().isActive()) - .collect(LinkedHashMap::new, (m, e) -> m.put(e.getKey(), e.getValue()), LinkedHashMap::putAll); - } - - /** - * Lists all trusted publisher providers. - */ - public Map getAllTrustedPublisherProviders() { - ensureEnabled(); - return providers; + .collect( + Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (a, b) -> a, LinkedHashMap::new)); } /** diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java index 2658df8f3..8092420e2 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProvider.java @@ -36,7 +36,7 @@ *

* Every instance - the public one, the Eclipse Foundation one, any self-hosted one - is served by this * class: they only differ in id, name, URL and OIDC issuer, which are configured through - * {@link org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance}. + * {@link org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig.GitLabInstance}. * * @see GitLab OpenID Connect */ diff --git a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql index e3dd5eadf..cf3e5bab3 100644 --- a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql +++ b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql @@ -12,7 +12,11 @@ CREATE TABLE IF NOT EXISTS public.trusted_publisher provider CHARACTER VARYING(32) NOT NULL, registration JSONB NOT NULL, claims JSONB NOT NULL, - created_by BIGINT NOT NULL REFERENCES public.user_data(id), + -- Only a namespace owner may register a trusted publisher, so a registration cannot outlive its + -- author any more than it outlives their ownership. Deleting a user already revokes their + -- registrations by way of their memberships; this is the constraint saying the same thing, and keeps + -- this reference consistent with the others in this migration. + created_by BIGINT NOT NULL REFERENCES public.user_data(id) ON DELETE CASCADE, created_timestamp TIMESTAMP without time zone NOT NULL ); diff --git a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java index 10792e4d9..995edafdb 100644 --- a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java @@ -51,7 +51,6 @@ import org.eclipse.openvsx.search.SimilarityCheckService; import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.TempFile; import org.eclipse.openvsx.util.VersionService; @@ -129,7 +128,7 @@ void setUp() { integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(new TrustedPublishingProperties()), + new TrustedPublishingConfig(), Duration.ofSeconds(30)); // A permissive default for a void method rather than a per-test expectation: the tests of diff --git a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java index d0306b7b9..93298c972 100644 --- a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java @@ -81,7 +81,6 @@ import org.eclipse.openvsx.storage.*; import org.eclipse.openvsx.storage.log.DownloadCountService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.ChangesCursor; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.NamingUtil; @@ -3751,7 +3750,7 @@ PublishingConfig publishingConfig() { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(new TrustedPublishingProperties()); + return new TrustedPublishingConfig(); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index c0b7299db..1b6deee63 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -65,7 +65,6 @@ import org.eclipse.openvsx.security.SecurityConfig; import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.TargetPlatform; import org.eclipse.openvsx.util.TargetPlatformVersion; @@ -1255,7 +1254,7 @@ LocalRegistryService localRegistryService( integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(new TrustedPublishingProperties()), + new TrustedPublishingConfig(), Duration.ofSeconds(30)); } diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index b87ba9a1c..3a2a96ae1 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -104,7 +104,6 @@ import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.storage.log.DownloadCountService; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import org.eclipse.openvsx.util.LogService; import org.eclipse.openvsx.util.TargetPlatform; import org.eclipse.openvsx.util.TargetPlatformVersion; @@ -2654,7 +2653,7 @@ LocalRegistryService localRegistryService( integrityService, similarityCheckService, new PublishingConfig(), - new TrustedPublishingConfig(new TrustedPublishingProperties()), + new TrustedPublishingConfig(), Duration.ofSeconds(30)); } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java index 3c4cf84b2..e7ede1a55 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingConfigTest.java @@ -32,11 +32,11 @@ * The GitLab instances are configuration rather than code, so what matters is that the public instance * is there, that configured instances are added to it, and that a broken instance is caught at startup. */ -class TrustedPublishingPropertiesTest { +class TrustedPublishingConfigTest { @Test void onlyThePublicInstanceIsConfiguredByDefault() { - var instances = new TrustedPublishingProperties().getGitlab(); + var instances = new TrustedPublishingConfig().getGitlab(); assertThat(instances).containsOnlyKeys(GitLabTrustedPublishingProvider.PROVIDER_ID); var gitlab = instances.get(GitLabTrustedPublishingProvider.PROVIDER_ID); @@ -83,24 +83,24 @@ void configuredInstanceCanRedefineThePublicOne() { @Test void redefiningTheDefaultInstanceReplacesItAsAWhole() { // only the URL is given, so the default name is gone rather than kept - and startup says so - var properties = bind( + var config = bind( Map.of("ovsx.trusted-publishing.gitlab.gitlab.url", "https://gitlab.staging.example")); - assertThat(properties.getGitlab().get(GitLabTrustedPublishingProvider.PROVIDER_ID).getName()).isNull(); - assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + assertThat(config.getGitlab().get(GitLabTrustedPublishingProvider.PROVIDER_ID).getName()).isNull(); + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(config).validate()) .withMessageContaining("no name or no URL"); } @Test void issuedTokensExpireAfterFiveMinutesByDefault() { - assertThat(new TrustedPublishingProperties().getTokenExpiration()).isEqualTo(Duration.ofMinutes(5)); + assertThat(new TrustedPublishingConfig().getTokenExpiration()).isEqualTo(Duration.ofMinutes(5)); } @Test void tokenExpirationIsConfigurable() { - var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT30S")); + var config = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT30S")); - assertThat(properties.getTokenExpiration()).isEqualTo(Duration.ofSeconds(30)); + assertThat(config.getTokenExpiration()).isEqualTo(Duration.ofSeconds(30)); } // A token that never expires is a long-lived credential, which is what trusted publishing exists to @@ -108,9 +108,9 @@ void tokenExpirationIsConfigurable() { @Test void configRejectsANonPositiveTokenExpiration() { for (var value : List.of("PT0S", "PT-5M")) { - var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", value)); + var config = bind(Map.of("ovsx.trusted-publishing.token-expiration", value)); - assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(config).validate()) .withMessageContaining("token-expiration must be a positive duration"); } } @@ -118,8 +118,7 @@ void configRejectsANonPositiveTokenExpiration() { // ... and it is checked whether or not the feature is switched on, so a typo cannot lie in wait @Test void aBrokenTokenExpirationIsRejectedEvenWhileDisabled() { - var properties = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT0S")); - var config = new TrustedPublishingConfig(properties); + var config = bind(Map.of("ovsx.trusted-publishing.token-expiration", "PT0S")); assertThatIllegalStateException().isThrownBy(config::validate) .withMessageContaining("token-expiration must be a positive duration"); @@ -127,45 +126,43 @@ void aBrokenTokenExpirationIsRejectedEvenWhileDisabled() { @Test void enabledConfigAcceptsTheDefaultInstance() { - assertThatCode(() -> enabledConfig(new TrustedPublishingProperties()).validate()).doesNotThrowAnyException(); + assertThatCode(() -> enabledConfig(new TrustedPublishingConfig()).validate()).doesNotThrowAnyException(); } @Test void enabledConfigRejectsAnInstanceTakingTheGitHubProviderId() { - var properties = bind( + var config = bind( Map.of( "ovsx.trusted-publishing.gitlab.github.name", "Not GitHub", "ovsx.trusted-publishing.gitlab.github.url", "https://gitlab.acme.example")); - assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(config).validate()) .withMessageContaining("provider id of the GitHub provider"); } @Test void enabledConfigRejectsAMalformedInstanceUrl() { - var properties = bind( + var config = bind( Map.of( "ovsx.trusted-publishing.gitlab.acme-gitlab.name", "ACME GitLab", "ovsx.trusted-publishing.gitlab.acme-gitlab.url", "gitlab.acme.example")); - assertThatIllegalStateException().isThrownBy(() -> enabledConfig(properties).validate()) + assertThatIllegalStateException().isThrownBy(() -> enabledConfig(config).validate()) .withMessageContaining("malformed URL"); } - private static TrustedPublishingProperties bind(Map properties) { + private static TrustedPublishingConfig bind(Map properties) { return new Binder(new MapConfigurationPropertySource(properties)) - .bind( - "ovsx.trusted-publishing", - Bindable.ofInstance(new TrustedPublishingProperties())) - .orElseGet(TrustedPublishingProperties::new); + .bind("ovsx.trusted-publishing", Bindable.ofInstance(new TrustedPublishingConfig())) + .orElseGet(TrustedPublishingConfig::new); } - private static TrustedPublishingConfig enabledConfig(TrustedPublishingProperties properties) { - var config = new TrustedPublishingConfig(properties); + // @Value is not resolved for a hand-built instance, so the scalar settings are set directly + private static TrustedPublishingConfig enabledConfig(TrustedPublishingConfig config) { ReflectionTestUtils.setField(config, "enabled", true); ReflectionTestUtils.setField(config, "audience", "https://open-vsx.org"); ReflectionTestUtils.setField(config, "forbiddenJwtHeaders", List.of("x5u")); diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java index d33721842..2a5f82f11 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingServiceTest.java @@ -29,7 +29,7 @@ import org.eclipse.openvsx.entities.TrustedPublisher; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.repositories.RepositoryService; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties.GitLabInstance; +import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig.GitLabInstance; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.when; @@ -102,14 +102,12 @@ void offersProvidersInAStableOrderGitHubFirst() { var instances = new LinkedHashMap(); instances.put("gitlab", new GitLabInstance("GitLab", "https://gitlab.com")); instances.put("eclipse-gitlab", new GitLabInstance("Eclipse GitLab", "https://gitlab.eclipse.org")); - when(config.getGitLabInstances()).thenReturn(instances); + when(config.getGitlab()).thenReturn(instances); var service = new TrustedPublishingService(config, repositories, tokens, entityManager); assertThat(service.getTrustedPublisherProviders().keySet()) .containsExactly("github", "gitlab", "eclipse-gitlab"); - assertThat(service.getAllTrustedPublisherProviders().keySet()) - .containsExactly("github", "gitlab", "eclipse-gitlab"); } private Extension extension(long id, String name) { diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java index 5c0a3af50..bebb7a85f 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/github/GitHubTrustedPublishingProviderTest.java @@ -23,7 +23,6 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -156,7 +155,7 @@ void pinnedEnvironment() { static class TestConfig { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(new TrustedPublishingProperties()); + return new TrustedPublishingConfig(); } } } diff --git a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java index a837ff4f9..bb848ec23 100644 --- a/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java +++ b/server/src/test/java/org/eclipse/openvsx/trustedpublishing/gitlab/GitLabTrustedPublishingProviderTest.java @@ -23,7 +23,6 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.eclipse.openvsx.trustedpublishing.TrustedPublishingConfig; -import org.eclipse.openvsx.trustedpublishing.TrustedPublishingProperties; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -204,7 +203,7 @@ void malformedInstanceUrlIsRejected() { static class TestConfig { @Bean TrustedPublishingConfig trustedPublishingConfig() { - return new TrustedPublishingConfig(new TrustedPublishingProperties()); + return new TrustedPublishingConfig(); } } } From 1ed2f726241f44a4270cb3bb7fb3904968806c4c Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 16:46:43 +0200 Subject: [PATCH 21/25] feat: generate token values from random bytes, marked by kind A token value was the deployment prefix plus a UUIDv7. A UUID is the wrong shape for a secret: it is an identifier type, and the time-ordered v7 this application generates elsewhere spends 48 of its bits on a readable timestamp, leaving 74 random ones. A value is now the prefix, a marker saying which kind of token it is, and 32 bytes from a CSPRNG - 256 bits in 43 characters, fewer than a UUID takes. The marker (at_, ot_, tp_) lives on PersonalAccessTokenType, so a leaked token says what it can do at a glance, secret scanning can tell the kinds apart, and logs can be redacted per kind. The registry names itself in the deployment prefix, which is why the marker does not repeat it - the dev configuration's prefix becomes dev_ovsx_ accordingly. Nothing migrates: only the hash is stored and lookup is by hash, so the value format is internal to generation and existing tokens keep working until they expire or are revoked. UUIDService stays as it is for the ids that genuinely want a UUID, and drops out of AccessTokenService. Co-Authored-By: Claude Opus 5 (1M context) --- server/src/dev/resources/application.yml | 3 +- .../accesstoken/AccessTokenService.java | 25 +++++++++---- .../entities/PersonalAccessTokenType.java | 19 +++++++--- .../org/eclipse/openvsx/RegistryAPITest.java | 3 +- .../java/org/eclipse/openvsx/UserAPITest.java | 5 ++- .../accesstoken/AccessTokenServiceTest.java | 36 ++++++++++++------- .../eclipse/openvsx/admin/AdminAPITest.java | 3 +- 7 files changed, 63 insertions(+), 31 deletions(-) diff --git a/server/src/dev/resources/application.yml b/server/src/dev/resources/application.yml index 7993ed138..64b92915d 100644 --- a/server/src/dev/resources/application.yml +++ b/server/src/dev/resources/application.yml @@ -150,7 +150,8 @@ ovsx: local: directory: /tmp/ovsx access-token: - prefix: dev_ovsxat_ # use a token prefix that clearly indicates that it's for development + prefix: dev_ovsx_ # use a token prefix that clearly indicates that it's for development; the kind + # of token (at_, tp_) is appended by the code expiration: 0 # do not expire tokens in a dev environment notification: 0 mail: diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 06cc3389a..90f04991b 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -15,8 +15,10 @@ import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; import java.time.Duration; import java.time.LocalDateTime; +import java.util.Base64; import jakarta.persistence.EntityManager; import jakarta.transaction.Transactional; @@ -41,7 +43,6 @@ import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.util.NotFoundException; import org.eclipse.openvsx.util.TimeUtil; -import org.eclipse.openvsx.util.UUIDService; import org.eclipse.openvsx.util.UrlUtil; import org.eclipse.openvsx.util.auth.AccessTokenAuthentication; @@ -60,28 +61,31 @@ public class AccessTokenService { private static final Logger logger = LoggerFactory.getLogger(AccessTokenService.class); + /** 256 bits; far beyond guessing, and the encoded form is still shorter than a UUID. */ + private static final int TOKEN_BYTES = 32; + + private static final Base64.Encoder TOKEN_ENCODER = Base64.getUrlEncoder().withoutPadding(); + private static final int TOKEN_VERSION_0 = 0; private static final int TOKEN_VERSION_1 = 1; private static final int[] ALL_TOKEN_VERSIONS = { TOKEN_VERSION_0, TOKEN_VERSION_1 }; private static final int TOKEN_CURRENT_VERSION = TOKEN_VERSION_1; private final AccessTokenConfig config; - private final UUIDService uuidService; private final EntityManager entityManager; private final RepositoryService repositories; private final MailService mail; private final DSLContext dsl; + private final SecureRandom random = new SecureRandom(); public AccessTokenService( AccessTokenConfig config, - UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, MailService mail, DSLContext dsl ) { this.config = config; - this.uuidService = uuidService; this.entityManager = entityManager; this.repositories = repositories; this.mail = mail; @@ -133,7 +137,7 @@ private AccessTokenJson createAccessToken( @Nullable Namespace scopeNamespace, PersonalAccessTokenType type ) { - var rawValue = generateTokenValue(); + var rawValue = generateTokenValue(type); var token = new PersonalAccessToken(); token.setUser(user); token.setValue(hashTokenValue(rawValue)); @@ -172,13 +176,20 @@ private AccessTokenJson createAccessToken( } /** + * Generates a token value: the deployment's prefix, the marker saying which kind of token this is, and + * 32 bytes from a CSPRNG. A UUID would be the wrong shape here - it is an identifier type, and the + * time-ordered v7 this application generates elsewhere spends 48 of its bits on a readable timestamp, + * leaving 74 random ones where raw bytes give 256. + *

* Uniqueness is the {@code UNIQUE (value)} constraint's to enforce, not this method's. It is the only * check that can work: it applies to the hash that actually gets stored, and it holds across every pod * writing to the database, which a check-then-insert here could not. */ // public to be accessible from tests - public String generateTokenValue() { - return config.getPrefix() + uuidService.generateRandom(); + public String generateTokenValue(PersonalAccessTokenType type) { + var bytes = new byte[TOKEN_BYTES]; + random.nextBytes(bytes); + return config.getPrefix() + type.getTokenMarker() + TOKEN_ENCODER.encodeToString(bytes); } @Transactional diff --git a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessTokenType.java b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessTokenType.java index 5d0422912..256fe8a68 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessTokenType.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessTokenType.java @@ -19,26 +19,37 @@ public enum PersonalAccessTokenType { /** * Long-lived personal access token (classic). */ - LLT(false, true), + LLT("at_", false, true), /** * One time usable general personal access token. * Legacy: was used until 1.2.0; but is not anymore. */ @Deprecated - OTT(true, false), + OTT("ot_", true, false), /** * One time usable, trusted publishing issued access token. */ - TPT(true, false); + TPT("tp_", true, false); + private final String tokenMarker; private final boolean oneTime; private final boolean notify; - PersonalAccessTokenType(boolean oneTime, boolean notify) { + PersonalAccessTokenType(String tokenMarker, boolean oneTime, boolean notify) { + this.tokenMarker = tokenMarker; this.oneTime = oneTime; this.notify = notify; } + /** + * Marks which kind of token a value is, so that a leaked one says what it can do at a glance and + * secret scanning can tell them apart. It follows the deployment's own token prefix, which is where + * the registry names itself. + */ + public String getTokenMarker() { + return tokenMarker; + } + public boolean isOneTime() { return oneTime; } diff --git a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java index 93298c972..c52f48e3f 100644 --- a/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java @@ -3677,13 +3677,12 @@ AccessTokenConfig tokenConfig() { @Bean AccessTokenService tokenService( AccessTokenConfig config, - UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, MailService mailService, DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); + return new AccessTokenService(config, entityManager, repositories, mailService, dsl); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index 1b6deee63..c9eb21de1 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -186,7 +186,7 @@ void testAccessTokensNotLoggedIn() throws Exception { @Test void testCreateAccessToken() throws Exception { mockUserData(); - Mockito.doReturn("foobar").when(accessTokenService).generateTokenValue(); + Mockito.doReturn("foobar").when(accessTokenService).generateTokenValue(any()); mockMvc.perform( post("/user/token/create?description={description}", "This is my token") .with(user("test_user")) @@ -1189,13 +1189,12 @@ AccessTokenConfig tokenConfig() { @Bean AccessTokenService accessTokenService( AccessTokenConfig config, - UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, MailService mailService, DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); + return new AccessTokenService(config, entityManager, repositories, mailService, dsl); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index d26b36e48..73492f4cb 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -16,7 +16,6 @@ import java.time.LocalDateTime; import java.time.ZoneId; import java.util.Map; -import java.util.UUID; import jakarta.persistence.EntityManager; import org.jooq.DSLContext; @@ -38,7 +37,6 @@ import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.mail.MailService; import org.eclipse.openvsx.repositories.RepositoryService; -import org.eclipse.openvsx.util.UUIDService; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; @@ -53,9 +51,6 @@ class AccessTokenServiceTest { @Mock AccessTokenConfig config; - @Mock - UUIDService uuidService; - @Mock EntityManager entityManager; @@ -149,16 +144,34 @@ void upgradesTokensWhenItWinsTheLock() { // only it can work across pods anyway. @Test void generatesATokenValueWithoutAskingTheDatabase() { - var uuid = UUID.randomUUID(); - when(config.getPrefix()).thenReturn("ovsxat_"); - when(uuidService.generateRandom()).thenReturn(uuid); + when(config.getPrefix()).thenReturn("ovsx_"); - var value = accessTokenService.generateTokenValue(); + var value = accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT); - assertThat(value).isEqualTo("ovsxat_" + uuid); + // prefix, then the marker saying what kind of token this is, then 256 bits base64url encoded + assertThat(value).startsWith("ovsx_at_"); + assertThat(value.substring("ovsx_at_".length())).hasSize(43).doesNotContain("=", "+", "/"); verifyNoInteractions(repositories); } + @Test + void marksEachKindOfTokenDistinctly() { + when(config.getPrefix()).thenReturn("ovsx_"); + + assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.TPT)).startsWith("ovsx_tp_"); + assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT)).startsWith("ovsx_at_"); + } + + @Test + void generatesADifferentValueEveryTime() { + when(config.getPrefix()).thenReturn(""); + + var values = java.util.stream.Stream.generate( + () -> accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT)).limit(100).toList(); + + assertThat(values).doesNotHaveDuplicates(); + } + // How long a publishing token lives is the trusted publishing configuration's to decide, so this // service applies whatever it is handed instead of reading a setting of its own. @Test @@ -173,8 +186,7 @@ void appliesTheExpirationItIsGivenToATrustedPublishingToken() { trustedPublisher.setExtension(extension); trustedPublisher.setCreatedBy(user); trustedPublisher.setRegistration(Map.of()); - when(config.getPrefix()).thenReturn("ovsxat_"); - when(uuidService.generateRandom()).thenReturn(UUID.randomUUID()); + when(config.getPrefix()).thenReturn("ovsx_"); var before = LocalDateTime.now(ZoneId.of("UTC")); var json = accessTokenService diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index 3a2a96ae1..19aad3c1f 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -2561,13 +2561,12 @@ AccessTokenConfig tokenConfig() { @Bean AccessTokenService tokenService( AccessTokenConfig config, - UUIDService uuidService, EntityManager entityManager, RepositoryService repositories, MailService mailService, DSLContext dsl ) { - return new AccessTokenService(config, uuidService, entityManager, repositories, mailService, dsl); + return new AccessTokenService(config, entityManager, repositories, mailService, dsl); } @Bean From 5483a0e067a0a4e61a8ccda0d396f4fed5005a3a Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 16:55:08 +0200 Subject: [PATCH 22/25] chore: let the token marker supply the separator after the prefix The marker already ends in an underscore, so the dev prefix drops its own: dev_ovsx gives dev_ovsxat_ and dev_ovsxtp_. Co-Authored-By: Claude Opus 5 (1M context) --- server/src/dev/resources/application.yml | 4 ++-- .../accesstoken/AccessTokenServiceTest.java | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/server/src/dev/resources/application.yml b/server/src/dev/resources/application.yml index 64b92915d..0ba9f1a11 100644 --- a/server/src/dev/resources/application.yml +++ b/server/src/dev/resources/application.yml @@ -150,8 +150,8 @@ ovsx: local: directory: /tmp/ovsx access-token: - prefix: dev_ovsx_ # use a token prefix that clearly indicates that it's for development; the kind - # of token (at_, tp_) is appended by the code + prefix: dev_ovsx # use a token prefix that clearly indicates that it's for development; the kind of + # token (at_, tp_) is appended by the code, so no trailing separator here expiration: 0 # do not expire tokens in a dev environment notification: 0 mail: diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index 73492f4cb..4138aa2d2 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -144,22 +144,22 @@ void upgradesTokensWhenItWinsTheLock() { // only it can work across pods anyway. @Test void generatesATokenValueWithoutAskingTheDatabase() { - when(config.getPrefix()).thenReturn("ovsx_"); + when(config.getPrefix()).thenReturn("ovsx"); var value = accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT); // prefix, then the marker saying what kind of token this is, then 256 bits base64url encoded - assertThat(value).startsWith("ovsx_at_"); - assertThat(value.substring("ovsx_at_".length())).hasSize(43).doesNotContain("=", "+", "/"); + assertThat(value).startsWith("ovsxat_"); + assertThat(value.substring("ovsxat_".length())).hasSize(43).doesNotContain("=", "+", "/"); verifyNoInteractions(repositories); } @Test void marksEachKindOfTokenDistinctly() { - when(config.getPrefix()).thenReturn("ovsx_"); + when(config.getPrefix()).thenReturn("ovsx"); - assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.TPT)).startsWith("ovsx_tp_"); - assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT)).startsWith("ovsx_at_"); + assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.TPT)).startsWith("ovsxtp_"); + assertThat(accessTokenService.generateTokenValue(PersonalAccessTokenType.LLT)).startsWith("ovsxat_"); } @Test @@ -186,7 +186,7 @@ void appliesTheExpirationItIsGivenToATrustedPublishingToken() { trustedPublisher.setExtension(extension); trustedPublisher.setCreatedBy(user); trustedPublisher.setRegistration(Map.of()); - when(config.getPrefix()).thenReturn("ovsx_"); + when(config.getPrefix()).thenReturn("ovsx"); var before = LocalDateTime.now(ZoneId.of("UTC")); var json = accessTokenService From 162d807e3e98499f6f43026b935b9d9e51f02c75 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 17:10:24 +0200 Subject: [PATCH 23/25] fix: delete a one-time token when it expires, not just deactivate it Using a one-time token deletes it outright, and so does losing its trusted publisher registration or having its extension purged - but expiry only flipped active to false, in both the scheduled job and the check on use. So a trusted publishing token that worked left no trace while one that was never used was kept forever, which is the wrong way round under any reading: the used ones are the ones a question would be about, and one row is minted per exchange. Expiry now removes the row for one-time types and keeps deactivating long-lived ones, which a user is shown and the notification mails read. TPT does not notify, so nothing is lost from the expiry mail flow. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 31 ++++++++- .../PersonalAccessTokenRepository.java | 12 ++++ .../repositories/RepositoryService.java | 8 +++ .../AccessTokenConcurrentWriteTest.java | 63 ++++++++++++++++++- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 90f04991b..2ebf56b15 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -18,7 +18,9 @@ import java.security.SecureRandom; import java.time.Duration; import java.time.LocalDateTime; +import java.util.Arrays; import java.util.Base64; +import java.util.List; import jakarta.persistence.EntityManager; import jakarta.transaction.Transactional; @@ -66,6 +68,12 @@ public class AccessTokenService { private static final Base64.Encoder TOKEN_ENCODER = Base64.getUrlEncoder().withoutPadding(); + /** The token types that are retired by deleting the row rather than deactivating it. */ + private static final List ONE_TIME_TOKEN_TYPES = Arrays + .stream(PersonalAccessTokenType.values()) + .filter(PersonalAccessTokenType::isOneTime) + .toList(); + private static final int TOKEN_VERSION_0 = 0; private static final int TOKEN_VERSION_1 = 1; private static final int[] ALL_TOKEN_VERSIONS = { TOKEN_VERSION_0, TOKEN_VERSION_1 }; @@ -241,7 +249,11 @@ public AccessTokenAuthentication useAccessToken(String tokenValue, AccessTokenAc // findByExpiresTimestampLessThanEqual...: a token expiring at exactly `now` is expired. LocalDateTime now = TimeUtil.getCurrentUTC(); if (token.getExpiresTimestamp() != null && !token.getExpiresTimestamp().isAfter(now)) { - token.setActive(false); + if (token.getType().isOneTime()) { + entityManager.remove(token); + } else { + token.setActive(false); + } return null; } // Deleting a registration takes its tokens with it, so this should not be reachable; kept as a @@ -278,9 +290,22 @@ private AccessTokenScope getScope(PersonalAccessToken token) { } } + /** + * Retires every token that has expired. + *

+ * A one-time token is deleted rather than deactivated, the same as when one is used, when its trusted + * publisher registration goes, or when its extension is purged: it can never be used again, nothing + * reads the row afterwards, and one is minted per exchange. Keeping the unused ones while deleting the + * used ones would retain exactly the rows nobody has a question about. + *

+ * Long-lived tokens stay as deactivated rows: a user is shown their own expired tokens, and the + * expiry notification mails read them. + */ @Transactional public int expireAccessTokens() { - var expiredAccessTokens = repositories.expirePersonalAccessTokens(TimeUtil.getCurrentUTC()); + var now = TimeUtil.getCurrentUTC(); + var deletedAccessTokens = repositories.deleteExpiredPersonalAccessTokens(now, ONE_TIME_TOKEN_TYPES); + var expiredAccessTokens = repositories.expirePersonalAccessTokens(now); if (config.isSendExpiredMailEnabled()) { for (var token : expiredAccessTokens) { if (token.getType().isNotify()) { @@ -288,7 +313,7 @@ public int expireAccessTokens() { } } } - return expiredAccessTokens.size(); + return deletedAccessTokens.size() + expiredAccessTokens.size(); } @Transactional diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/PersonalAccessTokenRepository.java b/server/src/main/java/org/eclipse/openvsx/repositories/PersonalAccessTokenRepository.java index 46fc0f9e5..83a2c873d 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/PersonalAccessTokenRepository.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/PersonalAccessTokenRepository.java @@ -10,6 +10,7 @@ package org.eclipse.openvsx.repositories; import java.time.LocalDateTime; +import java.util.Collection; import java.util.List; import org.springframework.data.domain.Pageable; @@ -61,4 +62,15 @@ List findByExpiresTimestampLessThanEqualAndActiveTrueAndNot nativeQuery = true ) List expireAccessTokens(LocalDateTime timestamp); + + /** + * Deletes the expired tokens of the given types outright. A one-time token that expired unused can + * never be used, and nothing reads the row afterwards - the same reason using one deletes it. + */ + @Modifying + @Query( + value = "delete from personal_access_token where expires_timestamp <= ?1 and type in ?2 returning *", + nativeQuery = true + ) + List deleteExpiredAccessTokens(LocalDateTime timestamp, Collection types); } diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index fba943fbc..4449338db 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -590,6 +590,14 @@ public List expirePersonalAccessTokens(LocalDateTime timest return personalAccessTokenRepo.expireAccessTokens(timestamp); } + public List deleteExpiredPersonalAccessTokens( + LocalDateTime timestamp, + Collection types + ) { + return personalAccessTokenRepo + .deleteExpiredAccessTokens(timestamp, types.stream().map(Enum::name).toList()); + } + public int updateExpiresTimeForLegacyPersonalAccessTokens(LocalDateTime timestamp, PersonalAccessTokenType type) { return personalAccessTokenRepo.updateExpiresTimeForLegacyAccessTokens(timestamp, type); } diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java index 334c1c673..7eb03cc82 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java @@ -12,6 +12,8 @@ *****************************************************************************/ package org.eclipse.openvsx.accesstoken; +import java.time.LocalDateTime; + import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; import org.junit.jupiter.api.Test; @@ -32,13 +34,16 @@ import static org.assertj.core.api.Assertions.assertThat; /** - * The paths that write a token row each touch a different column, so one must not carry the others back - * to whatever it happened to load. Only a real database shows this: it is Hibernate's generated UPDATE - * that decides, and with the default full-row update the loser's stale columns win. + * Token rows against a real database: what a write actually sends, and what expiry actually leaves behind. + * Neither is visible without one - the first is decided by Hibernate's generated UPDATE, the second by a + * native query. */ @SpringBootTest class AccessTokenConcurrentWriteTest extends AbstractPostgresContainerTest { + @Autowired + AccessTokenService accessTokens; + @Autowired EntityManager em; @@ -80,6 +85,51 @@ void upgradingATokenDoesNotResurrectOneRevokedMeanwhile() { cleanUp(tokenId); } + // Using a one-time token deletes it, so keeping the ones that expired unused would retain exactly the + // rows nobody has a question about - and one is minted per trusted publishing exchange. + @Test + void expiryDeletesAOneTimeTokenAndOnlyDeactivatesALongLivedOne() { + var expired = TimeUtil.getCurrentUTC().minusMinutes(1); + var oneTime = persistToken("expiry-tpt", PersonalAccessTokenType.TPT, 1, expired); + var longLived = persistToken("expiry-llt", PersonalAccessTokenType.LLT, 1, expired); + + new TransactionTemplate(txManager).executeWithoutResult(status -> accessTokens.expireAccessTokens()); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + assertThat(em.find(PersonalAccessToken.class, oneTime)).isNull(); + var kept = em.find(PersonalAccessToken.class, longLived); + // a user is shown their own expired tokens, and the notification mails read them + assertThat(kept).isNotNull(); + assertThat(kept.isActive()).isFalse(); + }); + + cleanUp(longLived); + // the one-time token's own row is gone, but its user is not + cleanUpUser("expiry-tpt-user"); + } + + private long persistToken(String name, PersonalAccessTokenType type, int version, LocalDateTime expires) { + return new TransactionTemplate(txManager).execute(status -> { + var user = new UserData(); + user.setLoginName(name + "-user"); + user.setProvider("github"); + em.persist(user); + + var token = new PersonalAccessToken(); + token.setUser(user); + token.setValue(name + "-token-value"); + token.setActive(true); + token.setCreatedTimestamp(TimeUtil.getCurrentUTC()); + token.setExpiresTimestamp(expires); + token.setVersion(version); + token.setType(type); + token.setDescription(name); + em.persist(token); + em.flush(); + return token.getId(); + }); + } + private long persistLegacyToken() { return new TransactionTemplate(txManager).execute(status -> { var user = new UserData(); @@ -110,6 +160,13 @@ private void revokeInAnotherTransaction(long tokenId) { .executeUpdate()); } + private void cleanUpUser(String loginName) { + new TransactionTemplate(txManager).executeWithoutResult( + status -> em.createQuery("delete from UserData u where u.loginName = :name") + .setParameter("name", loginName) + .executeUpdate()); + } + /** The container is shared with every other test, so leave nothing of this one behind. */ private void cleanUp(long tokenId) { new TransactionTemplate(txManager).executeWithoutResult(status -> { From a2e7a1ffed40fa010bd2cd3bbcdb4883c7bd6f45 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 17:47:45 +0200 Subject: [PATCH 24/25] feat: record which token published a version, as best-effort provenance Without it, a leaked long-lived token cannot be answered for: the version records who published and with what kind of token, so response degrades to revoking every token that user holds and treating everything they published as suspect. extension_version.published_with_id comes back, nullable and ON DELETE SET NULL, which is what V1_72's own comment already described before the SQL below it dropped the column instead. Authorship stays where it is - in published_by_id and published_with_tt, copied rather than referenced - so this reference is free to decay when the token row goes: a one-time token used up, or a forgotten user's tokens, simply leave it null. It is stable where it matters, since a long-lived token is only ever deactivated, never deleted, outside GDPR erasure. The publish path had no token to record - AccessTokenAuthentication carried the user and the type only - so it now carries the token id as well. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 2 +- .../openvsx/entities/ExtensionVersion.java | 20 ++++++ .../PublishExtensionVersionHandler.java | 1 + .../util/auth/AccessTokenAuthentication.java | 9 ++- .../db/migration/V1_72__Trusted_Publisher.sql | 12 ++-- .../eclipse/openvsx/ExtensionServiceTest.java | 8 ++- .../openvsx/LocalRegistryServiceTest.java | 2 +- .../AccessTokenConcurrentWriteTest.java | 72 +++++++++++++++++++ ...ublishExtensionVersionConcurrencyTest.java | 5 +- .../PublishExtensionVersionHandlerTest.java | 2 +- .../RepositoryServiceSmokeTest.java | 2 + 11 files changed, 124 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 2ebf56b15..15dfc5b7c 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -277,7 +277,7 @@ public AccessTokenAuthentication useAccessToken(String tokenValue, AccessTokenAc entityManager.remove(token); } } - return new AccessTokenAuthentication(token.getUser(), token.getType()); + return new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()); } private AccessTokenScope getScope(PersonalAccessToken token) { diff --git a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java index e4a7ac52e..52dd4ff7d 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java @@ -29,6 +29,7 @@ import jakarta.persistence.Table; import jakarta.persistence.Transient; import org.apache.commons.lang3.StringUtils; +import org.jspecify.annotations.Nullable; import org.eclipse.openvsx.json.ExtensionJson; import org.eclipse.openvsx.json.ExtensionReferenceJson; @@ -92,6 +93,16 @@ public enum Type { @Enumerated(EnumType.STRING) private PersonalAccessTokenType publishedWithTt; + /** + * The token this version was published with, as best-effort provenance: it answers "what did this + * credential publish" after a leak. Nullable and allowed to decay - the token row is deleted when a + * one-time token is used or a forgotten user's tokens go - which is why authorship is recorded + * separately in {@link #publishedBy} and {@link #publishedWithTt} instead of being read through it. + */ + @Column(name = "published_with_id") + @Nullable + private Long publishedWithId; + private boolean active; private boolean potentiallyMalicious; @@ -351,6 +362,15 @@ public PersonalAccessTokenType getPublishedWithTt() { return publishedWithTt; } + @Nullable + public Long getPublishedWithId() { + return publishedWithId; + } + + public void setPublishedWithId(@Nullable Long publishedWithId) { + this.publishedWithId = publishedWithId; + } + public void setPublishedWithTt(PersonalAccessTokenType publishedWithTt) { this.publishedWithTt = publishedWithTt; } diff --git a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java index af47545e8..70349a758 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java @@ -250,6 +250,7 @@ private ExtensionVersion createExtensionVersion( extVersion.setPublishedBy(au.userData()); if (au instanceof AccessTokenAuthentication ata) { extVersion.setPublishedWithTt(ata.type()); + extVersion.setPublishedWithId(ata.tokenId()); } extVersion.setActive(false); diff --git a/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java b/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java index 9728fc708..9f792aa2a 100644 --- a/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java +++ b/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java @@ -20,7 +20,14 @@ /** * Represents user who presented a valid access token. */ -public record AccessTokenAuthentication(UserData userData, PersonalAccessTokenType type) implements AuthenticatedUser { +/** + * @param tokenId the token that authenticated this request, recorded on whatever it publishes as + * best-effort provenance. Kept as an id rather than the entity: nothing here should hold + * a credential open, and the reference is allowed to decay when the token row goes. + */ +public record AccessTokenAuthentication(UserData userData, PersonalAccessTokenType type, long tokenId) + implements + AuthenticatedUser { @Override public @NonNull AuthenticationType authenticationType() { return AuthenticationType.TOKEN; diff --git a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql index cf3e5bab3..60112e4dd 100644 --- a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql +++ b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql @@ -90,13 +90,17 @@ ALTER TABLE public.extension_version CREATE INDEX extension_version__published_by_id__idx ON public.extension_version (published_by_id); -- published_with_id keeps recording which credential was used, but as best-effort provenance rather --- than a hard dependency: deleting a token in future clears the reference instead of being refused by --- the database. The base migration left this constraint named by Hibernate; drop it by that generated --- name and recreate under the naming convention used everywhere else on this table. +-- than a hard dependency: it answers "what did this token publish" after a leak, while authorship is +-- recorded permanently in published_by_id above. Deleting a token clears the reference instead of being +-- refused by the database, so a one-time token used up, or a forgotten user's tokens, simply leave it +-- null. The base migration left this constraint named by Hibernate; drop it by that generated name and +-- recreate under the naming convention used everywhere else on this table. ALTER TABLE public.extension_version DROP CONSTRAINT fk70khj8pm0vacasuiiaq0w0r80; ALTER TABLE public.extension_version - DROP COLUMN published_with_id; + ADD CONSTRAINT extension_version_published_with_id_fkey + FOREIGN KEY (published_with_id) REFERENCES public.personal_access_token(id) ON DELETE SET NULL; + -- and now we can delete all inactive one-time-usable personal access tokens DELETE FROM public.personal_access_token pat diff --git a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java b/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java index e84891c0d..a81549335 100644 --- a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java @@ -318,7 +318,9 @@ void shouldNotScanWhenPublishPreconditionsFail() { var content = new ByteArrayInputStream("extension package".getBytes(StandardCharsets.UTF_8)); assertThatThrownBy( - () -> svc.publishVersion(content, new AccessTokenAuthentication(token.getUser(), token.getType()))) + () -> svc.publishVersion( + content, + new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()))) .isInstanceOf(ErrorResultException.class) .hasMessageContaining("Insufficient access rights"); @@ -344,7 +346,9 @@ void shouldRejectAPackageExceedingTheMaxContentSize() { var content = new DrainOnCloseInputStream(raw, maxContentSize); assertThatThrownBy( - () -> svc.publishVersion(content, new AccessTokenAuthentication(token.getUser(), token.getType()))) + () -> svc.publishVersion( + content, + new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()))) .isInstanceOf(ErrorResultException.class) .hasMessageContaining("exceeds the size limit") .extracting(exc -> ((ErrorResultException) exc).getStatus()) diff --git a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java index 995edafdb..bbc31a327 100644 --- a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java @@ -164,7 +164,7 @@ void shouldNotDeleteTempFileOnceOwnershipIsHandedToAsyncPublish() throws IOExcep extVersion.setExtension(extension); extVersion.setVersion("1.0.0"); - var tau = new AccessTokenAuthentication(token.getUser(), token.getType()); + var tau = new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()); when(extensions.createExtensionFile(any())).thenReturn(tempFile); when(tokens.useAccessToken(eq("tok"), any())).thenReturn(tau); diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java index 7eb03cc82..cc53dfd28 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java @@ -25,10 +25,14 @@ import org.springframework.transaction.support.TransactionTemplate; import org.eclipse.openvsx.AbstractPostgresContainerTest; +import org.eclipse.openvsx.entities.Extension; +import org.eclipse.openvsx.entities.ExtensionVersion; +import org.eclipse.openvsx.entities.Namespace; import org.eclipse.openvsx.entities.PersonalAccessToken; import org.eclipse.openvsx.entities.PersonalAccessTokenType; import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.search.SearchUtilService; +import org.eclipse.openvsx.util.TargetPlatform; import org.eclipse.openvsx.util.TimeUtil; import static org.assertj.core.api.Assertions.assertThat; @@ -108,6 +112,74 @@ void expiryDeletesAOneTimeTokenAndOnlyDeactivatesALongLivedOne() { cleanUpUser("expiry-tpt-user"); } + // Best-effort provenance: it answers "what did this credential publish" after a leak, and must give + // way rather than stand in the way when the token itself is deleted. + @Test + void aVersionRemembersTheTokenItWasPublishedWithUntilThatTokenIsDeleted() { + var tokenId = persistToken("provenance", PersonalAccessTokenType.LLT, 1, null); + var versionId = persistVersionPublishedWith(tokenId); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + assertThat(em.find(ExtensionVersion.class, versionId).getPublishedWithId()).isEqualTo(tokenId); + }); + + new TransactionTemplate(txManager) + .executeWithoutResult(status -> em.remove(em.find(PersonalAccessToken.class, tokenId))); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + var version = em.find(ExtensionVersion.class, versionId); + // the reference decays, the authorship does not + assertThat(version.getPublishedWithId()).isNull(); + assertThat(version.getPublishedBy()).isNotNull(); + assertThat(version.getPublishedWithTt()).isEqualTo(PersonalAccessTokenType.LLT); + }); + + cleanUpVersion(versionId); + cleanUpUser("provenance-user"); + } + + private long persistVersionPublishedWith(long tokenId) { + return new TransactionTemplate(txManager).execute(status -> { + var token = em.find(PersonalAccessToken.class, tokenId); + + var namespace = new Namespace(); + namespace.setName("provenance-ns"); + em.persist(namespace); + + var extension = new Extension(); + extension.setName("provenance-ext"); + extension.setNamespace(namespace); + extension.setActive(true); + em.persist(extension); + + var version = new ExtensionVersion(); + version.setExtension(extension); + version.setVersion("1.0.0"); + version.setTargetPlatform(TargetPlatform.NAME_UNIVERSAL); + version.setTimestamp(TimeUtil.getCurrentUTC()); + version.setPublishedBy(token.getUser()); + version.setPublishedWithTt(token.getType()); + version.setPublishedWithId(token.getId()); + em.persist(version); + em.flush(); + return version.getId(); + }); + } + + private void cleanUpVersion(long versionId) { + new TransactionTemplate(txManager).executeWithoutResult(status -> { + var version = em.find(ExtensionVersion.class, versionId); + var extension = version.getExtension(); + var namespaceId = extension.getNamespace().getId(); + em.remove(version); + em.flush(); + em.createQuery("delete from Extension e where e.id = :id").setParameter("id", extension.getId()) + .executeUpdate(); + em.createQuery("delete from Namespace n where n.id = :id").setParameter("id", namespaceId) + .executeUpdate(); + }); + } + private long persistToken(String name, PersonalAccessTokenType type, int version, LocalDateTime expires) { return new TransactionTemplate(txManager).execute(status -> { var user = new UserData(); diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java index bd0ac7a89..92545e472 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java @@ -286,7 +286,10 @@ private ExtensionVersion publish(String targetPlatform) { return publishHandler .createExtensionVersion( processor, - new AccessTokenAuthentication(publishToken().getUser(), publishToken().getType()), + new AccessTokenAuthentication( + publishToken().getUser(), + publishToken().getType(), + publishToken().getId()), LocalDateTime.now(), false); } diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java index 5afb3b26e..e61aeb300 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java @@ -167,7 +167,7 @@ void shouldRecordTheTokenTypeUsedToPublish() throws IOException { var namespace = buildNamespace("publisher"); var user = new UserData(); - var ata = new AccessTokenAuthentication(user, PersonalAccessTokenType.TPT); + var ata = new AccessTokenAuthentication(user, PersonalAccessTokenType.TPT, 1L); when(repositories.findNamespace("publisher")).thenReturn(namespace); when(users.hasPublishPermission(user, namespace)).thenReturn(true); diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index 3376845ac..cfdd5642a 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -371,6 +371,8 @@ void testExecuteQueries() { () -> repositories.isDeleteAllActiveVersions("namespaceName", "extensionName"), () -> repositories.deactivatePersonalAccessTokens(userData), () -> repositories.expirePersonalAccessTokens(NOW), + () -> repositories + .deleteExpiredPersonalAccessTokens(NOW, List.of(PersonalAccessTokenType.TPT)), () -> repositories.findExpiringPersonalAccessTokensWithoutNotification(NOW, page), () -> repositories.updateExpiresTimeForLegacyPersonalAccessTokens(NOW, PersonalAccessTokenType.LLT), () -> repositories.findSimilarExtensionsByLevenshtein( From 44654dc2685094ae3539218ec6839db018a03f8a Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Tue, 1 Sep 2026 18:25:00 +0200 Subject: [PATCH 25/25] feat: record the OIDC identity that produced a trusted-published version The link from a published artifact back to the workflow run that produced it is most of what trusted publishing buys over a token pasted into CI, and none of it was kept. The claims the provider asserts are validated at the exchange, matched against the registration, and dropped; the version was left saying only that some trusted publishing token was used, by the user who had registered it. The token that could have said more is deleted as it is used, and the registration can be revoked - so nothing durable remained. The claims now ride from the exchange to the publish on personal_access_token.claims, and are copied onto the version as extension_version.published_provenance. Both jsonb, both nullable, and the copy is what makes it durable, exactly as for published_by_id. What that records is the immutable identity rather than the names: the repository and owner ids, and the workflow reference including the ref it ran on - which the registration deliberately strips, since it trusts any ref, and which is the detail worth having afterwards. Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 22 +++++++++--- .../openvsx/entities/ExtensionVersion.java | 23 ++++++++++++ .../openvsx/entities/PersonalAccessToken.java | 22 ++++++++++++ .../PublishExtensionVersionHandler.java | 1 + .../TrustedPublishingService.java | 3 +- .../util/auth/AccessTokenAuthentication.java | 17 ++++++--- .../db/migration/V1_72__Trusted_Publisher.sql | 15 ++++++-- .../eclipse/openvsx/ExtensionServiceTest.java | 4 +-- .../openvsx/LocalRegistryServiceTest.java | 2 +- .../AccessTokenConcurrentWriteTest.java | 36 +++++++++++++++++-- .../accesstoken/AccessTokenServiceTest.java | 5 ++- ...ublishExtensionVersionConcurrencyTest.java | 3 +- .../PublishExtensionVersionHandlerTest.java | 2 +- 13 files changed, 135 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index 15dfc5b7c..f2f9421c3 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.Base64; import java.util.List; +import java.util.Map; import jakarta.persistence.EntityManager; import jakarta.transaction.Transactional; @@ -109,7 +110,15 @@ public AccessTokenJson createLongLivedAccessToken(UserData user, String descript final LocalDateTime expiresTimestamp = config.isTokenExpiryEnabled() ? TimeUtil.getCurrentUTC().plus(config.getExpiration()) : null; - return createAccessToken(user, description, expiresTimestamp, null, null, null, PersonalAccessTokenType.LLT); + return createAccessToken( + user, + description, + expiresTimestamp, + null, + null, + null, + null, + PersonalAccessTokenType.LLT); } /** @@ -121,16 +130,19 @@ public AccessTokenJson createLongLivedAccessToken(UserData user, String descript public AccessTokenJson createTrustedPublishingAccessToken( TrustedPublisher trustedPublisher, String description, - Duration expiration + Duration expiration, + Map claims ) { requireNonNull(trustedPublisher); requireNonNull(expiration); + requireNonNull(claims); final LocalDateTime expiresTimestamp = TimeUtil.getCurrentUTC().plus(expiration); return createAccessToken( trustedPublisher.getCreatedBy(), description, expiresTimestamp, trustedPublisher, + claims, null, null, PersonalAccessTokenType.TPT); @@ -141,6 +153,7 @@ private AccessTokenJson createAccessToken( String description, @Nullable LocalDateTime expiresTimestamp, @Nullable TrustedPublisher trustedPublisher, + @Nullable Map claims, @Nullable Extension scopeExtension, @Nullable Namespace scopeNamespace, PersonalAccessTokenType type @@ -160,8 +173,9 @@ private AccessTokenJson createAccessToken( if (type != PersonalAccessTokenType.TPT) { throw new IllegalArgumentException("Only TPT token may be created with TP"); } - // link TP and scope to TP.ext + // link TP and scope to TP.ext, and carry the exchange's claims to the publish that uses this token.setTrustedPublisher(trustedPublisher); + token.setClaims(claims); token.setScopeExtension(trustedPublisher.getExtension()); } else if (scopeExtension != null) { // scope to ext @@ -277,7 +291,7 @@ public AccessTokenAuthentication useAccessToken(String tokenValue, AccessTokenAc entityManager.remove(token); } } - return new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()); + return new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId(), token.getClaims()); } private AccessTokenScope getScope(PersonalAccessToken token) { diff --git a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java index 52dd4ff7d..c8bfdf432 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/ExtensionVersion.java @@ -29,6 +29,8 @@ import jakarta.persistence.Table; import jakarta.persistence.Transient; import org.apache.commons.lang3.StringUtils; +import org.hibernate.annotations.JdbcTypeCode; +import org.hibernate.type.SqlTypes; import org.jspecify.annotations.Nullable; import org.eclipse.openvsx.json.ExtensionJson; @@ -103,6 +105,18 @@ public enum Type { @Nullable private Long publishedWithId; + /** + * The OIDC identity that produced this version, for versions published through trusted publishing: the + * immutable repository and owner ids and the workflow reference including the ref it ran on, as the + * provider asserted them at the exchange. Null for everything published with an ordinary token. + *

+ * Copied at publish time rather than reached through the token, which is deleted as it is used. + */ + @JdbcTypeCode(SqlTypes.JSON) + @Column(name = "published_provenance", columnDefinition = "jsonb") + @Nullable + private Map publishedProvenance; + private boolean active; private boolean potentiallyMalicious; @@ -362,6 +376,15 @@ public PersonalAccessTokenType getPublishedWithTt() { return publishedWithTt; } + @Nullable + public Map getPublishedProvenance() { + return publishedProvenance; + } + + public void setPublishedProvenance(@Nullable Map publishedProvenance) { + this.publishedProvenance = publishedProvenance; + } + @Nullable public Long getPublishedWithId() { return publishedWithId; diff --git a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java index 3d0cd2749..794e8a4c5 100644 --- a/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java +++ b/server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java @@ -12,10 +12,14 @@ import java.io.Serial; import java.io.Serializable; import java.time.LocalDateTime; +import java.util.Map; import java.util.Objects; import jakarta.persistence.*; import org.hibernate.annotations.DynamicUpdate; +import org.hibernate.annotations.JdbcTypeCode; +import org.hibernate.type.SqlTypes; +import org.jspecify.annotations.Nullable; import org.eclipse.openvsx.json.AccessTokenJson; import org.eclipse.openvsx.util.TimeUtil; @@ -78,6 +82,15 @@ public class PersonalAccessToken implements Serializable { @JoinColumn(name = "trusted_publisher_id") private TrustedPublisher trustedPublisher; + /** + * The OIDC claims this token was exchanged for, carried from the exchange to the publish that uses it + * and copied onto the version there. Null for every token that is not a trusted publishing one. + */ + @JdbcTypeCode(SqlTypes.JSON) + @Column(columnDefinition = "jsonb") + @Nullable + private Map claims; + /** * Convert to a JSON object. */ @@ -203,6 +216,15 @@ public void setScopeNamespace(Namespace scopeNamespace) { this.scopeNamespace = scopeNamespace; } + @Nullable + public Map getClaims() { + return claims; + } + + public void setClaims(@Nullable Map claims) { + this.claims = claims; + } + public TrustedPublisher getTrustedPublisher() { return trustedPublisher; } diff --git a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java index 70349a758..62d9e6cc7 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java @@ -251,6 +251,7 @@ private ExtensionVersion createExtensionVersion( if (au instanceof AccessTokenAuthentication ata) { extVersion.setPublishedWithTt(ata.type()); extVersion.setPublishedWithId(ata.tokenId()); + extVersion.setPublishedProvenance(ata.claims()); } extVersion.setActive(false); diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index 2fbfae8de..106ff249f 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -312,7 +312,8 @@ public AccessTokenJson requestPublishToken(String namespaceName, String extensio return tokens.createTrustedPublishingAccessToken( match, TOKEN_DESCRIPTION_TEMPLATE.formatted(provider.getProviderId()), - config.getTokenExpiration()); + config.getTokenExpiration(), + claims); } private Namespace requireOwnedNamespace(UserData user, String namespaceName) { diff --git a/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java b/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java index 9f792aa2a..1ace37eaa 100644 --- a/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java +++ b/server/src/main/java/org/eclipse/openvsx/util/auth/AccessTokenAuthentication.java @@ -12,22 +12,29 @@ *****************************************************************************/ package org.eclipse.openvsx.util.auth; +import java.util.Map; + import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; import org.eclipse.openvsx.entities.PersonalAccessTokenType; import org.eclipse.openvsx.entities.UserData; /** * Represents user who presented a valid access token. - */ -/** + * * @param tokenId the token that authenticated this request, recorded on whatever it publishes as * best-effort provenance. Kept as an id rather than the entity: nothing here should hold * a credential open, and the reference is allowed to decay when the token row goes. + * @param claims for a trusted publishing token, the OIDC identity the provider asserted at the exchange, + * which the publish copies onto the version. Null for any other kind of token. */ -public record AccessTokenAuthentication(UserData userData, PersonalAccessTokenType type, long tokenId) - implements - AuthenticatedUser { +public record AccessTokenAuthentication( + UserData userData, + PersonalAccessTokenType type, + long tokenId, + @Nullable Map claims +) implements AuthenticatedUser { @Override public @NonNull AuthenticationType authenticationType() { return AuthenticationType.TOKEN; diff --git a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql index 60112e4dd..315b06b61 100644 --- a/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql +++ b/server/src/main/resources/db/migration/V1_72__Trusted_Publisher.sql @@ -41,7 +41,11 @@ ALTER TABLE ONLY public.personal_access_token -- optional; the trusted publisher that the token was created for. Deleting a registration retires the -- tokens issued under it: they may only ever publish the one extension it was made for, so once it is -- gone they can authorize nothing. - ADD COLUMN IF NOT EXISTS trusted_publisher_id BIGINT REFERENCES public.trusted_publisher(id) ON DELETE CASCADE; + ADD COLUMN IF NOT EXISTS trusted_publisher_id BIGINT REFERENCES public.trusted_publisher(id) ON DELETE CASCADE, + -- optional; the OIDC claims the token was exchanged for, carried from the exchange to the publish that + -- uses it. This row does not keep them - a one-time token is deleted as it is used - it hands them to + -- the version being published, see extension_version.published_provenance below. + ADD COLUMN IF NOT EXISTS claims JSONB; -- set OTT based on description (is hardwired in codebase) UPDATE public.personal_access_token @@ -70,7 +74,14 @@ ALTER TABLE ONLY public.personal_access_token -- Every read path that asks "who published this?" uses this column from now on. ALTER TABLE public.extension_version ADD COLUMN published_by_id BIGINT, - ADD COLUMN published_with_tt CHARACTER VARYING(32); + ADD COLUMN published_with_tt CHARACTER VARYING(32), + -- The OIDC identity that produced this version, for versions published through trusted publishing: + -- the immutable repository and owner ids and the workflow reference including the ref it ran on, as + -- the provider asserted them at the exchange. Copied rather than referenced for the usual reason - + -- the token is deleted as it is used, and the registration can be revoked - and this link from a + -- published artifact back to the workflow run is most of what trusted publishing buys over a token + -- somebody pasted into CI. + ADD COLUMN published_provenance JSONB; -- Backfill from the only place the answer exists today. Rows whose published_with_id is already NULL -- have nothing to derive it from and stay NULL, which is why this column is deliberately left nullable: diff --git a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java b/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java index a81549335..02d6f536c 100644 --- a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/ExtensionServiceTest.java @@ -320,7 +320,7 @@ void shouldNotScanWhenPublishPreconditionsFail() { assertThatThrownBy( () -> svc.publishVersion( content, - new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()))) + new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId(), null))) .isInstanceOf(ErrorResultException.class) .hasMessageContaining("Insufficient access rights"); @@ -348,7 +348,7 @@ void shouldRejectAPackageExceedingTheMaxContentSize() { assertThatThrownBy( () -> svc.publishVersion( content, - new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()))) + new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId(), null))) .isInstanceOf(ErrorResultException.class) .hasMessageContaining("exceeds the size limit") .extracting(exc -> ((ErrorResultException) exc).getStatus()) diff --git a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java index bbc31a327..6044c5835 100644 --- a/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/LocalRegistryServiceTest.java @@ -164,7 +164,7 @@ void shouldNotDeleteTempFileOnceOwnershipIsHandedToAsyncPublish() throws IOExcep extVersion.setExtension(extension); extVersion.setVersion("1.0.0"); - var tau = new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId()); + var tau = new AccessTokenAuthentication(token.getUser(), token.getType(), token.getId(), null); when(extensions.createExtensionFile(any())).thenReturn(tempFile); when(tokens.useAccessToken(eq("tok"), any())).thenReturn(tau); diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java index cc53dfd28..c7c4863ca 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenConcurrentWriteTest.java @@ -13,6 +13,7 @@ package org.eclipse.openvsx.accesstoken; import java.time.LocalDateTime; +import java.util.Map; import jakarta.persistence.EntityManager; import org.jobrunr.scheduling.JobRequestScheduler; @@ -138,16 +139,46 @@ void aVersionRemembersTheTokenItWasPublishedWithUntilThatTokenIsDeleted() { cleanUpUser("provenance-user"); } + // The link from a published artifact back to the workflow run is most of what trusted publishing buys + // over a token pasted into CI, and the token that carried it is deleted the moment it is used. + @Test + void aVersionKeepsItsTrustedPublishingProvenanceAfterTheTokenIsGone() { + var claims = Map.of( + "repository_id", + "74", + "repository_owner_id", + "65", + "workflow_ref", + "octo-org/octo-repo/.github/workflows/publish.yml@refs/tags/v1.2.0"); + var tokenId = persistToken("provenance-tpt", PersonalAccessTokenType.TPT, 1, null); + new TransactionTemplate(txManager) + .executeWithoutResult(status -> em.find(PersonalAccessToken.class, tokenId).setClaims(claims)); + var versionId = persistVersionPublishedWith(tokenId); + + // using a one-time token deletes it, so the copy is the only thing left + new TransactionTemplate(txManager) + .executeWithoutResult(status -> em.remove(em.find(PersonalAccessToken.class, tokenId))); + + new TransactionTemplate(txManager).executeWithoutResult(status -> { + var version = em.find(ExtensionVersion.class, versionId); + assertThat(version.getPublishedProvenance()).isEqualTo(claims); + assertThat(version.getPublishedWithId()).isNull(); + }); + + cleanUpVersion(versionId); + cleanUpUser("provenance-tpt-user"); + } + private long persistVersionPublishedWith(long tokenId) { return new TransactionTemplate(txManager).execute(status -> { var token = em.find(PersonalAccessToken.class, tokenId); var namespace = new Namespace(); - namespace.setName("provenance-ns"); + namespace.setName("provenance-ns-" + tokenId); em.persist(namespace); var extension = new Extension(); - extension.setName("provenance-ext"); + extension.setName("provenance-ext-" + tokenId); extension.setNamespace(namespace); extension.setActive(true); em.persist(extension); @@ -160,6 +191,7 @@ private long persistVersionPublishedWith(long tokenId) { version.setPublishedBy(token.getUser()); version.setPublishedWithTt(token.getType()); version.setPublishedWithId(token.getId()); + version.setPublishedProvenance(token.getClaims()); em.persist(version); em.flush(); return version.getId(); diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index 4138aa2d2..2ba63edc4 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -193,7 +193,8 @@ void appliesTheExpirationItIsGivenToATrustedPublishingToken() { .createTrustedPublishingAccessToken( trustedPublisher, "Trusted publishing (github)", - Duration.ofMinutes(7)); + Duration.ofMinutes(7), + Map.of("repository_id", "74")); var after = LocalDateTime.now(ZoneId.of("UTC")); var persisted = ArgumentCaptor.forClass(PersonalAccessToken.class); @@ -203,6 +204,8 @@ void appliesTheExpirationItIsGivenToATrustedPublishingToken() { .isBetween(before.plusMinutes(7), after.plusMinutes(7)); // the token is scoped to the registration's extension, whatever its lifetime assertThat(persisted.getValue().getScopeExtension()).isSameAs(extension); + // carried to the publish that uses this token, which copies them onto the version + assertThat(persisted.getValue().getClaims()).containsEntry("repository_id", "74"); assertThat(json.getValue()).isNotNull(); } } diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java index 92545e472..4639beaff 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionConcurrencyTest.java @@ -289,7 +289,8 @@ private ExtensionVersion publish(String targetPlatform) { new AccessTokenAuthentication( publishToken().getUser(), publishToken().getType(), - publishToken().getId()), + publishToken().getId(), + null), LocalDateTime.now(), false); } diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java index e61aeb300..c0da662c8 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandlerTest.java @@ -167,7 +167,7 @@ void shouldRecordTheTokenTypeUsedToPublish() throws IOException { var namespace = buildNamespace("publisher"); var user = new UserData(); - var ata = new AccessTokenAuthentication(user, PersonalAccessTokenType.TPT, 1L); + var ata = new AccessTokenAuthentication(user, PersonalAccessTokenType.TPT, 1L, null); when(repositories.findNamespace("publisher")).thenReturn(namespace); when(users.hasPublishPermission(user, namespace)).thenReturn(true);