Delete Dataset: Contributor deleting a draft of a published version is not indexed.

[kcondon]

When a contributor deletes a draft of a published dataset, it is removed but not reindexed so the draft card stays around. This happens in the private url branch and does not happen in v4.4 on demo.

See related ticket: #3201

[pdurbin]

Whoops. This a regression. DatasetPage is reporting (for example) "Can't execute command edu.harvard.iq.dataverse.engine.command.impl.GetPrivateUrlCommand@144d3d0c, because request [DataverseRequest user:edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser@3@0.0.0.0] is missing permissions [ManageDatasetPermissions] on Object Darwin's Finches".

I'm calling GetPrivateUrlCommand and DeletePrivateUrlCommand from DeleteDatasetVersionCommand and made an assumption that if you're allowed to call DeleteDatasetVersionCommand you have ManageDatasetPermissions. It turns out DeleteDatasetVersionCommand only requires DeleteDatasetDraft.

We don't want to change the permissions required on GetPrivateUrlCommand and DeletePrivateUrlCommand from ManageDatasetPermissions to DeleteDatasetDraft because most of the time we don't want a Contributor to be able to delete a Private URL. We only want to let a Contributor delete a Private URL when the Contributor is deleting a draft. So maybe we'll just call the service bean directly (I may need to add a delete method) rather than going through the command engine.

This is mostly a brain dump from a chat with @scolapasta but since @michbarsinai will be in town as soon as tomorrow I'll see what he thinks before pushing a fix. One question is if commands can be treated like transactions. If a command calling another command and the second command fails is the first command rolled back? It seems like not. The dataset version is gone.

[michbarsinai]

1. As far as transactions go, commands use whatever the beans they call specify. So by default they all run in the same transaction, but some bean methods might require new transactions. Note, though, that rolling back only applies for database. So Solr, Email etc. actions are not rolled back. There's a long-term plan to have commands run in their own transaction, and execute other commands after their transaction is committed. We didn't get to this yet, though.
2. Permission-wise, I can think of two options. One is to create another command but with different required permissions. Another option is to use dynamic permissions on the command (i.e. override getRequiredPermissions). I wouldn't go "straight to bean" here, as you'd have to make sure the permissions are enforced in every use case. And the action won't be logged. I actually like the first one better, since it remains declarative. You can use inheritance to minimize code duplication.

[pdurbin]

@michbarsinai to be clear, there are three commands that would need to be duplicated to have more relaxed permissions, to allow a Contributor to revoke a role, which isn't allowed currently. Maybe we would append "AsContributor" to them or something:

• GetPrivateUrlCommandAsContributor
• DeletePrivateUrlCommandAsContributor
• RevokeRoleCommandAsContributor

Right now a Contributor can never revoke a role. The chain of commands would look like this (assuming a Private URL is found):

1. DeleteDatasetVersionCommand
2. GetPrivateUrlCommandAsContributor
3. DeletePrivateUrlCommandAsContributor
4. RevokeRoleCommandAsContributor

Here's where it starts:

dataverse/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDatasetVersionCommand.java

Line 71 in b629598

PrivateUrl privateUrl = ctxt.engine().submit(new GetPrivateUrlCommand(getRequest(), doomed));

[pdurbin]

@scolapasta the change for the approach we talked about would be fairly small, I think. I still need to write API tests around this but I think these changes to DeleteDatasetVersionCommand would fix this bug and #3201:

/**
+   * We avoid using GetPrivateUrlCommand and
+   * DeletePrivateUrlCommand because both require
+   * ManageDatasetPermissions (because ManageDatasetPermissions is
+   * required by RevokeRoleCommand) and a Contributor does not
+   * have ManageDatasetPermissions. A Contributor *does* have
+   * DeleteDatasetDraft (or else wouldn't be able to execute this
+   * command) so we bypass the usual commands and run their
+   * equivalent directly through service beans.
+  */
-  PrivateUrl privateUrl = ctxt.engine().submit(new GetPrivateUrlCommand(getRequest(), doomed));
+  PrivateUrl privateUrl = ctxt.privateUrl().getPrivateUrlFromDatasetId(doomed.getId());
   if (privateUrl != null) {
       logger.fine("Deleting Private URL for dataset id " + doomed.getId());
-      ctxt.engine().submit(new DeletePrivateUrlCommand(getRequest(), doomed));
+      PrivateUrlUser privateUrlUser = new PrivateUrlUser(doomed.getId());
+      List<RoleAssignment> roleAssignments = ctxt.roles().directRoleAssignments(privateUrlUser, doomed);
+      for (RoleAssignment roleAssignment : roleAssignments) {
+          ctxt.roles().revoke(roleAssignment);
+      }

Maybe you and @michbarsinai can duke it out over which approach to take. 😄

[pdurbin]

In 4015d35 I added tests to exercise this bug and #3201. @scolapasta and @michbarsinai please take a look.

[pdurbin]

@scolapasta I pushed a fix in 24df280 to a new branch. This is the one we talked about where we call service beans directly. Let's chat with @michbarsinai about it next week at the Community Meeting. If we're ok with this we can merge the branch in to the active pull request for Private URL at #3111.

[pdurbin]

@michbarsinai I'm going to co-assign this issue to you. I think 24df280 is an adequate solution but let's chat about it next week.

[pdurbin]

@scolapasta I described my "3200-3201-privateurl-perms-command-bypass" fix at 24df280 to @michbarsinai and he didn't seem to like it so I just pushed an alternative "3200-3201-duplicate-commands" fix at 4f6cc0c and will leave it to you two to decide if you are happy with either fix. Please advise.

[pdurbin]

@scolapasta and @michbarsinai blessed the fix at 24df280 so I merged it into pull request #3111. Passing to QA.

[kcondon]

Works, closing.