Exclude empty limits when finding the user's matched rates
When a user belongs to more than one group having different
limits on different types (e.g. RESTAPI and UPLOADPACK),
the search for the first matching limit was considering the
first group ownership rather than the first group for that type.
In the example below, the search for the first matching limit
would have always returned the first group for both uploadpack
and restapi types, causing the user belonging to both groups
not being limited on restapi (2nd match) but only on uploadpack.
Example:
[group "groupLimitedOnUploadPack"]
uploadpack = 10s burst 20
[group "groupLimitedOnRestApi"]
restapi = 20s burst 40
When looking for the first matching maximum rate for a user,
do not consider the empty limits as they would hinder the ability
to find the correct configured value and keep on searching for the
first non-empty limit with a group that the current user belongs to.
Add a specific validation test on the integration test suite
for this specific quota configuration, asserting that the correct
quota is read and applied to the user.
Also make sure that the admin user in the tests, that does not belong
to any of the rate-limited groups, is not impacted and keeps on being
unlimited.
Change-Id: If82b8265b6be2d0ac8454a1a1029767b55aeff43
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
index a9b8fde..87413af 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
@@ -63,7 +63,10 @@
if (!maybeInternalGroup.isPresent()) {
log.debug("Ignoring limits for non-internal group ''{}'' in quota.config", groupName);
} else if (memberShip.contains(maybeInternalGroup.get().getGroupUUID())) {
- return Optional.ofNullable(limits.get().get(groupName));
+ RateLimit groupLimit = limits.get().get(groupName);
+ if (groupLimit != null) {
+ return Optional.of(groupLimit);
+ }
}
} catch (ResourceNotFoundException e) {
log.debug("Ignoring limits for unknown group ''{}'' in quota.config", groupName);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/quota/QuotaPluginIT.java b/src/test/java/com/googlesource/gerrit/plugins/quota/QuotaPluginIT.java
index 3e7bc31..3762cee 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/quota/QuotaPluginIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/quota/QuotaPluginIT.java
@@ -14,13 +14,139 @@
package com.googlesource.gerrit.plugins.quota;
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.groups.GroupApi;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.inject.Inject;
+import com.googlesource.gerrit.plugins.quota.AccountLimitsConfig.Type;
+import java.util.Optional;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Ref;
+import org.junit.Before;
import org.junit.Test;
@TestPlugin(name = "quota", sysModule = "com.googlesource.gerrit.plugins.quota.Module")
public class QuotaPluginIT extends LightweightPluginDaemonTest {
+ private static final String GROUP_LIMITED_ON_UPLOADPACK = "groupLimitedOnUploadPack";
+ private static final String GROUP_LIMITED_ON_RESTAPI = "groupLimitedOnRestApi";
+ private static final int UPLOADPACK_LIMIT = 100;
+ private static final int UPLOADPACK_BURST = 1;
+ private static final int RESTAPI_LIMIT = 200;
+ private static final int RESTAPI_BURST = 2;
+
+ @Inject private IdentifiedUser.GenericFactory userFactory;
+
+ @Before
+ public void setup() throws Exception {
+ addUserToGroup(user, GROUP_LIMITED_ON_UPLOADPACK);
+ addUserToGroup(user, GROUP_LIMITED_ON_RESTAPI);
+
+ try (TestRepository<InMemoryRepository> allProjectsRepo = cloneProject(allProjects)) {
+ GitUtil.fetch(allProjectsRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ Ref configRef = allProjectsRepo.getRepository().exactRef(RefNames.REFS_CONFIG);
+ allProjectsRepo.reset(configRef.getObjectId());
+ pushFactory
+ .create(
+ admin.newIdent(),
+ allProjectsRepo,
+ "Set quota",
+ "quota.config",
+ "[group \""
+ + GROUP_LIMITED_ON_UPLOADPACK
+ + "\"]\n"
+ + "uploadpack = "
+ + UPLOADPACK_LIMIT
+ + "/s burst "
+ + UPLOADPACK_BURST * UPLOADPACK_LIMIT
+ + "\n"
+ + "[group \""
+ + GROUP_LIMITED_ON_RESTAPI
+ + "\"]\n"
+ + "restapi = "
+ + RESTAPI_LIMIT
+ + "/s burst "
+ + RESTAPI_BURST * RESTAPI_LIMIT
+ + "\n")
+ .to(RefNames.REFS_CONFIG)
+ .assertOkStatus();
+ }
+ }
@Test
public void pluginCanLoad() throws Exception {}
+
+ @Test
+ public void userShouldBeLimitedOnUploadPacks() {
+ asserUserLimitedOn(
+ Type.UPLOADPACK, GROUP_LIMITED_ON_UPLOADPACK, UPLOADPACK_LIMIT, UPLOADPACK_BURST);
+ }
+
+ @Test
+ public void userShouldBeLimitedOnRestApi() {
+ asserUserLimitedOn(Type.RESTAPI, GROUP_LIMITED_ON_RESTAPI, RESTAPI_LIMIT, RESTAPI_BURST);
+ }
+
+ @Test
+ public void adminShouldNotBeLimitedOnRestApi() {
+ assertAdminUserHasNoLimitsOn(Type.RESTAPI);
+ }
+
+ @Test
+ public void adminShouldNotBeLimitedOnUploadPack() {
+ assertAdminUserHasNoLimitsOn(Type.UPLOADPACK);
+ }
+
+ private void assertAdminUserHasNoLimitsOn(Type type) {
+ AccountLimitsFinder accountLimitsFinder = getAccountLimitsFinder();
+ assertThat(accountLimitsFinder.getRateLimit(type, "Administrators")).isEmpty();
+ assertThat(accountLimitsFinder.firstMatching(type, userFactory.create(admin.id()))).isEmpty();
+ }
+
+ private void asserUserLimitedOn(
+ Type limitType, String groupName, int ratePerSecLimit, int burstSecs) {
+ AccountLimitsFinder accountLimitsFinder = getAccountLimitsFinder();
+ Optional<AccountLimitsConfig.RateLimit> groupLimit =
+ accountLimitsFinder.getRateLimit(limitType, groupName);
+ assertThat(groupLimit).isPresent();
+ assertRateLimits(groupLimit.get(), limitType, ratePerSecLimit, burstSecs);
+
+ Optional<AccountLimitsConfig.RateLimit> rateLimit =
+ accountLimitsFinder.firstMatching(limitType, userFactory.create(user.id()));
+ assertThat(rateLimit).isPresent();
+
+ assertRateLimitsAreEqual(rateLimit.get(), groupLimit.get());
+ }
+
+ private void addUserToGroup(TestAccount user, String groupLimitedOnUploadpack)
+ throws RestApiException {
+ GroupApi groupApi = gApi.groups().create(groupLimitedOnUploadpack);
+ groupApi.removeMembers(admin.username());
+ groupApi.addMembers(user.username());
+ }
+
+ private AccountLimitsFinder getAccountLimitsFinder() {
+ return plugin.getSysInjector().getInstance(AccountLimitsFinder.class);
+ }
+
+ private static void assertRateLimitsAreEqual(
+ AccountLimitsConfig.RateLimit rateLimit, AccountLimitsConfig.RateLimit groupLimit) {
+ assertThat(rateLimit.getRatePerSecond()).isEqualTo(groupLimit.getRatePerSecond());
+ assertThat(rateLimit.getMaxBurstSeconds()).isEqualTo(groupLimit.getMaxBurstSeconds());
+ assertThat(rateLimit.getType()).isEqualTo(groupLimit.getType());
+ }
+
+ private static void assertRateLimits(
+ AccountLimitsConfig.RateLimit rateLimit, Type rateType, int ratePerSecond, int burstSeconds) {
+ assertThat(rateLimit.getRatePerSecond()).isEqualTo(ratePerSecond);
+ assertThat(rateLimit.getMaxBurstSeconds()).isEqualTo(burstSeconds);
+ assertThat(rateLimit.getType()).isEqualTo(rateType);
+ }
}
