Cache authorized actions and group membership when Context is in READ-ONLY mode

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

@@ -371,6 +371,11 @@ public class AuthorizeServiceImpl implements AuthorizeService
             return false;
         }
 
+        Boolean cachedResult = c.getCachedAuthorizationResult(o, Constants.ADMIN, c.getCurrentUser());
+        if (cachedResult != null) {
+            return cachedResult.booleanValue();
+        }
+
         //
         // First, check all Resource Policies directly on this object
         //
@@ -383,6 +388,7 @@ public class AuthorizeServiceImpl implements AuthorizeService
             {
                 if (rp.getEPerson() != null && rp.getEPerson().equals(c.getCurrentUser()))
                 {
+                    c.cacheAuthorizedAction(o, Constants.ADMIN, c.getCurrentUser(), true);
                     return true; // match
                 }
 
@@ -391,6 +397,7 @@ public class AuthorizeServiceImpl implements AuthorizeService
                 {
                     // group was set, and eperson is a member
                     // of that group
+                    c.cacheAuthorizedAction(o, Constants.ADMIN, c.getCurrentUser(), true);
                     return true;
                 }
             }
@@ -403,9 +410,12 @@ public class AuthorizeServiceImpl implements AuthorizeService
         DSpaceObject parent = serviceFactory.getDSpaceObjectService(o).getParentObject(c, o);
         if (parent != null)
         {
-            return isAdmin(c, parent);
+            boolean admin = isAdmin(c, parent);
+            c.cacheAuthorizedAction(o, Constants.ADMIN, c.getCurrentUser(), admin);
+            return admin;
         }
 
+        c.cacheAuthorizedAction(o, Constants.ADMIN, c.getCurrentUser(), false);
         return false;
     }
 

dspace-api/src/main/java/org/dspace/core/Context.java

@@ -7,7 +7,10 @@
  */
 package org.dspace.core;
 
+import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.tuple.ImmutablePair;
 import org.apache.commons.lang3.tuple.ImmutableTriple;
+import org.apache.commons.lang3.tuple.Pair;
 import org.apache.commons.lang3.tuple.Triple;
 import org.apache.log4j.Logger;
 import org.dspace.content.DSpaceObject;
@@ -80,9 +83,8 @@ public class Context
     /** Context mode */
     private Mode mode = Mode.READ_WRITE;
 
-    /** Authorized actions cache that is used when the context is in READ_ONLY mode.
+    /** Cache that is only used the context is in READ_ONLY mode */
-     * The key of the cache is: DSpace Object ID, action ID, Eperson ID. */
+    private ContextReadOnlyCache readOnlyCache = new ContextReadOnlyCache();
-    private final HashMap<Triple<String, Integer, String>, Boolean> authorizedActionsCache = new HashMap<>();
 
     protected EventService eventService;
 
@@ -391,12 +393,16 @@ public class Context
             log.info("commit() was called on a closed Context object. No changes to commit.");
         }
 
