From f8c4c844c1eda6856068c40b865a4a4a5638f56b Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Tue, 10 Feb 2026 11:52:57 -0800 Subject: [PATCH 1/7] CADC-14486 fix new doi permissions, simplify status change and permission logic --- .../main/java/ca/nrc/cadc/doi/PostAction.java | 43 +++++-------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java index dcc77815..919f6916 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java @@ -378,10 +378,6 @@ private void setPermissions(Node node, GroupURI doiGroup) { node.isPublic = false; node.getReadOnlyGroup().add(doiGroup); node.getReadWriteGroup().add(doiGroup); - if (publisherGroupURI != null) { - node.getReadOnlyGroup().add(publisherGroupURI); - node.getReadWriteGroup().add(publisherGroupURI); - } } // @@ -540,7 +536,7 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { } // If the status changed, update the node permissions - if (requestedStatus != null) { + if (requestedStatus != null && isAlternativeConfiguration()) { log.debug("requested status: " + requestedStatus); log.debug("current status: " + currentStatus); if (!requestedStatus.equals(currentStatus)) { @@ -549,7 +545,7 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { // can only update to 'review ready' from 'in progress' (author submits for review) if (requestedStatus.equals(Status.REVIEW_READY.getValue()) && currentStatus.equals(Status.DRAFT.getValue())) { - if (isAlternativeConfiguration() && (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin())) { + if (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()) { // 'review ready' node permissions: doi-group:r publisher-group:r public:false doiNode.clearReadWriteGroups = true; doiNode.getReadWriteGroup().clear(); @@ -565,7 +561,7 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { // can only update to 'in review' from 'review ready' (publisher/reviewer accepts) } else if (requestedStatus.equals(Status.IN_REVIEW.getValue()) && currentStatus.equals(Status.REVIEW_READY.getValue())) { - if (isAlternativeConfiguration() && (isCallingUserPublisher() || isCallingUserDOIAdmin())) { + if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { // 'in review' node permissions: doi-group:r reviewer-group:r public:false doiNode.clearReadWriteGroups = true; doiNode.getReadWriteGroup().clear(); @@ -591,13 +587,13 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { // - from 'approved': requester or admin (author wants to edit after approval) boolean canTransition = false; if (currentStatus.equals(Status.REVIEW_READY.getValue())) { - canTransition = isAlternativeConfiguration() && (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()); + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); } else if (currentStatus.equals(Status.IN_REVIEW.getValue())) { - canTransition = isAlternativeConfiguration() && (isCallingUserRequester(doiNode) || isCallingUserPublisher() || isCallingUserDOIAdmin()); + canTransition = isCallingUserRequester(doiNode) || isCallingUserPublisher() || isCallingUserDOIAdmin(); } else if (currentStatus.equals(Status.REJECTED.getValue())) { - canTransition = isAlternativeConfiguration() && (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()); + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); } else if (currentStatus.equals(Status.APPROVED.getValue())) { - canTransition = isAlternativeConfiguration() && (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()); + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); } else { String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); throw new IllegalArgumentException(message); @@ -618,9 +614,10 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { } // can only update to 'approved' from 'in review' (publisher only) - } else if (requestedStatus.equals(Status.APPROVED.getValue()) && currentStatus.equals(Status.IN_REVIEW.getValue())) { - if (isAlternativeConfiguration() && (isCallingUserPublisher() || isCallingUserDOIAdmin())) { - // 'approved' node permissions: doi-group:r reviewer-group:r public:false + } else if ((requestedStatus.equals(Status.APPROVED.getValue()) || requestedStatus.equals(Status.REJECTED.getValue())) + && currentStatus.equals(Status.IN_REVIEW.getValue())) { + if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { + // 'approved' & 'rejected' node permissions: doi-group:r reviewer-group:r public:false doiNode.clearReadWriteGroups = true; doiNode.getReadWriteGroup().clear(); doiNode.clearReadOnlyGroups = true; @@ -632,24 +629,6 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); throw new IllegalArgumentException(message); } - - // can only update to 'rejected' from 'in review' (publisher only) - } else if (requestedStatus.equals(Status.REJECTED.getValue()) && currentStatus.equals(Status.IN_REVIEW.getValue())) { - if (isAlternativeConfiguration() && (isCallingUserPublisher() || isCallingUserDOIAdmin())) { - // 'rejected' node permissions: doi-group:rw reviewer-group:r public:false - doiNode.clearReadWriteGroups = true; - doiNode.getReadWriteGroup().clear(); - doiNode.clearReadOnlyGroups = true; - doiNode.getReadWriteGroup().add(doiGroupUri); - doiNode.getReadOnlyGroup().clear(); - doiNode.getReadOnlyGroup().add(doiGroupUri); - doiNode.getReadOnlyGroup().add(publisherGroupURI); - doiNode.isPublic = false; - } else { - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } - } else { // all other updates are not allowed String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); From 8d1b1daddf2892ab22e6622d8d09a9a03a8eafe5 Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Mon, 16 Feb 2026 08:11:52 -0800 Subject: [PATCH 2/7] CADC-14486 update publisher permissions after removing read/write publisher access for new doi's --- .../main/java/ca/nrc/cadc/doi/DoiAction.java | 21 +- .../main/java/ca/nrc/cadc/doi/PostAction.java | 253 ++++++++++-------- 2 files changed, 158 insertions(+), 116 deletions(-) diff --git a/doi/src/main/java/ca/nrc/cadc/doi/DoiAction.java b/doi/src/main/java/ca/nrc/cadc/doi/DoiAction.java index 835dfa5a..06cc44a4 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/DoiAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/DoiAction.java @@ -279,6 +279,8 @@ protected boolean isCallingUserRequester(Node node) { } protected List getAccessibleDOIs() throws Exception { + Set readStatus = Set.of(Status.REVIEW_READY, Status.IN_REVIEW, Status.APPROVED, Status.REJECTED); + List ownedNodes = new ArrayList<>(); ContainerNode doiRootNode = vospaceDoiClient.getContainerNode(""); if (doiRootNode != null) { @@ -297,10 +299,18 @@ protected List getAccessibleDOIs() throws Exception { } Long requesterUserId = Long.parseLong(requester.getValue()); - if (callersNumericId.equals(requesterUserId) - || isCallingUserDOIAdmin() - || isCallingUserPublisher()) { + if (callersNumericId.equals(requesterUserId) || isCallingUserDOIAdmin()) { ownedNodes.add(childNode); + continue; + } + + // if the caller is a publisher and the node status is readable for a publisher + NodeProperty statusProperty = childNode.getProperty(DOI.VOSPACE_DOI_STATUS_PROPERTY); + if (statusProperty != null && statusProperty.getValue() != null) { + Status status = Status.toValue(statusProperty.getValue()); + if (isCallingUserPublisher() && readStatus.contains(status)) { + ownedNodes.add(childNode); + } } } catch (NumberFormatException e) { log.error(String.format("Unable to parse requester uid[%s] for doi: %s", @@ -480,7 +490,10 @@ protected void uploadDOIDocument(Resource resource, VOSURI docVOSUIRI) throws Re protected static class DoiOutputStream implements OutputStreamWrapper { private final Resource streamResource; - public DoiOutputStream(Resource streamRes) { this.streamResource = streamRes; } + + public DoiOutputStream(Resource streamRes) { + this.streamResource = streamRes; } + public void write(OutputStream out) throws IOException { DoiXmlWriter writer = new DoiXmlWriter(); writer.write(streamResource, out); diff --git a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java index 919f6916..34ddfa48 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java @@ -392,8 +392,8 @@ private void updateDOI() throws Exception { } // perform the update - updateResource(doiMetaData); updateNodeProperties(doiNodeData); + updateResource(doiMetaData); // Done, send redirect to GET for the XML file just uploaded String redirectUrl = syncInput.getRequestURI(); @@ -419,8 +419,8 @@ private void updateResource(Resource resourceFromUser) throws Exception { private Resource merge(Resource sourceResource, Resource targetResource) { - log.info("sourceResource: " + sourceResource); - log.info("targetResource: " + targetResource); + log.debug("sourceResource: " + sourceResource); + log.debug("targetResource: " + targetResource); // A user is only allowed to update creators and titles verifyImmutableFields(sourceResource, targetResource); @@ -428,12 +428,11 @@ private Resource merge(Resource sourceResource, Resource targetResource) { // update editable fields targetResource.getCreators().clear(); targetResource.getCreators().addAll(sourceResource.getCreators()); - log.info("targetResource.getCreators(): " + targetResource.getCreators()); targetResource.getTitles().clear(); targetResource.getTitles().addAll(sourceResource.getTitles()); targetResource.getPublicationYear().setValue(sourceResource.getPublicationYear().getValue()); targetResource.language = sourceResource.language; - log.info("merged Resource: " + targetResource); + log.debug("merged Resource: " + targetResource); return targetResource; } @@ -493,151 +492,179 @@ private void verifyNull(Object o1, Object o2, String field) { private void updateNodeProperties(JSONObject nodeData) throws Exception { // get a map of NodeProperty key(URI) and value(String) - Map propertyMap = getNodeProperties(nodeData); + Map updatedProperties = getNodeProperties(nodeData); // Get existing node properties ContainerNode doiNode = vospaceDoiClient.getContainerNode(doiSuffix); VOSURI vosuri = getVOSURI(doiSuffix); // Update the node properties - String requestedStatus = null; - String currentStatus = null; - for (Map.Entry entry : propertyMap.entrySet()) { - NodeProperty nodeProperty = doiNode.getProperty(entry.getKey()); + Status currentStatus = null; + Status updatedStatus = null; + for (Map.Entry updatedProperty : updatedProperties.entrySet()) { + URI updatedKey = updatedProperty.getKey(); + String updatedValue = updatedProperty.getValue(); + NodeProperty currentProperty = doiNode.getProperty(updatedProperty.getKey()); // save the status for possible permission updates - if (entry.getKey().equals(DOI.VOSPACE_DOI_STATUS_PROPERTY)) { - requestedStatus = entry.getValue(); - currentStatus = nodeProperty.getValue(); + if (updatedKey.equals(DOI.VOSPACE_DOI_STATUS_PROPERTY)) { + currentStatus = Status.toValue(currentProperty.getValue()); + updatedStatus = Status.toValue(updatedProperty.getValue()); + log.debug(String.format("update status '%s' -> '%s'", currentStatus.getValue(), updatedStatus.getValue())); } // only a publisher can set/update the reviewer node property - if (entry.getKey().equals(DOI.VOSPACE_DOI_REVIEWER_PROPERTY)) { + if (updatedKey.equals(DOI.VOSPACE_DOI_REVIEWER_PROPERTY)) { if (!isCallingUserPublisher() && !isCallingUserDOIAdmin()) { throw new IllegalArgumentException("Not authorized to set reviewer"); } } // update the node property - if (nodeProperty == null) { + if (currentProperty == null) { // node property does not exist, add it - NodeProperty newProperty = new NodeProperty(entry.getKey(), entry.getValue()); + NodeProperty newProperty = new NodeProperty(updatedKey, updatedValue); doiNode.getProperties().add(newProperty); } else { - if (entry.getValue() == null || entry.getValue().isEmpty()) { + if (updatedValue == null || updatedValue.isEmpty()) { // new value is null or empty, delete - doiNode.getProperties().remove(nodeProperty); - doiNode.getProperties().add(new NodeProperty(entry.getKey())); + doiNode.getProperties().remove(currentProperty); + doiNode.getProperties().add(new NodeProperty(updatedKey)); } else { // update existing property - doiNode.getProperty(entry.getKey()).setValue(entry.getValue()); + doiNode.getProperty(updatedKey).setValue(updatedValue); } } } - // If the status changed, update the node permissions - if (requestedStatus != null && isAlternativeConfiguration()) { - log.debug("requested status: " + requestedStatus); - log.debug("current status: " + currentStatus); - if (!requestedStatus.equals(currentStatus)) { - GroupURI doiGroupUri = createGroupURI(doiGroupPrefix + doiSuffix); - log.debug("updating status..."); - - // can only update to 'review ready' from 'in progress' (author submits for review) - if (requestedStatus.equals(Status.REVIEW_READY.getValue()) && currentStatus.equals(Status.DRAFT.getValue())) { - if (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()) { - // 'review ready' node permissions: doi-group:r publisher-group:r public:false - doiNode.clearReadWriteGroups = true; - doiNode.getReadWriteGroup().clear(); - doiNode.clearReadOnlyGroups = true; - doiNode.getReadOnlyGroup().clear(); - doiNode.getReadOnlyGroup().add(doiGroupUri); - doiNode.getReadOnlyGroup().add(publisherGroupURI); - doiNode.isPublic = false; - } else { - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } + // If the status changed, update the node permissions for an alternate configuration + if (updatedStatus != null && isAlternativeConfiguration()) { + updatePermissions(doiNode, vosuri, currentStatus, updatedStatus); + } + vospaceDoiClient.getVOSpaceClient().setNode(vosuri, doiNode); + } + + private void updatePermissions(Node doiNode, VOSURI vosuri, Status current, Status updated) throws Exception { + + if (current != updated) { + GroupURI doiGroupUri = createGroupURI(doiGroupPrefix + doiSuffix); + + // can only update to 'review ready' from 'in progress' (author submits for review) + if (current == Status.DRAFT && updated == Status.REVIEW_READY) { + if (isCallingUserRequester(doiNode) || isCallingUserDOIAdmin()) { + log.debug("update status: 'in progress' -> 'review ready'"); + + // update doi parent node permissions + Set readGroups = Set.of(doiGroupUri, publisherGroupURI); + Set writeGroups = null; + updateNodePermissions(doiNode, readGroups, writeGroups); + + // update doi metadata file and data folder permissions + updateDOIPermissions(doiSuffix, readGroups, writeGroups); + } else { + throw new IllegalArgumentException("Invalid status change: 'in progress' -> 'review ready'"); + } // can only update to 'in review' from 'review ready' (publisher/reviewer accepts) - } else if (requestedStatus.equals(Status.IN_REVIEW.getValue()) && currentStatus.equals(Status.REVIEW_READY.getValue())) { - if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { - // 'in review' node permissions: doi-group:r reviewer-group:r public:false - doiNode.clearReadWriteGroups = true; - doiNode.getReadWriteGroup().clear(); - doiNode.clearReadOnlyGroups = true; - doiNode.getReadOnlyGroup().clear(); - doiNode.getReadOnlyGroup().add(doiGroupUri); - doiNode.getReadOnlyGroup().add(publisherGroupURI); - doiNode.isPublic = false; - } else { - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } + // permissions are the same for each status + } else if (current == Status.REVIEW_READY && updated == Status.IN_REVIEW) { + if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { + log.debug("update permissions: 'review ready' -> 'in review' - no permission changes"); + } else { + throw new IllegalArgumentException("Invalid status change: 'review ready' -> 'in review'"); + } // can only update to 'in progress' from 'review ready', 'in review', 'rejected', or 'approved' - } else if (requestedStatus.equals(Status.DRAFT.getValue()) && (currentStatus.equals(Status.REVIEW_READY.getValue()) - || currentStatus.equals(Status.IN_REVIEW.getValue()) || currentStatus.equals(Status.REJECTED.getValue()) - || currentStatus.equals(Status.APPROVED.getValue()))) { - - // alt config only: - // - from 'review ready': requester or admin (author withdraws) - // - from 'in review': requester, publisher, or admin - // - from 'rejected': requester or admin - // - from 'approved': requester or admin (author wants to edit after approval) - boolean canTransition = false; - if (currentStatus.equals(Status.REVIEW_READY.getValue())) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); - } else if (currentStatus.equals(Status.IN_REVIEW.getValue())) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserPublisher() || isCallingUserDOIAdmin(); - } else if (currentStatus.equals(Status.REJECTED.getValue())) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); - } else if (currentStatus.equals(Status.APPROVED.getValue())) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); - } else { - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } + } else if ((current == Status.REVIEW_READY || current == Status.IN_REVIEW + || current == Status.REJECTED || current == Status.APPROVED) + && updated == Status.DRAFT) { + log.debug(String.format("update status: '%s' -> '%s'", current, updated)); + + // alt config only: + // - from 'review ready': requester or admin (author withdraws) + // - from 'in review': requester, publisher, or admin + // - from 'rejected': requester or admin + // - from 'approved': requester or admin (author wants to edit after approval) + boolean canTransition = false; + if (current == Status.REVIEW_READY) { + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); + } else if (current == Status.IN_REVIEW) { + canTransition = isCallingUserRequester(doiNode) || isCallingUserPublisher() || isCallingUserDOIAdmin(); + } else if (current == Status.REJECTED) { + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); + } else if (current == Status.APPROVED) { + canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); + } else { + String message = String.format("Invalid status change: '%s' -> '%s'", current, updated); + throw new IllegalArgumentException(message); + } - if (canTransition) { - // 'in progress' node permissions: doi-group:rw reviewer-group:- public:false - doiNode.clearReadWriteGroups = true; - doiNode.getReadWriteGroup().clear(); - doiNode.getReadWriteGroup().add(doiGroupUri); - doiNode.clearReadOnlyGroups = true; - doiNode.getReadOnlyGroup().clear(); - doiNode.getReadOnlyGroup().add(doiGroupUri); - doiNode.isPublic = false; - } else { - String message = String.format("Status change denied: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } + if (canTransition) { + log.debug(String.format("update permissions: '%s' -> '%s'", current, updated)); - // can only update to 'approved' from 'in review' (publisher only) - } else if ((requestedStatus.equals(Status.APPROVED.getValue()) || requestedStatus.equals(Status.REJECTED.getValue())) - && currentStatus.equals(Status.IN_REVIEW.getValue())) { - if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { - // 'approved' & 'rejected' node permissions: doi-group:r reviewer-group:r public:false - doiNode.clearReadWriteGroups = true; - doiNode.getReadWriteGroup().clear(); - doiNode.clearReadOnlyGroups = true; - doiNode.getReadOnlyGroup().clear(); - doiNode.getReadOnlyGroup().add(doiGroupUri); - doiNode.getReadOnlyGroup().add(publisherGroupURI); - doiNode.isPublic = false; - } else { - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); - throw new IllegalArgumentException(message); - } + // 'in progress' node permissions: doi-group:rw reviewer-group:- public:false + // update doi parent node permissions + Set readGroups = Set.of(doiGroupUri); + Set writeGroups = Set.of(doiGroupUri); + updateNodePermissions(doiNode, readGroups, writeGroups); + + // update doi data folder permissions + updateDOIPermissions(doiSuffix, readGroups, writeGroups); } else { - // all other updates are not allowed - String message = String.format("Invalid status change requested: from '%s' to '%s'", currentStatus, requestedStatus); + String message = String.format("Status change denied: '%s' -> '%s'", current, updated); throw new IllegalArgumentException(message); } - log.debug("status updated"); + + // can only update to 'approved' or 'rejected' from 'in review' (publisher only) + // status have same permissions + } else if (current == Status.IN_REVIEW && (updated == Status.APPROVED || updated == Status.REJECTED)) { + if (isCallingUserPublisher() || isCallingUserDOIAdmin()) { + log.debug(String.format("update status: '%s' -> '%s' - no permission changes", current, updated)); + } else { + String message = String.format("Invalid status change: '%s' -> '%s'", current, updated); + throw new IllegalArgumentException(message); + } + } else { + // all other updates are not allowed + String message = String.format("Invalid status change: from '%s' -> '%s'", current, updated); + throw new IllegalArgumentException(message); } } - vospaceDoiClient.getVOSpaceClient().setNode(vosuri, doiNode); + } + + private void updateNodePermissions(Node doiNode, Set readGroups, Set writeGroups) { + doiNode.clearReadWriteGroups = true; + doiNode.getReadWriteGroup().clear(); + doiNode.clearReadOnlyGroups = true; + doiNode.getReadOnlyGroup().clear(); + if (readGroups != null) { + for (GroupURI readGroup : readGroups) { + doiNode.getReadOnlyGroup().add(readGroup); + } + } + if (writeGroups != null) { + for (GroupURI writeGroup : writeGroups) { + doiNode.getReadWriteGroup().add(writeGroup); + } + } + doiNode.isPublic = false; + } + + private void updateDOIPermissions(String doiSuffix, Set readGroups, Set writeGroups) throws Exception { + // update metadata file permissions + String metadataFilename = getDoiFilename(doiSuffix); + String metadataPath = String.format("%s/%s", doiSuffix, metadataFilename); + VOSURI metadataVOSURI = getVOSURI(metadataPath); + DataNode metadataNode = vospaceDoiClient.getDataNode(metadataPath); + updateNodePermissions(metadataNode, readGroups, writeGroups); + vospaceDoiClient.getVOSpaceClient().setNode(metadataVOSURI, metadataNode); + + // update data folder permissions + String dataPath = String.format("%s/data", doiSuffix); + VOSURI dataURI = getVOSURI(dataPath); + ContainerNode dataFolderNode = vospaceDoiClient.getContainerNode(dataPath); + updateNodePermissions(dataFolderNode, readGroups, writeGroups); + vospaceDoiClient.getVOSpaceClient().setNode(dataURI, dataFolderNode); } private Map getNodeProperties(JSONObject nodeData) { @@ -677,6 +704,8 @@ private void performDOIAction() throws Exception { Status mintingStatus = Status.toValue(doiContainerNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); switch (mintingStatus) { case DRAFT: + case IN_REVIEW: + case APPROVED: case ERROR_LOCKING_DATA: lockData(doiContainerNode); break; From 278f011150005ae88708ca4e7e0d75649853cb1d Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Mon, 16 Feb 2026 08:13:24 -0800 Subject: [PATCH 3/7] CAC-14486 - update integration tests for more restrictive publisher permissions --- .../ca/nrc/cadc/doi/AltPermissionsTest.java | 56 ++++--- .../java/ca/nrc/cadc/doi/AltStatusTest.java | 27 +--- .../java/ca/nrc/cadc/doi/IntTestBase.java | 33 +++- .../java/ca/nrc/cadc/doi/LifecycleTest.java | 152 +++++++++++------- 4 files changed, 162 insertions(+), 106 deletions(-) diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/AltPermissionsTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/AltPermissionsTest.java index 6ad19feb..3d1322c2 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/AltPermissionsTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/AltPermissionsTest.java @@ -1,7 +1,5 @@ package ca.nrc.cadc.doi; -import ca.nrc.cadc.doi.datacite.Date; -import ca.nrc.cadc.doi.datacite.DateType; import ca.nrc.cadc.doi.datacite.Resource; import ca.nrc.cadc.doi.io.DoiParsingException; import ca.nrc.cadc.doi.io.DoiXmlReader; @@ -15,17 +13,11 @@ import ca.nrc.cadc.util.Log4jInit; import java.io.InputStream; import java.io.InputStreamReader; -import java.time.LocalDate; -import java.time.ZoneId; -import java.time.format.DateTimeFormatter; -import java.util.ArrayList; -import java.util.Arrays; import org.apache.log4j.Level; import org.apache.log4j.Logger; import org.json.JSONArray; import org.json.JSONObject; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Test; import org.opencadc.vospace.ContainerNode; @@ -40,8 +32,8 @@ import java.security.PrivilegedExceptionAction; import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Optional; +import org.opencadc.vospace.Node; public class AltPermissionsTest extends LifecycleTest { @@ -102,14 +94,10 @@ public void testDOILifecycleWithAlternateSettings() throws Exception { ContainerNode doiNode = getContainerNode(doiID, doiAltParentPathURI, doiAltVosClient); Assert.assertNotNull(doiNode); - Assert.assertEquals(2, doiNode.getReadOnlyGroup().size()); - Assert.assertEquals(2, doiNode.getReadWriteGroup().size()); + Assert.assertEquals(1, doiNode.getReadOnlyGroup().size()); + Assert.assertEquals(1, doiNode.getReadWriteGroup().size()); log.debug("readWriteSubject - DOI created successfully: " + doiID); - // update - success expected - update(actual, doiID, doiAltServiceURL); - log.debug("readWriteSubject - DOI updated successfully: " + doiID); - // publish: failure expected mintDOI403Expected(doiID); log.debug("readWriteSubject - DOI minting failed successfully: " + doiID); @@ -128,20 +116,29 @@ public void testDOILifecycleWithAlternateSettings() throws Exception { List draftDoiStatusList = searchDOIStatuses(null, List.of("in progress")); long draftDOIsCount = draftDoiStatusList.stream().filter(e -> e.getStatus().equals(Status.DRAFT)).count(); Assert.assertEquals(draftDoiStatusList.size(), draftDOIsCount); + + // update doi status to 'review ready' so publisher has read access + log.debug("readWriteSubject - update status to 'review ready'"); + updateStatus(doiID, Status.REVIEW_READY, true); + doiNode = getContainerNode(doiID, doiAltParentPathURI, doiAltVosClient); + checkStatus(doiNode, Status.REVIEW_READY); + log.debug("readWriteSubject - checked status"); + return doiID; }); Subject.doAs(publisherSubject, (PrivilegedExceptionAction) () -> { URL doiURL = new URL(String.format("%s/%s", doiAltServiceURL, doiSuffix)); + log.debug("readWriteSubject - URL: " + doiURL); // Get DOI : Success expected Resource actual = getDOI(doiURL, doiSuffix); log.debug("publisherSubject - get DOI success for DOI ID : " + doiSuffix); - // get all DOIs - publisher should find the created draft DOI + // get all DOIs - publisher should find the 'review ready' DOI List doiStatusList = getDoiStatuses(); - Optional createdDOIStatus = doiStatusList.stream().filter(doiStatus -> getDOISuffix(doiStatus.getIdentifier().getValue()).equals(doiSuffix)).findFirst(); - Assert.assertTrue(createdDOIStatus.isPresent()); + Optional reviewReadyStatus = doiStatusList.stream().filter(doiStatus -> getDOISuffix(doiStatus.getIdentifier().getValue()).equals(doiSuffix)).findFirst(); + Assert.assertTrue(reviewReadyStatus.isPresent()); log.debug("publisherSubject - get All DOI Status success"); // Get DOI status : Success expected @@ -155,6 +152,13 @@ public void testDOILifecycleWithAlternateSettings() throws Exception { Assert.assertEquals(200, get.getResponseCode()); log.debug("publisherSubject - get DOI status success for DOI ID : " + doiSuffix); + // update status to 'in review' to allow the publisher to publish the doi + log.debug("publisherSubject - update status to 'in review'"); + updateStatus(doiSuffix, Status.IN_REVIEW, true); + ContainerNode doiNode = getContainerNode(doiSuffix, doiAltParentPathURI, doiAltVosClient); + checkStatus(doiNode, Status.IN_REVIEW); + log.debug("publisherSubject - checked status"); + //publish DOI : Success expected publish(actual, doiSuffix, DOISettingsType.ALT_DOI); @@ -342,10 +346,26 @@ public void testDOISearchEndpoint() throws PrivilegedActionException, DoiParsing Resource actual = create(expected, DOISettingsType.ALT_DOI); String doiID = getDOISuffix(actual.getIdentifier().getValue()); Assert.assertNotNull(doiID); + + // update doi status to review ready so publisher has read access + log.debug("readWriteSubject - update status to 'review ready'"); + updateStatus(doiID, Status.REVIEW_READY, true); + Node doiNode = getContainerNode(doiID, doiAltParentPathURI, vosClient); + checkStatus(doiNode, Status.REVIEW_READY); + log.debug("readWriteSubject - checked status"); + return doiID; }); Subject.doAs(publisherSubject, (PrivilegedExceptionAction) () -> { + + // update doi status to in review ready so publisher can publish + log.debug("publisherSubject - update status to 'in review'"); + updateStatus(mintedDOIId, Status.IN_REVIEW, true); + Node doiNode = getContainerNode(mintedDOIId, doiAltParentPathURI, vosClient); + checkStatus(doiNode, Status.IN_REVIEW); + log.debug("readWriteSubject - checked status"); + URL doiURL = new URL(String.format("%s/%s", doiAltServiceURL, mintedDOIId)); Resource actual = getDOI(doiURL, mintedDOIId); diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java index 34cee7b3..2dddb4ff 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java @@ -72,18 +72,13 @@ import ca.nrc.cadc.doi.datacite.Resource; import ca.nrc.cadc.doi.status.Status; import ca.nrc.cadc.util.Log4jInit; -import java.net.URL; import java.security.PrivilegedExceptionAction; -import java.util.HashMap; -import java.util.Map; import javax.security.auth.Subject; import org.apache.log4j.Level; import org.apache.log4j.Logger; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Test; import org.opencadc.vospace.Node; -import org.opencadc.vospace.NodeProperty; import org.opencadc.vospace.VOSURI; import org.opencadc.vospace.client.VOSpaceClient; @@ -218,7 +213,7 @@ public void testUpdateStatus() { log.debug("publisher - checked status"); // 'rejected' node permissions, doi-group:r reviewer-group:r public:false - checkPermissions(doiNode, false, false, 2,1); + checkPermissions(doiNode, false, false, 2,0); log.debug("publisher - checked permissions"); return null; @@ -304,26 +299,6 @@ public void testUpdateStatus() { } } - void updateStatus(String doiID, Status requestedStatus, boolean followRedirect) - throws Exception { - log.debug(String.format("update status to '%s'", requestedStatus.getValue())); - URL doiURL = new URL(String.format("%s/%s", doiAltServiceURL, doiID)); - Map params = new HashMap<>(); - params.put(DOI.STATUS_NODE_PARAMETER, requestedStatus.getValue()); - postDOI(doiURL , null, params, followRedirect); - log.debug(String.format("status updated to '%s'", requestedStatus.getValue())); - } - - void checkStatus(Node doiNode, Status expectedStatus) { - - Assert.assertNotNull(doiNode); - - // check the status mode property - NodeProperty status = doiNode.getProperty(DOI.VOSPACE_DOI_STATUS_PROPERTY); - Assert.assertNotNull(status); - Assert.assertEquals(expectedStatus.getValue(), status.getValue()); - } - void checkPermissions(Node doiNode, boolean isLocked, boolean isPublic, int readOnlyGroups, int readWriteGroups) { diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/IntTestBase.java b/doi/src/intTest/java/ca/nrc/cadc/doi/IntTestBase.java index 85a766eb..2d7417d0 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/IntTestBase.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/IntTestBase.java @@ -73,6 +73,7 @@ import ca.nrc.cadc.auth.SSLUtil; import ca.nrc.cadc.doi.datacite.Resource; import ca.nrc.cadc.doi.io.DoiXmlWriter; +import ca.nrc.cadc.doi.status.Status; import ca.nrc.cadc.net.FileContent; import ca.nrc.cadc.net.HttpPost; import ca.nrc.cadc.reg.Standards; @@ -96,6 +97,8 @@ import org.junit.BeforeClass; import org.opencadc.vospace.ContainerNode; import org.opencadc.vospace.DataNode; +import org.opencadc.vospace.Node; +import org.opencadc.vospace.NodeProperty; import org.opencadc.vospace.VOSURI; import org.opencadc.vospace.client.VOSpaceClient; import org.opencadc.vospace.client.async.RecursiveDeleteNode; @@ -221,7 +224,7 @@ protected DataNode createDataNode(String path, String name, DOISettingsType doiS return (DataNode) getVOSClient(doiSettingsType).createNode(nodeURI, node); } - protected ContainerNode getContainerNode(String path, VOSURI doiParentPathURI, VOSpaceClient vosClient) throws Exception { + protected ContainerNode getContainerNode(String path, VOSURI doiParentPathURI, VOSpaceClient vosClient) throws Exception { String nodePath = doiParentPathURI.getPath(); if (StringUtil.hasText(path)) { nodePath = String.format("%s/%s", nodePath, path); @@ -229,6 +232,34 @@ protected ContainerNode getContainerNode(String path, VOSURI doiParentPathURI, return (ContainerNode) vosClient.getNode(nodePath); } + protected DataNode getDataNode(String path, VOSURI doiParentPathURI, VOSpaceClient vosClient) throws Exception { + String nodePath = doiParentPathURI.getPath(); + if (StringUtil.hasText(path)) { + nodePath = String.format("%s/%s", nodePath, path); + } + return (DataNode) vosClient.getNode(nodePath); + } + + void updateStatus(String doiID, Status requestedStatus, boolean followRedirect) + throws Exception { + log.debug(String.format("update status to '%s'", requestedStatus.getValue())); + URL doiURL = new URL(String.format("%s/%s", doiAltServiceURL, doiID)); + Map params = new HashMap<>(); + params.put(DOI.STATUS_NODE_PARAMETER, requestedStatus.getValue()); + postDOI(doiURL , null, params, followRedirect); + log.debug(String.format("status updated to '%s'", requestedStatus.getValue())); + } + + void checkStatus(Node doiNode, Status expectedStatus) { + + Assert.assertNotNull(doiNode); + + // check the status mode property + NodeProperty status = doiNode.getProperty(DOI.VOSPACE_DOI_STATUS_PROPERTY); + Assert.assertNotNull(status); + Assert.assertEquals(expectedStatus.getValue(), status.getValue()); + } + protected String postDOI(URL postUrl, String doiXML, Map nodeMetadata, boolean followRedirect) throws Exception { Map params = new HashMap<>(); diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java index b4ef4ee7..cb8233d3 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java @@ -105,6 +105,7 @@ import org.junit.Test; import org.opencadc.vospace.ContainerNode; import org.opencadc.vospace.DataNode; +import org.opencadc.vospace.Node; import org.opencadc.vospace.VOS; import org.opencadc.vospace.VOSURI; import org.opencadc.vospace.client.ClientTransfer; @@ -303,50 +304,63 @@ void publish(Resource expected, String doiSuffix, DOISettingsType doiSettingsTyp VOSpaceClient vosClient = getVOSClient(doiSettingsType); VOSURI doiParentPathURI = getDoiParentPathURI(doiSettingsType); - // verify the DOI containerNode properties - ContainerNode doiNode = getContainerNode(doiSuffix , doiParentPathURI, vosClient); - Assert.assertFalse("incorrect isPublic property", - doiNode.isPublic != null && doiNode.isPublic); - Assert.assertFalse("should have group read property", - doiNode.getReadOnlyGroup().isEmpty()); + ContainerNode doiNode = getContainerNode(doiSuffix, doiParentPathURI, vosClient); ContainerNode dataNode = getContainerNode(doiSuffix + "/data", doiParentPathURI, vosClient); - Assert.assertFalse("should have group write", - dataNode.getReadWriteGroup().isEmpty()); + ContainerNode dataSubDirNode = null; + String subDir = "subDir"; - // add a file and a subdirectory with a file to the data directory - String testFile1 = "test-file-1.txt"; - String testFile1Path = String.format("%s/data/%s", doiSuffix, testFile1); - DataNode testFileNode = createDataNode(testFile1Path, testFile1, doiSettingsType); + Assert.assertFalse("incorrect isPublic property", doiNode.isPublic != null && doiNode.isPublic); + Assert.assertFalse("should have group read property", doiNode.getReadOnlyGroup().isEmpty()); - String subDir = "subDir"; - String subDirPath = String.format("%s/data/%s", doiSuffix, subDir); - ContainerNode dataSubDirNode = createContainerNode(subDirPath, subDir, doiSettingsType); + // for alternative configuration, only a publisher can publish, but the publisher cannot write to data folder + if (doiSettingsType == DOISettingsType.DOI) { + // verify the DOI containerNode properties + Assert.assertFalse("should have group write", dataNode.getReadWriteGroup().isEmpty()); - String testFile2 = "test-file-2.txt"; - String testFile2Path = String.format("%s/data/%s/%s", doiSuffix, subDir, testFile2); - DataNode testFile2Node = createDataNode(testFile2Path, testFile2, doiSettingsType); + // add a file and a subdirectory with a file to the data directory + String testFile1 = "test-file-1.txt"; + String testFile1Path = String.format("%s/data/%s", doiSuffix, testFile1); + DataNode testFileNode = createDataNode(testFile1Path, testFile1, doiSettingsType); - // mint the document, DRAFT ==> LOCKING_DATA + String subDirPath = String.format("%s/data/%s", doiSuffix, subDir); + dataSubDirNode = createContainerNode(subDirPath, subDir, doiSettingsType); + + String testFile2 = "test-file-2.txt"; + String testFile2Path = String.format("%s/data/%s/%s", doiSuffix, subDir, testFile2); + DataNode testFile2Node = createDataNode(testFile2Path, testFile2, doiSettingsType); + } + + // mint the document, DRAFT or IN REVIEW ==> LOCKING_DATA doMintTest(doiURL); doiNode = getContainerNode(doiSuffix , doiParentPathURI, vosClient); dataNode = getContainerNode(doiSuffix + "/data" , doiParentPathURI, vosClient); - dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir , doiParentPathURI, vosClient); + if (doiSettingsType == DOISettingsType.DOI) { + dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir , doiParentPathURI, vosClient); + } Assert.assertEquals("incorrect status", Status.LOCKING_DATA.getValue(), doiNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); - verifyNodeProperties(doiNode, dataNode, dataSubDirNode); + verifyNodeProperties(doiNode, dataNode, dataSubDirNode, doiSettingsType); log.debug("locking data"); // mint the document, ERROR_LOCKING_DATA ==> LOCKING_DATA doiNode.getProperty(DOI.VOSPACE_DOI_STATUS_PROPERTY).setValue(Status.ERROR_LOCKING_DATA.getValue()); VOSURI vosuri = getVOSURI(doiNode.getName(), doiSettingsType); - vosClient.setNode(vosuri, doiNode); + ContainerNode parentNode = doiNode; + + // calls using the vospace client to update a node, bypassing the doi service, must be done as doiadmin + Subject.doAs(adminSubject, (PrivilegedExceptionAction) () -> { + vosClient.setNode(vosuri, parentNode); + return null; + }); doMintTest(doiURL); doiNode = getContainerNode(doiSuffix , doiParentPathURI, vosClient); dataNode = getContainerNode(doiSuffix + "/data" , doiParentPathURI, vosClient); - dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir , doiParentPathURI, vosClient); + if (doiSettingsType == DOISettingsType.DOI) { + dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir, doiParentPathURI, vosClient); + } Assert.assertEquals("incorrect status", Status.LOCKING_DATA.getValue(), doiNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); - verifyNodeProperties(doiNode, dataNode, dataSubDirNode); + verifyNodeProperties(doiNode, dataNode, dataSubDirNode, doiSettingsType); log.debug("locking data again"); // getStatus() changes LOCKING_DATA == > LOCKED_DATA @@ -354,17 +368,19 @@ void publish(Resource expected, String doiSuffix, DOISettingsType doiSettingsTyp Assert.assertEquals("identifier from DOI status is different", expected.getIdentifier().getValue(), doiStatus.getIdentifier().getValue()); Assert.assertEquals("status is incorrect", Status.LOCKED_DATA, doiStatus.getStatus()); - verifyLockedDataPropertyChanges(doiNode, dataNode, dataSubDirNode); + verifyLockedDataPropertyChanges(doiNode, dataNode, dataSubDirNode, doiSettingsType); log.debug("locked data"); // mint the document, LOCKED_DATA == REGISTERING doMintTest(doiURL); doiNode = getContainerNode(doiSuffix , doiParentPathURI, vosClient); dataNode = getContainerNode(doiSuffix + "/data" , doiParentPathURI, vosClient); - dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir , doiParentPathURI, vosClient); + if (doiSettingsType == DOISettingsType.DOI) { + dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir, doiParentPathURI, vosClient); + } Assert.assertEquals("incorrect status", Status.MINTED.getValue(), doiNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); - verifyMintedStatePropertyChanges(doiNode, dataNode, dataSubDirNode); + verifyMintedStatePropertyChanges(doiNode, dataNode, dataSubDirNode, doiSettingsType); log.debug("registering"); // mint the document, ERROR_REGISTERING ==> REGISTERING @@ -382,10 +398,12 @@ void publish(Resource expected, String doiSuffix, DOISettingsType doiSettingsTyp doMintTest(doiURL); doiNode = getContainerNode(doiSuffix , doiParentPathURI, vosClient); dataNode = getContainerNode(doiSuffix + "/data" , doiParentPathURI, vosClient); - dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir , doiParentPathURI, vosClient); + if (doiSettingsType == DOISettingsType.DOI) { + dataSubDirNode = getContainerNode(doiSuffix + "/data/" + subDir, doiParentPathURI, vosClient); + } Assert.assertEquals("incorrect status", Status.MINTED.getValue(), doiNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); - verifyMintedStatePropertyChanges(doiNode, dataNode, dataSubDirNode); + verifyMintedStatePropertyChanges(doiNode, dataNode, dataSubDirNode, doiSettingsType); // getStatus() changes REGISTERING == > MINTED doiStatus = getStatus(doiURL); @@ -435,51 +453,63 @@ private DoiStatus getStatus(URL doiURL) return reader.read(new StringReader(bos.toString(StandardCharsets.UTF_8))); } - private void verifyDataDirNodeProperties(ContainerNode dataContainerNode, - ContainerNode dataSubDirContainerNode) { + private void verifyDataDirNodeProperties(Node dataNode, Node dataSubNode, DOISettingsType doiType) { + // verify the DOI data containerNode properties - Assert.assertTrue("should be public", dataContainerNode.isPublic != null && dataContainerNode.isPublic); - Assert.assertTrue("should not have group read", dataContainerNode.getReadOnlyGroup().isEmpty()); - Assert.assertTrue("should not have group write", dataContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertTrue("incorrect lock property", dataContainerNode.isLocked != null && dataContainerNode.isLocked); - - // verify the DOI data subDir containerNode properties - Assert.assertTrue("should be public", dataSubDirContainerNode.isPublic != null && dataSubDirContainerNode.isPublic); - Assert.assertTrue("should not have group read", dataSubDirContainerNode.getReadOnlyGroup().isEmpty()); - Assert.assertTrue("should not have group write", dataSubDirContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertTrue("incorrect lock property", dataSubDirContainerNode.isLocked != null && dataSubDirContainerNode.isLocked); + Assert.assertTrue("data folder should be public", dataNode.isPublic != null && dataNode.isPublic); + Assert.assertTrue("should not have group read", dataNode.getReadOnlyGroup().isEmpty()); + Assert.assertTrue("data folder should not have group write", dataNode.getReadWriteGroup().isEmpty()); + Assert.assertTrue("data folder incorrect lock property", dataNode.isLocked != null && dataNode.isLocked); + + // a publisher does not have permissions to write to the data directory + if (doiType == DOISettingsType.DOI) { + // verify the DOI data subDir containerNode properties + Assert.assertTrue("data folder should be public", dataSubNode.isPublic != null && dataSubNode.isPublic); + Assert.assertTrue("data folder should not have group read", dataSubNode.getReadOnlyGroup().isEmpty()); + Assert.assertTrue("data folder should not have group write", dataSubNode.getReadWriteGroup().isEmpty()); + Assert.assertTrue("data folder incorrect lock property", dataSubNode.isLocked != null && dataSubNode.isLocked); + } } - private void verifyNodeProperties(ContainerNode doiContainerNode, ContainerNode dataContainerNode, - ContainerNode dataSubDirContainerNode) { + private void verifyNodeProperties(Node doiNode, Node dataNode, Node dataSubNode, DOISettingsType doiType) { // verify the DOI containerNode properties - Assert.assertFalse("incorrect isPublic property", doiContainerNode.isPublic != null && doiContainerNode.isPublic); - Assert.assertFalse("should have group read", doiContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertFalse("should have group write", doiContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertFalse("incorrect lock property", doiContainerNode.isLocked != null && doiContainerNode.isLocked); + Assert.assertFalse("doi folder incorrect isPublic property", doiNode.isPublic != null && doiNode.isPublic); + Assert.assertFalse("doi folder should have group read", doiNode.getReadOnlyGroup().isEmpty()); + Assert.assertFalse("doi folder incorrect lock property", doiNode.isLocked != null && doiNode.isLocked); + + // a doi should have a write group, and alt-doi should not since the requester and publisher only have read-access to the doi + if (doiType == DOISettingsType.DOI) { + Assert.assertFalse("doi folder should have group write", doiNode.getReadWriteGroup().isEmpty()); + } else { + Assert.assertTrue("doi folder should not have group write", doiNode.getReadWriteGroup().isEmpty()); + } - verifyDataDirNodeProperties(dataContainerNode, dataSubDirContainerNode); + verifyDataDirNodeProperties(dataNode, dataSubNode, doiType); } - private void verifyLockedDataPropertyChanges(ContainerNode doiContainerNode, ContainerNode dataContainerNode, - ContainerNode dataSubDirContainerNode) { + private void verifyLockedDataPropertyChanges(Node doiNode, Node dataNode, Node dataSubNode, DOISettingsType doiType) { // verify the DOI containerNode properties - Assert.assertFalse("incorrect isPublic property", doiContainerNode.isPublic != null && doiContainerNode.isPublic); - Assert.assertFalse("should have group read", doiContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertFalse("should have group write", doiContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertFalse("incorrect lock property", doiContainerNode.isLocked != null && doiContainerNode.isLocked); + Assert.assertFalse("locked doi folder incorrect isPublic property", doiNode.isPublic != null && doiNode.isPublic); + Assert.assertFalse("locked doi folder should have group read", doiNode.getReadOnlyGroup().isEmpty()); + Assert.assertFalse("locked doi folder incorrect lock property", doiNode.isLocked != null && doiNode.isLocked); + + // a doi data folder should have a write group, and alt-doi should not since the requester and publisher only have read-access to the doi + if (doiType == DOISettingsType.DOI) { + Assert.assertFalse("locked doi folder should have group write", doiNode.getReadWriteGroup().isEmpty()); + } else { + Assert.assertTrue("locked doi folder should have group write", doiNode.getReadWriteGroup().isEmpty()); + } - verifyDataDirNodeProperties(dataContainerNode, dataSubDirContainerNode); + verifyDataDirNodeProperties(dataNode, dataSubNode, doiType); } - private void verifyMintedStatePropertyChanges(ContainerNode doiContainerNode, ContainerNode dataContainerNode, - ContainerNode dataSubDirContainerNode) { + private void verifyMintedStatePropertyChanges(Node doiNode, Node dataNode, Node dataSubNode, DOISettingsType doiType) { // verify the DOI containerNode properties - Assert.assertTrue("incorrect isPublic property", doiContainerNode.isPublic != null && doiContainerNode.isPublic); - Assert.assertTrue("should not have group read", doiContainerNode.getReadWriteGroup().isEmpty()); - Assert.assertTrue("should not have group write", doiContainerNode.getReadWriteGroup().isEmpty()); + Assert.assertTrue("minted doi folder incorrect isPublic property", doiNode.isPublic != null && doiNode.isPublic); + Assert.assertTrue("minted doi folder should not have group read", doiNode.getReadOnlyGroup().isEmpty()); + Assert.assertTrue("minted doi folder should not have group write", doiNode.getReadWriteGroup().isEmpty()); - verifyDataDirNodeProperties(dataContainerNode, dataSubDirContainerNode); + verifyDataDirNodeProperties(dataNode, dataSubNode, doiType); } } From fa2ac2fe1e36c9d00f1542a0a44325bfeb98691e Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Mon, 16 Feb 2026 14:13:08 -0800 Subject: [PATCH 4/7] CADC-14486 code review rework --- .../java/ca/nrc/cadc/doi/AltStatusTest.java | 19 +--------------- .../main/java/ca/nrc/cadc/doi/PostAction.java | 22 +++++++++++-------- 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java index 2dddb4ff..722b9bc8 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java @@ -155,7 +155,7 @@ public void testUpdateStatus() { checkStatus(doiNode, Status.IN_REVIEW); log.debug("publisher - checked status"); - // 'review ready' node permissions, doi-group:r reviewer-group:r public:false + // 'inready' node permissions, doi-group:r reviewer-group:r public:false checkPermissions(doiNode, false, false, 2,0); log.debug("publisher - checked permissions"); @@ -276,23 +276,6 @@ public void testUpdateStatus() { return null; }); - Subject.doAs(readWriteSubject, (PrivilegedExceptionAction) () -> { - - // update status = 'in progress' - log.debug("submitter - update status to 'in progress"); - updateStatus(doiSuffix, Status.DRAFT, false); - - Node doiNode = getContainerNode(doiSuffix, doiParentPathURI, vosClient); - checkStatus(doiNode, Status.DRAFT); - log.debug("submitter - checked status"); - - // 'in progress' node permissions, doi-group:rw reviewer-group:- public:false - checkPermissions(doiNode, false, false, 1,1); - log.debug("submitter - checked permissions"); - - return null; - }); - } catch (Exception unexpected) { log.debug("unexpected error: " + unexpected); Assert.fail("unexpected error: " + unexpected.getMessage()); diff --git a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java index 34ddfa48..792be9d9 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java @@ -575,25 +575,21 @@ private void updatePermissions(Node doiNode, VOSURI vosuri, Status current, Stat } // can only update to 'in progress' from 'review ready', 'in review', 'rejected', or 'approved' - } else if ((current == Status.REVIEW_READY || current == Status.IN_REVIEW - || current == Status.REJECTED || current == Status.APPROVED) - && updated == Status.DRAFT) { + } else if (current == Status.REVIEW_READY || current == Status.IN_REVIEW + || current == Status.REJECTED && updated == Status.DRAFT) { log.debug(String.format("update status: '%s' -> '%s'", current, updated)); - // alt config only: + // alt config only, update status to 'in progress': // - from 'review ready': requester or admin (author withdraws) - // - from 'in review': requester, publisher, or admin + // - from 'in review': publisher, or admin // - from 'rejected': requester or admin - // - from 'approved': requester or admin (author wants to edit after approval) boolean canTransition = false; if (current == Status.REVIEW_READY) { canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); } else if (current == Status.IN_REVIEW) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserPublisher() || isCallingUserDOIAdmin(); + canTransition = isCallingUserPublisher() || isCallingUserDOIAdmin(); } else if (current == Status.REJECTED) { canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); - } else if (current == Status.APPROVED) { - canTransition = isCallingUserRequester(doiNode) || isCallingUserDOIAdmin(); } else { String message = String.format("Invalid status change: '%s' -> '%s'", current, updated); throw new IllegalArgumentException(message); @@ -704,6 +700,14 @@ private void performDOIAction() throws Exception { Status mintingStatus = Status.toValue(doiContainerNode.getPropertyValue(DOI.VOSPACE_DOI_STATUS_PROPERTY)); switch (mintingStatus) { case DRAFT: + if (isAlternativeConfiguration()) { + throw new IllegalArgumentException("Cannot publish an 'in progress' DOI for an alternative configuration"); + } + lockData(doiContainerNode); + break; + case REVIEW_READY: + case REJECTED: + throw new IllegalArgumentException("Cannot publish a DOI with status '" + mintingStatus + "'"); case IN_REVIEW: case APPROVED: case ERROR_LOCKING_DATA: From fe3390e08cf35af7a17a5a327512ff37498d00be Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Mon, 16 Feb 2026 15:28:05 -0800 Subject: [PATCH 5/7] CADC-14486 code review rework --- doi/src/main/java/ca/nrc/cadc/doi/PostAction.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java index 792be9d9..ec238e6c 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java @@ -539,12 +539,12 @@ private void updateNodeProperties(JSONObject nodeData) throws Exception { // If the status changed, update the node permissions for an alternate configuration if (updatedStatus != null && isAlternativeConfiguration()) { - updatePermissions(doiNode, vosuri, currentStatus, updatedStatus); + updatePermissions(doiNode, currentStatus, updatedStatus); } vospaceDoiClient.getVOSpaceClient().setNode(vosuri, doiNode); } - private void updatePermissions(Node doiNode, VOSURI vosuri, Status current, Status updated) throws Exception { + private void updatePermissions(Node doiNode, Status current, Status updated) throws Exception { if (current != updated) { GroupURI doiGroupUri = createGroupURI(doiGroupPrefix + doiSuffix); @@ -574,7 +574,7 @@ private void updatePermissions(Node doiNode, VOSURI vosuri, Status current, Stat throw new IllegalArgumentException("Invalid status change: 'review ready' -> 'in review'"); } - // can only update to 'in progress' from 'review ready', 'in review', 'rejected', or 'approved' + // can only update to 'in progress' from 'review ready', 'in review', or 'rejected' } else if (current == Status.REVIEW_READY || current == Status.IN_REVIEW || current == Status.REJECTED && updated == Status.DRAFT) { log.debug(String.format("update status: '%s' -> '%s'", current, updated)); From 8d799cc5757f7c71da4b9fbf61703ad36f7662da Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Tue, 17 Feb 2026 09:45:51 -0800 Subject: [PATCH 6/7] CADC-14486 code review rework --- doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java | 2 ++ doi/src/main/java/ca/nrc/cadc/doi/PostAction.java | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java index cb8233d3..2b136b65 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/LifecycleTest.java @@ -283,6 +283,8 @@ void update(Resource expected, String doiSuffix, URL doiServiceURL) throws Excep // Update the DOI Resource actual = doUpdateTest(expected, doiURL); + // tests randomly fails when the transfer to vospace is slow, hence the sleep cycles + Thread.sleep(3000); compareResource(expected, actual, true); // remove updated properties diff --git a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java index ec238e6c..ae2ef2b5 100644 --- a/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java +++ b/doi/src/main/java/ca/nrc/cadc/doi/PostAction.java @@ -575,8 +575,8 @@ private void updatePermissions(Node doiNode, Status current, Status updated) thr } // can only update to 'in progress' from 'review ready', 'in review', or 'rejected' - } else if (current == Status.REVIEW_READY || current == Status.IN_REVIEW - || current == Status.REJECTED && updated == Status.DRAFT) { + } else if ((current == Status.REVIEW_READY || current == Status.IN_REVIEW + || current == Status.REJECTED) && updated == Status.DRAFT) { log.debug(String.format("update status: '%s' -> '%s'", current, updated)); // alt config only, update status to 'in progress': From 9b6e37193f243fad23df17dd8bdacfe513180400 Mon Sep 17 00:00:00 2001 From: Jeff Burke Date: Tue, 17 Feb 2026 10:14:42 -0800 Subject: [PATCH 7/7] CADC-14486 fix typo --- doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java index 722b9bc8..0c84921f 100644 --- a/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java +++ b/doi/src/intTest/java/ca/nrc/cadc/doi/AltStatusTest.java @@ -155,7 +155,7 @@ public void testUpdateStatus() { checkStatus(doiNode, Status.IN_REVIEW); log.debug("publisher - checked status"); - // 'inready' node permissions, doi-group:r reviewer-group:r public:false + // 'in review' node permissions, doi-group:r reviewer-group:r public:false checkPermissions(doiNode, false, false, 2,0); log.debug("publisher - checked permissions");