Gate Keeping: Add support for "on behalf of" for submit operation

This change allows submit operation to be done on behalf of another user.

To use this option the caller must have been granted both `Submit` and
`SubmitAs` permission.  The user named by `on_behalf_of` does not need to
be granted `Submit` permissions.

This feature is aimed for CI solutions with "Gate Keeping" characteristic
(OpenStack's Zuul [1]): the Gate Keeper (CI account) can be granted both
permissions, so individual users don't need `Submit` permission themselves
(in fact they can't submit at all, only Gate Keeper can submit in this
scenario).  Still the changes are submitted on behalf of real users and
not with the identity of the Gate Keeper.

[1] https://github.com/openstack-infra/zuul

Inspired-by: Chad Horohoe <chorohoe@wikimedia.org>
Change-Id: Ifc250c0b576ebd4b7dce26c357557898b87ec705
This commit is contained in:
David Ostrovsky 2014-01-30 19:50:57 +01:00
parent a57ac7e0f5
commit 868e341d0c
10 changed files with 92 additions and 4 deletions

View File

@ -753,7 +753,10 @@ on the web UI.
Submitting a change causes it to be merged into the destination Submitting a change causes it to be merged into the destination
branch as soon as possible, making it a permanent part of the branch as soon as possible, making it a permanent part of the
project's history. project's history. There is also a `SubmitAs` permission that enables
submitting on behalf of another user. To be able to submit on behalf of
another user the caller must have been granted both `Submit` and
`SubmitAs` permission.
In order to submit, all labels (such as `Verified` and `Code-Review`, In order to submit, all labels (such as `Verified` and `Code-Review`,
above) must enable submit, and also must not block it. See above for above) must enable submit, and also must not block it. See above for

View File

@ -3284,6 +3284,15 @@ The status of the change after submitting, can be `MERGED` or
If `wait_for_merge` in the link:#submit-input[SubmitInput] was set to If `wait_for_merge` in the link:#submit-input[SubmitInput] was set to
`false` the returned status is `SUBMITTED` and the caller can't know `false` the returned status is `SUBMITTED` and the caller can't know
whether the change could be merged successfully. whether the change could be merged successfully.
|`on_behalf_of`|optional|
The link:rest-api-accounts.html#account-id[\{account-id\}] of the user on
whose behalf the action should be done. To use this option the caller must
have been granted both `Submit` and `SubmitAs` permission. The user named
by `on_behalf_of` does not need to be granted `Submit` permissions. This
feature is aimed for CI solutions: the CI account can be granted both
permssions, so individual users don't need `Submit` permission themselves.
Still the changes can be submited on behalf of real users and not with the
identity of the CI account.
|========================== |==========================
[[submit-input]] [[submit-input]]

View File

@ -122,6 +122,12 @@ public class AccountCreator {
"Administrators"); "Administrators");
} }
public TestAccount admin2()
throws UnsupportedEncodingException, OrmException, JSchException {
return create("admin2", "admin2@example.com", "Administrator2",
"Administrators");
}
public TestAccount user() public TestAccount user()
throws UnsupportedEncodingException, OrmException, JSchException { throws UnsupportedEncodingException, OrmException, JSchException {
return create("user", "user@example.com", "User"); return create("user", "user@example.com", "User");

View File

@ -17,13 +17,17 @@ package com.google.gerrit.acceptance.api.revision;
import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.CherryPickInput; import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.GitAPIException;
import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import java.io.IOException; import java.io.IOException;
@ -31,6 +35,13 @@ import java.io.IOException;
@NoHttpd @NoHttpd
public class RevisionIT extends AbstractDaemonTest { public class RevisionIT extends AbstractDaemonTest {
private TestAccount admin2;
@Before
public void setUp() throws Exception {
admin2 = accounts.admin2();
}
@Test @Test
public void reviewTriplet() throws GitAPIException, public void reviewTriplet() throws GitAPIException,
IOException, RestApiException { IOException, RestApiException {
@ -81,6 +92,23 @@ public class RevisionIT extends AbstractDaemonTest {
.submit(); .submit();
} }
@Test(expected = AuthException.class)
public void submitOnBehalfOf() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = createChange();
gApi.changes()
.id("p~master~" + r.getChangeId())
.current()
.review(ReviewInput.approve());
SubmitInput in = new SubmitInput();
in.onBehalfOf = admin2.email;
in.waitForMerge = true;
gApi.changes()
.id("p~master~" + r.getChangeId())
.current()
.submit(in);
}
@Test @Test
public void deleteDraft() throws GitAPIException, public void deleteDraft() throws GitAPIException,
IOException, RestApiException { IOException, RestApiException {

View File

@ -40,6 +40,7 @@ public class Permission implements Comparable<Permission> {
public static final String REBASE = "rebase"; public static final String REBASE = "rebase";
public static final String REMOVE_REVIEWER = "removeReviewer"; public static final String REMOVE_REVIEWER = "removeReviewer";
public static final String SUBMIT = "submit"; public static final String SUBMIT = "submit";
public static final String SUBMIT_AS = "submitAs";
public static final String VIEW_DRAFTS = "viewDrafts"; public static final String VIEW_DRAFTS = "viewDrafts";
private static final List<String> NAMES_LC; private static final List<String> NAMES_LC;
@ -64,6 +65,7 @@ public class Permission implements Comparable<Permission> {
NAMES_LC.add(REBASE.toLowerCase()); NAMES_LC.add(REBASE.toLowerCase());
NAMES_LC.add(REMOVE_REVIEWER.toLowerCase()); NAMES_LC.add(REMOVE_REVIEWER.toLowerCase());
NAMES_LC.add(SUBMIT.toLowerCase()); NAMES_LC.add(SUBMIT.toLowerCase());
NAMES_LC.add(SUBMIT_AS.toLowerCase());
NAMES_LC.add(VIEW_DRAFTS.toLowerCase()); NAMES_LC.add(VIEW_DRAFTS.toLowerCase());
NAMES_LC.add(EDIT_TOPIC_NAME.toLowerCase()); NAMES_LC.add(EDIT_TOPIC_NAME.toLowerCase());
NAMES_LC.add(DELETE_DRAFTS.toLowerCase()); NAMES_LC.add(DELETE_DRAFTS.toLowerCase());

View File

@ -16,4 +16,5 @@ package com.google.gerrit.extensions.api.changes;
public class SubmitInput { public class SubmitInput {
public boolean waitForMerge; public boolean waitForMerge;
public String onBehalfOf;
} }

View File

@ -121,6 +121,7 @@ permissionNames = \
rebase, \ rebase, \
removeReviewer, \ removeReviewer, \
submit, \ submit, \
submitAs, \
viewDrafts viewDrafts
abandon = Abandon abandon = Abandon
@ -140,6 +141,7 @@ read = Read
rebase = Rebase rebase = Rebase
removeReviewer = Remove Reviewer removeReviewer = Remove Reviewer
submit = Submit submit = Submit
submitAs = Submit (On Behalf Of)
viewDrafts = View Drafts viewDrafts = View Drafts
refErrorEmpty = Reference must be supplied refErrorEmpty = Reference must be supplied

View File

@ -18,6 +18,7 @@ import static com.google.gerrit.common.data.SubmitRecord.Status.OK;
import com.google.common.base.Optional; import com.google.common.base.Optional;
import com.google.common.base.Predicate; import com.google.common.base.Predicate;
import com.google.common.base.Strings;
import com.google.common.collect.HashBasedTable; import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
@ -29,6 +30,7 @@ import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
@ -42,6 +44,7 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ProjectUtil; import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.change.ChangeJson.ChangeInfo; import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LabelNormalizer; import com.google.gerrit.server.git.LabelNormalizer;
@ -95,6 +98,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
private final MergeQueue mergeQueue; private final MergeQueue mergeQueue;
private final ChangeIndexer indexer; private final ChangeIndexer indexer;
private final LabelNormalizer labelNormalizer; private final LabelNormalizer labelNormalizer;
private final AccountsCollection accounts;
private final ChangesCollection changes;
@Inject @Inject
Submit(@GerritPersonIdent PersonIdent serverIdent, Submit(@GerritPersonIdent PersonIdent serverIdent,
@ -104,6 +109,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ChangeUpdate.Factory updateFactory, ChangeUpdate.Factory updateFactory,
ApprovalsUtil approvalsUtil, ApprovalsUtil approvalsUtil,
MergeQueue mergeQueue, MergeQueue mergeQueue,
AccountsCollection accounts,
ChangesCollection changes,
ChangeIndexer indexer, ChangeIndexer indexer,
LabelNormalizer labelNormalizer) { LabelNormalizer labelNormalizer) {
this.serverIdent = serverIdent; this.serverIdent = serverIdent;
@ -113,6 +120,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
this.updateFactory = updateFactory; this.updateFactory = updateFactory;
this.approvalsUtil = approvalsUtil; this.approvalsUtil = approvalsUtil;
this.mergeQueue = mergeQueue; this.mergeQueue = mergeQueue;
this.accounts = accounts;
this.changes = changes;
this.indexer = indexer; this.indexer = indexer;
this.labelNormalizer = labelNormalizer; this.labelNormalizer = labelNormalizer;
} }
@ -120,11 +129,16 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
@Override @Override
public Output apply(RevisionResource rsrc, SubmitInput input) public Output apply(RevisionResource rsrc, SubmitInput input)
throws AuthException, ResourceConflictException, throws AuthException, ResourceConflictException,
RepositoryNotFoundException, IOException, OrmException { RepositoryNotFoundException, IOException, OrmException,
UnprocessableEntityException {
input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf);
if (input.onBehalfOf != null) {
rsrc = onBehalfOf(rsrc, input);
}
ChangeControl control = rsrc.getControl(); ChangeControl control = rsrc.getControl();
IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser(); IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
Change change = rsrc.getChange(); Change change = rsrc.getChange();
if (!control.canSubmit()) { if (input.onBehalfOf == null && !control.canSubmit()) {
throw new AuthException("submit not permitted"); throw new AuthException("submit not permitted");
} else if (!change.getStatus().isOpen()) { } else if (!change.getStatus().isOpen()) {
throw new ResourceConflictException("change is " + status(change)); throw new ResourceConflictException("change is " + status(change));
@ -422,6 +436,19 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
return change != null ? change.getStatus().name().toLowerCase() : "deleted"; return change != null ? change.getStatus().name().toLowerCase() : "deleted";
} }
private RevisionResource onBehalfOf(RevisionResource rsrc, SubmitInput in)
throws AuthException, UnprocessableEntityException, OrmException {
ChangeControl caller = rsrc.getControl();
if (!caller.canSubmit()) {
throw new AuthException("submit not permitted");
}
if (!caller.canSubmitAs()) {
throw new AuthException("submit on behalf of not permitted");
}
ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
return new RevisionResource(changes.parse(target), rsrc.getPatchSet());
}
public static class CurrentRevision implements public static class CurrentRevision implements
RestModifyView<ChangeResource, SubmitInput> { RestModifyView<ChangeResource, SubmitInput> {
private final Provider<ReviewDb> dbProvider; private final Provider<ReviewDb> dbProvider;
@ -440,7 +467,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
@Override @Override
public ChangeInfo apply(ChangeResource rsrc, SubmitInput input) public ChangeInfo apply(ChangeResource rsrc, SubmitInput input)
throws AuthException, ResourceConflictException, throws AuthException, ResourceConflictException,
RepositoryNotFoundException, IOException, OrmException { RepositoryNotFoundException, IOException, OrmException,
UnprocessableEntityException {
PatchSet ps = dbProvider.get().patchSets() PatchSet ps = dbProvider.get().patchSets()
.get(rsrc.getChange().currentPatchSetId()); .get(rsrc.getChange().currentPatchSetId());
if (ps == null) { if (ps == null) {

View File

@ -425,6 +425,10 @@ public class ChangeControl {
return getRefControl().canSubmit(); return getRefControl().canSubmit();
} }
public boolean canSubmitAs() {
return getRefControl().canSubmitAs();
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) { public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) {
return canSubmit(db, patchSet, null, false, false, false); return canSubmit(db, patchSet, null, false, false, false);
} }

View File

@ -173,6 +173,11 @@ public class RefControl {
&& canWrite(); && canWrite();
} }
/** @return true if this user was granted submitAs to this ref */
public boolean canSubmitAs() {
return canPerform(Permission.SUBMIT_AS);
}
/** @return true if the user can update the reference as a fast-forward. */ /** @return true if the user can update the reference as a fast-forward. */
public boolean canUpdate() { public boolean canUpdate() {
if (RefNames.REFS_CONFIG.equals(refName) if (RefNames.REFS_CONFIG.equals(refName)