diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 68396f2971..ebda59f478 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -20,7 +20,9 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; @@ -60,6 +62,10 @@ public abstract class AbstractChangeUpdate { this.serverIdent = serverIdent; this.anonymousCowardName = anonymousCowardName; this.when = when; + checkArgument( + (ctl.getUser() instanceof IdentifiedUser) + || (ctl.getUser() instanceof InternalUser), + "user must be IdentifiedUser or InternalUser: %s", ctl.getUser()); } public ChangeNotes getChangeNotes() { @@ -74,8 +80,8 @@ public abstract class AbstractChangeUpdate { return when; } - public IdentifiedUser getUser() { - return ctl.getUser().asIdentifiedUser(); + public CurrentUser getUser() { + return ctl.getUser(); } public PatchSet.Id getPatchSetId() { @@ -87,6 +93,17 @@ public abstract class AbstractChangeUpdate { this.psId = psId; } + protected PersonIdent newAuthorIdent() { + CurrentUser u = getUser(); + if (u instanceof IdentifiedUser) { + return ChangeNoteUtil.newIdent(u.asIdentifiedUser().getAccount(), when, + serverIdent, anonymousCowardName); + } else if (u instanceof InternalUser) { + return serverIdent; + } + throw new IllegalStateException(); + } + protected PersonIdent newIdent(Account author, Date when) { return ChangeNoteUtil.newIdent(author, when, serverIdent, anonymousCowardName); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index b9e9409cad..eaf8843615 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -234,7 +234,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { protected CommitBuilder applyImpl(ObjectInserter ins) throws OrmException, IOException { CommitBuilder cb = new CommitBuilder(); - cb.setAuthor(newIdent(getUser().getAccount(), when)); + cb.setAuthor(newAuthorIdent()); cb.setCommitter(new PersonIdent(serverIdent, when)); cb.setMessage("Update draft comments"); AtomicBoolean removedAllComments = new AtomicBoolean(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 2eb19686da..e9f6d403f3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -217,7 +217,9 @@ class ChangeNotesParser implements AutoCloseable { } Account.Id accountId = parseIdent(commit); - ownerId = accountId; + if (accountId != null) { + ownerId = accountId; + } if (changeId == null) { changeId = parseChangeId(commit); @@ -339,6 +341,10 @@ class ChangeNotesParser implements AutoCloseable { private void parsePatchSet(PatchSet.Id psId, ObjectId rev, Account.Id accountId, Timestamp ts) throws ConfigInvalidException { + if (accountId == null) { + throw parseException( + "patch set %s requires an identified user as uploader", psId.get()); + } PatchSet ps = patchSets.get(psId); if (ps == null) { ps = new PatchSet(psId); @@ -518,6 +524,10 @@ class ChangeNotesParser implements AutoCloseable { private void parseApproval(PatchSet.Id psId, Account.Id accountId, Timestamp ts, String line) throws ConfigInvalidException { + if (accountId == null) { + throw parseException( + "patch set %s requires an identified user as uploader", psId.get()); + } if (line.startsWith("-")) { parseRemoveApproval(psId, accountId, line); } else { @@ -665,6 +675,14 @@ class ChangeNotesParser implements AutoCloseable { private Account.Id parseIdent(RevCommit commit) throws ConfigInvalidException { + // Check if the author name/email is the same as the committer name/email, + // i.e. was the server ident at the time this commit was made. + PersonIdent a = commit.getAuthorIdent(); + PersonIdent c = commit.getCommitterIdent(); + if (a.getName().equals(c.getName()) + && a.getEmailAddress().equals(c.getEmailAddress())) { + return null; + } return parseIdent(commit.getAuthorIdent()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java index 2fda648f87..8af85a57bc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java @@ -40,7 +40,9 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.StarredChange; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.git.ChainedReceiveCommands; import com.google.gerrit.server.git.GitRepositoryManager; @@ -81,6 +83,7 @@ public class ChangeRebuilder { private final GitRepositoryManager repoManager; private final ChangeControl.GenericFactory controlFactory; private final IdentifiedUser.GenericFactory userFactory; + private final InternalUser.Factory internalUserFactory; private final PatchListCache patchListCache; private final ChangeUpdate.Factory updateFactory; private final ChangeDraftUpdate.Factory draftUpdateFactory; @@ -91,6 +94,7 @@ public class ChangeRebuilder { GitRepositoryManager repoManager, ChangeControl.GenericFactory controlFactory, IdentifiedUser.GenericFactory userFactory, + InternalUser.Factory internalUserFactory, PatchListCache patchListCache, ChangeUpdate.Factory updateFactory, ChangeDraftUpdate.Factory draftUpdateFactory, @@ -99,6 +103,7 @@ public class ChangeRebuilder { this.repoManager = repoManager; this.controlFactory = controlFactory; this.userFactory = userFactory; + this.internalUserFactory = internalUserFactory; this.patchListCache = patchListCache; this.updateFactory = updateFactory; this.draftUpdateFactory = draftUpdateFactory; @@ -172,7 +177,9 @@ public class ChangeRebuilder { if (update != null) { manager.add(update); } - IdentifiedUser user = userFactory.create(Providers.of(db), e.who); + CurrentUser user = e.who != null + ? userFactory.create(Providers.of(db), e.who) + : internalUserFactory.create(); update = updateFactory.create( controlFactory.controlFor(db, change, user), e.when); update.setPatchSetId(e.psId); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 776652d270..17d517495e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -479,7 +479,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { protected CommitBuilder applyImpl(ObjectInserter ins) throws OrmException, IOException { CommitBuilder cb = new CommitBuilder(); - cb.setAuthor(newIdent(getUser().getAccount(), when)); + cb.setAuthor(newAuthorIdent()); cb.setCommitter(new PersonIdent(serverIdent, when)); int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get(); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 96c0c8693c..330d670473 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -29,8 +29,10 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.CapabilityControl; @@ -88,6 +90,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests { protected InMemoryRepository repo; protected InMemoryRepositoryManager repoManager; protected PersonIdent serverIdent; + protected InternalUser internalUser; protected Project.NameKey project; protected RevWalk rw; protected TestRepository tr; @@ -158,6 +161,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests { changeOwner = userFactory.create(co.getId()); otherUser = userFactory.create(ou.getId()); otherUserId = otherUser.getAccountId(); + internalUser = new InternalUser(null); } private void setTimeForTesting() { @@ -180,7 +184,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests { return c; } - protected ChangeUpdate newUpdate(Change c, IdentifiedUser user) + protected ChangeUpdate newUpdate(Change c, CurrentUser user) throws Exception { ChangeUpdate update = TestChanges.newUpdate( injector, repoManager, MIGRATION, c, allUsers, user); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index ff8f0ddfeb..acabfc7c1e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -381,6 +381,39 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { + "Groups: d,e,f\n"); } + @Test + public void parseServerIdent() throws Exception { + String msg = "Update change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n"; + assertParseSucceeds(msg); + assertParseSucceeds(writeCommit(msg, serverIdent)); + + msg = "Update change\n" + + "\n" + + "With a message." + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n"; + assertParseSucceeds(msg); + assertParseSucceeds(writeCommit(msg, serverIdent)); + + msg = "Update change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: refs/heads/master\n" + + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n" + + "Subject: Change subject\n" + + "Label: Label1=+1\n"; + assertParseSucceeds(msg); + assertParseFails(writeCommit(msg, serverIdent)); + } + private RevCommit writeCommit(String body) throws Exception { return writeCommit(body, ChangeNoteUtil.newIdent( changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent, @@ -404,7 +437,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { } private void assertParseSucceeds(String body) throws Exception { - try (ChangeNotesParser parser = newParser(writeCommit(body))) { + assertParseSucceeds(writeCommit(body)); + } + + private void assertParseSucceeds(RevCommit commit) throws Exception { + try (ChangeNotesParser parser = newParser(commit)) { parser.parseAll(); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 02374b245a..9491cd43bd 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -1702,6 +1702,22 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(notes.getComments()).hasSize(2); } + @Test + public void updateWithServerIdent() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, internalUser); + update.setChangeMessage("A message."); + update.commit(); + + ChangeMessage msg = Iterables.getLast(newNotes(c).getChangeMessages()); + assertThat(msg.getMessage()).isEqualTo("A message."); + assertThat(msg.getAuthor()).isNull(); + + update = newUpdate(c, internalUser); + exception.expect(UnsupportedOperationException.class); + update.putApproval("Code-Review", (short) 1); + } + private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception { ObjectId dataId = notes.getNoteMap().getNote(noteId).getData(); return new String( diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java index 1f7ea7a94e..88ab5514f3 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java @@ -27,7 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeDraftUpdate; @@ -88,14 +88,14 @@ public class TestChanges { public static ChangeUpdate newUpdate(Injector injector, GitRepositoryManager repoManager, NotesMigration migration, Change c, - final AllUsersName allUsers, final IdentifiedUser user) + final AllUsersName allUsers, final CurrentUser user) throws Exception { ChangeUpdate update = injector.createChildInjector(new FactoryModule() { @Override public void configure() { factory(ChangeUpdate.Factory.class); factory(ChangeDraftUpdate.Factory.class); - bind(IdentifiedUser.class).toInstance(user); + bind(CurrentUser.class).toInstance(user); } }).getInstance(ChangeUpdate.Factory.class).create( stubChangeControl(repoManager, migration, c, allUsers, user), @@ -112,8 +112,8 @@ public class TestChanges { // first patch set, so create one. try (Repository repo = repoManager.openRepository(c.getProject())) { TestRepository tr = new TestRepository<>(repo); - PersonIdent ident = - user.newCommitterIdent(update.getWhen(), TimeZone.getDefault()); + PersonIdent ident = user.asIdentifiedUser() + .newCommitterIdent(update.getWhen(), TimeZone.getDefault()); TestRepository.CommitBuilder cb = tr.commit() .author(ident) .committer(ident) @@ -132,7 +132,7 @@ public class TestChanges { private static ChangeControl stubChangeControl( GitRepositoryManager repoManager, NotesMigration migration, Change c, AllUsersName allUsers, - IdentifiedUser user) throws OrmException { + CurrentUser user) throws OrmException { ChangeControl ctl = EasyMock.createMock(ChangeControl.class); expect(ctl.getChange()).andStubReturn(c); expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));