Merge pull request #10774 from the-library-code/9212_bundle_policy_inheritance

Inherit custom, non-admin policies when creating new bundles

dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java

@@ -16,6 +16,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;
@@ -48,6 +49,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;
 
@@ -84,6 +86,8 @@ public class AuthorizeServiceImpl implements AuthorizeService {
     protected WorkflowItemService workflowItemService;
     @Autowired(required = true)
     private SearchService searchService;
+    @Autowired(required = true)
+    private ConfigurationService configurationService;
 
 
     protected AuthorizeServiceImpl() {
@@ -508,17 +512,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<ResourcePolicy> policies = getPolicies(c, src);
 
-        //Only inherit non-ADMIN policies (since ADMIN policies are automatically inherited)
+        // Only inherit non-ADMIN policies (since ADMIN policies are automatically inherited)
-        //and non-custom policies as these are manually applied when appropriate
+        // 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<ResourcePolicy> 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);
             }
         }
@@ -944,4 +957,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<ResourcePolicy> 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<ResourcePolicy> 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<ResourcePolicy> 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);
+    }
 }

dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java

@@ -322,6 +322,19 @@ public interface AuthorizeService {
      */
     public List<ResourcePolicy> 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<ResourcePolicy> defaultCollectionPolicies) throws SQLException, AuthorizeException;
+
+    public void addCustomPoliciesNotInPlace(Context context, DSpaceObject dso,
+            List<ResourcePolicy> defaultCollectionPolicies) throws SQLException, AuthorizeException;
+
 }

dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java

@@ -480,7 +480,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl<Item> 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<Item> 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);
+            authorizeService.addCustomPoliciesNotInPlace(context, mybundle, defaultItemPolicies);
-            addDefaultPoliciesNotInPlace(context, mybundle, defaultCollectionBundlePolicies);
+            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<Item> implements It
         throws SQLException, AuthorizeException {
         authorizeService.removeAllPoliciesByDSOAndType(context, bitstream, ResourcePolicy.TYPE_SUBMISSION);
         authorizeService.removeAllPoliciesByDSOAndType(context, bitstream, ResourcePolicy.TYPE_WORKFLOW);
-        addCustomPoliciesNotInPlace(context, bitstream, defaultItemPolicies);
+        authorizeService.addCustomPoliciesNotInPlace(context, bitstream, defaultItemPolicies);
-        addDefaultPoliciesNotInPlace(context, bitstream, defaultCollectionPolicies);
+        authorizeService.addDefaultPoliciesNotInPlace(context, bitstream, defaultCollectionPolicies);
     }
 
     @Override
@@ -1132,7 +1132,7 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl<Item> 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<ResourcePolicy> 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<ResourcePolicy> 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<ResourcePolicy> 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

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<ResourcePolicy> 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<ResourcePolicy> 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<ResourcePolicy> 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<ResourcePolicy> newCustomPolicies = authorizeService
+                .findPoliciesByDSOAndType(context, bundle, ResourcePolicy.TYPE_CUSTOM);
+            List<ResourcePolicy> 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 {