Add getReadableIds and use in filterReadAccess #277

src/main/config/run.properties.example

@@ -45,6 +45,10 @@ log.list = SESSION WRITE READ INFO
 # Lucene
 lucene.url = https://localhost:8181
 lucene.populateBlockSize = 10000
+# Recommend setting lucene.searchBlockSize equal to maxIdsInQuery, so that all Lucene results can be authorised at once
+# If lucene.searchBlockSize > maxIdsInQuery, then multiple auth checks may be needed for a single search to Lucene
+# The optimal value depends on how likely a user's auth request fails: larger values are more efficient when rejection is more likely
+lucene.searchBlockSize = 1000
 lucene.directory = ${HOME}/data/icat/lucene
 lucene.backlogHandlerIntervalSeconds = 60
 lucene.enqueuedRequestIntervalSeconds = 5

src/main/java/org/icatproject/core/entity/EntityBaseBean.java

@@ -31,14 +31,15 @@
 import org.icatproject.core.manager.EntityInfoHandler;
 import org.icatproject.core.manager.EntityInfoHandler.Relationship;
 import org.icatproject.core.manager.GateKeeper;
+import org.icatproject.core.manager.HasEntityId;
 import org.icatproject.core.manager.LuceneManager;
 import org.icatproject.core.parser.IncludeClause.Step;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 @SuppressWarnings("serial")
 @MappedSuperclass
