Inconsistent behaviour of dataset delete API

Author: stevenwinship

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. 

src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java

@@ -248,7 +248,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

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<Long, String> deleteStorageLocations = fileService.getPhysicalFilesToDelete(doomedVersion, destroy);
+            // Gather the locations of the physical files that will need to be deleted
+            Map<Long, String> 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

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

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDatasetCommand.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 {

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DestroyDatasetCommand.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());