diff --git a/doc/release-notes/11253-inconsistent-dataset-delete-api.md b/doc/release-notes/11253-inconsistent-dataset-delete-api.md new file mode 100644 index 00000000000..387edca329f --- /dev/null +++ b/doc/release-notes/11253-inconsistent-dataset-delete-api.md @@ -0,0 +1,2 @@ +## Bug Fix +When calling DELETE "/api/datasets/{id}", of a released dataset, as a superuser, the call will no longer be 'upgraded' to a dataset destroy action. The user will receive an 'unauthorized' response with the message to call the API with /destroy in order to delete the dataset. diff --git a/src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java b/src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java index 4821d0c7289..daa483bb69c 100644 --- a/src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java +++ b/src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java @@ -249,7 +249,7 @@ public boolean canIssueUpdateDatasetCommand(DvObject dvo){ // DELETE DATASET public boolean canIssueDeleteDatasetCommand(DvObject dvo){ - return canIssueCommand(dvo, DeleteDatasetCommand.class); + return canIssueCommand(dvo, DeleteDatasetVersionCommand.class); } // PUBLISH DATASET diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java index 136b6dbb69b..8b0b0144689 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java @@ -297,40 +297,25 @@ public Response exportDataset(@Context ContainerRequestContext crc, @QueryParam( @AuthRequired @Path("{id}") public Response deleteDataset(@Context ContainerRequestContext crc, @PathParam("id") String id) { - // Internally, "DeleteDatasetCommand" simply redirects to "DeleteDatasetVersionCommand" - // (and there's a comment that says "TODO: remove this command") - // do we need an exposed API call for it? - // And DeleteDatasetVersionCommand further redirects to DestroyDatasetCommand, - // if the dataset only has 1 version... In other words, the functionality - // currently provided by this API is covered between the "deleteDraftVersion" and - // "destroyDataset" API calls. - // (The logic below follows the current implementation of the underlying - // commands!) - User u = getRequestUser(crc); return response( req -> { Dataset doomed = findDatasetOrDie(id); DatasetVersion doomedVersion = doomed.getLatestVersion(); - boolean destroy = false; - if (doomed.getVersions().size() == 1) { - if (doomed.isReleased() && (!(u instanceof AuthenticatedUser) || !u.isSuperuser())) { - throw new WrappedResponse(error(Response.Status.UNAUTHORIZED, "Only superusers can delete published datasets")); - } - destroy = true; - } else { - if (!doomedVersion.isDraft()) { - throw new WrappedResponse(error(Response.Status.UNAUTHORIZED, "This is a published dataset with multiple versions. This API can only delete the latest version if it is a DRAFT")); + if (!doomedVersion.isDraft()) { + String msg = "This API can only delete the latest version if it is a DRAFT."; + if (u.isSuperuser()) { + msg += " Please use '/destroy' to delete the published version"; } + throw new WrappedResponse(error(Response.Status.UNAUTHORIZED, msg)); } - // Gather the locations of the physical files that will need to be - // deleted once the destroy command execution has been finalized: - Map deleteStorageLocations = fileService.getPhysicalFilesToDelete(doomedVersion, destroy); + // Gather the locations of the physical files that will need to be deleted + Map deleteStorageLocations = fileService.getPhysicalFilesToDelete(doomedVersion); - execCommand( new DeleteDatasetCommand(req, findDatasetOrDie(id))); + execCommand(new DeleteDatasetVersionCommand(req, doomed)); - // If we have gotten this far, the destroy command has succeeded, + // If we have gotten this far, the delete command has succeeded, // so we can finalize it by permanently deleting the physical files: // (DataFileService will double-check that the datafiles no // longer exist in the database, before attempting to delete diff --git a/src/main/java/edu/harvard/iq/dataverse/api/datadeposit/ContainerManagerImpl.java b/src/main/java/edu/harvard/iq/dataverse/api/datadeposit/ContainerManagerImpl.java index 4d4d1d08b51..4186db72678 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/datadeposit/ContainerManagerImpl.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/datadeposit/ContainerManagerImpl.java @@ -12,7 +12,6 @@ import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; import edu.harvard.iq.dataverse.engine.command.exception.CommandException; import edu.harvard.iq.dataverse.engine.command.exception.CommandExecutionException; -import edu.harvard.iq.dataverse.engine.command.impl.DeleteDatasetCommand; import edu.harvard.iq.dataverse.engine.command.impl.DeleteDatasetVersionCommand; import edu.harvard.iq.dataverse.engine.command.impl.PublishDatasetCommand; import edu.harvard.iq.dataverse.engine.command.impl.PublishDataverseCommand; @@ -214,12 +213,6 @@ public void deleteContainer(String uri, AuthCredentials authCredentials, SwordCo Dataset dataset = dataset = datasetService.findByGlobalId(globalId); if (dataset != null) { Dataverse dvThatOwnsDataset = dataset.getOwner(); - /** - * We are checking if DeleteDatasetVersionCommand can be - * called even though DeleteDatasetCommand can be called - * when a dataset hasn't been published. They should be - * equivalent in terms of a permission check. - */ DeleteDatasetVersionCommand deleteDatasetVersionCommand = new DeleteDatasetVersionCommand(dvRequest, dataset); if (!permissionService.isUserAllowedOn(user, deleteDatasetVersionCommand, dataset)) { throw new SwordError(UriRegistry.ERROR_BAD_REQUEST, "User " + user.getDisplayInfo().getTitle() + " is not authorized to modify " + dvThatOwnsDataset.getAlias()); @@ -253,7 +246,7 @@ public void deleteContainer(String uri, AuthCredentials authCredentials, SwordCo // dataset has never been published, this is just a sanity check (should always be draft) if (datasetVersionState.equals(DatasetVersion.VersionState.DRAFT)) { try { - engineSvc.submit(new DeleteDatasetCommand(dvRequest, dataset)); + engineSvc.submit(new DeleteDatasetVersionCommand(dvRequest, dataset)); logger.fine("dataset deleted"); } catch (CommandExecutionException ex) { // internal error diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDatasetCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDatasetCommand.java deleted file mode 100644 index 419bf5522e9..00000000000 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDatasetCommand.java +++ /dev/null @@ -1,33 +0,0 @@ -package edu.harvard.iq.dataverse.engine.command.impl; - -import edu.harvard.iq.dataverse.Dataset; -import edu.harvard.iq.dataverse.authorization.Permission; -import edu.harvard.iq.dataverse.engine.command.AbstractVoidCommand; -import edu.harvard.iq.dataverse.engine.command.CommandContext; -import edu.harvard.iq.dataverse.engine.command.DataverseRequest; -import edu.harvard.iq.dataverse.engine.command.RequiredPermissions; -import edu.harvard.iq.dataverse.engine.command.exception.CommandException; - -/** - * Deletes a dataset. - * - * @author michael - */ -@RequiredPermissions(Permission.DeleteDatasetDraft) -public class DeleteDatasetCommand extends AbstractVoidCommand { - - private final Dataset doomed; - - public DeleteDatasetCommand(DataverseRequest aRequest, Dataset dataset) { - super(aRequest, dataset); - this.doomed = dataset; - } - - @Override - protected void executeImpl(CommandContext ctxt) throws CommandException { - // TODO: REMOVE THIS COMMAND - // for now it just calls DeleteDatasetVersion - ctxt.engine().submit(new DeleteDatasetVersionCommand(getRequest(), doomed)); - } - -} diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DestroyDatasetCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DestroyDatasetCommand.java index 49861e084b6..e1f5afcb813 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DestroyDatasetCommand.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DestroyDatasetCommand.java @@ -41,12 +41,12 @@ import org.apache.solr.client.solrj.SolrServerException; /** - * Same as {@link DeleteDatasetCommand}, but does not stop if the dataset is + * Same as {@link DeleteDatasetVersionCommand}, but does not stop if the dataset is * published. This command is reserved for super-users, if at all. * * @author michael */ -// Since this is used by DeleteDatasetCommand, must have at least that permission +// Since this is used by DeleteDatasetVersionCommand, must have at least that permission // (for released, user is checked for superuser) @RequiredPermissions( Permission.DeleteDatasetDraft ) public class DestroyDatasetCommand extends AbstractVoidCommand { diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java index 583a76f07a7..7859c68806c 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java @@ -260,6 +260,10 @@ private void testDatasetSchemaValidationHelper(String dataverseAlias, String api @Test public void testCreateDataset() { + Response createSuperUser = UtilIT.createRandomUser(); + String superusername = UtilIT.getUsernameFromResponse(createSuperUser); + UtilIT.setSuperuserStatus(superusername, true); + String superuserApiToken = UtilIT.getApiTokenFromResponse(createSuperUser); Response createUser = UtilIT.createRandomUser(); createUser.prettyPrint(); @@ -339,19 +343,31 @@ public void testCreateDataset() { datasetAsJson.then().assertThat() .statusCode(OK.getStatusCode()); - // OK, let's delete this dataset as well, and then delete the dataverse... - - deleteDatasetResponse = UtilIT.deleteDatasetViaNativeApi(datasetId, apiToken); + // Now publish the dataset and try to delete it (as superuser) + Response publishResponse = UtilIT.publishDatasetViaNativeApi(datasetId, "major", apiToken); + assertEquals(200, publishResponse.getStatusCode()); + deleteDatasetResponse = UtilIT.deleteDatasetViaNativeApi(datasetId, superuserApiToken); + deleteDatasetResponse.prettyPrint(); + deleteDatasetResponse.then().assertThat() + .body("message", containsString("/destroy")) + .statusCode(UNAUTHORIZED.getStatusCode()); + // Try /destroy to get rid of the dataset + deleteDatasetResponse = UtilIT.destroyDataset(datasetId, superuserApiToken); deleteDatasetResponse.prettyPrint(); assertEquals(200, deleteDatasetResponse.getStatusCode()); - + + // Delete the dataverse Response deleteDataverseResponse = UtilIT.deleteDataverse(dataverseAlias, apiToken); deleteDataverseResponse.prettyPrint(); assertEquals(200, deleteDataverseResponse.getStatusCode()); + // Delete the Users Response deleteUserResponse = UtilIT.deleteUser(username); deleteUserResponse.prettyPrint(); assertEquals(200, deleteUserResponse.getStatusCode()); + deleteUserResponse = UtilIT.deleteUser(superusername); + deleteUserResponse.prettyPrint(); + assertEquals(200, deleteUserResponse.getStatusCode()); } @@ -3785,7 +3801,7 @@ public void testSemanticMetadataAPIs() { deleteDraftResponse.then().assertThat().statusCode(OK.getStatusCode()); //Delete the published dataset - Response deletePublishedResponse = UtilIT.deleteDatasetViaNativeApi(datasetId, apiToken); + Response deletePublishedResponse = UtilIT.destroyDataset(datasetId, apiToken); deletePublishedResponse.prettyPrint(); deleteDraftResponse.then().assertThat().statusCode(OK.getStatusCode());