From 669c6cf68ba7a07b29ec64df2146fa99b6ca7415 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 15 Jun 2016 16:10:52 -0700 Subject: [PATCH] Avoid 500s when a topic includes changes that are not visible /action and /submitted_together requests are throwing OrmException when they are not able to examine non-visible changes in the topic: com.google.gwtorm.server.OrmException: Failed to get submit type for 12345: Patch set 12345,6 not found at com.google.gerrit.server.git.MergeSuperSet.logErrorAndThrow(MergeSuperSet.java:218) at com.google.gerrit.server.git.MergeSuperSet.completeChangeSetWithoutTopic(MergeSuperSet.java:122) at com.google.gerrit.server.git.MergeSuperSet.completeChangeSetIncludingTopics(MergeSuperSet.java:180) at com.google.gerrit.server.git.MergeSuperSet.completeChangeSet(MergeSuperSet.java:101) at com.google.gerrit.server.change.SubmittedTogether.getForOpenChange(SubmittedTogether.java:105) at com.google.gerrit.server.change.SubmittedTogether.apply(SubmittedTogether.java:78) at com.google.gerrit.server.change.SubmittedTogether.apply(SubmittedTogether.java:46) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:332) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) The result is a lightbox with a 500 when the user views a change in the same topic as a draft change the user cannot see. As a first step toward fixing that, use response code 403 instead. This allows automated callers to get a better sense of what is going on (it is a permissions error, not an internal server error) without being confused by an unexpected response (e.g., an abbreviated list of changes). Change-Id: I560508c8d941fa6be140363f5bb103c3da4fac05 --- .../acceptance/rest/change/ActionsIT.java | 41 +++++++++++++++++-- .../server/change/SubmittedTogetherIT.java | 37 +++++++++++++++++ .../server/change/GetRevisionActions.java | 1 + .../google/gerrit/server/change/Submit.java | 3 ++ .../server/change/SubmittedTogether.java | 6 ++- .../google/gerrit/server/git/ChangeSet.java | 20 +++++++-- .../com/google/gerrit/server/git/MergeOp.java | 14 +++++++ .../gerrit/server/git/MergeSuperSet.java | 19 +++++++-- 8 files changed, 129 insertions(+), 12 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java index f8b10bf3f9..880fe89aa2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java @@ -111,6 +111,28 @@ public class ActionsIT extends AbstractDaemonTest { } } + @Test + public void revisionActionsETagWithHiddenDraftInTopic() throws Exception { + String change = createChangeWithTopic().getChangeId(); + approve(change); + + setApiUser(user); + String etag1 = getRevisionActions.getETag(parseCurrentRevisionResource(change)); + + setApiUser(admin); + String draft = createDraftWithTopic().getChangeId(); + approve(draft); + + setApiUser(user); + String etag2 = getRevisionActions.getETag(parseCurrentRevisionResource(change)); + + if (isSubmitWholeTopicEnabled()) { + assertThat(etag2).isNotEqualTo(etag1); + } else { + assertThat(etag2).isEqualTo(etag1); + } + } + @Test public void revisionActionsAnonymousETag() throws Exception { String parent = createChange().getChangeId(); @@ -262,13 +284,20 @@ public class ActionsIT extends AbstractDaemonTest { assertThat(actions).containsKey("rebase"); } + private PushOneCommit.Result createCommitAndPush( + TestRepository repo, String ref, + String commitMsg, String fileName, String content) throws Exception { + return pushFactory + .create(db, admin.getIdent(), repo, commitMsg, fileName, content) + .to(ref); + } + private PushOneCommit.Result createChangeWithTopic( TestRepository repo, String topic, String commitMsg, String fileName, String content) throws Exception { - PushOneCommit push = pushFactory.create(db, admin.getIdent(), - repo, commitMsg, fileName, content); assertThat(topic).isNotEmpty(); - return push.to("refs/for/master/" + name(topic)); + return createCommitAndPush(repo, "refs/for/master/" + name(topic), + commitMsg, fileName, content); } private PushOneCommit.Result createChangeWithTopic() @@ -276,4 +305,10 @@ public class ActionsIT extends AbstractDaemonTest { return createChangeWithTopic(testRepo, "foo2", "a message", "a.txt", "content\n"); } + + private PushOneCommit.Result createDraftWithTopic() + throws Exception { + return createCommitAndPush(testRepo, "refs/drafts/master/" + name("foo2"), + "a message", "a.txt", "content\n"); + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java index 4674ad2bb0..884fd550d1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java @@ -16,21 +16,30 @@ package com.google.gerrit.acceptance.server.change; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static org.junit.Assert.fail; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.testutil.ConfigSuite; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; public class SubmittedTogetherIT extends AbstractDaemonTest { + @ConfigSuite.Config + public static Config submitWholeTopicEnabled() { + return submitWholeTopicEnabledConfig(); + } @Test public void returnsAncestors() throws Exception { @@ -112,6 +121,34 @@ public class SubmittedTogetherIT extends AbstractDaemonTest { } } + @Test + public void hiddenDraftInTopic() throws Exception { + RevCommit initialHead = getRemoteHead(); + RevCommit a = commitBuilder().add("a", "1").message("change 1").create(); + pushHead(testRepo, "refs/for/master/" + name("topic"), false); + String id1 = getChangeId(a); + + testRepo.reset(initialHead); + RevCommit b = + commitBuilder().add("b", "2").message("invisible change").create(); + pushHead(testRepo, "refs/drafts/master/" + name("topic"), false); + + setApiUser(user); + if (isSubmitWholeTopicEnabled()) { + try { + gApi.changes().id(id1).submittedTogether(); + fail("Expected AuthException"); + } catch (RestApiException e) { + // TODO(jrn): fix extension API not to wrap the RestApiException. + assertThat(e.getCause()).isInstanceOf(AuthException.class); + assertThat(e.getCause()).hasMessage( + "change would be submitted with a change that you cannot see"); + } + } else { + assertSubmittedTogether(id1); + } + } + @Test public void testTopicChaining() throws Exception { RevCommit initialHead = getRemoteHead(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java index d758a7765d..eae67a25ff 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java @@ -76,6 +76,7 @@ public class GetRevisionActions implements ETagView { for (ChangeData cd : cs.changes()) { changeResourceFactory.create(cd.changeControl()).prepareETag(h, user); } + h.putBoolean(cs.furtherHiddenChanges()); } catch (IOException | OrmException e) { throw new OrmRuntimeException(e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index b4a49390b1..47501976e7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -258,6 +258,9 @@ public class Submit implements RestModifyView, try { @SuppressWarnings("resource") ReviewDb db = dbProvider.get(); + if (cs.furtherHiddenChanges()) { + return BLOCKED_HIDDEN_SUBMIT_TOOLTIP; + } for (ChangeData c : cs.changes()) { ChangeControl changeControl = c.changeControl(user); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java index 3cee393c32..0896a1806f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java @@ -101,8 +101,12 @@ public class SubmittedTogether implements RestReadView { } private List getForOpenChange(Change c, CurrentUser user) - throws OrmException, IOException { + throws OrmException, IOException, AuthException { ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c, user); + if (cs.furtherHiddenChanges()) { + throw new AuthException( + "change would be submitted with a change that you cannot see"); + } return cs.changes().asList(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java index a999678ed8..c61e5f8491 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java @@ -43,7 +43,13 @@ import java.util.Set; public class ChangeSet { private final ImmutableMap changeData; - public ChangeSet(Iterable changes) { + /** + * Whether additional changes are not included in changeData because they + * are not visible to current user. + */ + private final boolean furtherHiddenChanges; + + public ChangeSet(Iterable changes, boolean furtherHiddenChanges) { Map cds = new LinkedHashMap<>(); for (ChangeData cd : changes) { if (!cds.containsKey(cd.getId())) { @@ -51,10 +57,11 @@ public class ChangeSet { } } changeData = ImmutableMap.copyOf(cds); + this.furtherHiddenChanges = furtherHiddenChanges; } public ChangeSet(ChangeData change) { - this(ImmutableList.of(change)); + this(ImmutableList.of(change), false); } public ImmutableSet ids() { @@ -107,12 +114,17 @@ public class ChangeSet { return changeData.values(); } + public boolean furtherHiddenChanges() { + return furtherHiddenChanges; + } + public int size() { - return changeData.size(); + return changeData.size() + (furtherHiddenChanges ? 1 : 0); } @Override public String toString() { - return getClass().getSimpleName() + ids(); + return getClass().getSimpleName() + ids() + + (furtherHiddenChanges ? " and further hidden changes" : ""); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 91765e05cb..e2ead25f21 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.git; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -40,6 +41,7 @@ import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Branch; @@ -113,6 +115,8 @@ public class MergeOp implements AutoCloseable { private final Multimap problems; private CommitStatus(ChangeSet cs) throws OrmException { + checkArgument(!cs.furtherHiddenChanges(), + "CommitStatus must not be called with hidden changes"); changes = cs.changesById(); ImmutableSetMultimap.Builder bb = ImmutableSetMultimap.builder(); @@ -369,6 +373,8 @@ public class MergeOp implements AutoCloseable { } private void checkSubmitRulesAndState(ChangeSet cs) { + checkArgument(!cs.furtherHiddenChanges(), + "checkSubmitRulesAndState called for topic with hidden change"); for (ChangeData cd : cs.changes()) { try { if (cd.change().getStatus() != Change.Status.NEW) { @@ -388,6 +394,8 @@ public class MergeOp implements AutoCloseable { } private void bypassSubmitRules(ChangeSet cs) { + checkArgument(!cs.furtherHiddenChanges(), + "cannot bypass submit rules for topic with hidden change"); for (ChangeData cd : cs.changes()) { List records; try { @@ -426,6 +434,10 @@ public class MergeOp implements AutoCloseable { ChangeSet cs = mergeSuperSet.completeChangeSet(db, change, caller); checkState(cs.ids().contains(change.getId()), "change %s missing from %s", change.getId(), cs); + if (cs.furtherHiddenChanges()) { + throw new AuthException("A change to be submitted with " + + change.getId() + " is not visible"); + } this.commits = new CommitStatus(cs); MergeSuperSet.reloadChanges(cs); logDebug("Calculated to merge {}", cs); @@ -465,6 +477,8 @@ public class MergeOp implements AutoCloseable { private void integrateIntoHistory(ChangeSet cs) throws IntegrationException, RestApiException { + checkArgument(!cs.furtherHiddenChanges(), + "cannot integrate hidden changes into history"); logDebug("Beginning merge attempt on {}", cs); Map toSubmit = new HashMap<>(); logDebug("Perform the merges"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index b74221a392..f29626bfbe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -107,6 +107,7 @@ public class MergeSuperSet { CurrentUser user) throws MissingObjectException, IncorrectObjectTypeException, IOException, OrmException { List ret = new ArrayList<>(); + boolean sawHiddenChange = changes.furtherHiddenChanges(); Multimap pc = changes.changesByProject(); for (Project.NameKey project : pc.keySet()) { @@ -114,7 +115,10 @@ public class MergeSuperSet { RevWalk rw = CodeReviewCommit.newRevWalk(repo)) { for (Change.Id cId : pc.get(project)) { ChangeData cd = changeDataFactory.create(db, project, cId); - cd.changeControl(user); + if (!cd.changeControl(user).isVisible(db, cd)) { + sawHiddenChange = true; + continue; + } SubmitTypeRecord str = cd.submitTypeRecord(); if (!str.isOk()) { @@ -167,7 +171,7 @@ public class MergeSuperSet { } } - return new ChangeSet(ret); + return new ChangeSet(ret, sawHiddenChange); } private ChangeSet completeChangeSetIncludingTopics( @@ -179,17 +183,24 @@ public class MergeSuperSet { ChangeSet newCs = completeChangeSetWithoutTopic(db, changes, user); while (!done) { List chgs = new ArrayList<>(); + boolean sawHiddenChange = newCs.furtherHiddenChanges(); done = true; for (ChangeData cd : newCs.changes()) { chgs.add(cd); String topic = cd.change().getTopic(); if (!Strings.isNullOrEmpty(topic) && !topicsTraversed.contains(topic)) { - chgs.addAll(query().byTopicOpen(topic)); + for (ChangeData topicCd : query().byTopicOpen(topic)) { + if (!topicCd.changeControl(user).isVisible(db, topicCd)) { + sawHiddenChange = true; + continue; + } + chgs.add(topicCd); + } done = false; topicsTraversed.add(topic); } } - changes = new ChangeSet(chgs); + changes = new ChangeSet(chgs, sawHiddenChange); newCs = completeChangeSetWithoutTopic(db, changes, user); } return newCs;