Make project state check in ADD_PATCH_SET explicit

The majority of code in {Project,Ref,Change}Control is now about
permissions, but not all. Exceptions include checks for a project's
state. This is confusing, because users are presented with an exception
telling them that they lack some kind of permission while the real
reason for the failed operation is that the project's current state
doesn't permit the operation.

This is part of a series of commits to remove all project state checks
from *Control classes and make explicit checks instead.

This commit also adds more documentation to the individual states and
adds a convenience method for validating that a project is writeable.

Change-Id: I3edfe80e745657a1fd41e7ddaf019c9dee4fa8f6

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;
     }
@@ -2391,6 +2391,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()) {
-      throw new AuthException("project state does not permit write");
-    }
+    ps.checkStatePermitsWrite();
 
     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)
-        && isProjectStatePermittingWrite();
+    return projectControl
+        .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);
     }