Merge "Limit the number of files per change"

2020-01-07 16:13:11 +00:00
parent 5a099946ac 56c74d7d0d
commit c2665bab3f
5 changed files with 126 additions and 33 deletions
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -44,6 +44,11 @@ import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.ValidationError;
 import com.google.gerrit.server.git.validators.ValidationMessage.Type;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.permissions.RefPermission;
@@ -76,7 +81,8 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.SystemReader;

 /**
- * Represents a list of CommitValidationListeners to run for a push to one branch of one project.
+ * Represents a list of {@link CommitValidationListener}s to run for a push to one branch of one
+ * project.
 */
 public class CommitValidators {
  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -94,15 +100,16 @@ public class CommitValidators {
    private final AllProjectsName allProjects;
    private final ExternalIdsConsistencyChecker externalIdsConsistencyChecker;
    private final AccountValidator accountValidator;
-    private final String installCommitMsgHookCommand;
    private final ProjectCache projectCache;
    private final ProjectConfig.Factory projectConfigFactory;
+    private final PatchListCache patchListCache;
+    private final Config config;

    @Inject
    Factory(
        @GerritPersonIdent PersonIdent gerritIdent,
        DynamicItem<UrlFormatter> urlFormatter,
-        @GerritServerConfig Config cfg,
+        @GerritServerConfig Config config,
        PluginSetContext<CommitValidationListener> pluginValidators,
        GitRepositoryManager repoManager,
        AllUsersName allUsers,
@@ -110,19 +117,20 @@ public class CommitValidators {
        ExternalIdsConsistencyChecker externalIdsConsistencyChecker,
        AccountValidator accountValidator,
        ProjectCache projectCache,
-        ProjectConfig.Factory projectConfigFactory) {
+        ProjectConfig.Factory projectConfigFactory,
+        PatchListCache patchListCache) {
      this.gerritIdent = gerritIdent;
      this.urlFormatter = urlFormatter;
+      this.config = config;
      this.pluginValidators = pluginValidators;
      this.repoManager = repoManager;
      this.allUsers = allUsers;
      this.allProjects = allProjects;
      this.externalIdsConsistencyChecker = externalIdsConsistencyChecker;
      this.accountValidator = accountValidator;
-      this.installCommitMsgHookCommand =
-          cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null;
      this.projectCache = projectCache;
      this.projectConfigFactory = projectConfigFactory;
+      this.patchListCache = patchListCache;
    }

    public CommitValidators forReceiveCommits(
@@ -146,12 +154,7 @@ public class CommitValidators {
              new CommitterUploaderValidator(user, perm, urlFormatter.get()),
              new SignedOffByValidator(user, perm, projectState),
              new ChangeIdValidator(
-                  projectState,
-                  user,
-                  urlFormatter.get(),
-                  installCommitMsgHookCommand,
-                  sshInfo,
-                  change),
+                  projectState, user, urlFormatter.get(), config, sshInfo, change),
              new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
              new BannedCommitsValidator(rejectCommits),
              new PluginCommitValidationListener(pluginValidators, skipValidation),
@@ -176,14 +179,10 @@ public class CommitValidators {
              new ProjectStateValidationListener(projectState),
              new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
              new AuthorUploaderValidator(user, perm, urlFormatter.get()),
+              new FileCountValidator(patchListCache, config),
              new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.project())),
              new ChangeIdValidator(
-                  projectState,
-                  user,
-                  urlFormatter.get(),
-                  installCommitMsgHookCommand,
-                  sshInfo,
-                  change),
+                  projectState, user, urlFormatter.get(), config, sshInfo, change),
              new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
              new PluginCommitValidationListener(pluginValidators),
              new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@@ -268,14 +267,14 @@ public class CommitValidators {
        ProjectState projectState,
        IdentifiedUser user,
        UrlFormatter urlFormatter,
-        String installCommitMsgHookCommand,
+        Config config,
        SshInfo sshInfo,
        Change change) {
      this.projectState = projectState;
-      this.urlFormatter = urlFormatter;
-      this.installCommitMsgHookCommand = installCommitMsgHookCommand;
-      this.sshInfo = sshInfo;
      this.user = user;
+      this.urlFormatter = urlFormatter;
+      installCommitMsgHookCommand = config.getString("gerrit", null, "installCommitMsgHookCommand");
+      this.sshInfo = sshInfo;
      this.change = change;
    }

@@ -387,6 +386,40 @@ public class CommitValidators {
    }
  }

+  /** Limits the number of files per change. */
+  private static class FileCountValidator implements CommitValidationListener {
+
+    private final PatchListCache patchListCache;
+    private final int maxFileCount;
+
+    FileCountValidator(PatchListCache patchListCache, Config config) {
+      this.patchListCache = patchListCache;
+      maxFileCount = config.getInt("change", null, "maxFiles", 50_000);
+    }
+
+    @Override
+    public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+        throws CommitValidationException {
+      PatchListKey patchListKey =
+          PatchListKey.againstBase(
+              receiveEvent.commit.getId(), receiveEvent.commit.getParentCount());
+      DiffSummaryKey diffSummaryKey = DiffSummaryKey.fromPatchListKey(patchListKey);
+      try {
+        DiffSummary diffSummary =
+            patchListCache.getDiffSummary(diffSummaryKey, receiveEvent.project.getNameKey());
+        if (diffSummary.getPaths().size() > maxFileCount) {
+          throw new CommitValidationException(
+              String.format(
+                  "Exceeding maximum number of files per change (%d > %d)",
+                  diffSummary.getPaths().size(), maxFileCount));
+        }
+      } catch (PatchListNotAvailableException e) {
+        logger.atWarning().withCause(e).log("Failed to validate file count");
+      }
+      return Collections.emptyList();
+    }
+  }
+
  /** If this is the special project configuration branch, validate the config. */
  public static class ConfigValidator implements CommitValidationListener {
    private final ProjectConfig.Factory projectConfigFactory;
--- a/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
@@ -138,11 +138,10 @@ public class PatchListCacheImpl implements PatchListCache {
      throws PatchListNotAvailableException {
    Project.NameKey project = change.getProject();
    ObjectId b = patchSet.commitId();
-    Whitespace ws = Whitespace.IGNORE_NONE;
    if (parentNum != null) {
-      return get(PatchListKey.againstParentNum(parentNum, b, ws), project);
+      return get(PatchListKey.againstParentNum(parentNum, b, Whitespace.IGNORE_NONE), project);
    }
-    return get(PatchListKey.againstDefaultBase(b, ws), project);
+    return get(PatchListKey.againstDefaultBase(b, Whitespace.IGNORE_NONE), project);
  }

  @Override
--- a/java/com/google/gerrit/server/patch/PatchListKey.java
+++ b/java/com/google/gerrit/server/patch/PatchListKey.java
@@ -59,6 +59,12 @@ public class PatchListKey implements Serializable {
    return new PatchListKey(otherCommitId, newId, whitespace);
  }

+  public static PatchListKey againstBase(ObjectId id, int parentCount) {
+    return parentCount > 1
+        ? PatchListKey.againstParentNum(1, id, Whitespace.IGNORE_NONE)
+        : PatchListKey.againstDefaultBase(id, Whitespace.IGNORE_NONE);
+  }
+
  /**
   * Old patch-set ID
   *
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -45,7 +45,6 @@ import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RefNames;
 import com.google.gerrit.entities.RobotComment;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -396,12 +395,7 @@ public class ChangeData {
        return Optional.empty();
      }

-      ObjectId id = ps.commitId();
-      Whitespace ws = Whitespace.IGNORE_NONE;
-      PatchListKey pk =
-          parentCount > 1
-              ? PatchListKey.againstParentNum(1, id, ws)
-              : PatchListKey.againstDefaultBase(id, ws);
+      PatchListKey pk = PatchListKey.againstBase(ps.commitId(), parentCount);
      DiffSummaryKey key = DiffSummaryKey.fromPatchListKey(pk);
      try {
        diffSummary = Optional.of(patchListCache.getDiffSummary(key, c.getProject()));
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;

+import com.google.common.cache.Cache;
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.SubmitRecord;
@@ -35,10 +36,13 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.Sequences;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gerrit.testing.InMemoryTestEnvironment;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.name.Named;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
@@ -57,8 +61,9 @@ public class BatchUpdateTest {
      new InMemoryTestEnvironment(
          () -> {
            Config cfg = new Config();
-            cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
+            cfg.setInt("change", null, "maxFiles", 2);
            cfg.setInt("change", null, "maxPatchSets", MAX_PATCH_SETS);
+            cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
            return cfg;
          });

@@ -70,6 +75,9 @@ public class BatchUpdateTest {
  @Inject private Provider<CurrentUser> user;
  @Inject private Sequences sequences;

+  @Inject
+  private @Named("diff_summary") Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
+
  private Project.NameKey project;
  private TestRepository<Repository> repo;

@@ -243,6 +251,59 @@ public class BatchUpdateTest {
    assertThat(getMetaId(changeId)).isEqualTo(oldMetaId);
  }

+  @Test
+  public void limitFileCount_exceed() throws Exception {
+    Change.Id changeId = createChangeWithUpdates(1);
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+
+    try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+      ObjectId commitId =
+          repo.amend(notes.getCurrentPatchSet().commitId())
+              .add("bar.txt", "bar")
+              .add("baz.txt", "baz")
+              .add("boom.txt", "boom")
+              .message("blah")
+              .create();
+      bu.addOp(
+          changeId,
+          patchSetInserterFactory
+              .create(notes, PatchSet.id(changeId, 2), commitId)
+              .setMessage("blah"));
+      ResourceConflictException thrown = assertThrows(ResourceConflictException.class, bu::execute);
+      assertThat(thrown)
+          .hasMessageThat()
+          .contains("Exceeding maximum number of files per change (3 > 2)");
+    }
+  }
+
+  @Test
+  public void limitFileCount_cacheKeyMatches() throws Exception {
+    Change.Id changeId = createChangeWithUpdates(1);
+    ChangeNotes notes = changeNotesFactory.create(project, changeId);
+
+    int cacheSizeBefore = diffSummaryCache.asMap().size();
+
+    // We don't want to depend on the test helper used above so we perform an explicit commit here.
+    try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+      ObjectId commitId =
+          repo.amend(notes.getCurrentPatchSet().commitId())
+              .add("bar.txt", "bar")
+              .add("baz.txt", "baz")
+              .message("blah")
+              .create();
+      bu.addOp(
+          changeId,
+          patchSetInserterFactory
+              .create(notes, PatchSet.id(changeId, 3), commitId)
+              .setMessage("blah"));
+      bu.execute();
+    }
+
+    // Assert that we only performed the diff computation once. This would e.g. catch
+    // bugs/deviations in the computation of the cache key.
+    assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1);
+  }
+
  private Change.Id createChangeWithUpdates(int totalUpdates) throws Exception {
    checkArgument(totalUpdates > 0);
    checkArgument(totalUpdates <= MAX_UPDATES);