+        if(isReadOnly()) {
+            throw new UnsupportedOperationException("You cannot commit a read-only context");
+        }
+
         // Our DB Connection (Hibernate) will decide if an actual commit is required or not
         try
         {
             // As long as we have a valid, writeable database connection,
             // commit any changes made as part of the transaction
-            if (isValid() && !isReadOnly())
+            if (isValid())
             {
                 dispatchEvents();
             }
@@ -721,7 +727,7 @@ public class Context
 
     public Boolean getCachedAuthorizationResult(DSpaceObject dspaceObject, int action, EPerson eperson) {
         if(isReadOnly()) {
-            return authorizedActionsCache.get(buildAuthorizedActionKey(dspaceObject, action, eperson));
+            return readOnlyCache.getCachedAuthorizationResult(dspaceObject, action, eperson);
         } else {
             return null;
         }
@@ -729,14 +735,36 @@ public class Context
 
     public void cacheAuthorizedAction(DSpaceObject dspaceObject, int action, EPerson eperson, Boolean result) {
         if(isReadOnly()) {
-            authorizedActionsCache.put(buildAuthorizedActionKey(dspaceObject, action, eperson), result);
+            readOnlyCache.cacheAuthorizedAction(dspaceObject, action, eperson, result);
         }
     }
 
-    private ImmutableTriple<String, Integer, String> buildAuthorizedActionKey(DSpaceObject dspaceObject, int action, EPerson eperson) {
+    public Boolean getCachedGroupMembership(Group group, EPerson eperson) {
-        return new ImmutableTriple<>(dspaceObject == null ? "" : dspaceObject.getID().toString(),
+        if(isReadOnly()) {
-                Integer.valueOf(action),
+            return readOnlyCache.getCachedGroupMembership(group, eperson);
-                eperson == null ? "" : eperson.getID().toString());
+        } else {
+            return null;
+        }
+    }
+
+    public void cacheGroupMembership(Group group, EPerson eperson, Boolean isMember) {
+        if(isReadOnly()) {
+            readOnlyCache.cacheGroupMembership(group, eperson, isMember);
+        }
+    }
+
+    public void cacheAllMemberGroupsSet(EPerson ePerson, Set<Group> groups) {
+        if(isReadOnly()) {
+            readOnlyCache.cacheAllMemberGroupsSet(ePerson, groups);
+        }
+    }
+
+    public Set<Group> getCachedAllMemberGroupsSet(EPerson ePerson) {
+        if(isReadOnly()) {
+            return readOnlyCache.getCachedAllMemberGroupsSet(ePerson);
+        } else {
+            return null;
+        }
     }
 
     /**

dspace-api/src/main/java/org/dspace/core/ContextReadOnlyCache.java

@@ -0,0 +1,93 @@
+package org.dspace.core;
+
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.commons.lang3.tuple.ImmutableTriple;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.commons.lang3.tuple.Triple;
+import org.dspace.content.DSpaceObject;
+import org.dspace.eperson.EPerson;
+import org.dspace.eperson.Group;
+import org.springframework.util.CollectionUtils;
+
+import java.util.HashMap;
+import java.util.Set;
+
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+public class ContextReadOnlyCache {
+
+    /**
+     * Authorized actions cache that is used when the context is in READ_ONLY mode.
+     * The key of the cache is: DSpace Object ID, action ID, Eperson ID.
+     */
+    private final HashMap<Triple<String, Integer, String>, Boolean> authorizedActionsCache = new HashMap<>();
+
+    /**
+     * Group membership cache that is used when the context is in READ_ONLY mode.
+     * The key of the cache is: Group Name, Eperson ID.
+     */
+    private final HashMap<Pair<String, String>, Boolean> groupMembershipCache = new HashMap<>();
+
+    /**
+     * Cache for all the groups the current ePerson is a member of when the context is in READ_ONLY mode.
+     */
+    private final HashMap<String, Set<Group>> allMemberGroupsCache = new HashMap<>();
+
+    public Boolean getCachedAuthorizationResult(DSpaceObject dspaceObject, int action, EPerson eperson) {
+        return authorizedActionsCache.get(buildAuthorizedActionKey(dspaceObject, action, eperson));
+    }
+
+    public void cacheAuthorizedAction(DSpaceObject dspaceObject, int action, EPerson eperson, Boolean result) {
+        authorizedActionsCache.put(buildAuthorizedActionKey(dspaceObject, action, eperson), result);
+    }
+
+    public Boolean getCachedGroupMembership(Group group, EPerson eperson) {
+        String allMembersGroupKey = buildAllMembersGroupKey(eperson);
+
+        if (CollectionUtils.isEmpty(allMemberGroupsCache.get(allMembersGroupKey))) {
+            return groupMembershipCache.get(buildGroupMembershipKey(group, eperson));
+
+        } else {
+            return allMemberGroupsCache.get(allMembersGroupKey).contains(group);
+        }
+
+    }
+
+    public void cacheGroupMembership(Group group, EPerson eperson, Boolean isMember) {
+        if (CollectionUtils.isEmpty(allMemberGroupsCache.get(buildAllMembersGroupKey(eperson)))) {
+            groupMembershipCache.put(buildGroupMembershipKey(group, eperson), isMember);
+        }
+    }
+
+    public void cacheAllMemberGroupsSet(EPerson ePerson, Set<Group> groups) {
+        allMemberGroupsCache.put(buildAllMembersGroupKey(ePerson),
+                groups);
+
+        //clear the individual groupMembershipCache as we have all memberships now.
+        groupMembershipCache.clear();
+    }
+
+    public Set<Group> getCachedAllMemberGroupsSet(EPerson ePerson) {
+        return allMemberGroupsCache.get(buildAllMembersGroupKey(ePerson));
+    }
+
+    private String buildAllMembersGroupKey(EPerson ePerson) {
+        return ePerson == null ? "" : ePerson.getID().toString();
+    }
+
+    private ImmutableTriple<String, Integer, String> buildAuthorizedActionKey(DSpaceObject dspaceObject, int action, EPerson eperson) {
+        return new ImmutableTriple<>(dspaceObject == null ? "" : dspaceObject.getID().toString(),
+                Integer.valueOf(action),
+                eperson == null ? "" : eperson.getID().toString());
+    }
+
+    private Pair<String, String> buildGroupMembershipKey(Group group, EPerson eperson) {
+        return new ImmutablePair<>(group == null ? "" : group.getName(),
+                eperson == null ? "" : eperson.getID().toString());
+    }
+}

dspace-api/src/main/java/org/dspace/discovery/SolrServiceResourceRestrictionPlugin.java

@@ -29,10 +29,8 @@ import org.dspace.eperson.service.GroupService;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import java.sql.SQLException;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Iterator;
+
-import java.util.List;
-import java.util.UUID;
 import org.dspace.services.factory.DSpaceServicesFactory;
 
 /**
@@ -98,7 +96,7 @@ public class SolrServiceResourceRestrictionPlugin implements SolrServiceIndexPlu
                 }
 
                 //Retrieve all the groups the current user is a member of !
-                List<Group> groups = groupService.allMemberGroups(context, currentUser);
+                Set<Group> groups = groupService.allMemberGroupsSet(context, currentUser);
                 for (Group group : groups) {
                     resourceQuery.append(" OR g").append(group.getID());
                 }

dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java

@@ -158,39 +158,55 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
 
     @Override
     public boolean isMember(Context context, Group group) throws SQLException {
-        return isMember(context, group.getName());
+        EPerson currentUser = context.getCurrentUser();
-    }
 
-    @Override
+        if(group == null) {
-    public boolean isMember(final Context context, final String groupName) throws SQLException {
+            return false;
-        // special, everyone is member of group 0 (anonymous)
+
-        if (StringUtils.equals(groupName, Group.ANONYMOUS))
+            // special, everyone is member of group 0 (anonymous)
-        {
+        } else if (StringUtils.equals(group.getName(), Group.ANONYMOUS)) {
             return true;
-        } else if(context.getCurrentUser() != null) {
-            EPerson currentUser = context.getCurrentUser();
 
-            //First check the special groups
+        } else if(currentUser != null) {
-            List<Group> specialGroups = context.getSpecialGroups();
+
-            if(CollectionUtils.isNotEmpty(specialGroups)) {
+            Boolean cachedGroupMembership = context.getCachedGroupMembership(group, currentUser);
-                for (Group specialGroup : specialGroups)
+            if(cachedGroupMembership != null) {
-                {
+                return cachedGroupMembership.booleanValue();
-                    //Check if the current special group is the one we are looking for OR retrieve all groups & make a check here.
+
-                    if(StringUtils.equals(specialGroup.getName(), groupName) || allMemberGroups(context, currentUser).contains(findByName(context, groupName)))
+            } else if(CollectionUtils.isNotEmpty(context.getSpecialGroups())) {
-                    {
+                Set<Group> allMemberGroups = allMemberGroupsSet(context, currentUser);
-                        return true;
+                boolean result = allMemberGroups.contains(group);
-                    }
+
-                }
+                context.cacheGroupMembership(group, currentUser, result);
+                return result;
+            } else {
+                //lookup eperson in normal groups and subgroups
+                boolean result = epersonInGroup(context, group.getName(), currentUser);
+                context.cacheGroupMembership(group, currentUser, result);
+                return result;
             }
-            //lookup eperson in normal groups and subgroups
-            return epersonInGroup(context, groupName, currentUser);
         } else {
             return false;
         }
     }
 
+    @Override
+    public boolean isMember(final Context context, final String groupName) throws SQLException {
+        return isMember(context, findByName(context, groupName));
+    }
+
     @Override
     public List<Group> allMemberGroups(Context context, EPerson ePerson) throws SQLException {
+        return new ArrayList<>(allMemberGroupsSet(context, ePerson));
+    }
+
+    @Override
+    public Set<Group> allMemberGroupsSet(Context context, EPerson ePerson) throws SQLException {
+        Set<Group> cachedGroupMembership = context.getCachedAllMemberGroupsSet(ePerson);
+        if(cachedGroupMembership != null) {
+            return cachedGroupMembership;
+        }
+
         Set<Group> groups = new HashSet<>();
 
         if (ePerson != null)
@@ -216,7 +232,6 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
         // all the users are members of the anonymous group
         groups.add(findByName(context, Group.ANONYMOUS));
 
-
         List<Group2GroupCache> groupCache = group2GroupCacheDAO.findByChildren(context, groups);
         // now we have all owning groups, also grab all parents of owning groups
         // yes, I know this could have been done as one big query and a union,
@@ -225,7 +240,8 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
             groups.add(group2GroupCache.getParent());
         }
 
-        return new ArrayList<>(groups);
+        context.cacheAllMemberGroupsSet(ePerson, groups);
+        return groups;
     }
 
     @Override

dspace-api/src/main/java/org/dspace/eperson/service/GroupService.java

@@ -9,6 +9,7 @@ package org.dspace.eperson.service;
 
 import java.sql.SQLException;
 import java.util.List;
+import java.util.Set;
 
 import org.dspace.authorize.AuthorizeException;
 import org.dspace.content.MetadataField;
@@ -154,6 +155,8 @@ public interface GroupService extends DSpaceObjectService<Group>, DSpaceObjectLe
      */
     public List<Group> allMemberGroups(Context context, EPerson ePerson) throws SQLException;
 
+    Set<Group> allMemberGroupsSet(Context context, EPerson ePerson) throws SQLException;
+
     /**
      * Get all of the epeople who are a member of the
      * specified group, or a member of a sub-group of the

dspace-api/src/main/java/org/dspace/xmlworkflow/storedcomponents/PoolTaskServiceImpl.java

@@ -20,10 +20,7 @@ import org.springframework.beans.factory.annotation.Autowired;
 
 import java.io.IOException;
 import java.sql.SQLException;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
 
 /**
  * Service implementation for the PoolTask object.
@@ -92,7 +89,7 @@ public class PoolTaskServiceImpl implements PoolTaskService {
             else{
                 //If the user does not have a claimedtask yet, see whether one of the groups of the user has pooltasks
                 //for this workflow item
-                List<Group> groups = groupService.allMemberGroups(context, ePerson);
+                Set<Group> groups = groupService.allMemberGroupsSet(context, ePerson);
                 for (Group group : groups) {
                     poolTask = poolTaskDAO.findByWorkflowItemAndGroup(context, group, workflowItem);
                     if(poolTask != null)

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/administrative/ItemExport.java

@@ -246,7 +246,7 @@ public class ItemExport extends AbstractDSpaceTransformer implements
 
 					validity.add(context, eperson);
 
-					java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+					java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 					for (Group group : groups) {
 						validity.add(context, group);
 					}

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/administrative/Navigation.java

@@ -165,7 +165,7 @@ public class Navigation extends AbstractDSpaceTransformer implements CacheablePr
 		            
 		            validity.add(context, eperson);
 		            
-		            java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+		            java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 		            for (Group group : groups)
 		            {
 		            	validity.add(context, group);

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/administrative/eperson/EditEPersonForm.java

@@ -381,7 +381,7 @@ public class EditEPersonForm extends AbstractDSpaceTransformer
 	        List member = edit.addList("eperson-member-of");
 	        member.setHead(T_member_head);
 
-	        java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+	        java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 	        for (Group group : groups)
 	        {
 	        	String url = contextPath + "/admin/groups?administrative-continue="+knot.getId()+"&submit_edit_group&groupID="+group.getID();
@@ -433,7 +433,7 @@ public class EditEPersonForm extends AbstractDSpaceTransformer
 		// Otherwise check what group this eperson is a member through
 		java.util.List<Group> targets = group.getMemberGroups();
 		
-		java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+		java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 		for (Group member : groups)
 		{
 			for (Group target : targets)

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/discovery/SidebarFacetsTransformer.java

@@ -115,6 +115,9 @@ public class SidebarFacetsTransformer extends AbstractDSpaceTransformer implemen
         if (this.validity == null) {
 
             try {
+                Context.Mode originalMode = context.getCurrentMode();
+                context.setMode(Context.Mode.READ_ONLY);
+
                 DSpaceObject dso = HandleUtil.obtainHandle(objectModel);
                 DSpaceValidity val = new DSpaceValidity();
 
@@ -143,6 +146,7 @@ public class SidebarFacetsTransformer extends AbstractDSpaceTransformer implemen
                 }
 
                 this.validity = val.complete();
+                context.setMode(originalMode);
             }
             catch (Exception e) {
                 log.error(e.getMessage(),e);
@@ -173,6 +177,9 @@ public class SidebarFacetsTransformer extends AbstractDSpaceTransformer implemen
     @Override
     public void addOptions(Options options) throws SAXException, WingException, SQLException, IOException, AuthorizeException {
 
+        Context.Mode originalMode = context.getCurrentMode();
+        context.setMode(Context.Mode.READ_ONLY);
+
         Request request = ObjectModelHelper.getRequest(objectModel);
 
         try {
@@ -263,6 +270,8 @@ public class SidebarFacetsTransformer extends AbstractDSpaceTransformer implemen
                 }
             }
         }
+
+        context.setMode(originalMode);
     }
 
     /**

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/discovery/SimpleSearch.java

@@ -25,6 +25,7 @@ import org.dspace.app.xmlui.wing.element.Item;
 import org.dspace.app.xmlui.wing.element.List;
 import org.dspace.authorize.AuthorizeException;
 import org.dspace.content.*;
+import org.dspace.core.Context;
 import org.dspace.services.factory.DSpaceServicesFactory;
 import org.dspace.discovery.*;
 import org.dspace.discovery.configuration.DiscoveryConfiguration;
@@ -112,6 +113,9 @@ public class SimpleSearch extends AbstractSearch implements CacheableProcessingC
     public void addBody(Body body) throws SAXException, WingException,
             SQLException, IOException, AuthorizeException {
 
+        Context.Mode originalMode = context.getCurrentMode();
+        context.setMode(Context.Mode.READ_ONLY);
+
         Request request = ObjectModelHelper.getRequest(objectModel);
         String queryString = getQuery();
 
@@ -231,6 +235,7 @@ public class SimpleSearch extends AbstractSearch implements CacheableProcessingC
             throw new UIException(e.getMessage(), e);
         }
 
+        context.setMode(originalMode);
     }
 
     protected void addFilterRow(java.util.List<DiscoverySearchFilter> filterFields, int index, Row row, String selectedFilterType, String relationalOperator, String value) throws WingException {

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/eperson/EditProfile.java

@@ -452,7 +452,7 @@ public class EditProfile extends AbstractDSpaceTransformer
        if (!registering)
        {
                 // Add a list of groups that this user is apart of.
-                        java.util.List<Group> memberships = groupService.allMemberGroups(context, context.getCurrentUser());
+                        java.util.Set<Group> memberships = groupService.allMemberGroupsSet(context, context.getCurrentUser());
                 
                 
                         // Not a member of any groups then don't do anything.

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/eperson/Navigation.java

@@ -145,7 +145,7 @@ public class Navigation extends AbstractDSpaceTransformer implements CacheablePr
 		            
 		            validity.add(context, eperson);
 		            
-		            java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+		            java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 		            for (Group group : groups)
 		            {
 		            	validity.add(context, group);

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/submission/CollectionViewer.java

@@ -11,6 +11,7 @@ import java.io.IOException;
 import java.io.Serializable;
 import java.sql.SQLException;
 import java.util.List;
+import java.util.Set;
 
 import org.apache.cocoon.caching.CacheableProcessingComponent;
 import org.apache.cocoon.util.HashUtil;
@@ -118,7 +119,7 @@ public class CollectionViewer extends AbstractDSpaceTransformer implements Cache
 	            validity.add(context, eperson);
 	            
 	            // Include any groups they are a member of
-	            List<Group> groups = groupService.allMemberGroups(context, eperson);
+	            Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 	            for (Group group : groups)
 	            {
 	            	validity.add(context, group);

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/versioning/Navigation.java

@@ -117,7 +117,7 @@ public class Navigation extends AbstractDSpaceTransformer implements CacheablePr
 
 		            validity.add(context, eperson);
 
-		            java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+		            java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 		            for (Group group : groups)
 		            {
 		            	validity.add(context, group);

dspace-xmlui/src/main/java/org/dspace/app/xmlui/aspect/xmlworkflow/Navigation.java

@@ -98,7 +98,7 @@ public class Navigation extends AbstractDSpaceTransformer implements CacheablePr
 
 		            validity.add(context, eperson);
 
-		            java.util.List<Group> groups = groupService.allMemberGroups(context, eperson);
+		            java.util.Set<Group> groups = groupService.allMemberGroupsSet(context, eperson);
 		            for (Group group : groups)
 		            {
 		            	validity.add(context, group);