diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java index 7d9663a9b9..82eae1b4e8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java @@ -378,7 +378,7 @@ public class ImpersonationIT extends AbstractDaemonTest { SubmitInput in = new SubmitInput(); in.onBehalfOf = admin2.email; exception.expect(AuthException.class); - exception.expectMessage("submit on behalf of not permitted"); + exception.expectMessage("submit as not permitted"); gApi.changes().id(project.get() + "~master~" + r.getChangeId()).current().submit(in); } @@ -393,7 +393,7 @@ public class ImpersonationIT extends AbstractDaemonTest { SubmitInput in = new SubmitInput(); in.onBehalfOf = user.email; exception.expect(UnprocessableEntityException.class); - exception.expectMessage("on_behalf_of account " + user.id + " cannot see destination ref"); + exception.expectMessage("on_behalf_of account " + user.id + " cannot see change"); gApi.changes().id(changeId).current().submit(in); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 43be8df9fd..dac9debcdc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -70,6 +70,7 @@ import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.change.TestSubmitType; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.update.UpdateException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -219,7 +220,7 @@ class RevisionApiImpl implements RevisionApi { public void submit(SubmitInput in) throws RestApiException { try { submit.apply(revision, in); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | PermissionBackendException e) { throw new RestApiException("Cannot submit change", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java index 4d35f9ee9f..32132bc191 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java @@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ChangeControl; import com.google.inject.TypeLiteral; import java.util.Optional; @@ -51,6 +52,10 @@ public class RevisionResource implements RestResource, HasETag { return cacheable; } + public PermissionBackend.ForChange permissions() { + return change.permissions(); + } + public ChangeResource getChangeResource() { return change; } 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 01020fa8a6..b996133f32 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 @@ -51,7 +51,9 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MergeSuperSet; import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.permissions.ChangePermission; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; @@ -62,6 +64,7 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.Collection; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -122,13 +125,13 @@ public class Submit private final Provider dbProvider; private final GitRepositoryManager repoManager; + private final PermissionBackend permissionBackend; private final ChangeData.Factory changeDataFactory; private final ChangeMessagesUtil cmUtil; private final ChangeNotes.Factory changeNotesFactory; private final Provider mergeOpProvider; private final Provider mergeSuperSet; private final AccountsCollection accounts; - private final ChangesCollection changes; private final String label; private final String labelWithParents; private final ParameterizedString titlePattern; @@ -143,25 +146,25 @@ public class Submit Submit( Provider dbProvider, GitRepositoryManager repoManager, + PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory, ChangeMessagesUtil cmUtil, ChangeNotes.Factory changeNotesFactory, Provider mergeOpProvider, Provider mergeSuperSet, AccountsCollection accounts, - ChangesCollection changes, @GerritServerConfig Config cfg, Provider queryProvider, PatchSetUtil psUtil) { this.dbProvider = dbProvider; this.repoManager = repoManager; + this.permissionBackend = permissionBackend; this.changeDataFactory = changeDataFactory; this.cmUtil = cmUtil; this.changeNotesFactory = changeNotesFactory; this.mergeOpProvider = mergeOpProvider; this.mergeSuperSet = mergeSuperSet; this.accounts = accounts; - this.changes = changes; this.label = MoreObjects.firstNonNull( Strings.emptyToNull(cfg.getString("change", null, "submitLabel")), "Submit"); @@ -193,17 +196,19 @@ public class Submit @Override public Output apply(RevisionResource rsrc, SubmitInput input) - throws RestApiException, RepositoryNotFoundException, IOException, OrmException { + throws RestApiException, RepositoryNotFoundException, IOException, OrmException, + PermissionBackendException { input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf); + IdentifiedUser submitter; if (input.onBehalfOf != null) { - rsrc = onBehalfOf(rsrc, input); + submitter = onBehalfOf(rsrc, input); + } else { + rsrc.permissions().check(ChangePermission.SUBMIT); + submitter = rsrc.getUser().asIdentifiedUser(); } - ChangeControl control = rsrc.getControl(); - IdentifiedUser caller = control.getUser().asIdentifiedUser(); + Change change = rsrc.getChange(); - if (input.onBehalfOf == null && !control.canSubmit()) { - throw new AuthException("submit not permitted"); - } else if (!change.getStatus().isOpen()) { + if (!change.getStatus().isOpen()) { throw new ResourceConflictException("change is " + status(change)); } else if (!ProjectUtil.branchExists(repoManager, change.getDest())) { throw new ResourceConflictException( @@ -217,7 +222,7 @@ public class Submit try (MergeOp op = mergeOpProvider.get()) { ReviewDb db = dbProvider.get(); - op.merge(db, change, caller, true, input, false); + op.merge(db, change, submitter, true, input, false); try { change = changeNotesFactory.createChecked(db, change.getProject(), change.getId()).getChange(); @@ -250,18 +255,20 @@ public class Submit */ private String problemsForSubmittingChangeset(ChangeData cd, ChangeSet cs, CurrentUser user) { 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); - - if (!changeControl.isVisible(db)) { + Set can = + permissionBackend + .user(user) + .database(dbProvider) + .change(c) + .test(EnumSet.of(ChangePermission.READ, ChangePermission.SUBMIT)); + if (!can.contains(ChangePermission.READ)) { return BLOCKED_HIDDEN_SUBMIT_TOOLTIP; } - if (!changeControl.canSubmit()) { + if (!can.contains(ChangePermission.SUBMIT)) { return BLOCKED_SUBMIT_TOOLTIP; } MergeOp.checkSubmitRule(c); @@ -281,7 +288,7 @@ public class Submit } } catch (ResourceConflictException e) { return BLOCKED_SUBMIT_TOOLTIP; - } catch (OrmException | IOException e) { + } catch (PermissionBackendException | OrmException | IOException e) { log.error("Error checking if change is submittable", e); throw new OrmRuntimeException("Could not determine problems for the change", e); } @@ -290,20 +297,23 @@ public class Submit @Override public UiAction.Description getDescription(RevisionResource resource) { - PatchSet.Id current = resource.getChange().currentPatchSetId(); - String topic = resource.getChange().getTopic(); - boolean visible = - !resource.getPatchSet().isDraft() - && resource.getChange().getStatus().isOpen() - && resource.getPatchSet().getId().equals(current) - && resource.getControl().canSubmit(); + Change change = resource.getChange(); + String topic = change.getTopic(); ReviewDb db = dbProvider.get(); ChangeData cd = changeDataFactory.create(db, resource.getControl()); - + boolean visible; try { + visible = + change.getStatus().isOpen() + && resource.isCurrent() + && !resource.getPatchSet().isDraft() + && resource.permissions().test(ChangePermission.SUBMIT); MergeOp.checkSubmitRule(cd); } catch (ResourceConflictException e) { visible = false; + } catch (PermissionBackendException e) { + log.error("Error checking if change is submittable", e); + throw new OrmRuntimeException("Could not check submit permission", e); } catch (OrmException e) { log.error("Error checking if change is submittable", e); throw new OrmRuntimeException("Could not determine problems for the change", e); @@ -367,7 +377,7 @@ public class Submit Map params = ImmutableMap.of( "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()), - "branch", resource.getChange().getDest().getShortName(), + "branch", change.getDest().getShortName(), "commit", ObjectId.fromString(revId.get()).abbreviate(7).name(), "submitSize", String.valueOf(cs.size())); ParameterizedString tp = cs.size() > 1 ? titlePatternWithAncestors : titlePattern; @@ -458,24 +468,21 @@ public class Submit return commits; } - 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.parseOnBehalfOf(caller.getUser(), in.onBehalfOf)); - if (!target.getRefControl().isVisible()) { + private IdentifiedUser onBehalfOf(RevisionResource rsrc, SubmitInput in) + throws AuthException, UnprocessableEntityException, OrmException, PermissionBackendException { + PermissionBackend.ForChange perm = rsrc.permissions().database(dbProvider); + perm.check(ChangePermission.SUBMIT); + perm.check(ChangePermission.SUBMIT_AS); + + CurrentUser caller = rsrc.getUser(); + IdentifiedUser submitter = accounts.parseOnBehalfOf(caller, in.onBehalfOf); + try { + perm.user(submitter).check(ChangePermission.READ); + } catch (AuthException e) { throw new UnprocessableEntityException( - String.format( - "on_behalf_of account %s cannot see destination ref", - target.getUser().getAccountId())); + String.format("on_behalf_of account %s cannot see change", submitter.getAccountId())); } - return new RevisionResource(changes.parse(target), rsrc.getPatchSet()); + return submitter; } public static boolean wholeTopicEnabled(Config config) { @@ -510,7 +517,8 @@ public class Submit @Override public ChangeInfo apply(ChangeResource rsrc, SubmitInput input) - throws RestApiException, RepositoryNotFoundException, IOException, OrmException { + throws RestApiException, RepositoryNotFoundException, IOException, OrmException, + PermissionBackendException { PatchSet ps = psUtil.current(dbProvider.get(), rsrc.getNotes()); if (ps == null) { throw new ResourceConflictException("current revision is missing"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java index 90d27547b9..4b06861231 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ChangePermission.java @@ -30,7 +30,8 @@ public enum ChangePermission implements ChangePermissionOrLabel { REMOVE_REVIEWER(Permission.REMOVE_REVIEWER), ADD_PATCH_SET(Permission.ADD_PATCH_SET), REBASE(Permission.REBASE), - SUBMIT(Permission.SUBMIT); + SUBMIT(Permission.SUBMIT), + SUBMIT_AS(Permission.SUBMIT_AS); private final String name; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index 4d2120ff8f..11b69804e1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -472,14 +472,6 @@ public class ChangeControl { || getRefControl().canEditHashtags(); // user can edit hashtag on a specific ref } - public boolean canSubmit() { - return getRefControl().canSubmit(isOwner()); - } - - public boolean canSubmitAs() { - return getRefControl().canSubmitAs(); - } - private boolean match(String destBranch, String refPattern) { return RefPatternMatcher.getMatcher(refPattern).match(destBranch, getUser()); } @@ -594,8 +586,10 @@ public class ChangeControl { case RESTORE: return canRestore(db()); case SUBMIT: - return canSubmit(); + return getRefControl().canSubmit(isOwner()); + case REMOVE_REVIEWER: // TODO Honor specific removal filters? + case SUBMIT_AS: return getRefControl().canPerform(perm.permissionName().get()); } } catch (OrmException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index ca567e26b7..9f6f8de476 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -187,11 +187,6 @@ public class RefControl { return canPerform(Permission.SUBMIT, isChangeOwner) && 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. */ public boolean canUpdate() { if (RefNames.REFS_CONFIG.equals(refName) && !projectControl.isOwner()) {