Merge "Make project state check in ADD_PATCH_SET explicit"

java/com/google/gerrit/extensions/client/ProjectState.java

@@ -15,8 +15,14 @@
 package com.google.gerrit.extensions.client;
 
 public enum ProjectState {
+  /** Permits reading project state and contents as well as mutating data. */
   ACTIVE(true, true),
+  /** Permits reading project state and contents. Does not permit any modifications. */
   READ_ONLY(true, false),
+  /**
+   * Hides the project as if it was deleted, but makes requests fail with an error message that
+   * reveals the project's existence.
+   */
   HIDDEN(false, false);
 
   private final boolean permitsRead;

java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -319,6 +319,7 @@ public class PatchSetInserter implements BatchUpdateOp {
           .change(origNotes)
           .check(ChangePermission.ADD_PATCH_SET);
     }
+    projectCache.checkedGet(ctx.getProject()).checkStatePermitsWrite();
     if (!validate) {
       return;
     }

java/com/google/gerrit/server/edit/ChangeEditModifier.java

@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
 import com.google.gerrit.extensions.restapi.RawInput;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -40,6 +41,7 @@ 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.InvalidChangeOperationException;
+import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -83,6 +85,7 @@ public class ChangeEditModifier {
   private final PermissionBackend permissionBackend;
   private final ChangeEditUtil changeEditUtil;
   private final PatchSetUtil patchSetUtil;
+  private final ProjectCache projectCache;
 
   @Inject
   ChangeEditModifier(
@@ -92,7 +95,8 @@ public class ChangeEditModifier {
       Provider<CurrentUser> currentUser,
       PermissionBackend permissionBackend,
       ChangeEditUtil changeEditUtil,
-      PatchSetUtil patchSetUtil) {
+      PatchSetUtil patchSetUtil,
+      ProjectCache projectCache) {
     this.indexer = indexer;
     this.reviewDb = reviewDb;
     this.currentUser = currentUser;
@@ -100,6 +104,7 @@ public class ChangeEditModifier {
     this.tz = gerritIdent.getTimeZone();
     this.changeEditUtil = changeEditUtil;
     this.patchSetUtil = patchSetUtil;
+    this.projectCache = projectCache;
   }
 
   /**
@@ -113,7 +118,7 @@ public class ChangeEditModifier {
    */
   public void createEdit(Repository repository, ChangeNotes notes)
       throws AuthException, IOException, InvalidChangeOperationException, OrmException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     assertCanEdit(notes);
 
     Optional<ChangeEdit> changeEdit = lookupChangeEdit(notes);
@@ -141,7 +146,7 @@ public class ChangeEditModifier {
    */
   public void rebaseEdit(Repository repository, ChangeNotes notes)
       throws AuthException, InvalidChangeOperationException, IOException, OrmException,
-          MergeConflictException, PermissionBackendException {
+          MergeConflictException, PermissionBackendException, ResourceConflictException {
     assertCanEdit(notes);
 
     Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(notes);
@@ -206,7 +211,7 @@ public class ChangeEditModifier {
    */
   public void modifyMessage(Repository repository, ChangeNotes notes, String newCommitMessage)
       throws AuthException, IOException, UnchangedCommitMessageException, OrmException,
-          PermissionBackendException, BadRequestException {
+          PermissionBackendException, BadRequestException, ResourceConflictException {
     assertCanEdit(notes);
     newCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(newCommitMessage);
 
@@ -244,11 +249,12 @@ public class ChangeEditModifier {
    * @throws AuthException if the user isn't authenticated or not allowed to use change edits
    * @throws InvalidChangeOperationException if the file already had the specified content
    * @throws PermissionBackendException
+   * @throws ResourceConflictException if the project state does not permit the operation
    */
   public void modifyFile(
       Repository repository, ChangeNotes notes, String filePath, RawInput newContent)
       throws AuthException, InvalidChangeOperationException, IOException, OrmException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     modifyTree(repository, notes, new ChangeFileContentModification(filePath, newContent));
   }
 
@@ -262,10 +268,11 @@ public class ChangeEditModifier {
    * @throws AuthException if the user isn't authenticated or not allowed to use change edits
    * @throws InvalidChangeOperationException if the file does not exist
    * @throws PermissionBackendException
+   * @throws ResourceConflictException if the project state does not permit the operation
    */
   public void deleteFile(Repository repository, ChangeNotes notes, String file)
       throws AuthException, InvalidChangeOperationException, IOException, OrmException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     modifyTree(repository, notes, new DeleteFileModification(file));
   }
 
@@ -281,11 +288,12 @@ public class ChangeEditModifier {
    * @throws InvalidChangeOperationException if the file was already renamed to the specified new
    *     name
    * @throws PermissionBackendException
+   * @throws ResourceConflictException if the project state does not permit the operation
    */
   public void renameFile(
       Repository repository, ChangeNotes notes, String currentFilePath, String newFilePath)
       throws AuthException, InvalidChangeOperationException, IOException, OrmException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     modifyTree(repository, notes, new RenameFileModification(currentFilePath, newFilePath));
   }
 
@@ -303,14 +311,14 @@ public class ChangeEditModifier {
    */
   public void restoreFile(Repository repository, ChangeNotes notes, String file)
       throws AuthException, InvalidChangeOperationException, IOException, OrmException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     modifyTree(repository, notes, new RestoreFileModification(file));
   }
 
   private void modifyTree(
       Repository repository, ChangeNotes notes, TreeModification treeModification)
       throws AuthException, IOException, OrmException, InvalidChangeOperationException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceConflictException {
     assertCanEdit(notes);
 
     Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(notes);
@@ -355,7 +363,7 @@ public class ChangeEditModifier {
       PatchSet patchSet,
       List<TreeModification> treeModifications)
       throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException,
-          OrmException, PermissionBackendException {
+          OrmException, PermissionBackendException, ResourceConflictException {
     assertCanEdit(notes);
 
     Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(notes);
@@ -385,7 +393,8 @@ public class ChangeEditModifier {
     return createEdit(repository, notes, patchSet, newEditCommit, nowTimestamp);
   }
 
-  private void assertCanEdit(ChangeNotes notes) throws AuthException, PermissionBackendException {
+  private void assertCanEdit(ChangeNotes notes)
+      throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
     if (!currentUser.get().isIdentifiedUser()) {
       throw new AuthException("Authentication required");
     }
@@ -395,6 +404,7 @@ public class ChangeEditModifier {
           .database(reviewDb)
           .change(notes)
           .check(ChangePermission.ADD_PATCH_SET);
+      projectCache.checkedGet(notes.getProjectName()).checkStatePermitsWrite();
     } catch (AuthException denied) {
       throw new AuthException("edit not permitted", denied);
     }

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -1037,7 +1037,7 @@ class ReceiveCommits {
       // Must pass explicit user instead of injecting a provider into CreateRefControl, since
       // Provider<CurrentUser> within ReceiveCommits will always return anonymous.
       createRefControl.checkCreateRef(Providers.of(user), rp.getRepository(), branch, obj);
-    } catch (AuthException denied) {
+    } catch (AuthException | ResourceConflictException denied) {
       reject(cmd, "prohibited by Gerrit: " + denied.getMessage());
       return;
     }
@@ -2390,6 +2390,10 @@ class ReceiveCommits {
         return false;
       }
 
+      if (!projectState.statePermitsWrite()) {
+        reject(inputCommand, "cannot add patch set to " + ontoChange + ".");
+        return false;
+      }
       if (change.getStatus().isClosed()) {
         reject(inputCommand, "change " + ontoChange + " closed");
         return false;

java/com/google/gerrit/server/project/CreateRefControl.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -60,20 +61,20 @@ public class CreateRefControl {
    * @param object the object the user will start the reference with
    * @throws AuthException if creation is denied; the message explains the denial.
    * @throws PermissionBackendException on failure of permission checks.
+   * @throws ResourceConflictException if the project state does not permit the operation
    */
   public void checkCreateRef(
       Provider<? extends CurrentUser> user,
       Repository repo,
       Branch.NameKey branch,
       RevObject object)
-      throws AuthException, PermissionBackendException, NoSuchProjectException, IOException {
+      throws AuthException, PermissionBackendException, NoSuchProjectException, IOException,
+          ResourceConflictException {
     ProjectState ps = projectCache.checkedGet(branch.getParentKey());
     if (ps == null) {
       throw new NoSuchProjectException(branch.getParentKey());
     }
-    if (!ps.getProject().getState().permitsWrite()) {
+    ps.checkStatePermitsWrite();
-      throw new AuthException("project state does not permit write");
-    }
 
     PermissionBackend.ForRef perm = permissionBackend.user(user).ref(branch);
     if (object instanceof RevCommit) {

java/com/google/gerrit/server/project/ProjectState.java

@@ -32,6 +32,7 @@ import com.google.gerrit.common.data.SubscribeSection;
 import com.google.gerrit.extensions.api.projects.CommentLinkInfo;
 import com.google.gerrit.extensions.api.projects.ThemeInfo;
 import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.reviewdb.client.Branch;
@@ -258,6 +259,17 @@ public class ProjectState {
     return config.getMaxObjectSizeLimit();
   }
 
+  public boolean statePermitsWrite() {
+    return getProject().getState().permitsWrite();
+  }
+
+  public void checkStatePermitsWrite() throws ResourceConflictException {
+    if (!statePermitsWrite()) {
+      throw new ResourceConflictException(
+          "project state " + getProject().getState().name() + " does not permit write");
+    }
+  }
+
   /** Get the sections that pertain only to this project. */
   List<SectionMatcher> getLocalAccessSections() {
     List<SectionMatcher> sm = localAccessSections;

java/com/google/gerrit/server/project/RefControl.java

@@ -32,6 +32,7 @@ import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.permissions.RefPermission;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.util.MagicBranch;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.util.Providers;
 import java.util.ArrayList;
@@ -108,8 +109,9 @@ class RefControl {
 
   /** @return true if this user can add a new patch set to this ref */
   boolean canAddPatchSet() {
-    return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.ADD_PATCH_SET)
+    return projectControl
-        && isProjectStatePermittingWrite();
+        .controlForRef(MagicBranch.NEW_CHANGE + refName)
+        .canPerform(Permission.ADD_PATCH_SET);
   }
 
   /** @return true if this user can rebase changes on this ref */
@@ -563,7 +565,7 @@ class RefControl {
           return canPerform(Permission.CREATE_TAG);
 
         case UPDATE_BY_SUBMIT:
-          return projectControl.controlForRef("refs/for/" + refName).canSubmit(true);
+          return projectControl.controlForRef(MagicBranch.NEW_CHANGE + refName).canSubmit(true);
 
         case READ_PRIVATE_CHANGES:
           return canViewPrivateChanges();

java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java

@@ -130,6 +130,9 @@ public class CreateMergePatchSet
           UpdateException, PermissionBackendException {
     rsrc.permissions().database(db).check(ChangePermission.ADD_PATCH_SET);
 
+    ProjectState projectState = projectCache.checkedGet(rsrc.getProject());
+    projectState.checkStatePermitsWrite();
+
     MergeInput merge = in.merge;
     if (merge == null || Strings.isNullOrEmpty(merge.source)) {
       throw new BadRequestException("merge.source must be non-empty");
@@ -137,7 +140,6 @@ public class CreateMergePatchSet
     in.baseChange = Strings.nullToEmpty(in.baseChange).trim();
 
     PatchSet ps = psUtil.current(db.get(), rsrc.getNotes());
-    ProjectState projectState = projectCache.checkedGet(rsrc.getProject());
     Change change = rsrc.getChange();
     Project.NameKey project = change.getProject();
     Branch.NameKey dest = change.getDest();

java/com/google/gerrit/server/restapi/change/PutMessage.java

@@ -179,7 +179,7 @@ public class PutMessage
   }
 
   private void ensureCanEditCommitMessage(ChangeNotes changeNotes)
-      throws AuthException, PermissionBackendException {
+      throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
     if (!currentUserProvider.get().isIdentifiedUser()) {
       throw new AuthException("Authentication required");
     }
@@ -189,6 +189,7 @@ public class PutMessage
           .database(db.get())
           .change(changeNotes)
           .check(ChangePermission.ADD_PATCH_SET);
+      projectCache.checkedGet(changeNotes.getProjectName()).checkStatePermitsWrite();
     } catch (AuthException denied) {
       throw new AuthException("modifying commit message not permitted", denied);
     }