Merge "Reject push with invalid project watch config"
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 1ad00aa..93525a4 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -59,7 +59,9 @@
import com.google.gerrit.gpg.testutil.TestKey;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountExternalId;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.WatchConfig;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.project.RefPattern;
@@ -90,6 +92,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
+import java.util.EnumSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
@@ -483,6 +486,40 @@
}
@Test
+ public void pushWatchConfigToUserBranch() throws Exception {
+ // change something in the user preferences to ensure that the user branch
+ // is created
+ GeneralPreferencesInfo input = new GeneralPreferencesInfo();
+ input.changesPerPage =
+ GeneralPreferencesInfo.defaults().changesPerPage + 10;
+ gApi.accounts().self().setPreferences(input);
+
+ TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
+ fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef");
+ allUsersRepo.reset("userRef");
+
+ Config wc = new Config();
+ wc.setString(WatchConfig.PROJECT, project.get(), WatchConfig.KEY_NOTIFY,
+ WatchConfig.NotifyValue
+ .create(null, EnumSet.of(NotifyType.ALL_COMMENTS)).toString());
+ PushOneCommit push = pushFactory.create(db, admin.getIdent(), allUsersRepo,
+ "Add project watch", WatchConfig.WATCH_CONFIG, wc.toText());
+ push.to(RefNames.REFS_USERS_SELF).assertOkStatus();
+
+ String invalidNotifyValue = "]invalid[";
+ wc.setString(WatchConfig.PROJECT, project.get(), WatchConfig.KEY_NOTIFY,
+ invalidNotifyValue);
+ push = pushFactory.create(db, admin.getIdent(), allUsersRepo,
+ "Add invalid project watch", WatchConfig.WATCH_CONFIG, wc.toText());
+ PushOneCommit.Result r = push.to(RefNames.REFS_USERS_SELF);
+ r.assertErrorStatus("invalid watch configuration");
+ r.assertMessage(String.format(
+ "%s: Invalid project watch of account %d for project %s: %s",
+ WatchConfig.WATCH_CONFIG, admin.getId().get(), project.get(),
+ invalidNotifyValue));
+ }
+
+ @Test
public void addGpgKey() throws Exception {
TestKey key = validKeyWithoutExpiration();
String id = key.getKeyIdString();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java
index eae3349..8e783ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/WatchConfig.java
@@ -25,6 +25,7 @@
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
@@ -38,6 +39,7 @@
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.git.VersionedMetaData;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -89,7 +91,8 @@
* <p>
* Unknown notify types are ignored and removed on save.
*/
-public class WatchConfig extends VersionedMetaData implements AutoCloseable {
+public class WatchConfig extends VersionedMetaData
+ implements AutoCloseable, ValidationError.Sink {
@Singleton
public static class Accessor {
private final GitRepositoryManager repoManager;
@@ -176,15 +179,16 @@
public abstract @Nullable String filter();
}
- private static final String WATCH_CONFIG = "watch.config";
- private static final String PROJECT = "project";
- private static final String KEY_NOTIFY = "notify";
+ public static final String WATCH_CONFIG = "watch.config";
+ public static final String PROJECT = "project";
+ public static final String KEY_NOTIFY = "notify";
private final Account.Id accountId;
private final String ref;
private Repository git;
private Map<ProjectWatchKey, Set<NotifyType>> projectWatches;
+ private List<ValidationError> validationErrors;
public WatchConfig(Account.Id accountId) {
this.accountId = accountId;
@@ -206,12 +210,13 @@
@Override
protected void onLoad() throws IOException, ConfigInvalidException {
Config cfg = readConfig(WATCH_CONFIG);
- projectWatches = parse(accountId, cfg);
+ projectWatches = parse(accountId, cfg, this);
}
@VisibleForTesting
public static Map<ProjectWatchKey, Set<NotifyType>> parse(
- Account.Id accountId, Config cfg) throws ConfigInvalidException {
+ Account.Id accountId, Config cfg,
+ ValidationError.Sink validationErrorSink) {
Map<ProjectWatchKey, Set<NotifyType>> projectWatches = new HashMap<>();
for (String projectName : cfg.getSubsections(PROJECT)) {
String[] notifyValues =
@@ -221,7 +226,12 @@
continue;
}
- NotifyValue notifyValue = NotifyValue.parse(accountId, projectName, nv);
+ NotifyValue notifyValue =
+ NotifyValue.parse(accountId, projectName, nv, validationErrorSink);
+ if (notifyValue == null) {
+ continue;
+ }
+
ProjectWatchKey key = ProjectWatchKey
.create(new Project.NameKey(projectName), notifyValue.filter());
if (!projectWatches.containsKey(key)) {
@@ -283,16 +293,38 @@
checkState(projectWatches != null, "project watches not loaded yet");
}
+ @Override
+ public void error(ValidationError error) {
+ if (validationErrors == null) {
+ validationErrors = new ArrayList<>(4);
+ }
+ validationErrors.add(error);
+ }
+
+ /**
+ * Get the validation errors, if any were discovered during load.
+ *
+ * @return list of errors; empty list if there are no errors.
+ */
+ public List<ValidationError> getValidationErrors() {
+ if (validationErrors != null) {
+ return ImmutableList.copyOf(validationErrors);
+ }
+ return ImmutableList.of();
+ }
+
@AutoValue
- abstract static class NotifyValue {
+ public abstract static class NotifyValue {
public static NotifyValue parse(Account.Id accountId, String project,
- String notifyValue) throws ConfigInvalidException {
+ String notifyValue, ValidationError.Sink validationErrorSink) {
notifyValue = notifyValue.trim();
int i = notifyValue.lastIndexOf('[');
if (i < 0 || notifyValue.charAt(notifyValue.length() - 1) != ']') {
- throw new ConfigInvalidException(String.format(
- "Invalid project watch of account %d for project %s: %s",
- accountId.get(), project, notifyValue));
+ validationErrorSink.error(new ValidationError(WATCH_CONFIG,
+ String.format(
+ "Invalid project watch of account %d for project %s: %s",
+ accountId.get(), project, notifyValue)));
+ return null;
}
String filter = notifyValue.substring(0, i).trim();
if (filter.isEmpty() || AccountProjectWatch.FILTER_ALL.equals(filter)) {
@@ -306,10 +338,12 @@
Optional<NotifyType> notifyType =
Enums.getIfPresent(NotifyType.class, nt);
if (!notifyType.isPresent()) {
- throw new ConfigInvalidException(String.format(
- "Invalid notify type %s in project watch "
- + "of account %d for project %s: %s",
- nt, accountId.get(), project, notifyValue));
+ validationErrorSink.error(new ValidationError(WATCH_CONFIG,
+ String.format(
+ "Invalid notify type %s in project watch "
+ + "of account %d for project %s: %s",
+ nt, accountId.get(), project, notifyValue)));
+ continue;
}
notifyTypes.add(notifyType.get());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
index ad2003e..e7b19aa 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -22,8 +22,12 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.WatchConfig;
+import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.events.CommitReceivedEvent;
@@ -86,22 +90,26 @@
private final SshInfo sshInfo;
private final Repository repo;
private final DynamicSet<CommitValidationListener> commitValidationListeners;
+ private final AllUsersName allUsers;
@Inject
- CommitValidators(@GerritPersonIdent final PersonIdent gerritIdent,
- @CanonicalWebUrl @Nullable final String canonicalWebUrl,
- @GerritServerConfig final Config config,
- final DynamicSet<CommitValidationListener> commitValidationListeners,
- @Assisted final SshInfo sshInfo,
- @Assisted final Repository repo, @Assisted final RefControl refControl) {
+ CommitValidators(@GerritPersonIdent PersonIdent gerritIdent,
+ @CanonicalWebUrl @Nullable String canonicalWebUrl,
+ @GerritServerConfig Config config,
+ DynamicSet<CommitValidationListener> commitValidationListeners,
+ AllUsersName allUsers,
+ @Assisted SshInfo sshInfo,
+ @Assisted Repository repo,
+ @Assisted RefControl refControl) {
this.gerritIdent = gerritIdent;
- this.refControl = refControl;
this.canonicalWebUrl = canonicalWebUrl;
this.installCommitMsgHookCommand =
config.getString("gerrit", null, "installCommitMsgHookCommand");
+ this.commitValidationListeners = commitValidationListeners;
+ this.allUsers = allUsers;
this.sshInfo = sshInfo;
this.repo = repo;
- this.commitValidationListeners = commitValidationListeners;
+ this.refControl = refControl;
}
public List<CommitValidationMessage> validateForReceiveCommits(
@@ -122,7 +130,7 @@
validators.add(new ChangeIdValidator(refControl, canonicalWebUrl,
installCommitMsgHookCommand, sshInfo));
}
- validators.add(new ConfigValidator(refControl, repo));
+ validators.add(new ConfigValidator(refControl, repo, allUsers));
validators.add(new BannedCommitsValidator(rejectCommits));
validators.add(new PluginCommitValidationListener(commitValidationListeners));
@@ -156,7 +164,7 @@
validators.add(new ChangeIdValidator(refControl, canonicalWebUrl,
installCommitMsgHookCommand, sshInfo));
}
- validators.add(new ConfigValidator(refControl, repo));
+ validators.add(new ConfigValidator(refControl, repo, allUsers));
validators.add(new PluginCommitValidationListener(commitValidationListeners));
List<CommitValidationMessage> messages = new LinkedList<>();
@@ -321,10 +329,13 @@
public static class ConfigValidator implements CommitValidationListener {
private final RefControl refControl;
private final Repository repo;
+ private final AllUsersName allUsers;
- public ConfigValidator(RefControl refControl, Repository repo) {
+ public ConfigValidator(RefControl refControl, Repository repo,
+ AllUsersName allUsers) {
this.refControl = refControl;
this.repo = repo;
+ this.allUsers = allUsers;
}
@Override
@@ -355,6 +366,35 @@
messages);
}
}
+
+ if (allUsers.equals(
+ refControl.getProjectControl().getProject().getNameKey())
+ && RefNames.isRefsUsers(refControl.getRefName())) {
+ List<CommitValidationMessage> messages = new LinkedList<>();
+ Account.Id accountId = Account.Id.fromRef(refControl.getRefName());
+ if (accountId != null) {
+ try {
+ @SuppressWarnings("resource")
+ WatchConfig wc = new WatchConfig(accountId);
+ wc.load(repo, receiveEvent.command.getNewId());
+ if (!wc.getValidationErrors().isEmpty()) {
+ addError("Invalid project configuration:", messages);
+ for (ValidationError err : wc.getValidationErrors()) {
+ addError(" " + err.getMessage(), messages);
+ }
+ throw new ConfigInvalidException("invalid watch configuration");
+ }
+ } catch (IOException | ConfigInvalidException e) {
+ log.error("User " + currentUser.getUserName()
+ + " tried to push an invalid watch configuration "
+ + receiveEvent.command.getNewId().name() + " for account "
+ + accountId.get(), e);
+ throw new CommitValidationException("invalid watch configuration",
+ messages);
+ }
+ }
+ }
+
return Collections.emptyList();
}
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java
index 0133ec2..0619a78 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/account/WatchConfigTest.java
@@ -15,28 +15,32 @@
package com.google.gerrit.server.account;
import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.account.WatchConfig.NotifyValue;
import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
+import com.google.gerrit.server.git.ValidationError;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
-import org.junit.Rule;
+import org.junit.Before;
import org.junit.Test;
-import org.junit.rules.ExpectedException;
+import java.util.ArrayList;
import java.util.EnumSet;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Set;
-public class WatchConfigTest {
- @Rule
- public ExpectedException exception = ExpectedException.none();
+public class WatchConfigTest implements ValidationError.Sink {
+ private List<ValidationError> validationErrors = new ArrayList<>();
+
+ @Before
+ public void setup() {
+ validationErrors.clear();
+ }
@Test
public void parseWatchConfig() throws Exception {
@@ -50,7 +54,9 @@
+ " notify = [NEW_PATCHSETS]\n"
+ " notify = * [NEW_PATCHSETS, ALL_COMMENTS]\n");
Map<ProjectWatchKey, Set<NotifyType>> projectWatches =
- WatchConfig.parse(new Account.Id(1000000), cfg);
+ WatchConfig.parse(new Account.Id(1000000), cfg, this);
+
+ assertThat(validationErrors).isEmpty();
Project.NameKey myProject = new Project.NameKey("myProject");
Project.NameKey otherProject = new Project.NameKey("otherProject");
@@ -79,11 +85,12 @@
+ "[project \"otherProject\"]\n"
+ " notify = [NEW_PATCHSETS]\n");
- exception.expect(ConfigInvalidException.class);
- exception.expectMessage(
- "Invalid notify type INVALID in project watch of account 1000000"
- + " for project myProject: branch:master [INVALID, NEW_CHANGES]");
- WatchConfig.parse(new Account.Id(1000000), cfg);
+ WatchConfig.parse(new Account.Id(1000000), cfg, this);
+ assertThat(validationErrors).hasSize(1);
+ assertThat(validationErrors.get(0).getMessage()).isEqualTo(
+ "watch.config: Invalid notify type INVALID in project watch of"
+ + " account 1000000 for project myProject: branch:master"
+ + " [INVALID, NEW_CHANGES]");
}
@Test
@@ -104,6 +111,8 @@
"branch:master",
EnumSet.of(NotifyType.ALL_COMMENTS, NotifyType.NEW_PATCHSETS));
assertParseNotifyValue("* [ALL]", null, EnumSet.of(NotifyType.ALL));
+
+ assertThat(validationErrors).isEmpty();
}
@Test
@@ -136,9 +145,8 @@
assertToNotifyValue("*", EnumSet.of(NotifyType.ALL), "* [ALL]");
}
- private static void assertParseNotifyValue(String notifyValue,
- String expectedFilter, Set<NotifyType> expectedNotifyTypes)
- throws ConfigInvalidException {
+ private void assertParseNotifyValue(String notifyValue,
+ String expectedFilter, Set<NotifyType> expectedNotifyTypes) {
NotifyValue nv = parseNotifyValue(notifyValue);
assertThat(nv.filter()).isEqualTo(expectedFilter);
assertThat(nv.notifyTypes()).containsExactlyElementsIn(expectedNotifyTypes);
@@ -150,17 +158,21 @@
assertThat(nv.toString()).isEqualTo(expectedNotifyValue);
}
- private static void assertParseNotifyValueFails(String notifyValue) {
- try {
- parseNotifyValue(notifyValue);
- fail("expected ConfigInvalidException for notifyValue: " + notifyValue);
- } catch (ConfigInvalidException e) {
- // Expected.
- }
+ private void assertParseNotifyValueFails(String notifyValue) {
+ assertThat(validationErrors).isEmpty();
+ parseNotifyValue(notifyValue);
+ assertThat(validationErrors)
+ .named("expected validation error for notifyValue: " + notifyValue)
+ .isNotEmpty();
+ validationErrors.clear();
}
- private static NotifyValue parseNotifyValue(String notifyValue)
- throws ConfigInvalidException {
- return NotifyValue.parse(new Account.Id(1000000), "project", notifyValue);
+ private NotifyValue parseNotifyValue(String notifyValue) {
+ return NotifyValue.parse(new Account.Id(1000000), "project", notifyValue, this);
+ }
+
+ @Override
+ public void error(ValidationError error) {
+ validationErrors.add(error);
}
}
