Merge changes I6a7f48e4,I3d0aacb0,I0b43eaef,I909a779b

* changes:
  DeleteRef: make sure permission and project state is checked
  DeleteRef: expose separate methods for deleting single/multi refs
  DeleteRef: remove its factory
  DeleteRef: move required input to method parameters
This commit is contained in:
xchangcheng
2018-07-18 11:18:19 +00:00
committed by Gerrit Code Review
7 changed files with 183 additions and 157 deletions

View File

@@ -23,9 +23,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -38,17 +36,12 @@ import java.io.IOException;
public class DeleteBranch implements RestModifyView<BranchResource, Input> {
private final Provider<InternalChangeQuery> queryProvider;
private final DeleteRef.Factory deleteRefFactory;
private final PermissionBackend permissionBackend;
private final DeleteRef deleteRef;
@Inject
DeleteBranch(
Provider<InternalChangeQuery> queryProvider,
DeleteRef.Factory deleteRefFactory,
PermissionBackend permissionBackend) {
DeleteBranch(Provider<InternalChangeQuery> queryProvider, DeleteRef deleteRef) {
this.queryProvider = queryProvider;
this.deleteRefFactory = deleteRefFactory;
this.permissionBackend = permissionBackend;
this.deleteRef = deleteRef;
}
@Override
@@ -60,14 +53,11 @@ public class DeleteBranch implements RestModifyView<BranchResource, Input> {
"not allowed to delete branch " + rsrc.getBranchKey().get());
}
permissionBackend.currentUser().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");
}
deleteRefFactory.create(rsrc).ref(rsrc.getRef()).prefix(R_HEADS).delete();
deleteRef.deleteSingleRef(rsrc.getProjectState(), rsrc.getRef(), R_HEADS);
return Response.none();
}
}

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.project;
import static org.eclipse.jgit.lib.Constants.R_HEADS;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,11 +31,11 @@ import java.io.IOException;
@Singleton
public class DeleteBranches implements RestModifyView<ProjectResource, DeleteBranchesInput> {
private final DeleteRef.Factory deleteRefFactory;
private final DeleteRef deleteRef;
@Inject
DeleteBranches(DeleteRef.Factory deleteRefFactory) {
this.deleteRefFactory = deleteRefFactory;
DeleteBranches(DeleteRef deleteRef) {
this.deleteRef = deleteRef;
}
@Override
@@ -43,7 +44,8 @@ public class DeleteBranches implements RestModifyView<ProjectResource, DeleteBra
if (input == null || input.branches == null || input.branches.isEmpty()) {
throw new BadRequestException("branches must be specified");
}
deleteRefFactory.create(project).refs(input.branches).prefix(R_HEADS).delete();
deleteRef.deleteMultipleRefs(
project.getProjectState(), ImmutableSet.copyOf(input.branches), R_HEADS);
return Response.none();
}
}

View File

