From 4a6006caca007fb40b04c2f51a75d419409b060a Mon Sep 17 00:00:00 2001 From: Milan Kuchtiak Date: Mon, 8 Jun 2026 23:21:04 +0200 Subject: [PATCH] Issue 1360: allow to change license in workflow item, in PATCH operation (ufal/clarin-dspace#1365) * Issue 1360: allow to change license in workflow item, in PATCH operation * improve fix + Integration test * resolve Copilot comments * resolve more MR comments * return 404 in case PATCH request references non existing license * improve error message for task claimed by other user * resolve second round of Copilot comments * small code refactoring * fixed failing test (cherry picked from commit 40672c32418aef02cab6e5124d811d959886af6f) --- .../WorkflowItemRestRepository.java | 75 ++++++++- .../ClarinWorkflowItemRestRepositoryIT.java | 149 ++++++++++++++++++ .../app/rest/TaskRestRepositoriesIT.java | 2 +- 3 files changed, 218 insertions(+), 8 deletions(-) diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowItemRestRepository.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowItemRestRepository.java index 022064576ea1..71872ac0e4cc 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowItemRestRepository.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/WorkflowItemRestRepository.java @@ -7,14 +7,18 @@ */ package org.dspace.app.rest.repository; +import static org.dspace.app.rest.repository.ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE; import static org.dspace.xmlworkflow.state.actions.processingaction.ProcessingAction.SUBMIT_EDIT_METADATA; import java.io.IOException; import java.sql.SQLException; import java.util.List; +import java.util.Objects; import java.util.UUID; import javax.servlet.http.HttpServletRequest; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.dspace.app.rest.Parameter; @@ -24,9 +28,12 @@ import org.dspace.app.rest.exception.UnprocessableEntityException; import org.dspace.app.rest.model.ErrorRest; import org.dspace.app.rest.model.WorkflowItemRest; +import org.dspace.app.rest.model.patch.JsonValueEvaluator; import org.dspace.app.rest.model.patch.Operation; import org.dspace.app.rest.model.patch.Patch; +import org.dspace.app.rest.model.patch.ReplaceOperation; import org.dspace.app.rest.submit.SubmissionService; +import org.dspace.app.rest.submit.step.ClarinLicenseSubmissionUtils; import org.dspace.app.rest.utils.SolrOAIReindexer; import org.dspace.app.util.SubmissionConfigReaderException; import org.dspace.authorize.AuthorizeException; @@ -200,7 +207,7 @@ public Class getDomainClass() { @Override public WorkflowItemRest upload(HttpServletRequest request, String apiCategory, String model, Integer id, - MultipartFile file) throws SQLException { + MultipartFile file) throws SQLException, AuthorizeException { Context context = obtainContext(); WorkflowItemRest wsi = findOne(context, id); @@ -226,12 +233,21 @@ public void patch(Context context, HttpServletRequest request, String apiCategor WorkflowItemRest wsi = findOne(context, id); XmlWorkflowItem source = wis.find(context, id); + if (source == null) { + throw new ResourceNotFoundException("WorkflowItem with id " + id + " not found"); + } + this.checkIfEditMetadataAllowedInCurrentStep(context, source); for (Operation op : operations) { //the value in the position 0 is a null value String[] path = op.getPath().substring(1).split("/", 3); - if (OPERATION_PATH_SECTIONS.equals(path[0])) { + if (OPERATION_PATH_LICENSE_RESOURCE.equals(path[0])) { + // Apply the CLARIN license change through the shared submission helper so the + // workflow `/license` path behaves the same as the submission license paths. + // A non-existing license surfaces as ClarinLicenseNotFoundException (404). + ClarinLicenseSubmissionUtils.applyLicense(context, source.getItem(), extractLicenseName(op)); + } else if (OPERATION_PATH_SECTIONS.equals(path[0])) { String section = path[1]; submissionService.evaluatePatchToInprogressSubmission(context, request, source, wsi, section, op); } else { @@ -278,20 +294,65 @@ protected void delete(Context context, Integer id) { } } + /** + * Extract the CLARIN license name from a JSON Patch {@code replace} operation on the {@code /license} + * path. The value is accepted either as a plain string or as an object wrapping a textual {@code value} + * field; a non-replace operation or any other value shape is rejected as a bad request. A blank name is + * passed through (the submission helper treats it as a request to clear the current license selection). + * @param op the JSON Patch operation targeting the license path + * @return the CLARIN license name to apply + */ + private String extractLicenseName(Operation op) { + if (!(op instanceof ReplaceOperation)) { + throw new DSpaceBadRequestException("The operation to update the license must be the 'replace' operation"); + } + if (op.getValue() instanceof String) { + return (String) op.getValue(); + } + if (!(op.getValue() instanceof JsonValueEvaluator)) { + throw wrongValueFormatException(op); + } + JsonValueEvaluator jsonValEvaluator = (JsonValueEvaluator) op.getValue(); + if (!(jsonValEvaluator.getValueNode() instanceof ObjectNode)) { + throw wrongValueFormatException(op); + } + // a replace operation may wrap the value in an ObjectNode under the "value" key + JsonNode jsonNodeValue = jsonValEvaluator.getValueNode().get("value"); + if (jsonNodeValue != null && jsonNodeValue.isTextual()) { + return jsonNodeValue.asText(); + } + throw wrongValueFormatException(op); + } + + private DSpaceBadRequestException wrongValueFormatException(Operation op) { + return new DSpaceBadRequestException("Unsupported value type for operation: " + op.getOp() + + ". Expected a string or an object with a textual 'value' field."); + } + /** * Checks if @link{SUBMIT_EDIT_METADATA} is a valid option in the workflow step this task is currently at. * Patching and uploading is only allowed if this is the case. * @param context Context * @param xmlWorkflowItem WorkflowItem of the task */ - private void checkIfEditMetadataAllowedInCurrentStep(Context context, XmlWorkflowItem xmlWorkflowItem) { + private void checkIfEditMetadataAllowedInCurrentStep(Context context, XmlWorkflowItem xmlWorkflowItem) + throws AuthorizeException { try { - ClaimedTask claimedTask = claimedTaskService.findByWorkflowIdAndEPerson(context, xmlWorkflowItem, - context.getCurrentUser()); - if (claimedTask == null) { + List claimTasks = claimedTaskService.findByWorkflowItem(context, xmlWorkflowItem); + if (claimTasks.isEmpty()) { throw new UnprocessableEntityException("WorkflowItem with id " + xmlWorkflowItem.getID() - + " has not been claimed yet."); + + " has not been claimed yet."); } + + ClaimedTask claimedTask = claimTasks.stream() + .filter(ct -> Objects.equals(ct.getOwner(), context.getCurrentUser())) + .findFirst() + .orElse(null); + if (claimedTask == null) { + throw new AuthorizeException("The current user hasn't claimed the workflow item with id " + + xmlWorkflowItem.getID() + ", so the user cannot patch this item"); + } + Workflow workflow = workflowFactory.getWorkflow(claimedTask.getWorkflowItem().getCollection()); Step step = workflow.getStep(claimedTask.getStepID()); WorkflowActionConfig currentActionConfig = step.getActionConfig(claimedTask.getActionID()); diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinWorkflowItemRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinWorkflowItemRestRepositoryIT.java index 6c2d84f76aa7..33aec1092e6b 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinWorkflowItemRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinWorkflowItemRestRepositoryIT.java @@ -15,16 +15,29 @@ import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertFalse; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.lang3.StringUtils; +import org.dspace.app.rest.model.patch.AddOperation; +import org.dspace.app.rest.model.patch.Operation; +import org.dspace.app.rest.model.patch.ReplaceOperation; +import org.dspace.app.rest.repository.ClarinLicenseRestRepository; import org.dspace.app.rest.test.AbstractControllerIntegrationTest; +import org.dspace.builder.ClaimedTaskBuilder; +import org.dspace.builder.ClarinLicenseBuilder; +import org.dspace.builder.ClarinLicenseLabelBuilder; import org.dspace.builder.CollectionBuilder; import org.dspace.builder.CommunityBuilder; import org.dspace.builder.EPersonBuilder; @@ -35,12 +48,18 @@ import org.dspace.content.Item; import org.dspace.content.MetadataValue; import org.dspace.content.WorkspaceItem; +import org.dspace.content.clarin.ClarinLicense; +import org.dspace.content.clarin.ClarinLicenseLabel; import org.dspace.content.service.ItemService; import org.dspace.content.service.WorkspaceItemService; +import org.dspace.content.service.clarin.ClarinLicenseLabelService; +import org.dspace.content.service.clarin.ClarinLicenseService; import org.dspace.eperson.EPerson; import org.dspace.license.service.CreativeCommonsService; import org.dspace.services.ConfigurationService; import org.dspace.xmlworkflow.factory.XmlWorkflowFactory; +import org.dspace.xmlworkflow.storedcomponents.ClaimedTask; +import org.dspace.xmlworkflow.storedcomponents.XmlWorkflowItem; import org.dspace.xmlworkflow.storedcomponents.service.CollectionRoleService; import org.dspace.xmlworkflow.storedcomponents.service.XmlWorkflowItemService; import org.hamcrest.Matchers; @@ -78,6 +97,11 @@ public class ClarinWorkflowItemRestRepositoryIT extends AbstractControllerIntegr @Autowired private ItemService itemService; + @Autowired + private ClarinLicenseService clarinLicenseService; + @Autowired + private ClarinLicenseLabelService clarinLicenseLabelService; + Item item; @Before @@ -364,4 +388,129 @@ public void shouldCreateItemWithCustomTypeBindField() throws Exception { assertFalse(mvList.isEmpty()); assertThat(mvList.get(0).getValue(), is(CITATION_VALUE)); } + + @Test + public void patchUpdateClarinLicense() throws Exception { + context.turnOffAuthorisationSystem(); + + // create Clarin License Label + ClarinLicenseLabel clarinLicenseLabel = ClarinLicenseLabelBuilder.createClarinLicenseLabel(context).build(); + clarinLicenseLabel.setLabel("CC"); + clarinLicenseLabel.setExtended(false); + clarinLicenseLabel.setTitle("CLL Title1"); + clarinLicenseLabelService.update(context, clarinLicenseLabel); + + // create Clarin License + ClarinLicense clarinLicense = ClarinLicenseBuilder.createClarinLicense(context).build(); + clarinLicense.setName("CL Name"); + clarinLicense.setConfirmation(ClarinLicense.Confirmation.NOT_REQUIRED); + clarinLicense.setDefinition("CL Definition"); + clarinLicense.setRequiredInfo("CL Req"); + // add clarinLicenseLabel to clarinLicense + Set clarinLicenseLabels = new HashSet<>(); + clarinLicenseLabels.add(clarinLicenseLabel); + clarinLicense.setLicenseLabels(clarinLicenseLabels); + clarinLicenseService.update(context, clarinLicense); + + // community with one collection. + parentCommunity = CommunityBuilder.createCommunity(context) + .withName("Parent Community") + .build(); + Collection col = CollectionBuilder.createCollection(context, parentCommunity).withName("Collection 1") + .withWorkflowGroup("editor", eperson).build(); + + // create a normal user to use as submitter + EPerson submitter = EPersonBuilder.createEPerson(context) + .withEmail("submitter@example.com") + .withPassword("dspace") + .build(); + + // claimed task with workflow item in edit step + ClaimedTask claimedTask = ClaimedTaskBuilder.createClaimedTask(context, col, eperson) + .withTitle("Workflow Item") + .withIssueDate("2026-05-18") + .withSubject("Extra Entry") + .grantLicense() + .build(); + claimedTask.setStepID("editstep"); + claimedTask.setActionID("editaction"); + XmlWorkflowItem wfItem = claimedTask.getWorkflowItem(); + + context.restoreAuthSystemState(); + + // prepare a patch targeting the clarin license resource path + List ops = new ArrayList<>(); + ops.add(new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, "CL Name")); + + String submitterToken = getAuthToken(submitter.getEmail(), "dspace"); + + // The submitter shouldn't be allowed to patch clarin license + // because the workflow item was claimed by the other user (eperson), + // and the submitter doesn't have permissions to edit it, + // so the patch request should be rejected with error 403(Forbidden) + getClient(submitterToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isForbidden()); + + String editorToken = getAuthToken(eperson.getEmail(), password); + + ops.set(0, new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, "Wrong CL")); + + // The wrong clarin license name value should be rejected with 404 Not Found + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isNotFound()); + + // The valid clarin license name can be in the form of a simple string or + // in the form of a map with "value" key, but it should be accepted in both cases + ops.set(0, new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, "CL Name")); + + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isOk()); + + Map wrappedValue = new HashMap(); + wrappedValue.put("value", "CL Name"); + ops.set(0, new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, + wrappedValue)); + + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isOk()); + + // The wrapped value should contain the "value" key, otherwise it is invalid + Map invalidWrappedValue1 = new HashMap(); + ops.set(0, new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, + invalidWrappedValue1)); + + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isBadRequest()); + + // The wrapped value should be in a map, not in a list + List invalidWrappedValue2 = new ArrayList<>(); + invalidWrappedValue2.add("CL Name"); + ops.set(0, new ReplaceOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, + invalidWrappedValue2)); + + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isBadRequest()); + + // The only accepted operation for clarin license resource is "replace", + // "add" operation should be rejected with 400 Bad Request even with the valid value + ops.set(0, new AddOperation("/" + ClarinLicenseRestRepository.OPERATION_PATH_LICENSE_RESOURCE, + "CL Name")); + + getClient(editorToken).perform(patch("/api/workflow/workflowitems/" + wfItem.getID()) + .content(getPatchContent(ops)) + .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isBadRequest()); + } } diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/TaskRestRepositoriesIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/TaskRestRepositoriesIT.java index a9b5c6a582b6..246f0e827071 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/TaskRestRepositoriesIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/TaskRestRepositoriesIT.java @@ -2893,7 +2893,7 @@ public void patchTest_ClaimedTask_EditMetadataOptionNotAllowed() throws Exceptio getClient(authToken).perform(patch("/api/workflow/workflowitems/" + witem.getID()) .content(patchBody) .contentType(javax.ws.rs.core.MediaType.APPLICATION_JSON_PATCH_JSON)) - .andExpect(status().isUnprocessableEntity()); + .andExpect(status().isForbidden()); } @Test