From 9e1fb490c1dcb97150739a9411e9579e9932ecf2 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 12 Jan 2018 11:47:57 +0100 Subject: [PATCH] Make project state check in DELETE 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. Change-Id: I261a535527bdfb2ae5449781115537cd7a71082c --- .../server/git/receive/ReceiveCommits.java | 2 +- .../google/gerrit/server/project/RefControl.java | 2 +- .../server/restapi/project/CreateBranch.java | 1 + .../gerrit/server/restapi/project/CreateTag.java | 2 +- .../server/restapi/project/DeleteBranch.java | 1 + .../gerrit/server/restapi/project/DeleteRef.java | 4 ++++ .../gerrit/server/restapi/project/DeleteTag.java | 1 + .../server/restapi/project/ListBranches.java | 12 ++++++++++-- .../gerrit/server/restapi/project/ListTags.java | 16 +++++++--------- 9 files changed, 27 insertions(+), 14 deletions(-) diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 74bf1168a2..5ee90172fa 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -1120,7 +1120,7 @@ class ReceiveCommits { private boolean canDelete(ReceiveCommand cmd) throws PermissionBackendException { try { permissions.ref(cmd.getRefName()).check(RefPermission.DELETE); - return true; + return projectState.statePermitsWrite(); } catch (AuthException e) { return false; } diff --git a/java/com/google/gerrit/server/project/RefControl.java b/java/com/google/gerrit/server/project/RefControl.java index 329b3a969b..552336a8ea 100644 --- a/java/com/google/gerrit/server/project/RefControl.java +++ b/java/com/google/gerrit/server/project/RefControl.java @@ -272,7 +272,7 @@ class RefControl { * @return {@code true} if the user specified can delete a Git ref. */ private boolean canDelete() { - if (!isProjectStatePermittingWrite() || (RefNames.REFS_CONFIG.equals(refName))) { + if (RefNames.REFS_CONFIG.equals(refName)) { // Never allow removal of the refs/meta/config branch. // Deleting the branch would destroy all Gerrit specific // metadata about the project, including its access rules. diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java index 0b62c154e9..38bc98257a 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java +++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java @@ -183,6 +183,7 @@ public class CreateBranch implements RestModifyView { result.getObjectId(), identifiedUser.get().getAccount()); try (RevWalk w = new RevWalk(repo)) { - return ListTags.createTagInfo(perm, result, w, resource.getNameKey(), links); + return ListTags.createTagInfo(perm, result, w, resource.getProjectState(), links); } } } catch (InvalidRevisionException e) { diff --git a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java index 09bbca92b3..3114f8a32d 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteBranch.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteBranch.java @@ -57,6 +57,7 @@ public class DeleteBranch implements RestModifyView { public Response apply(BranchResource rsrc, Input input) throws RestApiException, OrmException, IOException, PermissionBackendException { permissionBackend.user(user).ref(rsrc.getBranchKey()).check(RefPermission.DELETE); + rsrc.getProjectState().checkStatePermitsWrite(); if (!queryProvider.get().setLimit(1).byBranchOpen(rsrc.getBranchKey()).isEmpty()) { throw new ResourceConflictException("branch " + rsrc.getBranchKey() + " has open changes"); diff --git a/java/com/google/gerrit/server/restapi/project/DeleteRef.java b/java/com/google/gerrit/server/restapi/project/DeleteRef.java index b1b575b3fd..7bd1c4a79f 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteRef.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteRef.java @@ -235,6 +235,10 @@ public class DeleteRef { "it doesn't exist or you do not have permission to delete it"); } + if (!project.getProjectState().statePermitsWrite()) { + command.setResult(Result.REJECTED_OTHER_REASON, "project state does not permit write"); + } + if (!refName.startsWith(R_TAGS)) { Branch.NameKey branchKey = new Branch.NameKey(project.getNameKey(), ref.getName()); if (!queryProvider.get().setLimit(1).byBranchOpen(branchKey).isEmpty()) { diff --git a/java/com/google/gerrit/server/restapi/project/DeleteTag.java b/java/com/google/gerrit/server/restapi/project/DeleteTag.java index cce710389b..f432129e96 100644 --- a/java/com/google/gerrit/server/restapi/project/DeleteTag.java +++ b/java/com/google/gerrit/server/restapi/project/DeleteTag.java @@ -56,6 +56,7 @@ public class DeleteTag implements RestModifyView { .project(resource.getNameKey()) .ref(tag) .check(RefPermission.DELETE); + resource.getProjectState().checkStatePermitsWrite(); deleteRefFactory.create(resource).ref(tag).delete(); return Response.none(); } diff --git a/java/com/google/gerrit/server/restapi/project/ListBranches.java b/java/com/google/gerrit/server/restapi/project/ListBranches.java index 5675be1068..416a5c2f1d 100644 --- a/java/com/google/gerrit/server/restapi/project/ListBranches.java +++ b/java/com/google/gerrit/server/restapi/project/ListBranches.java @@ -204,7 +204,11 @@ public class ListBranches implements RestReadView { branches.add(b); if (!Constants.HEAD.equals(ref.getName())) { - b.canDelete = perm.ref(ref.getName()).testOrFalse(RefPermission.DELETE) ? true : null; + b.canDelete = + perm.ref(ref.getName()).testOrFalse(RefPermission.DELETE) + && rsrc.getProjectState().statePermitsWrite() + ? true + : null; } continue; } @@ -248,7 +252,11 @@ public class ListBranches implements RestReadView { info.ref = ref.getName(); info.revision = ref.getObjectId() != null ? ref.getObjectId().name() : null; info.canDelete = - !targets.contains(ref.getName()) && perm.testOrFalse(RefPermission.DELETE) ? true : null; + !targets.contains(ref.getName()) + && perm.testOrFalse(RefPermission.DELETE) + && projectState.statePermitsWrite() + ? true + : null; BranchResource rsrc = new BranchResource(projectState, user, ref); for (UiAction.Description d : uiActions.from(branchViews, rsrc)) { diff --git a/java/com/google/gerrit/server/restapi/project/ListTags.java b/java/com/google/gerrit/server/restapi/project/ListTags.java index a2b7082681..af4586ca87 100644 --- a/java/com/google/gerrit/server/restapi/project/ListTags.java +++ b/java/com/google/gerrit/server/restapi/project/ListTags.java @@ -140,7 +140,8 @@ public class ListTags implements RestReadView { visibleTags( resource.getProjectState(), repo, repo.getRefDatabase().getRefs(Constants.R_TAGS)); for (Ref ref : all.values()) { - tags.add(createTagInfo(perm.ref(ref.getName()), ref, rw, resource.getNameKey(), links)); + tags.add( + createTagInfo(perm.ref(ref.getName()), ref, rw, resource.getProjectState(), links)); } } @@ -180,7 +181,7 @@ public class ListTags implements RestReadView { .ref(ref.getName()), ref, rw, - resource.getNameKey(), + resource.getProjectState(), links); } } @@ -188,15 +189,12 @@ public class ListTags implements RestReadView { } public static TagInfo createTagInfo( - PermissionBackend.ForRef perm, - Ref ref, - RevWalk rw, - Project.NameKey projectName, - WebLinks links) + PermissionBackend.ForRef perm, Ref ref, RevWalk rw, ProjectState projectState, WebLinks links) throws MissingObjectException, IOException { RevObject object = rw.parseAny(ref.getObjectId()); - Boolean canDelete = perm.testOrFalse(RefPermission.DELETE) ? true : null; - List webLinks = links.getTagLinks(projectName.get(), ref.getName()); + Boolean canDelete = + perm.testOrFalse(RefPermission.DELETE) && projectState.statePermitsWrite() ? true : null; + List webLinks = links.getTagLinks(projectState.getName(), ref.getName()); if (object instanceof RevTag) { // Annotated or signed tag RevTag tag = (RevTag) object;