@@ -14,33 +14,35 @@
package com.google.gerrit.server.restapi.project;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef;
import static java.lang.String.format;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_REFS;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.DELETE;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
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.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefValidationHelper;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.errors.LockFailedException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -52,6 +54,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.transport.ReceiveCommand.Result;
@Singleton
public class DeleteRef {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -64,13 +67,6 @@ public class DeleteRef {
private final GitReferenceUpdated referenceUpdated;
private final RefValidationHelper refDeletionValidator;
private final Provider<InternalChangeQuery> queryProvider;
private final ProjectResource resource;
private final List<String> refsToDelete;
private String prefix;
public interface Factory {
DeleteRef create(ProjectResource r);
}
@Inject
DeleteRef(
@@ -79,135 +75,164 @@ public class DeleteRef {
GitRepositoryManager repoManager,
GitReferenceUpdated referenceUpdated,
RefValidationHelper.Factory refDeletionValidatorFactory,
Provider<InternalChangeQuery> queryProvider,
@Assisted ProjectResource resource) {
Provider<InternalChangeQuery> queryProvider) {
this.identifiedUser = identifiedUser;
this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.referenceUpdated = referenceUpdated;
this.refDeletionValidator = refDeletionValidatorFactory.create(DELETE);
this.queryProvider = queryProvider;
this.resource = resource;
this.refsToDelete = new ArrayList<>();
}
public DeleteRef ref(String ref) {
this.refsToDelete.add(ref);
return this;
/**
* Deletes a single ref from the repository.
*
* @param projectState the {@code ProjectState} of the project containing the target ref.
* @param ref the ref to be deleted.
* @throws IOException
* @throws ResourceConflictException
*/
public void deleteSingleRef(ProjectState projectState, String ref)
throws IOException, ResourceConflictException, AuthException, PermissionBackendException {
deleteSingleRef(projectState, ref, null);
}
public DeleteRef refs(List<String> refs) {
this.refsToDelete.addAll(refs);
return this;
}
public DeleteRef prefix(String prefix) {
this.prefix = prefix;
return this;
}
public void delete()
throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
if (!refsToDelete.isEmpty()) {
try (Repository r = repoManager.openRepository(resource.getNameKey())) {
if (refsToDelete.size() == 1) {
deleteSingleRef(r);
} else {
deleteMultipleRefs(r);
}
}
}
}
private void deleteSingleRef(Repository r) throws IOException, ResourceConflictException {
String ref = refsToDelete.get(0);
/**
* Deletes a single ref from the repository.
*
* @param projectState the {@code ProjectState} of the project containing the target ref.
* @param ref the ref to be deleted.
* @param prefix the prefix of the ref.
* @throws IOException
* @throws ResourceConflictException
*/
public void deleteSingleRef(ProjectState projectState, String ref, @Nullable String prefix)
throws IOException, ResourceConflictException, AuthException, PermissionBackendException {
if (prefix != null && !ref.startsWith(R_REFS)) {
ref = prefix + ref;
}
RefUpdate.Result result;
RefUpdate u = r.updateRef(ref);
u.setExpectedOldObjectId(r.exactRef(ref).getObjectId());
u.setNewObjectId(ObjectId.zeroId());
u.setForceUpdate(true);
refDeletionValidator.validateRefOperation(resource.getName(), identifiedUser.get(), u);
int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
for (; ; ) {
try {
result = u.delete();
} catch (LockFailedException e) {
result = RefUpdate.Result.LOCK_FAILURE;
} catch (IOException e) {
logger.atSevere().withCause(e).log("Cannot delete %s", ref);
throw e;
}
if (result == RefUpdate.Result.LOCK_FAILURE && --remainingLockFailureCalls > 0) {
projectState.checkStatePermitsWrite();
permissionBackend
.currentUser()
.project(projectState.getNameKey())
.ref(ref)
.check(RefPermission.DELETE);
try (Repository repository = repoManager.openRepository(projectState.getNameKey())) {
RefUpdate.Result result;
RefUpdate u = repository.updateRef(ref);
u.setExpectedOldObjectId(repository.exactRef(ref).getObjectId());
u.setNewObjectId(ObjectId.zeroId());
u.setForceUpdate(true);
refDeletionValidator.validateRefOperation(projectState.getName(), identifiedUser.get(), u);
int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
for (; ; ) {
try {
Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
} catch (InterruptedException ie) {
// ignore
result = u.delete();
} catch (LockFailedException e) {
result = RefUpdate.Result.LOCK_FAILURE;
} catch (IOException e) {
logger.atSevere().withCause(e).log("Cannot delete %s", ref);
throw e;
}
if (result == RefUpdate.Result.LOCK_FAILURE && --remainingLockFailureCalls > 0) {
try {
Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
} catch (InterruptedException ie) {
// ignore
}
} else {
break;
}
} else {
break;
}
}
switch (result) {
case NEW:
case NO_CHANGE:
case FAST_FORWARD:
case FORCED:
referenceUpdated.fire(
resource.getNameKey(), u, ReceiveCommand.Type.DELETE, identifiedUser.get().state());
break;
switch (result) {
case NEW:
case NO_CHANGE:
case FAST_FORWARD:
case FORCED:
referenceUpdated.fire(
projectState.getNameKey(),
u,
ReceiveCommand.Type.DELETE,
identifiedUser.get().state());
break;
case REJECTED_CURRENT_BRANCH:
logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
throw new ResourceConflictException("cannot delete current branch");
case REJECTED_CURRENT_BRANCH:
logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
throw new ResourceConflictException("cannot delete current branch");
case IO_FAILURE:
case LOCK_FAILURE:
case NOT_ATTEMPTED:
case REJECTED:
case RENAMED:
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
default:
logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
throw new ResourceConflictException("cannot delete: " + result.name());
case IO_FAILURE:
case LOCK_FAILURE:
case NOT_ATTEMPTED:
case REJECTED:
case RENAMED:
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
default:
logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
throw new ResourceConflictException("cannot delete: " + result.name());
}
}
}
private void deleteMultipleRefs(Repository r)
throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate();
batchUpdate.setAtomic(false);
List<String> refs =
prefix == null
? refsToDelete
: refsToDelete
.stream()
.map(ref -> ref.startsWith(R_REFS) ? ref : prefix + ref)
.collect(toList());
for (String ref : refs) {
batchUpdate.addCommand(createDeleteCommand(resource, r, ref));
/**
* Deletes a set of refs from the repository.
*
* @param projectState the {@code ProjectState} of the project whose refs are to be deleted.
* @param refsToDelete the refs to be deleted.
* @param prefix the prefix of the refs.
* @throws OrmException
* @throws IOException
* @throws ResourceConflictException
* @throws PermissionBackendException
*/
public void deleteMultipleRefs(
ProjectState projectState, ImmutableSet<String> refsToDelete, String prefix)
throws OrmException, IOException, ResourceConflictException, PermissionBackendException,
AuthException {
if (refsToDelete.isEmpty()) {
return;
}
try (RevWalk rw = new RevWalk(r)) {
batchUpdate.execute(rw, NullProgressMonitor.INSTANCE);
if (refsToDelete.size() == 1) {
deleteSingleRef(projectState, Iterables.getOnlyElement(refsToDelete), prefix);
return;
}
StringBuilder errorMessages = new StringBuilder();
for (ReceiveCommand command : batchUpdate.getCommands()) {
if (command.getResult() == Result.OK) {
postDeletion(resource, command);
} else {
appendAndLogErrorMessage(errorMessages, command);
try (Repository repository = repoManager.openRepository(projectState.getNameKey())) {
BatchRefUpdate batchUpdate = repository.getRefDatabase().newBatchUpdate();
batchUpdate.setAtomic(false);
ImmutableSet<String> refs =
prefix == null
? refsToDelete
: refsToDelete
.stream()
.map(ref -> ref.startsWith(R_REFS) ? ref : prefix + ref)
.collect(toImmutableSet());
for (String ref : refs) {
batchUpdate.addCommand(createDeleteCommand(projectState, repository, ref));
}
try (RevWalk rw = new RevWalk(repository)) {
batchUpdate.execute(rw, NullProgressMonitor.INSTANCE);
}
StringBuilder errorMessages = new StringBuilder();
for (ReceiveCommand command : batchUpdate.getCommands()) {
if (command.getResult() == Result.OK) {
postDeletion(projectState.getNameKey(), command);
} else {
appendAndLogErrorMessage(errorMessages, command);
}
}
if (errorMessages.length() > 0) {
throw new ResourceConflictException(errorMessages.toString());
}
}
if (errorMessages.length() > 0) {
throw new ResourceConflictException(errorMessages.toString());
}
}
private ReceiveCommand createDeleteCommand(ProjectResource project, Repository r, String refName)
private ReceiveCommand createDeleteCommand(
ProjectState projectState, Repository r, String refName)
throws OrmException, IOException, ResourceConflictException, PermissionBackendException {
Ref ref = r.getRefDatabase().getRef(refName);
ReceiveCommand command;
@@ -227,7 +252,7 @@ public class DeleteRef {
try {
permissionBackend
.currentUser()
.project(project.getNameKey())
.project(projectState.getNameKey())
.ref(refName)
.check(RefPermission.DELETE);
} catch (AuthException denied) {
@@ -237,12 +262,12 @@ public class DeleteRef {
}
}
if (!project.getProjectState().statePermitsWrite()) {
if (!projectState.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());
Branch.NameKey branchKey = new Branch.NameKey(projectState.getNameKey(), ref.getName());
if (!queryProvider.get().setLimit(1).byBranchOpen(branchKey).isEmpty()) {
command.setResult(Result.REJECTED_OTHER_REASON, "it has open changes");
}
@@ -252,7 +277,7 @@ public class DeleteRef {
u.setForceUpdate(true);
u.setExpectedOldObjectId(r.exactRef(refName).getObjectId());
u.setNewObjectId(ObjectId.zeroId());
refDeletionValidator.validateRefOperation(project.getName(), identifiedUser.get(), u);
refDeletionValidator.validateRefOperation(projectState.getName(), identifiedUser.get(), u);
return command;
}
@@ -281,7 +306,7 @@ public class DeleteRef {
errorMessages.append("\n");
}
private void postDeletion(ProjectResource project, ReceiveCommand cmd) {
referenceUpdated.fire(project.getNameKey(), cmd, identifiedUser.get().state());
private void postDeletion(Project.NameKey project, ReceiveCommand cmd) {
referenceUpdated.fire(project, cmd, identifiedUser.get().state());
}
}

View File

@@ -21,9 +21,7 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.RefUtil;
import com.google.gerrit.server.project.TagResource;
import com.google.gwtorm.server.OrmException;
@@ -34,13 +32,11 @@ import java.io.IOException;
@Singleton
public class DeleteTag implements RestModifyView<TagResource, Input> {
private final PermissionBackend permissionBackend;
private final DeleteRef.Factory deleteRefFactory;
private final DeleteRef deleteRef;
@Inject
DeleteTag(PermissionBackend permissionBackend, DeleteRef.Factory deleteRefFactory) {
this.permissionBackend = permissionBackend;
this.deleteRefFactory = deleteRefFactory;
DeleteTag(DeleteRef deleteRef) {
this.deleteRef = deleteRef;
}
@Override
@@ -53,13 +49,7 @@ public class DeleteTag implements RestModifyView<TagResource, Input> {
throw new MethodNotAllowedException("not allowed to delete " + tag);
}
permissionBackend
.currentUser()
.project(resource.getNameKey())
.ref(tag)
.check(RefPermission.DELETE);
resource.getProjectState().checkStatePermitsWrite();
deleteRefFactory.create(resource).ref(tag).delete();
deleteRef.deleteSingleRef(resource.getProjectState(), tag);
return Response.none();
}
}

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.project;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.api.projects.DeleteTagsInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
@@ -30,11 +31,11 @@ import java.io.IOException;
@Singleton
public class DeleteTags implements RestModifyView<ProjectResource, DeleteTagsInput> {
private final DeleteRef.Factory deleteRefFactory;
private final DeleteRef deleteRef;
@Inject
DeleteTags(DeleteRef.Factory deleteRefFactory) {
this.deleteRefFactory = deleteRefFactory;
DeleteTags(DeleteRef deleteRef) {
this.deleteRef = deleteRef;
}
@Override
@@ -43,7 +44,8 @@ public class DeleteTags implements RestModifyView<ProjectResource, DeleteTagsInp
if (input == null || input.tags == null || input.tags.isEmpty()) {
throw new BadRequestException("tags must be specified");
}
deleteRefFactory.create(project).refs(input.tags).prefix(R_TAGS).delete();
deleteRef.deleteMultipleRefs(
project.getProjectState(), ImmutableSet.copyOf(input.tags), R_TAGS);
return Response.none();
}
}

View File

@@ -104,7 +104,6 @@ public class Module extends RestApiModule {
put(PROJECT_KIND, "config").to(PutConfig.class);
post(COMMIT_KIND, "cherrypick").to(CherryPickCommit.class);
factory(DeleteRef.Factory.class);
factory(ProjectNode.Factory.class);
}
}

View File

@@ -28,6 +28,7 @@ import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.api.projects.ProjectApi;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -64,7 +65,24 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
}
@Test
public void deleteBranchesForbidden() throws Exception {
public void deleteOneBranchWithoutPermissionForbidden() throws Exception {
ImmutableList<String> branchToDelete = ImmutableList.of("refs/heads/test-1");
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = branchToDelete;
setApiUser(user);
try {
project().deleteBranches(input);
fail("Expected AuthException");
} catch (AuthException e) {
assertThat(e).hasMessageThat().isEqualTo("delete not permitted for refs/heads/test-1");
}
setApiUser(admin);
assertBranches(BRANCHES);
}
@Test
public void deleteMultiBranchesWithoutPermissionForbidden() throws Exception {
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = BRANCHES;
setApiUser(user);