Prevent pushing of patch sets onto invisible patch sets

A previous bug would allow users to push a patch set onto a draft patch
set they cannot see. This commit fixes the bug and adds some tests.

Change-Id: I58a8353f67a47361f6dbd3bf776969130d90a529

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -520,10 +520,16 @@ public abstract class AbstractDaemonTest {
 
   protected PushOneCommit.Result amendChange(String changeId, String ref)
       throws Exception {
+    return amendChange(changeId, ref, admin, testRepo);
+  }
+
+  protected PushOneCommit.Result amendChange(String changeId, String ref,
+      TestAccount testAccount, TestRepository<?> repo) throws Exception {
     Collections.shuffle(RANDOM);
     PushOneCommit push =
-        pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
+        pushFactory.create(db, testAccount.getIdent(), repo,
-            PushOneCommit.FILE_NAME, new String(Chars.toArray(RANDOM)), changeId);
+            PushOneCommit.SUBJECT, PushOneCommit.FILE_NAME,
+            new String(Chars.toArray(RANDOM)), changeId);
     return push.to(ref);
   }
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
+import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestProjectInput;
@@ -64,6 +65,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 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.change.ChangeResource;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
 import com.google.gerrit.server.git.ProjectConfig;
@@ -74,6 +76,8 @@ import com.google.gerrit.testutil.FakeEmailSender.Message;
 import com.google.gerrit.testutil.NoteDbMode;
 import com.google.gerrit.testutil.TestTimeUtil;
 
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
@@ -1244,6 +1248,71 @@ public class ChangeIT extends AbstractDaemonTest {
         .get();
   }
 