-public abstract class EntityBaseBean implements Serializable {
+public abstract class EntityBaseBean implements HasEntityId, Serializable {
 
 	private static final EntityInfoHandler eiHandler = EntityInfoHandler.getInstance();
 

src/main/java/org/icatproject/core/manager/EntityBeanManager.java

@@ -148,6 +148,7 @@ public enum PersistMode {
 	private boolean luceneActive;
 
 	private int maxEntities;
+	private int luceneSearchBlockSize;
 
 	private long exportCacheSize;
 	private Set<String> rootUserNames;
@@ -781,27 +782,58 @@ private void exportTable(String beanName, Set<Long> ids, OutputStream output,
 		}
 	}
 
-	private void filterReadAccess(List<ScoredEntityBaseBean> results, List<ScoredEntityBaseBean> allResults,
+	/**
+	 * Performs authorisation for READ access on the newResults. Instead of
+	 * returning the entries which can be READ, they are added to the end of
+	 * acceptedResults, ensuring it doesn't exceed maxCount or maxEntities.
+	 * 
+	 * @param acceptedResults List containing already authorised entities. Entries
+	 *                        in newResults that pass authorisation will be added to
+	 *                        acceptedResults.
+	 * @param newResults      List containing new results to check READ access to.
+	 *                        Entries in newResults that pass authorisation will be
+	 *                        added to acceptedResults.
+	 * @param maxCount        The maximum size of acceptedResults. Once reached, no
+	 *                        more entries from newResults will be added.
+	 * @param userId          The user attempting to read the newResults.
+	 * @param manager         The EntityManager to use.
+	 * @param klass           The Class of the EntityBaseBean that is being
+	 *                        filtered.
+	 * @throws IcatException If more entities than the configuration option
+	 *                       maxEntities would be added to acceptedResults, then an
+	 *                       IcatException is thrown instead.
+	 */
+	private void filterReadAccess(List<ScoredEntityBaseBean> acceptedResults, List<ScoredEntityBaseBean> newResults,
 			int maxCount, String userId, EntityManager manager, Class<? extends EntityBaseBean> klass)
 			throws IcatException {
 
-		logger.debug("Got " + allResults.size() + " results from Lucene");
-		for (ScoredEntityBaseBean sr : allResults) {
-			long entityId = sr.getEntityBaseBeanId();
-			EntityBaseBean beanManaged = manager.find(klass, entityId);
-			if (beanManaged != null) {
-				try {
-					gateKeeper.performAuthorisation(userId, beanManaged, AccessType.READ, manager);
-					results.add(new ScoredEntityBaseBean(entityId, sr.getScore()));
-					if (results.size() > maxEntities) {
+		logger.debug("Got " + newResults.size() + " results from Lucene");
+		Set<Long> allowedIds = gateKeeper.getReadableIds(userId, newResults, klass.getSimpleName(), manager);
+		if (allowedIds == null) {
+			// A null result means there are no restrictions on the readable ids, so add as
+			// many newResults as we need to reach maxCount
+			int needed = maxCount - acceptedResults.size();
+			if (newResults.size() > needed) {
+				acceptedResults.addAll(newResults.subList(0, needed));
+			} else {
+				acceptedResults.addAll(newResults);
+			}
+			if (acceptedResults.size() > maxEntities) {
+				throw new IcatException(IcatExceptionType.VALIDATION,
+						"attempt to return more than " + maxEntities + " entities");
+			}
+		} else {
+			// Otherwise, add results in order until we reach maxCount
+			for (ScoredEntityBaseBean newResult : newResults) {
+				if (allowedIds.contains(newResult.getId())) {
+					acceptedResults.add(newResult);
+					if (acceptedResults.size() > maxEntities) {
 						throw new IcatException(IcatExceptionType.VALIDATION,
 								"attempt to return more than " + maxEntities + " entities");
 					}
-					if (results.size() == maxCount) {
+					if (acceptedResults.size() == maxCount) {
 						break;
 					}
-				} catch (IcatException e) {
-					// Nothing to do
 				}
 			}
 		}
@@ -1154,6 +1186,7 @@ void init() {
 		notificationRequests = propertyHandler.getNotificationRequests();
 		luceneActive = lucene.isActive();
 		maxEntities = propertyHandler.getMaxEntities();
+		luceneSearchBlockSize = propertyHandler.getLuceneSearchBlockSize();
 		exportCacheSize = propertyHandler.getImportCacheSize();
 		rootUserNames = propertyHandler.getRootUserNames();
 		key = propertyHandler.getKey();
@@ -1398,11 +1431,7 @@ public List<ScoredEntityBaseBean> luceneDatafiles(String userName, String user,
 			LuceneSearchResult last = null;
 			Long uid = null;
 			List<ScoredEntityBaseBean> allResults = Collections.emptyList();
-			/*
-			 * As results may be rejected and maxCount may be 1 ensure that we
-			 * don't make a huge number of calls to Lucene
-			 */
-			int blockSize = Math.max(1000, maxCount);
+			int blockSize = luceneSearchBlockSize;
 
 			do {
 				if (last == null) {
@@ -1423,7 +1452,7 @@ public List<ScoredEntityBaseBean> luceneDatafiles(String userName, String user,
 			try (JsonGenerator gen = Json.createGenerator(baos).writeStartObject()) {
 				gen.write("userName", userName);
 				if (results.size() > 0) {
-					gen.write("entityId", results.get(0).getEntityBaseBeanId());
+					gen.write("entityId", results.get(0).getId());
 				}
 				gen.writeEnd();
 			}
@@ -1441,11 +1470,7 @@ public List<ScoredEntityBaseBean> luceneDatasets(String userName, String user, S
 			LuceneSearchResult last = null;
 			Long uid = null;
 			List<ScoredEntityBaseBean> allResults = Collections.emptyList();
-			/*
-			 * As results may be rejected and maxCount may be 1 ensure that we
-			 * don't make a huge number of calls to Lucene
-			 */
-			int blockSize = Math.max(1000, maxCount);
+			int blockSize = luceneSearchBlockSize;
 
 			do {
 				if (last == null) {
@@ -1465,7 +1490,7 @@ public List<ScoredEntityBaseBean> luceneDatasets(String userName, String user, S
 			try (JsonGenerator gen = Json.createGenerator(baos).writeStartObject()) {
 				gen.write("userName", userName);
 				if (results.size() > 0) {
-					gen.write("entityId", results.get(0).getEntityBaseBeanId());
+					gen.write("entityId", results.get(0).getId());
 				}
 				gen.writeEnd();
 			}
@@ -1492,11 +1517,7 @@ public List<ScoredEntityBaseBean> luceneInvestigations(String userName, String u
 			LuceneSearchResult last = null;
 			Long uid = null;
 			List<ScoredEntityBaseBean> allResults = Collections.emptyList();
-			/*
-			 * As results may be rejected and maxCount may be 1 ensure that we
-			 * don't make a huge number of calls to Lucene
-			 */
-			int blockSize = Math.max(1000, maxCount);
+			int blockSize = luceneSearchBlockSize;
 
 			do {
 				if (last == null) {
@@ -1516,7 +1537,7 @@ public List<ScoredEntityBaseBean> luceneInvestigations(String userName, String u
 			try (JsonGenerator gen = Json.createGenerator(baos).writeStartObject()) {
 				gen.write("userName", userName);
 				if (results.size() > 0) {
-					gen.write("entityId", results.get(0).getEntityBaseBeanId());
+					gen.write("entityId", results.get(0).getId());
 				}
 				gen.writeEnd();
 			}

src/main/java/org/icatproject/core/manager/GateKeeper.java

@@ -164,36 +164,117 @@ public Set<String> getPublicTables() {
 		return publicTables;
 	}
 
-	public List<EntityBaseBean> getReadable(String userId, List<EntityBaseBean> beans, EntityManager manager) {
-
-		if (beans.size() == 0) {
-			return beans;
-		}
-
-		EntityBaseBean object = beans.get(0);
-
-		Class<? extends EntityBaseBean> objectClass = object.getClass();
-		String simpleName = objectClass.getSimpleName();
+	/**
+	 * Gets READ restrictions that apply to entities of type simpleName, that are
+	 * relevant for the given userId. If userId belongs to a root user, or one of
+	 * the restrictions is itself null, then null is returned. This corresponds to a
+	 * case where the user can READ any entity of type simpleName.
+	 * 
+	 * @param userId     The user making the READ request.
+	 * @param simpleName The name of the requested entity type.
+	 * @param manager    The EntityManager to use.
+	 * @return Returns a list of restrictions that apply to the requested entity
+	 *         type. If there are no restrictions, then returns null.
+	 */
+	private List<String> getRestrictions(String userId, String simpleName, EntityManager manager) {
 		if (rootUserNames.contains(userId)) {
 			logger.info("\"Root\" user " + userId + " is allowed READ to " + simpleName);
-			return beans;
+			return null;
 		}
 
 		TypedQuery<String> query = manager.createNamedQuery(Rule.INCLUDE_QUERY, String.class)
 				.setParameter("member", userId).setParameter("bean", simpleName);
 
 		List<String> restrictions = query.getResultList();
 		logger.debug("Got " + restrictions.size() + " authz queries for READ by " + userId + " to a "
-				+ objectClass.getSimpleName());
+				+ simpleName);
 
 		for (String restriction : restrictions) {
 			logger.debug("Query: " + restriction);
 			if (restriction == null) {
 				logger.info("Null restriction => READ permitted to " + simpleName);
-				return beans;
+				return null;
 			}
 		}
 
+		return restrictions;
+	}
+
+	/**
+	 * Returns a sub list of the passed entities that the user has READ access to.
+	 * Note that this method accepts and returns instances of EntityBaseBean, unlike
+	 * getReadableIds.
+	 * 
+	 * @param userId  The user making the READ request.
+	 * @param beans   The entities the user wants to READ.
+	 * @param manager The EntityManager to use.
+	 * @return A list of entities the user has read access to
+	 */
+	public List<EntityBaseBean> getReadable(String userId, List<EntityBaseBean> beans, EntityManager manager) {
+
+		if (beans.size() == 0) {
+			return beans;
+		}
+		EntityBaseBean object = beans.get(0);
+		Class<? extends EntityBaseBean> objectClass = object.getClass();
+		String simpleName = objectClass.getSimpleName();
+
+		List<String> restrictions = getRestrictions(userId, simpleName, manager);
+		if (restrictions == null) {
+			return beans;
+		}
+
+		Set<Long> readableIds = getReadableIds(userId, beans, restrictions, manager);
+
+		List<EntityBaseBean> results = new ArrayList<>();
+		for (EntityBaseBean bean : beans) {
+			if (readableIds.contains(bean.getId())) {
+				results.add(bean);
+			}
+		}
+		return results;
+	}
+
+	/**
+	 * Returns a set of ids that indicate entities of type simpleName that the user
+	 * has READ access to. If all of the entities can be READ (restrictions are
+	 * null) then null is returned. Note that while this accepts anything that
+	 * HasEntityId, the ids are returned as a Set<Long> unlike getReadable.
+	 * 
+	 * @param userId     The user making the READ request.
+	 * @param entities   The entities to check.
+	 * @param simpleName The name of the requested entity type.
+	 * @param manager    The EntityManager to use.
+	 * @return Set of the ids that the user has read access to. If there are no
+	 *         restrictions, then returns null.
+	 */
+	public Set<Long> getReadableIds(String userId, List<? extends HasEntityId> entities, String simpleName,
+			EntityManager manager) {
+
+		if (entities.size() == 0) {
+			return null;
+		}
+
+		List<String> restrictions = getRestrictions(userId, simpleName, manager);
+		if (restrictions == null) {
+			return null;
+		}
+
+		return getReadableIds(userId, entities, restrictions, manager);
+	}
+
+	/**
+	 * Returns a set of ids that indicate entities that the user has READ access to.
+	 * 
+	 * @param userId       The user making the READ request.
+	 * @param entities     The entities to check.
+	 * @param restrictions The restrictions applying to the entities.
+	 * @param manager      The EntityManager to use.
+	 * @return Set of the ids that the user has read access to.
+	 */
+	private Set<Long> getReadableIds(String userId, List<? extends HasEntityId> entities, List<String> restrictions,
+			EntityManager manager) {
+
 		/*
 		 * IDs are processed in batches to avoid Oracle error: ORA-01795:
 		 * maximum number of expressions in a list is 1000
@@ -203,13 +284,13 @@ public List<EntityBaseBean> getReadable(String userId, List<EntityBaseBean> bean
 		StringBuilder sb = null;
 
 		int i = 0;
-		for (EntityBaseBean bean : beans) {
+		for (HasEntityId entity : entities) {
 			if (i == 0) {
 				sb = new StringBuilder();
-				sb.append(bean.getId());
+				sb.append(entity.getId());
 				i = 1;
 			} else {
-				sb.append("," + bean.getId());
+				sb.append("," + entity.getId());
 				i++;
 			}
 			if (i == maxIdsInQuery) {
@@ -222,27 +303,21 @@ public List<EntityBaseBean> getReadable(String userId, List<EntityBaseBean> bean
 			idLists.add(sb.toString());
 		}
 
-		logger.debug("Check readability of " + beans.size() + " beans has been divided into " + idLists.size()
+		logger.debug("Check readability of " + entities.size() + " beans has been divided into " + idLists.size()
 				+ " queries.");
 
-		Set<Long> ids = new HashSet<>();
+		Set<Long> readableIds = new HashSet<>();
 		for (String idList : idLists) {
 			for (String qString : restrictions) {
 				TypedQuery<Long> q = manager.createQuery(qString.replace(":pkids", idList), Long.class);
 				if (qString.contains(":user")) {
 					q.setParameter("user", userId);
 				}
-				ids.addAll(q.getResultList());
+				readableIds.addAll(q.getResultList());
 			}
 		}
 
-		List<EntityBaseBean> results = new ArrayList<>();
-		for (EntityBaseBean bean : beans) {
-			if (ids.contains(bean.getId())) {
-				results.add(bean);
-			}
-		}
-		return results;
+		return readableIds;
 	}
 
 	public Set<String> getRootUserNames() {

src/main/java/org/icatproject/core/manager/HasEntityId.java

@@ -0,0 +1,8 @@
+package org.icatproject.core.manager;
+
+/**
+ * Interface for objects representing entities that hold the entity id.
+ */
+public interface HasEntityId {
+	public Long getId();
+}

src/main/java/org/icatproject/core/manager/PropertyHandler.java

@@ -302,6 +302,7 @@ public int getLifetimeMinutes() {
 	private String digestKey;
 	private URL luceneUrl;
 	private int lucenePopulateBlockSize;
+	private int luceneSearchBlockSize;
 	private Path luceneDirectory;
 	private long luceneBacklogHandlerIntervalMillis;
 	private Map<String, String> cluster = new HashMap<>();
@@ -463,6 +464,9 @@ private void init() {
 				lucenePopulateBlockSize = props.getPositiveInt("lucene.populateBlockSize");
 				formattedProps.add("lucene.populateBlockSize" + " " + lucenePopulateBlockSize);
 
+				luceneSearchBlockSize = props.getPositiveInt("lucene.searchBlockSize");
+				formattedProps.add("lucene.searchBlockSize" + " " + luceneSearchBlockSize);
+
 				luceneDirectory = props.getPath("lucene.directory");
 				if (!luceneDirectory.toFile().isDirectory()) {
 					String msg = luceneDirectory + " is not a directory";
@@ -612,6 +616,10 @@ public int getLucenePopulateBlockSize() {
 		return lucenePopulateBlockSize;
 	}
 
+	public int getLuceneSearchBlockSize() {
+		return luceneSearchBlockSize;
+	}
+
 	public long getLuceneBacklogHandlerIntervalMillis() {
 		return luceneBacklogHandlerIntervalMillis;
 	}