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
This commit is contained in:
Patrick Hiesel
2018-01-12 11:47:57 +01:00
parent 3ea2679096
commit 9e1fb490c1
9 changed files with 27 additions and 14 deletions

View File

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

View File

@@ -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.

View File

@@ -183,6 +183,7 @@ public class CreateBranch implements RestModifyView<ProjectResource, BranchInput
info.revision = revid.getName();
info.canDelete =
permissionBackend.user(identifiedUser).ref(name).testOrFalse(RefPermission.DELETE)
&& rsrc.getProjectState().statePermitsWrite()
? true
: null;
return info;

View File

@@ -147,7 +147,7 @@ public class CreateTag implements RestModifyView<ProjectResource, TagInput> {
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) {

View File

@@ -57,6 +57,7 @@ public class DeleteBranch implements RestModifyView<BranchResource, Input> {
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");

View File

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

View File

@@ -56,6 +56,7 @@ public class DeleteTag implements RestModifyView<TagResource, Input> {
.project(resource.getNameKey())
.ref(tag)
.check(RefPermission.DELETE);
resource.getProjectState().checkStatePermitsWrite();
deleteRefFactory.create(resource).ref(tag).delete();
return Response.none();
}

View File

@@ -204,7 +204,11 @@ public class ListBranches implements RestReadView<ProjectResource> {
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<ProjectResource> {
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)) {

View File

@@ -140,7 +140,8 @@ public class ListTags implements RestReadView<ProjectResource> {
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<ProjectResource> {
.ref(ref.getName()),
ref,
rw,
resource.getNameKey(),
resource.getProjectState(),
links);
}
}
@@ -188,15 +189,12 @@ public class ListTags implements RestReadView<ProjectResource> {
}
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<WebLinkInfo> webLinks = links.getTagLinks(projectName.get(), ref.getName());
Boolean canDelete =
perm.testOrFalse(RefPermission.DELETE) && projectState.statePermitsWrite() ? true : null;
List<WebLinkInfo> webLinks = links.getTagLinks(projectState.getName(), ref.getName());
if (object instanceof RevTag) {
// Annotated or signed tag
RevTag tag = (RevTag) object;