From 924678a092775124f56cc114126ea52a163550fa Mon Sep 17 00:00:00 2001 From: Kim Shepherd Date: Mon, 26 May 2025 10:15:40 +0200 Subject: [PATCH] [TLC-1117] Inherit custom, non-admin policies when creating new bundles Also shifted some resource policy methods from ItemService to AuthorizeService as they seemed better suited there. --- .../authorize/AuthorizeServiceImpl.java | 117 +++++++++++++++++- .../authorize/service/AuthorizeService.java | 19 +++ .../org/dspace/content/ItemServiceImpl.java | 96 +------------- .../authorize/AuthorizeServiceTest.java | 97 +++++++++++++++ 4 files changed, 235 insertions(+), 94 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java b/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java index 16273c4ea8..e1ea6a5343 100644 --- a/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java @@ -15,6 +15,7 @@ import java.time.LocalDate; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.UUID; import org.apache.commons.collections4.CollectionUtils; @@ -47,6 +48,7 @@ import org.dspace.discovery.indexobject.IndexableItem; import org.dspace.eperson.EPerson; import org.dspace.eperson.Group; import org.dspace.eperson.service.GroupService; +import org.dspace.services.ConfigurationService; import org.dspace.workflow.WorkflowItemService; import org.springframework.beans.factory.annotation.Autowired; @@ -83,6 +85,8 @@ public class AuthorizeServiceImpl implements AuthorizeService { protected WorkflowItemService workflowItemService; @Autowired(required = true) private SearchService searchService; + @Autowired(required = true) + private ConfigurationService configurationService; protected AuthorizeServiceImpl() { @@ -507,17 +511,26 @@ public class AuthorizeServiceImpl implements AuthorizeService { return resourcePolicyService.find(c, o, actionID); } + @Override + public void inheritPolicies(Context c, DSpaceObject src, DSpaceObject dest) + throws SQLException, AuthorizeException { + inheritPolicies(c, src, dest, false); + } + @Override public void inheritPolicies(Context c, DSpaceObject src, - DSpaceObject dest) throws SQLException, AuthorizeException { + DSpaceObject dest, boolean includeCustom) throws SQLException, AuthorizeException { // find all policies for the source object List policies = getPolicies(c, src); - //Only inherit non-ADMIN policies (since ADMIN policies are automatically inherited) - //and non-custom policies as these are manually applied when appropriate + // Only inherit non-ADMIN policies (since ADMIN policies are automatically inherited) + // and non-custom policies (usually applied manually?) UNLESS specified otherwise with includCustom + // (for example, item.addBundle() will inherit custom policies to enforce access conditions) List nonAdminPolicies = new ArrayList<>(); for (ResourcePolicy rp : policies) { - if (rp.getAction() != Constants.ADMIN && !StringUtils.equals(rp.getRpType(), ResourcePolicy.TYPE_CUSTOM)) { + if (rp.getAction() != Constants.ADMIN && (!StringUtils.equals(rp.getRpType(), ResourcePolicy.TYPE_CUSTOM) + || (includeCustom && StringUtils.equals(rp.getRpType(), ResourcePolicy.TYPE_CUSTOM) + && isNotAlreadyACustomRPOfThisTypeOnDSO(c, dest)))) { nonAdminPolicies.add(rp); } } @@ -943,4 +956,100 @@ public class AuthorizeServiceImpl implements AuthorizeService { return query + " AND "; } } + + /** + * Add the default policies, which have not been already added to the given DSpace object + * + * @param context The relevant DSpace Context. + * @param dso The DSpace Object to add policies to + * @param defaultCollectionPolicies list of policies + * @throws SQLException An exception that provides information on a database access error or other errors. + * @throws AuthorizeException Exception indicating the current user of the context does not have permission + * to perform a particular action. + */ + @Override + public void addDefaultPoliciesNotInPlace(Context context, DSpaceObject dso, + List defaultCollectionPolicies) throws SQLException, AuthorizeException { + boolean appendMode = configurationService + .getBooleanProperty("core.authorization.installitem.inheritance-read.append-mode", false); + for (ResourcePolicy defaultPolicy : defaultCollectionPolicies) { + if (!isAnIdenticalPolicyAlreadyInPlace(context, dso, defaultPolicy.getGroup(), Constants.READ, + defaultPolicy.getID()) && + (!appendMode && isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso) || + appendMode && shouldBeAppended(context, dso, defaultPolicy))) { + ResourcePolicy newPolicy = resourcePolicyService.clone(context, defaultPolicy); + newPolicy.setdSpaceObject(dso); + newPolicy.setAction(Constants.READ); + newPolicy.setRpType(ResourcePolicy.TYPE_INHERITED); + resourcePolicyService.update(context, newPolicy); + } + } + } + + /** + * Add a list of custom policies if there are already NO custom policies in place + * + */ + @Override + public void addCustomPoliciesNotInPlace(Context context, DSpaceObject dso, List customPolicies) + throws SQLException, AuthorizeException { + boolean customPoliciesAlreadyInPlace = + findPoliciesByDSOAndType(context, dso, ResourcePolicy.TYPE_CUSTOM).size() > 0; + if (!customPoliciesAlreadyInPlace) { + addPolicies(context, customPolicies, dso); + } + } + + /** + * Check whether or not there is already an RP on the given dso, which has actionId={@link Constants.READ} and + * resourceTypeId={@link ResourcePolicy.TYPE_CUSTOM} + * + * @param context DSpace context + * @param dso DSpace object to check for custom read RP + * @return True if there is no RP on the item with custom read RP, otherwise false + * @throws SQLException If something goes wrong retrieving the RP on the DSO + */ + private boolean isNotAlreadyACustomRPOfThisTypeOnDSO(Context context, DSpaceObject dso) throws SQLException { + return isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso, Constants.READ); + } + + private boolean isNotAlreadyACustomRPOfThisTypeOnDSO(Context context, DSpaceObject dso, int action) + throws SQLException { + List rps = resourcePolicyService.find(context, dso, action); + for (ResourcePolicy rp : rps) { + if (rp.getRpType() != null && rp.getRpType().equals(ResourcePolicy.TYPE_CUSTOM)) { + return false; + } + } + return true; + } + + /** + * Check if the provided default policy should be appended or not to the final + * item. If an item has at least one custom READ policy any anonymous READ + * policy with empty start/end date should be skipped + * + * @param context DSpace context + * @param dso DSpace object to check for custom read RP + * @param defaultPolicy The policy to check + * @return + * @throws SQLException If something goes wrong retrieving the RP on the DSO + */ + private boolean shouldBeAppended(Context context, DSpaceObject dso, ResourcePolicy defaultPolicy) + throws SQLException { + boolean hasCustomPolicy = resourcePolicyService.find(context, dso, Constants.READ) + .stream() + .filter(rp -> (Objects.nonNull(rp.getRpType()) && + Objects.equals(rp.getRpType(), ResourcePolicy.TYPE_CUSTOM))) + .findFirst() + .isPresent(); + + boolean isAnonymousGroup = Objects.nonNull(defaultPolicy.getGroup()) + && StringUtils.equals(defaultPolicy.getGroup().getName(), Group.ANONYMOUS); + + boolean datesAreNull = Objects.isNull(defaultPolicy.getStartDate()) + && Objects.isNull(defaultPolicy.getEndDate()); + + return !(hasCustomPolicy && isAnonymousGroup && datesAreNull); + } } diff --git a/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java b/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java index ef42ed9108..9161c4b00e 100644 --- a/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java +++ b/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java @@ -322,6 +322,19 @@ public interface AuthorizeService { */ public List getPoliciesActionFilterExceptRpType(Context c, DSpaceObject o, int actionID, String rpType) throws SQLException; + /** + * Add policies to an object to match those from a previous object + * + * @param c context + * @param src source of policies + * @param dest destination of inherited policies + * @param includeCustom whether TYPE_CUSTOM policies should be inherited + * @throws SQLException if there's a database problem + * @throws AuthorizeException if the current user is not authorized to add these policies + */ + public void inheritPolicies(Context c, DSpaceObject src, DSpaceObject dest, boolean includeCustom) + throws SQLException, AuthorizeException; + /** * Add policies to an object to match those from a previous object * @@ -605,4 +618,10 @@ public interface AuthorizeService { public void replaceAllPolicies(Context context, DSpaceObject source, DSpaceObject dest) throws SQLException, AuthorizeException; + public void addDefaultPoliciesNotInPlace(Context context, DSpaceObject dso, + List defaultCollectionPolicies) throws SQLException, AuthorizeException; + + public void addCustomPoliciesNotInPlace(Context context, DSpaceObject dso, + List defaultCollectionPolicies) throws SQLException, AuthorizeException; + } diff --git a/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java index 0324be09f9..6dcba8f0a1 100644 --- a/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java @@ -480,7 +480,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl implements It // now add authorization policies from owning item // hmm, not very "multiple-inclusion" friendly - authorizeService.inheritPolicies(context, item, bundle); + authorizeService.inheritPolicies(context, item, bundle, true); // Add the bundle to in-memory list item.addBundle(bundle); @@ -1046,8 +1046,8 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl implements It // if come from InstallItem: remove all submission/workflow policies authorizeService.removeAllPoliciesByDSOAndType(context, mybundle, ResourcePolicy.TYPE_SUBMISSION); authorizeService.removeAllPoliciesByDSOAndType(context, mybundle, ResourcePolicy.TYPE_WORKFLOW); - addCustomPoliciesNotInPlace(context, mybundle, defaultItemPolicies); - addDefaultPoliciesNotInPlace(context, mybundle, defaultCollectionBundlePolicies); + authorizeService.addCustomPoliciesNotInPlace(context, mybundle, defaultItemPolicies); + authorizeService.addDefaultPoliciesNotInPlace(context, mybundle, defaultCollectionBundlePolicies); for (Bitstream bitstream : mybundle.getBitstreams()) { // If collection has default READ policies, remove the bundle's READ policies. @@ -1093,8 +1093,8 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl implements It throws SQLException, AuthorizeException { authorizeService.removeAllPoliciesByDSOAndType(context, bitstream, ResourcePolicy.TYPE_SUBMISSION); authorizeService.removeAllPoliciesByDSOAndType(context, bitstream, ResourcePolicy.TYPE_WORKFLOW); - addCustomPoliciesNotInPlace(context, bitstream, defaultItemPolicies); - addDefaultPoliciesNotInPlace(context, bitstream, defaultCollectionPolicies); + authorizeService.addCustomPoliciesNotInPlace(context, bitstream, defaultItemPolicies); + authorizeService.addDefaultPoliciesNotInPlace(context, bitstream, defaultCollectionPolicies); } @Override @@ -1132,7 +1132,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl implements It authorizeService.removeAllPoliciesByDSOAndType(context, item, ResourcePolicy.TYPE_WORKFLOW); // add default policies only if not already in place - addDefaultPoliciesNotInPlace(context, item, defaultCollectionPolicies); + authorizeService.addDefaultPoliciesNotInPlace(context, item, defaultCollectionPolicies); } finally { context.restoreAuthSystemState(); } @@ -1322,91 +1322,7 @@ prevent the generation of resource policy entry values with null dspace_object a */ - /** - * Add the default policies, which have not been already added to the given DSpace object - * - * @param context The relevant DSpace Context. - * @param dso The DSpace Object to add policies to - * @param defaultCollectionPolicies list of policies - * @throws SQLException An exception that provides information on a database access error or other errors. - * @throws AuthorizeException Exception indicating the current user of the context does not have permission - * to perform a particular action. - */ - protected void addDefaultPoliciesNotInPlace(Context context, DSpaceObject dso, - List defaultCollectionPolicies) throws SQLException, AuthorizeException { - boolean appendMode = configurationService - .getBooleanProperty("core.authorization.installitem.inheritance-read.append-mode", false); - for (ResourcePolicy defaultPolicy : defaultCollectionPolicies) { - if (!authorizeService - .isAnIdenticalPolicyAlreadyInPlace(context, dso, defaultPolicy.getGroup(), Constants.READ, - defaultPolicy.getID()) && - (!appendMode && isNotAlreadyACustomRPOfThisTypeOnDSO(context, dso) || - appendMode && shouldBeAppended(context, dso, defaultPolicy))) { - ResourcePolicy newPolicy = resourcePolicyService.clone(context, defaultPolicy); - newPolicy.setdSpaceObject(dso); - newPolicy.setAction(Constants.READ); - newPolicy.setRpType(ResourcePolicy.TYPE_INHERITED); - resourcePolicyService.update(context, newPolicy); - } - } - } - private void addCustomPoliciesNotInPlace(Context context, DSpaceObject dso, List customPolicies) - throws SQLException, AuthorizeException { - boolean customPoliciesAlreadyInPlace = authorizeService - .findPoliciesByDSOAndType(context, dso, ResourcePolicy.TYPE_CUSTOM).size() > 0; - if (!customPoliciesAlreadyInPlace) { - authorizeService.addPolicies(context, customPolicies, dso); - } - } - - /** - * Check whether or not there is already an RP on the given dso, which has actionId={@link Constants.READ} and - * resourceTypeId={@link ResourcePolicy.TYPE_CUSTOM} - * - * @param context DSpace context - * @param dso DSpace object to check for custom read RP - * @return True if there is no RP on the item with custom read RP, otherwise false - * @throws SQLException If something goes wrong retrieving the RP on the DSO - */ - private boolean isNotAlreadyACustomRPOfThisTypeOnDSO(Context context, DSpaceObject dso) throws SQLException { - List readRPs = resourcePolicyService.find(context, dso, Constants.READ); - for (ResourcePolicy readRP : readRPs) { - if (readRP.getRpType() != null && readRP.getRpType().equals(ResourcePolicy.TYPE_CUSTOM)) { - return false; - } - } - return true; - } - - /** - * Check if the provided default policy should be appended or not to the final - * item. If an item has at least one custom READ policy any anonymous READ - * policy with empty start/end date should be skipped - * - * @param context DSpace context - * @param dso DSpace object to check for custom read RP - * @param defaultPolicy The policy to check - * @return - * @throws SQLException If something goes wrong retrieving the RP on the DSO - */ - private boolean shouldBeAppended(Context context, DSpaceObject dso, ResourcePolicy defaultPolicy) - throws SQLException { - boolean hasCustomPolicy = resourcePolicyService.find(context, dso, Constants.READ) - .stream() - .filter(rp -> (Objects.nonNull(rp.getRpType()) && - Objects.equals(rp.getRpType(), ResourcePolicy.TYPE_CUSTOM))) - .findFirst() - .isPresent(); - - boolean isAnonimousGroup = Objects.nonNull(defaultPolicy.getGroup()) - && StringUtils.equals(defaultPolicy.getGroup().getName(), Group.ANONYMOUS); - - boolean datesAreNull = Objects.isNull(defaultPolicy.getStartDate()) - && Objects.isNull(defaultPolicy.getEndDate()); - - return !(hasCustomPolicy && isAnonimousGroup && datesAreNull); - } /** * Returns an iterator of Items possessing the passed metadata field, or only diff --git a/dspace-api/src/test/java/org/dspace/authorize/AuthorizeServiceTest.java b/dspace-api/src/test/java/org/dspace/authorize/AuthorizeServiceTest.java index 70eaa2a0b9..e8bb428db5 100644 --- a/dspace-api/src/test/java/org/dspace/authorize/AuthorizeServiceTest.java +++ b/dspace-api/src/test/java/org/dspace/authorize/AuthorizeServiceTest.java @@ -9,14 +9,24 @@ package org.dspace.authorize; import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; import org.dspace.AbstractUnitTest; import org.dspace.authorize.factory.AuthorizeServiceFactory; import org.dspace.authorize.service.ResourcePolicyService; +import org.dspace.content.Bundle; +import org.dspace.content.Collection; import org.dspace.content.Community; +import org.dspace.content.Item; +import org.dspace.content.WorkspaceItem; import org.dspace.content.factory.ContentServiceFactory; +import org.dspace.content.service.BundleService; import org.dspace.content.service.CollectionService; import org.dspace.content.service.CommunityService; +import org.dspace.content.service.InstallItemService; +import org.dspace.content.service.ItemService; +import org.dspace.content.service.WorkspaceItemService; import org.dspace.core.Constants; import org.dspace.eperson.EPerson; import org.dspace.eperson.Group; @@ -38,6 +48,10 @@ public class AuthorizeServiceTest extends AbstractUnitTest { .getResourcePolicyService(); protected CommunityService communityService = ContentServiceFactory.getInstance().getCommunityService(); protected CollectionService collectionService = ContentServiceFactory.getInstance().getCollectionService(); + protected ItemService itemService = ContentServiceFactory.getInstance().getItemService(); + protected BundleService bundleService = ContentServiceFactory.getInstance().getBundleService(); + protected WorkspaceItemService workspaceItemService = ContentServiceFactory.getInstance().getWorkspaceItemService(); + protected InstallItemService installItemService = ContentServiceFactory.getInstance().getInstallItemService(); public AuthorizeServiceTest() { } @@ -127,6 +141,89 @@ public class AuthorizeServiceTest extends AbstractUnitTest { throw new AssertionError(ex); } } + + /** + * When a bundle is created it should inherit custom policies (deduped) + * from the item, as otherwise bitstream bundles created via filter-media etc. + * will be created without READ policies + */ + @Test + public void testInheritanceOfCustomPolicies() { + try { + context.turnOffAuthorisationSystem(); + Community community = communityService.create(null, context); + Collection collection = collectionService.create(context, community); + WorkspaceItem wsItem = workspaceItemService.create(context, collection, false); + Item item = installItemService.installItem(context, wsItem); + // Simulate access conditions adding READ policy to the item + ResourcePolicy itemCustomRead = resourcePolicyService.create(context, eperson, null); + itemCustomRead.setAction(Constants.READ); + itemCustomRead.setRpType(ResourcePolicy.TYPE_CUSTOM); + // Simulate a random ADMIN action policy that might have been added manually + ResourcePolicy itemCustomAdmin = resourcePolicyService.create(context, eperson, null); + itemCustomAdmin.setAction(Constants.ADMIN); + itemCustomAdmin.setRpType(ResourcePolicy.TYPE_CUSTOM); + List customPolicies = new ArrayList<>(); + customPolicies.add(itemCustomRead); + customPolicies.add(itemCustomAdmin); + authorizeService.addPolicies(context, customPolicies, item); + // Create a bundle, this should call inheritPolicies via itemService.addBundle + Bundle bundle = bundleService.create(context, item, "THUMBNAIL"); + List newPolicies = authorizeService + .findPoliciesByDSOAndType(context, bundle, ResourcePolicy.TYPE_CUSTOM); + Assert.assertEquals("Bundle should inherit custom policy from item", 1, newPolicies.size()); + Assert.assertNotEquals("Bundle should ONLY inherit non-admin custom policy from item", + Constants.ADMIN, newPolicies.get(0).getAction()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + context.restoreAuthSystemState(); + } + } + + /** + * For other DSOs (which pass false) and for a bundle explicitly calling + * inheritPolicies(..., false), the TYPE_CUSTOM policies should not be inherited + * but other non-admin policies should be inherited as usual + */ + @Test + public void testNonInheritanceOfCustomPolicies() { + try { + context.turnOffAuthorisationSystem(); + Community community = communityService.create(null, context); + Collection collection = collectionService.create(context, community); + WorkspaceItem wsItem = workspaceItemService.create(context, collection, false); + Item item = installItemService.installItem(context, wsItem); + Bundle bundle = bundleService.create(context, item, "THUMBNAIL"); + // Simulate a custom READ policy added by access conditions step + ResourcePolicy itemCustomRead = resourcePolicyService.create(context, eperson, null); + itemCustomRead.setAction(Constants.READ); + itemCustomRead.setRpType(ResourcePolicy.TYPE_CUSTOM); + // Simulate an ordinary default read item policy inherited from collection + ResourcePolicy itemDefaultRead = resourcePolicyService.create(context, eperson, null); + itemDefaultRead.setAction(Constants.READ); + itemDefaultRead.setRpType(ResourcePolicy.TYPE_INHERITED); + List customPolicies = new ArrayList<>(); + customPolicies.add(itemCustomRead); + customPolicies.add(itemDefaultRead); + authorizeService.addPolicies(context, customPolicies, item); + // Now, inherit policies for bundle with includeCustom=false (which is how other DSOs behave) + authorizeService.inheritPolicies(context, item, bundle, false); + List newCustomPolicies = authorizeService + .findPoliciesByDSOAndType(context, bundle, ResourcePolicy.TYPE_CUSTOM); + List newInheritedPolicies = authorizeService + .findPoliciesByDSOAndType(context, bundle, ResourcePolicy.TYPE_INHERITED); + Assert.assertEquals("Bundle should not inherit custom policy from item, if false passed", + 0, newCustomPolicies.size()); + Assert.assertEquals("Bundle should inherit non-custom, non-admin policies as usual", + ResourcePolicy.TYPE_INHERITED, newInheritedPolicies.get(0).getRpType()); + } catch (Exception e) { + throw new RuntimeException(e); + } finally { + context.restoreAuthSystemState(); + } + } + // // @Test // public void testIsCollectionAdmin() throws SQLException, AuthorizeException, IOException {