opendev/gerrit
Code Issues Proposed changes

Avoid 500s when a topic includes changes that are not visible

Browse Source 
/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
This commit is contained in:
Jonathan Nieder
2016-06-15 16:10:52 -07:00
parent 27d460cfa9
commit 669c6cf68b
8 changed files with 129 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
View File

@@ -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<InMemoryRepository> 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<InMemoryRepository> 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");
}
}

37
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
View File

@@ -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();

1
gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
View File

@@ -76,6 +76,7 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
for (ChangeData cd : cs.changes()) {
changeResourceFactory.create(cd.changeControl()).prepareETag(h, user);
}
h.putBoolean(cs.furtherHiddenChanges());
} catch (IOException | OrmException e) {
throw new OrmRuntimeException(e);
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -258,6 +258,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
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);

6
gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java
View File

@@ -101,8 +101,12 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
}
private List<ChangeData> 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();
}

20
gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
View File

@@ -43,7 +43,13 @@ import java.util.Set;
public class ChangeSet {
private final ImmutableMap<Change.Id, ChangeData> changeData;
public ChangeSet(Iterable<ChangeData> changes) {
/**
* Whether additional changes are not included in changeData because they
* are not visible to current user.
*/
private final boolean furtherHiddenChanges;
public ChangeSet(Iterable<ChangeData> changes, boolean furtherHiddenChanges) {
Map<Change.Id, ChangeData> 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<Change.Id> 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" : "");
}
}

14
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -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<Change.Id, String> problems;
private CommitStatus(ChangeSet cs) throws OrmException {
checkArgument(!cs.furtherHiddenChanges(),
"CommitStatus must not be called with hidden changes");
changes = cs.changesById();
ImmutableSetMultimap.Builder<Branch.NameKey, Change.Id> 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<SubmitRecord> 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<Branch.NameKey, BranchBatch> toSubmit = new HashMap<>();
logDebug("Perform the merges");

19
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java
View File

@@ -107,6 +107,7 @@ public class MergeSuperSet {
CurrentUser user) throws MissingObjectException,
IncorrectObjectTypeException, IOException, OrmException {
List<ChangeData> ret = new ArrayList<>();
boolean sawHiddenChange = changes.furtherHiddenChanges();
Multimap<Project.NameKey, Change.Id> 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<ChangeData> 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;