opendev/gerrit
Code Issues Proposed changes

OutputStreamQuery: Only return current patch set when visible to user

Browse Source 
When querying changes with the 'gerrit query' ssh command, and
passing the --current-patch-set option, the current patch set is
included even when it is not visible to the caller (for example when
the patch set is a draft, and the caller cannot see drafts).

This causes problems for example when the caller runs a query and
then tries to perform some operation on the revisions of the current
patchset revisions that were returned. For those revisions that are
not visible, the operation will fail.

The same is true when using the --patch-sets option.

Add a check for patch set visibility, and do not add it in the
results.

Bug: Issue 4070
Change-Id: Id68969bc49a9412ae81f252fd2d846539d9022fa
This commit is contained in:
David Pursehouse 2016-04-18 17:59:44 +09:00
parent 847fa63e02
commit e2f4c2f127
5 changed files with 68 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
Documentation/cmd-query.txt
View File

@ -54,15 +54,17 @@ command line parser in the server).
--current-patch-set::
Include information about the current patch set in the results.
Note that the information will only be included when the current
patch set is visible to the caller.
--patch-sets::
Include information about all patch sets. If combined with
the --current-patch-set flag then the current patch set
information will be output twice, once in each field.
Include information about all patch sets visible to the caller.
If combined with the --current-patch-set flag then the current patch
set information will be output twice, once in each field.
--all-approvals::
Include information about all patch sets along with the
approval information for each patch set. If combined with
Include information about all patch sets visible to the caller along
with the approval information for each patch set. If combined with
the --current-patch-set flag then the current patch set
information will be output twice, once in each field.
@ -76,7 +78,7 @@ command line parser in the server).
--comments::
Include comments for all changes. If combined with the
--patch-sets flag then all inline/file comments are included for
each patch set.
each patch set that is visible to the caller.
--commit-message::
Include the full commit message in the change description.

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -236,6 +236,11 @@ public abstract class AbstractDaemonTest {
return push.to(git, ref);
}
protected PushOneCommit.Result amendChangeAsDraft(String changeId)
throws GitAPIException, IOException {
return amendChange(changeId, "refs/drafts/master");
}
protected ChangeInfo getChange(String changeId, ListChangesOption... options)
throws IOException {
return getChange(adminSession, changeId, options);

38
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
View File

@ -16,11 +16,13 @@ package com.google.gerrit.acceptance.ssh;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.acceptance.GitUtil.initSsh;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.SshSession;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.Side;
@ -300,13 +302,39 @@ public class QueryIT extends AbstractDaemonTest {
assertThat(changes.get(0).submitRecords.size()).isEqualTo(1);
}
@Test
public void testQueryWithNonVisibleCurrentPatchSet() throws Exception {
String changeId = createChange().getChangeId();
amendChangeAsDraft(changeId);
String query = "--current-patch-set --patch-sets " + changeId;
List<ChangeAttribute> changes = executeSuccessfulQuery(query);
assertThat(changes.size()).isEqualTo(1);
assertThat(changes.get(0).patchSets).isNotNull();
assertThat(changes.get(0).patchSets).hasSize(2);
assertThat(changes.get(0).currentPatchSet).isNotNull();
SshSession userSession = new SshSession(server, user);
initSsh(user);
userSession.open();
changes = executeSuccessfulQuery(query, userSession);
assertThat(changes.size()).isEqualTo(1);
assertThat(changes.get(0).patchSets).hasSize(1);
assertThat(changes.get(0).currentPatchSet).isNull();
userSession.close();
}
private List<ChangeAttribute> executeSuccessfulQuery(String params,
SshSession session) throws Exception {
String rawResponse =
session.exec("gerrit query --format=JSON " + params);
assert_().withFailureMessage(session.getError())
.that(session.hasError()).isFalse();
return getChanges(rawResponse);
}
private List<ChangeAttribute> executeSuccessfulQuery(String params)
throws Exception {
String rawResponse =
sshSession.exec("gerrit query --format=JSON " + params);
assert_().withFailureMessage(sshSession.getError())
.that(sshSession.hasError()).isFalse();
return getChanges(rawResponse);
return executeSuccessfulQuery(params, sshSession);
}
private static List<ChangeAttribute> getChanges(String rawResponse) {

18
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -17,6 +17,8 @@ package com.google.gerrit.server.query.change;
import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
import com.google.common.base.MoreObjects;
import com.google.common.base.Predicate;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
@ -544,6 +546,22 @@ public class ChangeData {
return patches;
}
/**
* @return patches for the change visible to the current user.
* @throws OrmException an error occurred reading the database.
*/
public Collection<PatchSet> visiblePatches() throws OrmException {
return FluentIterable.from(patches()).filter(new Predicate<PatchSet>() {
@Override
public boolean apply(PatchSet input) {
try {
return changeControl().isPatchVisible(input, db);
} catch (OrmException e) {
return false;
}
}}).toList();
}
/**
* @return patch with the given ID, or null if it does not exist.
* @throws OrmException an error occurred reading the database.

8
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@ -209,11 +209,11 @@ public class OutputStreamQuery {
if (includePatchSets) {
if (includeFiles) {
eventFactory.addPatchSets(c, d.patches(),
eventFactory.addPatchSets(c, d.visiblePatches(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
} else {
eventFactory.addPatchSets(c, d.patches(),
eventFactory.addPatchSets(c, d.visiblePatches(),
includeApprovals ? d.approvals().asMap() : null,
labelTypes);
}
@ -221,7 +221,7 @@ public class OutputStreamQuery {
if (includeCurrentPatchSet) {
PatchSet current = d.currentPatchSet();
if (current != null) {
if (current != null && cc.isPatchVisible(current, d.db())) {
c.currentPatchSet = eventFactory.asPatchSetAttribute(current);
eventFactory.addApprovals(c.currentPatchSet,
d.currentApprovals(), labelTypes);
@ -240,7 +240,7 @@ public class OutputStreamQuery {
if (includeComments) {
eventFactory.addComments(c, d.messages());
if (includePatchSets) {
eventFactory.addPatchSets(c, d.patches(),
eventFactory.addPatchSets(c, d.visiblePatches(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
for (PatchSetAttribute attribute : c.patchSets) {