Check submit with PermissionBackend

Change-Id: I932103c669821b7cfe4f5d762052c35f3c9731dd

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);
   }
 

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);
     }
   }

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;
   }

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<ReviewDb> 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<MergeOp> mergeOpProvider;
   private final Provider<MergeSuperSet> 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<ReviewDb> dbProvider,
       GitRepositoryManager repoManager,
+      PermissionBackend permissionBackend,
       ChangeData.Factory changeDataFactory,
       ChangeMessagesUtil cmUtil,
       ChangeNotes.Factory changeNotesFactory,
       Provider<MergeOp> mergeOpProvider,
       Provider<MergeSuperSet> mergeSuperSet,
       AccountsCollection accounts,
-      ChangesCollection changes,
       @GerritServerConfig Config cfg,
       Provider<InternalChangeQuery> 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()) {
+    if (!change.getStatus().isOpen()) {
-      throw new AuthException("submit not permitted");
-    } else 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);
+        Set<ChangePermission> can =
-
+            permissionBackend
-        if (!changeControl.isVisible(db)) {
+                .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();
+    Change change = resource.getChange();
-    String topic = resource.getChange().getTopic();
+    String topic = change.getTopic();
-    boolean visible =
-        !resource.getPatchSet().isDraft()
-            && resource.getChange().getStatus().isOpen()
-            && resource.getPatchSet().getId().equals(current)
-            && resource.getControl().canSubmit();
     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<String, String> 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)
+  private IdentifiedUser onBehalfOf(RevisionResource rsrc, SubmitInput in)
-      throws AuthException, UnprocessableEntityException, OrmException {
+      throws AuthException, UnprocessableEntityException, OrmException, PermissionBackendException {
-    ChangeControl caller = rsrc.getControl();
+    PermissionBackend.ForChange perm = rsrc.permissions().database(dbProvider);
-    if (!caller.canSubmit()) {
+    perm.check(ChangePermission.SUBMIT);
-      throw new AuthException("submit not permitted");
+    perm.check(ChangePermission.SUBMIT_AS);
-    }
+
-    if (!caller.canSubmitAs()) {
+    CurrentUser caller = rsrc.getUser();
-      throw new AuthException("submit on behalf of not permitted");
+    IdentifiedUser submitter = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
-    }
+    try {
-    ChangeControl target =
+      perm.user(submitter).check(ChangePermission.READ);
-        caller.forUser(accounts.parseOnBehalfOf(caller.getUser(), in.onBehalfOf));
+    } catch (AuthException e) {
-    if (!target.getRefControl().isVisible()) {
       throw new UnprocessableEntityException(
-          String.format(
+          String.format("on_behalf_of account %s cannot see change", submitter.getAccountId()));
-              "on_behalf_of account %s cannot see destination ref",
-              target.getUser().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");

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;
 

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) {

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()) {