opendev/gerrit
Code Issues Proposed changes

More consistency in on_behalf_of implementation

Browse Source 
Use AccountsCollection#parse in both cases, which requires the user be
visible to the caller. Also ensure the destination branch is visible
in both cases.

Change-Id: Ib9954586bf8370dbee5353d05c8c1953273caf3f
This commit is contained in:
Dave Borowitz
2016-10-05 16:46:54 -04:00
parent 62b194e6aa
commit 5c570f61ef
3 changed files with 14 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
View File

@@ -251,7 +251,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
}
@Test
public void voteOnBehalfOfSucceedsWhenUserCannotSeeDestinationRef()
public void voteOnBehalfOfFailsWhenUserCannotSeeDestinationRef()
throws Exception {
blockRead(newGroup);
@@ -265,17 +265,10 @@ public class ImpersonationIT extends AbstractDaemonTest {
in.onBehalfOf = user.id.toString();
in.label("Code-Review", 1);
// TODO(dborowitz): Make this fail instead.
exception.expect(UnprocessableEntityException.class);
exception.expectMessage(
"on_behalf_of account " + user.id + " cannot see destination ref");
revision.review(in);
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
LabelInfo codeReview = c.labels.get("Code-Review");
assertThat(codeReview.all).hasSize(1);
ApprovalInfo approval = codeReview.all.get(0);
assertThat(approval._accountId).isEqualTo(user.id.get());
assertThat(approval.value).isEqualTo(1);
}
@GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
@@ -379,7 +372,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
@GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
@Test
public void submitOnBehalfOfInvisibleUserIsAllowed() throws Exception {
public void submitOnBehalfOfInvisibleUserNotAllowed() throws Exception {
allowSubmitOnBehalfOf();
setApiUser(accounts.user2());
assertThat(accountControlFactory.get().canSee(user.id)).isFalse();
@@ -392,13 +385,12 @@ public class ImpersonationIT extends AbstractDaemonTest {
.review(ReviewInput.approve());
SubmitInput in = new SubmitInput();
in.onBehalfOf = user.email;
// TODO(dborowitz): Make this fail instead.
exception.expect(UnprocessableEntityException.class);
exception.expectMessage("Account Not Found: " + in.onBehalfOf);
gApi.changes()
.id(changeId)
.current()
.submit(in);
assertThat(gApi.changes().id(changeId).get().status)
.isEqualTo(ChangeStatus.MERGED);
}
private void allowCodeReviewOnBehalfOf() throws Exception {

5
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -276,6 +276,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
if (!target.getRefControl().isVisible()) {
throw new UnprocessableEntityException(String.format(
"on_behalf_of account %s cannot see destination ref",
target.getUser().getAccountId()));
}
return new RevisionResource(changes.parse(target), rev.getPatchSet());
}

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

@@ -485,16 +485,11 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
if (!caller.canSubmitAs()) {
throw new AuthException("submit on behalf of not permitted");
}
IdentifiedUser targetUser = accounts.parseId(in.onBehalfOf);
if (targetUser == null) {
throw new UnprocessableEntityException(String.format(
"Account Not Found: %s", in.onBehalfOf));
}
ChangeControl target = caller.forUser(targetUser);
ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
if (!target.getRefControl().isVisible()) {
throw new UnprocessableEntityException(String.format(
"on_behalf_of account %s cannot see destination ref",
targetUser.getAccountId()));
target.getUser().getAccountId()));
}
return new RevisionResource(changes.parse(target), rsrc.getPatchSet());
}