opendev/gerrit
Code Issues Proposed changes

Check delete change with PermissionBackend

Browse Source 
Deleting the entire change is slightly differently from deleting a
draft patch set.  Update only the change deletion path.

Update test assertions to expect the new generic messages thrown
by the PermissionBackend.

Change-Id: If5a83e386304d8f84db37f6c02f9c35cd5ae4266
This commit is contained in:
Shawn Pearce
2017-02-20 16:53:42 -08:00
committed by David Pursehouse
parent b5798658f5
commit 79ce708c38
8 changed files with 81 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -557,11 +557,10 @@ public class ChangeIT extends AbstractDaemonTest {
PushOneCommit.Result changeResult =
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
setApiUser(user);
exception.expect(AuthException.class);
exception.expectMessage(String.format("Deleting change %s is not permitted", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
}
@@ -605,11 +604,10 @@ public class ChangeIT extends AbstractDaemonTest {
try {
PushOneCommit.Result changeResult = createChange();
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
setApiUser(user);
exception.expect(AuthException.class);
exception.expectMessage(String.format("Deleting change %s is not permitted", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
} finally {
removePermission(Permission.DELETE_OWN_CHANGES, project, "refs/*");
@@ -633,13 +631,12 @@ public class ChangeIT extends AbstractDaemonTest {
PushOneCommit.Result changeResult =
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
setApiUser(user);
gApi.changes().id(changeId).abandon();
exception.expect(AuthException.class);
exception.expectMessage(String.format("Deleting change %s is not permitted", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
}
@@ -661,12 +658,11 @@ public class ChangeIT extends AbstractDaemonTest {
public void deleteMergedChange() throws Exception {
PushOneCommit.Result changeResult = createChange();
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
merge(changeResult);
exception.expect(MethodNotAllowedException.class);
exception.expectMessage(String.format("Deleting merged change %s is not allowed", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
}
@@ -679,13 +675,12 @@ public class ChangeIT extends AbstractDaemonTest {
PushOneCommit.Result changeResult =
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
merge(changeResult);
setApiUser(user);
exception.expect(MethodNotAllowedException.class);
exception.expectMessage(String.format("Deleting merged change %s is not allowed", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
} finally {
removePermission(Permission.DELETE_OWN_CHANGES, project, "refs/*");

3
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
View File

@@ -82,14 +82,13 @@ public class DraftChangeIT extends AbstractDaemonTest {
PushOneCommit.Result changeResult = createDraftChange();
changeResult.assertOkStatus();
String changeId = changeResult.getChangeId();
Change.Id id = changeResult.getChange().getId();
// The user needs to be able to see the draft change (which reviewers can).
gApi.changes().id(changeId).addReviewer(user.fullName);
setApiUser(user);
exception.expect(AuthException.class);
exception.expectMessage(String.format("Deleting change %s is not permitted", id));
exception.expectMessage("delete not permitted");
gApi.changes().id(changeId).delete();
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -375,7 +375,7 @@ class ChangeApiImpl implements ChangeApi {
public void delete() throws RestApiException {
try {
deleteChange.apply(change, null);
} catch (UpdateException e) {
} catch (UpdateException | PermissionBackendException e) {
throw new RestApiException("Cannot delete change", e);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@@ -262,7 +262,7 @@ class RevisionApiImpl implements RevisionApi {
public void delete() throws RestApiException {
try {
deleteDraft.apply(revision, null);
} catch (UpdateException e) {
} catch (UpdateException | OrmException | PermissionBackendException e) {
throw new RestApiException("Cannot delete draft ps", e);
}
}

63
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java
View File

@@ -15,20 +15,24 @@
package com.google.gerrit.server.change;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.AuthException;
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.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Status;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.DeleteChange.Input;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.Order;
import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -42,6 +46,8 @@ public class DeleteChange
private final Provider<ReviewDb> db;
private final BatchUpdate.Factory updateFactory;
private final Provider<DeleteChangeOp> opProvider;
private final Provider<CurrentUser> user;
private final PermissionBackend permissionBackend;
private final boolean allowDrafts;
@Inject
@@ -49,16 +55,33 @@ public class DeleteChange
Provider<ReviewDb> db,
BatchUpdate.Factory updateFactory,
Provider<DeleteChangeOp> opProvider,
Provider<CurrentUser> user,
PermissionBackend permissionBackend,
@GerritServerConfig Config cfg) {
this.db = db;
this.updateFactory = updateFactory;
this.opProvider = opProvider;
this.user = user;
this.permissionBackend = permissionBackend;
this.allowDrafts = DeleteChangeOp.allowDrafts(cfg);
}
@Override
public Response<?> apply(ChangeResource rsrc, Input input)
throws RestApiException, UpdateException {
throws RestApiException, UpdateException, PermissionBackendException {
if (rsrc.getChange().getStatus() == Change.Status.MERGED) {
throw new MethodNotAllowedException("delete not permitted");
} else if (!allowDrafts && rsrc.getChange().getStatus() == Change.Status.DRAFT) {
// If drafts are disabled, only an administrator can delete a draft.
try {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
throw new MethodNotAllowedException("Draft workflow is disabled");
}
} else {
rsrc.permissions().database(db).check(ChangePermission.DELETE);
}
try (BatchUpdate bu =
updateFactory.create(db.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
Change.Id id = rsrc.getChange().getId();
@@ -71,21 +94,33 @@ public class DeleteChange
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
try {
Change.Status status = rsrc.getChange().getStatus();
ChangeControl changeControl = rsrc.getControl();
boolean visible =
isActionAllowed(changeControl, status) && changeControl.canDelete(db.get(), status);
PermissionBackend.ForChange perm = rsrc.permissions().database(db);
return new UiAction.Description()
.setLabel("Delete")
.setTitle("Delete change " + rsrc.getId())
.setVisible(visible);
} catch (OrmException e) {
throw new IllegalStateException(e);
}
.setVisible(couldDeleteWhenIn(status) && perm.testOrFalse(ChangePermission.DELETE));
}
private boolean isActionAllowed(ChangeControl changeControl, Status status) {
return status != Status.DRAFT || allowDrafts || changeControl.isAdmin();
private boolean couldDeleteWhenIn(Change.Status status) {
switch (status) {
case NEW:
case ABANDONED:
// New or abandoned changes can be deleted with the right permissions.
return true;
case MERGED:
// Merged changes should never be deleted.
return false;
case DRAFT:
if (allowDrafts) {
// Drafts can only be deleted if the server has drafts enabled.
return true;
}
// If drafts are disabled, only administrators may delete.
return permissionBackend.user(user).testOrFalse(GlobalPermission.ADMINISTRATE_SERVER);
}
return false;
}
}

17
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -27,7 +26,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.BatchUpdateReviewDb;
@@ -62,7 +60,6 @@ class DeleteChangeOp implements BatchUpdateOp {
private final PatchSetUtil psUtil;
private final StarredChangesUtil starredChangesUtil;
private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
private final boolean allowDrafts;
private Change.Id id;
@@ -70,12 +67,10 @@ class DeleteChangeOp implements BatchUpdateOp {
DeleteChangeOp(
PatchSetUtil psUtil,
StarredChangesUtil starredChangesUtil,
DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
@GerritServerConfig Config cfg) {
DynamicItem<AccountPatchReviewStore> accountPatchReviewStore) {
this.psUtil = psUtil;
this.starredChangesUtil = starredChangesUtil;
this.accountPatchReviewStore = accountPatchReviewStore;
this.allowDrafts = allowDrafts(cfg);
}
@Override
@@ -99,8 +94,7 @@ class DeleteChangeOp implements BatchUpdateOp {
}
private void ensureDeletable(ChangeContext ctx, Change.Id id, Collection<PatchSet> patchSets)
throws ResourceConflictException, MethodNotAllowedException, OrmException, AuthException,
IOException {
throws ResourceConflictException, MethodNotAllowedException, IOException {
Change.Status status = ctx.getChange().getStatus();
if (status == Change.Status.MERGED) {
throw new MethodNotAllowedException("Deleting merged change " + id + " is not allowed");
@@ -114,14 +108,7 @@ class DeleteChangeOp implements BatchUpdateOp {
}
}
if (!ctx.getControl().canDelete(ctx.getDb(), status)) {
throw new AuthException("Deleting change " + id + " is not permitted");
}
if (status == Change.Status.DRAFT) {
if (!allowDrafts && !ctx.getControl().isAdmin()) {
throw new MethodNotAllowedException("Draft workflow is disabled");
}
for (PatchSet ps : patchSets) {
if (!ps.isDraft()) {
throw new ResourceConflictException(

21
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.change;
import com.google.common.collect.Iterables;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -32,6 +33,8 @@ import com.google.gerrit.server.change.DeleteDraftPatchSet.Input;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -81,7 +84,12 @@ public class DeleteDraftPatchSet
@Override
public Response<?> apply(RevisionResource rsrc, Input input)
throws RestApiException, UpdateException {
throws RestApiException, UpdateException, OrmException, PermissionBackendException {
if (isDeletingOnlyPatchSet(rsrc)) {
// A change cannot have zero patch sets; the change is deleted instead.
rsrc.permissions().database(db).check(ChangePermission.DELETE);
}
try (BatchUpdate bu =
updateFactory.create(db.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
bu.setOrder(Order.DB_BEFORE_REPO);
@@ -91,6 +99,12 @@ public class DeleteDraftPatchSet
return Response.none();
}
private boolean isDeletingOnlyPatchSet(RevisionResource rsrc) throws OrmException {
Collection<PatchSet> patchSets = psUtil.byChange(db.get(), rsrc.getNotes());
return patchSets.size() == 1
&& Iterables.getOnlyElement(patchSets).getId().equals(rsrc.getPatchSet().getId());
}
private class Op implements BatchUpdateOp {
private final PatchSet.Id psId;
@@ -183,15 +197,14 @@ public class DeleteDraftPatchSet
@Override
public UiAction.Description getDescription(RevisionResource rsrc) {
try {
int psCount = psUtil.byChange(db.get(), rsrc.getNotes()).size();
return new UiAction.Description()
.setLabel("Delete")
.setTitle(String.format("Delete draft revision %d", rsrc.getPatchSet().getPatchSetId()))
.setVisible(
allowDrafts
&& rsrc.getPatchSet().isDraft()
&& rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT)
&& psCount > 1);
&& psUtil.byChange(db.get(), rsrc.getNotes()).size() > 1
&& rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT));
} catch (OrmException e) {
throw new IllegalStateException(e);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -275,7 +275,7 @@ public class ChangeControl {
switch (status) {
case DRAFT:
return (isOwner() || getRefControl().canDeleteDrafts());
return isOwner() || getRefControl().canDeleteDrafts() || isAdmin();
case NEW:
case ABANDONED:
return (isAdmin() || (isOwner() && getRefControl().canDeleteOwnChanges()));