From b6d34eb74a4092ffd87460f2b75f04955cbdb9b2 Mon Sep 17 00:00:00 2001 From: Martin Lowe Date: Fri, 20 Mar 2026 14:29:45 -0400 Subject: [PATCH 1/2] feat: Add logged in user identity cache key To cover logged in user sessions more cleanly when resolving identities, a new cachekey prefix was added to IdentityService.resoveIdentity. This "user_" prefix is used when there is an identifiable user session for the current request. Resolves https://github.com/EclipseFdn/open-vsx.org/issues/8954 --- .../openvsx/ratelimit/IdentityService.java | 24 +++- .../ratelimit/IdentityServiceTest.java | 111 ++++++++++++++++++ 2 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java diff --git a/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java b/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java index 8f6464a33..bc03f8158 100644 --- a/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java +++ b/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java @@ -12,8 +12,10 @@ *****************************************************************************/ package org.eclipse.openvsx.ratelimit; -import com.giffing.bucket4j.spring.boot.starter.context.ExpressionParams; -import jakarta.servlet.http.HttpServletRequest; +import java.util.Map; + +import org.apache.commons.lang3.StringUtils; +import org.eclipse.openvsx.UserService; import org.eclipse.openvsx.accesstoken.AccessTokenService; import org.eclipse.openvsx.ratelimit.config.RateLimitConfig; import org.eclipse.openvsx.ratelimit.config.RateLimitProperties; @@ -26,7 +28,9 @@ import org.springframework.expression.spel.support.StandardEvaluationContext; import org.springframework.stereotype.Service; -import java.util.Map; +import com.giffing.bucket4j.spring.boot.starter.context.ExpressionParams; + +import jakarta.servlet.http.HttpServletRequest; @Service @ConditionalOnBean(RateLimitConfig.class) @@ -41,6 +45,7 @@ public class IdentityService { private final CustomerService customerService; private final AccessTokenService tokenService; private final RateLimitProperties rateLimitProperties; + private final UserService userService; public IdentityService( ExpressionParser expressionParser, @@ -48,7 +53,8 @@ public IdentityService( TierService tierService, CustomerService customerService, AccessTokenService tokenService, - RateLimitProperties rateLimitProperties + RateLimitProperties rateLimitProperties, + UserService userService ) { this.expressionParser = expressionParser; this.beanFactory = beanFactory; @@ -56,6 +62,7 @@ public IdentityService( this.customerService = customerService; this.tokenService = tokenService; this.rateLimitProperties = rateLimitProperties; + this.userService = userService; } public ResolvedIdentity resolveIdentity(HttpServletRequest request) { @@ -74,6 +81,15 @@ public ResolvedIdentity resolveIdentity(HttpServletRequest request) { } } + // if we don't have a valid token, we check if the user is logged in to generate a cache key + // we want to only do this if we don't have a valid token to avoid unnecessary database calls + if (cacheKey == null) { + var user = userService.findLoggedInUser(); + if (user != null && StringUtils.isNotBlank(user.getAuthId())) { + cacheKey = "user_" + user.getAuthId(); + } + } + var customer = customerService.getCustomerByIpAddress(ipAddress); if (customer.isPresent() && cacheKey == null) { cacheKey = "customer_" + customer.get().getName(); diff --git a/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java b/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java new file mode 100644 index 000000000..1153b7d6f --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java @@ -0,0 +1,111 @@ +/** + * Copyright (c) 2026 Eclipse Foundation AISBL + * + * 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.ratelimit; + +import java.util.Optional; + +import org.eclipse.openvsx.UserService; +import org.eclipse.openvsx.accesstoken.AccessTokenService; +import org.eclipse.openvsx.entities.UserData; +import org.eclipse.openvsx.ratelimit.config.RateLimitProperties; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentMatchers; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.config.ConfigurableBeanFactory; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.expression.ExpressionParser; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import jakarta.servlet.http.HttpServletRequest; + +@ExtendWith(SpringExtension.class) +@MockitoBean(types = { + ConfigurableBeanFactory.class, + AccessTokenService.class +}) +public class IdentityServiceTest { + + @MockitoBean + CustomerService customerService; + + @MockitoBean + UserService users; + + @MockitoBean + TierService tierService; + + @Autowired + IdentityService service; + + @Test + public void testResolveIdentityAuthenticatedUser() { + var request = mockRequest(); + var userData = mockUserData(); + + Mockito.when(customerService.getCustomerByIpAddress(ArgumentMatchers.anyString())).thenReturn(Optional.empty()); + Mockito.when(tierService.getFreeTier()).thenReturn(Optional.empty()); + Mockito.when(tierService.getSafetyTier()).thenReturn(Optional.empty()); + + var resolvedIdentity = service.resolveIdentity(request); + + assertTrue(resolvedIdentity.cacheKey().startsWith("user_"), "Cache key should start with 'user_'"); + assertEquals("user_" + userData.getAuthId(), resolvedIdentity.cacheKey(), "Cache key should be based on user auth ID"); + } + + private HttpServletRequest mockRequest() { + var request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getParameter("token")).thenReturn(null); + return request; + } + + private UserData mockUserData() { + var userData = new UserData(); + userData.setLoginName("test_user"); + userData.setFullName("Test User"); + userData.setAuthId("test_auth_id"); + userData.setProviderUrl("http://example.com/test"); + Mockito.doReturn(userData).when(users).findLoggedInUser(); + return userData; + } + + @TestConfiguration + static class TestConfig { + + @Bean + public IdentityService identityService( + ExpressionParser expressionParser, + ConfigurableBeanFactory beanFactory, + TierService tierService, + CustomerService customerService, + AccessTokenService tokenService, + RateLimitProperties rateLimitProperties, + UserService userService + ) { + return new IdentityService(expressionParser, beanFactory, tierService, customerService, tokenService, rateLimitProperties, userService); + } + + @Bean + public ExpressionParser expressionParser() { + return new SpelExpressionParser(); + } + + @Bean + public RateLimitProperties rateLimitProperties() { + return new RateLimitProperties(); + } + } +} From 7083155310a90d0b52328d70f1695f9ccad34f4d Mon Sep 17 00:00:00 2001 From: Martin Lowe Date: Tue, 24 Mar 2026 10:28:00 -0400 Subject: [PATCH 2/2] fix: Swap user cachekey to be checked first --- .../openvsx/ratelimit/IdentityService.java | 29 +++++++++---------- .../ratelimit/IdentityServiceTest.java | 8 +---- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java b/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java index bc03f8158..60d18c304 100644 --- a/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java +++ b/server/src/main/java/org/eclipse/openvsx/ratelimit/IdentityService.java @@ -69,24 +69,23 @@ public ResolvedIdentity resolveIdentity(HttpServletRequest request) { String ipAddress = getIPAddress(request); String cacheKey = null; - var token = request.getParameter("token"); - if (token != null) { - // This will update the database with the time the token is last accessed, - // but we need to ensure that we only take valid tokens into account for rate limiting. - // If this turns out to be a bottleneck, we need to cache the token hashcode. - var tokenEntity = tokenService.useAccessToken(token); - if (tokenEntity != null) { - // if a valid token is present we use it as a cache key - cacheKey = "token_" + token.hashCode(); - } + // check first for user session to ensure that users can't extend their rate limit through tokens + var user = userService.findLoggedInUser(); + if (user != null && StringUtils.isNotBlank(user.getAuthId())) { + cacheKey = "user_" + user.getAuthId(); } - // if we don't have a valid token, we check if the user is logged in to generate a cache key - // we want to only do this if we don't have a valid token to avoid unnecessary database calls if (cacheKey == null) { - var user = userService.findLoggedInUser(); - if (user != null && StringUtils.isNotBlank(user.getAuthId())) { - cacheKey = "user_" + user.getAuthId(); + var token = request.getParameter("token"); + if (token != null) { + // This will update the database with the time the token is last accessed, + // but we need to ensure that we only take valid tokens into account for rate limiting. + // If this turns out to be a bottleneck, we need to cache the token hashcode. + var tokenEntity = tokenService.useAccessToken(token); + if (tokenEntity != null) { + // if a valid token is present we use it as a cache key + cacheKey = "token_" + token.hashCode(); + } } } diff --git a/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java b/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java index 1153b7d6f..f252e5745 100644 --- a/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/ratelimit/IdentityServiceTest.java @@ -53,7 +53,7 @@ public class IdentityServiceTest { @Test public void testResolveIdentityAuthenticatedUser() { - var request = mockRequest(); + var request = Mockito.mock(HttpServletRequest.class); var userData = mockUserData(); Mockito.when(customerService.getCustomerByIpAddress(ArgumentMatchers.anyString())).thenReturn(Optional.empty()); @@ -66,12 +66,6 @@ public void testResolveIdentityAuthenticatedUser() { assertEquals("user_" + userData.getAuthId(), resolvedIdentity.cacheKey(), "Cache key should be based on user auth ID"); } - private HttpServletRequest mockRequest() { - var request = Mockito.mock(HttpServletRequest.class); - Mockito.when(request.getParameter("token")).thenReturn(null); - return request; - } - private UserData mockUserData() { var userData = new UserData(); userData.setLoginName("test_user");