+  @Test
+  public void createNewPatchSetOnVisibleDraftPatchSet() throws Exception {
+    // Clone separate repositories of the same project as admin and as user
+    TestRepository<InMemoryRepository> adminTestRepo =
+        cloneProject(project, admin);
+    TestRepository<InMemoryRepository> userTestRepo =
+        cloneProject(project, user);
+
+    // Create change as admin
+    PushOneCommit push = pushFactory.create(
+        db, admin.getIdent(), adminTestRepo);
+    PushOneCommit.Result r1 = push.to("refs/for/master");
+    r1.assertOkStatus();
+
+    // Amend draft as admin
+    PushOneCommit.Result r2 = amendChange(
+        r1.getChangeId(), "refs/drafts/master", admin, adminTestRepo);
+    r2.assertOkStatus();
+
+    // Add user as reviewer to make this patch set visible
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    gApi.changes()
+        .id(r1.getChangeId())
+        .addReviewer(in);
+
+    // Fetch change
+    GitUtil.fetch(userTestRepo, r2.getPatchSet().getRefName() + ":ps");
+    userTestRepo.reset("ps");
+
+    // Amend change as user
+    PushOneCommit.Result r3 = amendChange(
+        r2.getChangeId(), "refs/drafts/master", user, userTestRepo);
+    r3.assertOkStatus();
+  }
+
+  @Test
+  public void createNewPatchSetOnInvisibleDraftPatchSet() throws Exception {
+    // Clone separate repositories of the same project as admin and as user
+    TestRepository<InMemoryRepository> adminTestRepo =
+        cloneProject(project, admin);
+    TestRepository<InMemoryRepository> userTestRepo =
+        cloneProject(project, user);
+
+    // Create change as admin
+    PushOneCommit push = pushFactory.create(
+        db, admin.getIdent(), adminTestRepo);
+    PushOneCommit.Result r1 = push.to("refs/for/master");
+    r1.assertOkStatus();
+    // Amend draft as admin
+    PushOneCommit.Result r2 = amendChange(
+        r1.getChangeId(), "refs/drafts/master", admin, adminTestRepo);
+    r2.assertOkStatus();
+
+    // Fetch change
+    GitUtil.fetch(userTestRepo, r1.getPatchSet().getRefName() + ":ps");
+    userTestRepo.reset("ps");
+
+    // Amend change as user
+    PushOneCommit.Result r3 = amendChange(
+        r1.getChangeId(), "refs/for/master", user, userTestRepo);
+    r3.assertErrorStatus("cannot replace "
+        + r3.getChange().change().getChangeId() + ".");
+  }
+
   private static Iterable<Account.Id> getReviewers(
       Collection<AccountInfo> r) {
     return Iterables.transform(r, new Function<AccountInfo, Account.Id>() {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -438,7 +438,8 @@ public class ReceiveCommits {
     rp.setRefFilter(new RefFilter() {
       @Override
       public Map<String, Ref> filter(Map<String, Ref> refs) {
-        Map<String, Ref> filteredRefs = Maps.newHashMapWithExpectedSize(refs.size());
+        Map<String, Ref> filteredRefs =
+            Maps.newHashMapWithExpectedSize(refs.size());
         for (Map.Entry<String, Ref> e : refs.entrySet()) {
           String name = e.getKey();
           if (!name.startsWith(REFS_CHANGES)
@@ -468,7 +469,8 @@ public class ReceiveCommits {
           } catch (ServiceMayNotContinueException e) {
             throw e;
           } catch (IOException e) {
-            ServiceMayNotContinueException ex = new ServiceMayNotContinueException();
+            ServiceMayNotContinueException ex =
+                new ServiceMayNotContinueException();
             ex.initCause(e);
             throw ex;
           }
@@ -698,7 +700,7 @@ public class ReceiveCommits {
             new Function<ReplaceRequest, Integer>() {
               @Override
               public Integer apply(ReplaceRequest in) {
-                return in.change.getId().get();
+                return in.notes.getChangeId().get();
               }
             }));
     if (!updated.isEmpty()) {
@@ -706,7 +708,7 @@ public class ReceiveCommits {
       addMessage("Updated Changes:");
       boolean edit = magicBranch != null && magicBranch.edit;
       for (ReplaceRequest u : updated) {
-        addMessage(formatChangeUrl(canonicalWebUrl, u.change,
+        addMessage(formatChangeUrl(canonicalWebUrl, u.notes.getChange(),
             u.info.getSubject(), edit));
       }
       addMessage("");
@@ -1832,7 +1834,7 @@ public class ReceiveCommits {
       bySha.put(r.commitId, r.change);
     }
     for (ReplaceRequest r : replace) {
-      bySha.put(r.newCommitId, r.change);
+      bySha.put(r.newCommitId, r.notes.getChange());
     }
     Change tipChange = bySha.get(magicBranch.cmd.getNewId());
     checkState(tipChange != null,
@@ -1908,7 +1910,7 @@ public class ReceiveCommits {
     for (CheckedFuture<ChangeNotes, OrmException> f : futures) {
       ChangeNotes notes = f.checkedGet();
       if (notes.getChange() != null) {
-        replaceByChange.get(notes.getChangeId()).change = notes.getChange();
+        replaceByChange.get(notes.getChangeId()).notes = notes;
       }
     }
   }
@@ -1918,7 +1920,7 @@ public class ReceiveCommits {
     final ObjectId newCommitId;
     final ReceiveCommand inputCommand;
     final boolean checkMergedInto;
-    Change change;
+    ChangeNotes notes;
     ChangeControl changeCtl;
     BiMap<RevCommit, PatchSet.Id> revisions;
     PatchSet.Id psId;
@@ -1954,12 +1956,12 @@ public class ReceiveCommits {
     boolean validate(boolean autoClose) throws IOException, OrmException {
       if (!autoClose && inputCommand.getResult() != NOT_ATTEMPTED) {
         return false;
-      } else if (change == null) {
+      } else if (notes == null) {
         reject(inputCommand, "change " + ontoChange + " not found");
         return false;
       }
 
-      priorPatchSet = change.currentPatchSetId();
+      priorPatchSet = notes.getChange().currentPatchSetId();
       if (!revisions.containsValue(priorPatchSet)) {
         reject(inputCommand, "change " + ontoChange + " missing revisions");
         return false;
@@ -1974,7 +1976,7 @@ public class ReceiveCommits {
         return false;
       }
 
-      changeCtl = projectControl.controlFor(db, change);
+      changeCtl = projectControl.controlFor(notes);
       if (!changeCtl.canAddPatchSet(db)) {
         String locked = ".";
         if (changeCtl.isPatchSetLocked(db)) {
@@ -1982,7 +1984,7 @@ public class ReceiveCommits {
         }
         reject(inputCommand, "cannot replace " + ontoChange + locked);
         return false;
-      } else if (change.getStatus().isClosed()) {
+      } else if (notes.getChange().getStatus().isClosed()) {
         reject(inputCommand, "change " + ontoChange + " closed");
         return false;
       } else if (revisions.containsKey(newCommit)) {
@@ -2057,7 +2059,7 @@ public class ReceiveCommits {
     }
 
     private boolean newEdit() {
-      psId = change.currentPatchSetId();
+      psId = notes.getChange().currentPatchSetId();
       Optional<ChangeEdit> edit = null;
 
       try {
@@ -2096,13 +2098,14 @@ public class ReceiveCommits {
           newCommitId,
           RefNames.refsEdit(
               user.getAccountId(),
-              change.getId(),
+              notes.getChangeId(),
               psId));
     }
 
     private void newPatchSet() throws IOException {
       RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
-      psId = ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId());
+      psId = ChangeUtil.nextPatchSetId(
+          allRefs, notes.getChange().currentPatchSetId());
       info = patchSetInfoFactory.get(
           rp.getRevWalk(), newCommit, psId);
       cmd = new ReceiveCommand(
@@ -2128,12 +2131,12 @@ public class ReceiveCommits {
 
       RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
       replaceOp = replaceOpFactory.create(requestScopePropagator,
-          projectControl, change.getDest(), checkMergedInto, priorPatchSet,
+          projectControl, notes.getChange().getDest(), checkMergedInto,
-          priorCommit, psId, newCommit, info, groups, magicBranch,
+          priorPatchSet, priorCommit, psId, newCommit, info, groups,
-          rp.getPushCertificate());
+          magicBranch, rp.getPushCertificate());
-      bu.addOp(change.getId(), replaceOp);
+      bu.addOp(notes.getChangeId(), replaceOp);
       if (progress != null) {
-        bu.addOp(change.getId(), new ChangeProgressOp(progress));
+        bu.addOp(notes.getChangeId(), new ChangeProgressOp(progress));
       }
     }
 
@@ -2270,7 +2273,8 @@ public class ReceiveCommits {
       return;
     }
 
-    boolean defaultName = Strings.isNullOrEmpty(user.getAccount().getFullName());
+    boolean defaultName =
+        Strings.isNullOrEmpty(user.getAccount().getFullName());
     RevWalk walk = rp.getRevWalk();
     walk.reset();
     walk.sort(RevSort.NONE);
@@ -2363,7 +2367,7 @@ public class ReceiveCommits {
       }
 
       SetMultimap<ObjectId, Ref> byCommit = changeRefsById();
-      Map<Change.Key, Change> byKey = null;
+      Map<Change.Key, ChangeNotes> byKey = null;
 
       COMMIT: for (RevCommit c; (c = rw.next()) != null;) {
         rw.parseBody(c);
@@ -2382,11 +2386,11 @@ public class ReceiveCommits {
             byKey = openChangesByBranch(branch);
           }
 
-          Change onto = byKey.get(new Change.Key(changeId.trim()));
+          ChangeNotes onto = byKey.get(new Change.Key(changeId.trim()));
           if (onto != null) {
-            Change.Id id = onto.getId();
+            Change.Id id = onto.getChangeId();
             final ReplaceRequest req = new ReplaceRequest(id, c, cmd, false);
-            req.change = onto;
+            req.notes = onto;
             if (req.validate(true)) {
               req.addOps(bu, null);
               bu.addOp(
@@ -2414,11 +2418,11 @@ public class ReceiveCommits {
     }
   }
 
-  private Map<Change.Key, Change> openChangesByBranch(Branch.NameKey branch)
+  private Map<Change.Key, ChangeNotes> openChangesByBranch(
-      throws OrmException {
+      Branch.NameKey branch) throws OrmException {
-    Map<Change.Key, Change> r = new HashMap<>();
+    Map<Change.Key, ChangeNotes> r = new HashMap<>();
     for (ChangeData cd : queryProvider.get().byBranchOpen(branch)) {
-      r.put(cd.change().getKey(), cd.change());
+      r.put(cd.change().getKey(), cd.notes());
     }
     return r;
   }

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
@@ -106,14 +107,17 @@ public class ChangeControl {
     private final ChangeData.Factory changeDataFactory;
     private final ChangeNotes.Factory notesFactory;
     private final ApprovalsUtil approvalsUtil;
+    private final PatchSetUtil patchSetUtil;
 
     @Inject
     Factory(ChangeData.Factory changeDataFactory,
         ChangeNotes.Factory notesFactory,
-        ApprovalsUtil approvalsUtil) {
+        ApprovalsUtil approvalsUtil,
+        PatchSetUtil patchSetUtil) {
       this.changeDataFactory = changeDataFactory;
       this.notesFactory = notesFactory;
       this.approvalsUtil = approvalsUtil;
+      this.patchSetUtil = patchSetUtil;
     }
 
     ChangeControl create(RefControl refControl, ReviewDb db, Project.NameKey
@@ -137,7 +141,7 @@ public class ChangeControl {
 
     ChangeControl create(RefControl refControl, ChangeNotes notes) {
       return new ChangeControl(changeDataFactory, approvalsUtil, refControl,
-          notes);
+          notes, patchSetUtil);
     }
   }
 
@@ -145,16 +149,19 @@ public class ChangeControl {
   private final ApprovalsUtil approvalsUtil;
   private final RefControl refControl;
   private final ChangeNotes notes;
+  private final PatchSetUtil patchSetUtil;
 
   ChangeControl(
       ChangeData.Factory changeDataFactory,
       ApprovalsUtil approvalsUtil,
       RefControl refControl,
-      ChangeNotes notes) {
+      ChangeNotes notes,
+      PatchSetUtil patchSetUtil) {
     this.changeDataFactory = changeDataFactory;
     this.approvalsUtil = approvalsUtil;
     this.refControl = refControl;
     this.notes = notes;
+    this.patchSetUtil = patchSetUtil;
   }
 
   public ChangeControl forUser(final CurrentUser who) {
@@ -162,7 +169,7 @@ public class ChangeControl {
       return this;
     }
     return new ChangeControl(changeDataFactory, approvalsUtil,
-        getRefControl().forUser(who), notes);
+        getRefControl().forUser(who), notes, patchSetUtil);
   }
 
   public RefControl getRefControl() {
@@ -307,8 +314,11 @@ public class ChangeControl {
   }
 
   /** Can this user add a patch set to this change? */
-  public boolean canAddPatchSet(ReviewDb db) throws OrmException {
+  public boolean canAddPatchSet(ReviewDb db)
-    return getRefControl().canUpload() && !isPatchSetLocked(db);
+      throws OrmException {
+    return getRefControl().canUpload()
+        && !isPatchSetLocked(db)
+        && isPatchVisible(patchSetUtil.current(db, notes), db);
   }
 
   /** Is the current patch set locked against state changes? */