From 3770bbf642594fa7eecae7572f1013bc4c55eade Mon Sep 17 00:00:00 2001 From: Patrick Austin Date: Fri, 11 Feb 2022 23:09:06 +0000 Subject: [PATCH 1/6] Add getReadableIds and use in filterReadAccess #277 --- .../core/manager/EntityBeanManager.java | 50 +++++------- .../icatproject/core/manager/GateKeeper.java | 78 +++++++++++++++++++ 2 files changed, 98 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java index 4654d6811..b46a2407c 100644 --- a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java +++ b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java @@ -148,6 +148,7 @@ public enum PersistMode { private boolean luceneActive; private int maxEntities; + private int maxIdsInQuery; private long exportCacheSize; private Set rootUserNames; @@ -786,23 +787,23 @@ private void filterReadAccess(List results, List allIds = new ArrayList<>(); + allResults.forEach(r -> allIds.add(r.getEntityBaseBeanId())); + List allowedIds = gateKeeper.getReadableIds(userId, allIds, klass, manager); 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) { - throw new IcatException(IcatExceptionType.VALIDATION, - "attempt to return more than " + maxEntities + " entities"); - } - if (results.size() == maxCount) { - break; - } - } catch (IcatException e) { - // Nothing to do + try { + if (allowedIds.contains(sr.getEntityBaseBeanId())) { + results.add(sr); } + if (results.size() > maxEntities) { + throw new IcatException(IcatExceptionType.VALIDATION, + "attempt to return more than " + maxEntities + " entities"); + } + if (results.size() == maxCount) { + break; + } + } catch (IcatException e) { + // Nothing to do } } } @@ -1154,6 +1155,7 @@ void init() { notificationRequests = propertyHandler.getNotificationRequests(); luceneActive = lucene.isActive(); maxEntities = propertyHandler.getMaxEntities(); + maxIdsInQuery = propertyHandler.getMaxIdsInQuery(); exportCacheSize = propertyHandler.getImportCacheSize(); rootUserNames = propertyHandler.getRootUserNames(); key = propertyHandler.getKey(); @@ -1398,11 +1400,7 @@ public List luceneDatafiles(String userName, String user, LuceneSearchResult last = null; Long uid = null; List 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 = maxIdsInQuery; do { if (last == null) { @@ -1441,11 +1439,7 @@ public List luceneDatasets(String userName, String user, S LuceneSearchResult last = null; Long uid = null; List 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 = maxIdsInQuery; do { if (last == null) { @@ -1492,11 +1486,7 @@ public List luceneInvestigations(String userName, String u LuceneSearchResult last = null; Long uid = null; List 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 = maxIdsInQuery; do { if (last == null) { diff --git a/src/main/java/org/icatproject/core/manager/GateKeeper.java b/src/main/java/org/icatproject/core/manager/GateKeeper.java index d01470f85..f4cfe2eb8 100644 --- a/src/main/java/org/icatproject/core/manager/GateKeeper.java +++ b/src/main/java/org/icatproject/core/manager/GateKeeper.java @@ -245,6 +245,84 @@ public List getReadable(String userId, List bean return results; } + public List getReadableIds(String userId, List ids, + Class objectClass, EntityManager manager) { + if (ids.size() == 0) { + return ids; + } + + String simpleName = objectClass.getSimpleName(); + if (rootUserNames.contains(userId)) { + logger.info("\"Root\" user " + userId + " is allowed READ to " + simpleName); + return ids; + } + + TypedQuery query = manager.createNamedQuery(Rule.INCLUDE_QUERY, String.class) + .setParameter("member", userId).setParameter("bean", simpleName); + + List restrictions = query.getResultList(); + logger.debug("Got " + restrictions.size() + " authz queries for READ by " + userId + " to a " + + objectClass.getSimpleName()); + + for (String restriction : restrictions) { + logger.debug("Query: " + restriction); + if (restriction == null) { + logger.info("Null restriction => READ permitted to " + simpleName); + return ids; + } + } + + /* + * IDs are processed in batches to avoid Oracle error: ORA-01795: + * maximum number of expressions in a list is 1000 + */ + + List idLists = new ArrayList<>(); + StringBuilder sb = null; + + int i = 0; + for (Long id : ids) { + if (i == 0) { + sb = new StringBuilder(); + sb.append(id); + i = 1; + } else { + sb.append("," + id); + i++; + } + if (i == maxIdsInQuery) { + i = 0; + idLists.add(sb.toString()); + sb = null; + } + } + if (sb != null) { + idLists.add(sb.toString()); + } + + logger.debug("Check readability of " + ids.size() + " beans has been divided into " + idLists.size() + + " queries."); + + Set readableIds = new HashSet<>(); + for (String idList : idLists) { + for (String qString : restrictions) { + TypedQuery q = manager.createQuery(qString.replace(":pkids", idList), Long.class); + if (qString.contains(":user")) { + q.setParameter("user", userId); + } + readableIds.addAll(q.getResultList()); + } + } + + List results = new ArrayList<>(); + for (Long id : ids) { + if (readableIds.contains(id)) { + results.add(id); + } + } + return results; + } + public Set getRootUserNames() { return rootUserNames; } From 7bf55e99992935239d617d66b55864f0041c3b2f Mon Sep 17 00:00:00 2001 From: Patrick Austin Date: Wed, 9 Mar 2022 23:43:20 +0000 Subject: [PATCH 2/6] Add lucene.searchBlockSize and refactor getReadable #277 --- src/main/config/run.properties.example | 4 + .../core/entity/EntityBaseBean.java | 3 +- .../core/manager/EntityBeanManager.java | 62 ++++---- .../icatproject/core/manager/GateKeeper.java | 147 ++++++++---------- .../icatproject/core/manager/HasEntityId.java | 8 + .../core/manager/PropertyHandler.java | 10 +- .../core/manager/ScoredEntityBaseBean.java | 12 +- .../org/icatproject/exposed/ICATRest.java | 2 +- src/main/resources/run.properties | 4 + .../icatproject/core/manager/TestLucene.java | 2 +- src/test/scripts/prepare_test.py | 1 + 11 files changed, 137 insertions(+), 118 deletions(-) create mode 100644 src/main/java/org/icatproject/core/manager/HasEntityId.java diff --git a/src/main/config/run.properties.example b/src/main/config/run.properties.example index 06e732716..d7f125921 100644 --- a/src/main/config/run.properties.example +++ b/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 diff --git a/src/main/java/org/icatproject/core/entity/EntityBaseBean.java b/src/main/java/org/icatproject/core/entity/EntityBaseBean.java index afcc54e0a..0b94d87dc 100644 --- a/src/main/java/org/icatproject/core/entity/EntityBaseBean.java +++ b/src/main/java/org/icatproject/core/entity/EntityBaseBean.java @@ -31,6 +31,7 @@ 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; @@ -38,7 +39,7 @@ @SuppressWarnings("serial") @MappedSuperclass -public abstract class EntityBaseBean implements Serializable { +public abstract class EntityBaseBean implements HasEntityId, Serializable { private static final EntityInfoHandler eiHandler = EntityInfoHandler.getInstance(); diff --git a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java index b46a2407c..44cbc8316 100644 --- a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java +++ b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java @@ -148,7 +148,7 @@ public enum PersistMode { private boolean luceneActive; private int maxEntities; - private int maxIdsInQuery; + private int luceneSearchBlockSize; private long exportCacheSize; private Set rootUserNames; @@ -782,28 +782,38 @@ private void exportTable(String beanName, Set ids, OutputStream output, } } - private void filterReadAccess(List results, List allResults, + private void filterReadAccess(List acceptedResults, List newResults, int maxCount, String userId, EntityManager manager, Class klass) throws IcatException { - logger.debug("Got " + allResults.size() + " results from Lucene"); - List allIds = new ArrayList<>(); - allResults.forEach(r -> allIds.add(r.getEntityBaseBeanId())); - List allowedIds = gateKeeper.getReadableIds(userId, allIds, klass, manager); - for (ScoredEntityBaseBean sr : allResults) { - try { - if (allowedIds.contains(sr.getEntityBaseBeanId())) { - results.add(sr); - } - if (results.size() > maxEntities) { - throw new IcatException(IcatExceptionType.VALIDATION, - "attempt to return more than " + maxEntities + " entities"); - } - if (results.size() == maxCount) { - break; + logger.debug("Got " + newResults.size() + " results from Lucene"); + Set 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 (acceptedResults.size() == maxCount) { + break; + } } - } catch (IcatException e) { - // Nothing to do } } } @@ -1155,7 +1165,7 @@ void init() { notificationRequests = propertyHandler.getNotificationRequests(); luceneActive = lucene.isActive(); maxEntities = propertyHandler.getMaxEntities(); - maxIdsInQuery = propertyHandler.getMaxIdsInQuery(); + luceneSearchBlockSize = propertyHandler.getLuceneSearchBlockSize(); exportCacheSize = propertyHandler.getImportCacheSize(); rootUserNames = propertyHandler.getRootUserNames(); key = propertyHandler.getKey(); @@ -1400,7 +1410,7 @@ public List luceneDatafiles(String userName, String user, LuceneSearchResult last = null; Long uid = null; List allResults = Collections.emptyList(); - int blockSize = maxIdsInQuery; + int blockSize = luceneSearchBlockSize; do { if (last == null) { @@ -1421,7 +1431,7 @@ public List 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(); } @@ -1439,7 +1449,7 @@ public List luceneDatasets(String userName, String user, S LuceneSearchResult last = null; Long uid = null; List allResults = Collections.emptyList(); - int blockSize = maxIdsInQuery; + int blockSize = luceneSearchBlockSize; do { if (last == null) { @@ -1459,7 +1469,7 @@ public List 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(); } @@ -1486,7 +1496,7 @@ public List luceneInvestigations(String userName, String u LuceneSearchResult last = null; Long uid = null; List allResults = Collections.emptyList(); - int blockSize = maxIdsInQuery; + int blockSize = luceneSearchBlockSize; do { if (last == null) { @@ -1506,7 +1516,7 @@ public List 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(); } diff --git a/src/main/java/org/icatproject/core/manager/GateKeeper.java b/src/main/java/org/icatproject/core/manager/GateKeeper.java index f4cfe2eb8..ae12fe5c1 100644 --- a/src/main/java/org/icatproject/core/manager/GateKeeper.java +++ b/src/main/java/org/icatproject/core/manager/GateKeeper.java @@ -164,19 +164,17 @@ public Set getPublicTables() { return publicTables; } - public List getReadable(String userId, List beans, EntityManager manager) { - - if (beans.size() == 0) { - return beans; - } - - EntityBaseBean object = beans.get(0); - - Class objectClass = object.getClass(); - String simpleName = objectClass.getSimpleName(); + /** + * @param userId The user making the READ request + * @param simpleName The name of the requested entity type + * @param manager + * @return Returns a list of restrictions that apply to the requested entity + * type. If there are no restrictions, then returns null. + */ + private List 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 query = manager.createNamedQuery(Rule.INCLUDE_QUERY, String.class) @@ -184,93 +182,84 @@ public List getReadable(String userId, List bean List 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; } } - /* - * IDs are processed in batches to avoid Oracle error: ORA-01795: - * maximum number of expressions in a list is 1000 - */ + return restrictions; + } - List idLists = new ArrayList<>(); - StringBuilder sb = null; + /** + * Returns a sub list of the passed entities that the user has READ access to. + * + * @param userId The user making the READ request + * @param beans The entities the user wants to READ + * @param manager + * @return A list of entities the user has read access to + */ + public List getReadable(String userId, List beans, EntityManager manager) { - int i = 0; - for (EntityBaseBean bean : beans) { - if (i == 0) { - sb = new StringBuilder(); - sb.append(bean.getId()); - i = 1; - } else { - sb.append("," + bean.getId()); - i++; - } - if (i == maxIdsInQuery) { - i = 0; - idLists.add(sb.toString()); - sb = null; - } - } - if (sb != null) { - idLists.add(sb.toString()); + if (beans.size() == 0) { + return beans; } + EntityBaseBean object = beans.get(0); + Class objectClass = object.getClass(); + String simpleName = objectClass.getSimpleName(); - logger.debug("Check readability of " + beans.size() + " beans has been divided into " + idLists.size() - + " queries."); - - Set ids = new HashSet<>(); - for (String idList : idLists) { - for (String qString : restrictions) { - TypedQuery q = manager.createQuery(qString.replace(":pkids", idList), Long.class); - if (qString.contains(":user")) { - q.setParameter("user", userId); - } - ids.addAll(q.getResultList()); - } + List restrictions = getRestrictions(userId, simpleName, manager); + if (restrictions == null) { + return beans; } + Set readableIds = getReadableIds(userId, beans, restrictions, manager); + List results = new ArrayList<>(); for (EntityBaseBean bean : beans) { - if (ids.contains(bean.getId())) { + if (readableIds.contains(bean.getId())) { results.add(bean); } } return results; } - public List getReadableIds(String userId, List ids, - Class objectClass, EntityManager manager) { - if (ids.size() == 0) { - return ids; - } + /** + * @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 + * @return Set of the ids that the user has read access to. If there are no + * restrictions, then returns null. + */ + public Set getReadableIds(String userId, List entities, String simpleName, + EntityManager manager) { - String simpleName = objectClass.getSimpleName(); - if (rootUserNames.contains(userId)) { - logger.info("\"Root\" user " + userId + " is allowed READ to " + simpleName); - return ids; + if (entities.size() == 0) { + return null; } - TypedQuery query = manager.createNamedQuery(Rule.INCLUDE_QUERY, String.class) - .setParameter("member", userId).setParameter("bean", simpleName); + List restrictions = getRestrictions(userId, simpleName, manager); + if (restrictions == null) { + return null; + } - List restrictions = query.getResultList(); - logger.debug("Got " + restrictions.size() + " authz queries for READ by " + userId + " to a " - + objectClass.getSimpleName()); + return getReadableIds(userId, entities, restrictions, manager); + } - for (String restriction : restrictions) { - logger.debug("Query: " + restriction); - if (restriction == null) { - logger.info("Null restriction => READ permitted to " + simpleName); - return ids; - } - } + /** + * @param userId The user making the READ request + * @param entities The entities to check + * @param restrictions The restrictions applying to the entities + * @param manager + * @return Set of the ids that the user has read access to + */ + private Set getReadableIds(String userId, List entities, List restrictions, + EntityManager manager) { /* * IDs are processed in batches to avoid Oracle error: ORA-01795: @@ -281,13 +270,13 @@ public List getReadableIds(String userId, List ids, StringBuilder sb = null; int i = 0; - for (Long id : ids) { + for (HasEntityId entity : entities) { if (i == 0) { sb = new StringBuilder(); - sb.append(id); + sb.append(entity.getId()); i = 1; } else { - sb.append("," + id); + sb.append("," + entity.getId()); i++; } if (i == maxIdsInQuery) { @@ -300,7 +289,7 @@ public List getReadableIds(String userId, List ids, idLists.add(sb.toString()); } - logger.debug("Check readability of " + ids.size() + " beans has been divided into " + idLists.size() + logger.debug("Check readability of " + entities.size() + " beans has been divided into " + idLists.size() + " queries."); Set readableIds = new HashSet<>(); @@ -314,13 +303,7 @@ public List getReadableIds(String userId, List ids, } } - List results = new ArrayList<>(); - for (Long id : ids) { - if (readableIds.contains(id)) { - results.add(id); - } - } - return results; + return readableIds; } public Set getRootUserNames() { diff --git a/src/main/java/org/icatproject/core/manager/HasEntityId.java b/src/main/java/org/icatproject/core/manager/HasEntityId.java new file mode 100644 index 000000000..8ad36eb85 --- /dev/null +++ b/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(); +} diff --git a/src/main/java/org/icatproject/core/manager/PropertyHandler.java b/src/main/java/org/icatproject/core/manager/PropertyHandler.java index f316bb647..7c1e45a99 100644 --- a/src/main/java/org/icatproject/core/manager/PropertyHandler.java +++ b/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 cluster = new HashMap<>(); @@ -460,9 +461,12 @@ private void init() { luceneUrl = props.getURL("lucene.url"); formattedProps.add("lucene.url" + " " + luceneUrl); - lucenePopulateBlockSize = props.getPositiveInt("lucene.populateBlockSize"); + lucenePopulateBlockSize = props.getPositiveInt("lucene.searchBlockSize"); 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; } diff --git a/src/main/java/org/icatproject/core/manager/ScoredEntityBaseBean.java b/src/main/java/org/icatproject/core/manager/ScoredEntityBaseBean.java index 34c58306d..6643e7ef4 100644 --- a/src/main/java/org/icatproject/core/manager/ScoredEntityBaseBean.java +++ b/src/main/java/org/icatproject/core/manager/ScoredEntityBaseBean.java @@ -1,17 +1,17 @@ package org.icatproject.core.manager; -public class ScoredEntityBaseBean { +public class ScoredEntityBaseBean implements HasEntityId { - private long entityBaseBeanId; + private Long id; private float score; - public ScoredEntityBaseBean(long id, float score) { - this.entityBaseBeanId = id; + public ScoredEntityBaseBean(Long id, float score) { + this.id = id; this.score = score; } - public long getEntityBaseBeanId() { - return entityBaseBeanId; + public Long getId() { + return id; } public float getScore() { diff --git a/src/main/java/org/icatproject/exposed/ICATRest.java b/src/main/java/org/icatproject/exposed/ICATRest.java index bd1a2b4aa..0871ef993 100644 --- a/src/main/java/org/icatproject/exposed/ICATRest.java +++ b/src/main/java/org/icatproject/exposed/ICATRest.java @@ -1108,7 +1108,7 @@ public String lucene(@Context HttpServletRequest request, @QueryParam("sessionId gen.writeStartArray(); for (ScoredEntityBaseBean sb : objects) { gen.writeStartObject(); - gen.write("id", sb.getEntityBaseBeanId()); + gen.write("id", sb.getId()); gen.write("score", sb.getScore()); gen.writeEnd(); } diff --git a/src/main/resources/run.properties b/src/main/resources/run.properties index 006bf2c99..2be4fe778 100644 --- a/src/main/resources/run.properties +++ b/src/main/resources/run.properties @@ -18,6 +18,10 @@ log.list = SESSION WRITE READ INFO lucene.url = https://localhost.localdomain: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/lucene lucene.backlogHandlerIntervalSeconds = 60 lucene.enqueuedRequestIntervalSeconds = 3 diff --git a/src/test/java/org/icatproject/core/manager/TestLucene.java b/src/test/java/org/icatproject/core/manager/TestLucene.java index 216aa4c23..82d90c747 100644 --- a/src/test/java/org/icatproject/core/manager/TestLucene.java +++ b/src/test/java/org/icatproject/core/manager/TestLucene.java @@ -159,7 +159,7 @@ private void checkLsr(LuceneSearchResult lsr, Long... n) { Set got = new HashSet<>(); for (ScoredEntityBaseBean q : lsr.getResults()) { - got.add(q.getEntityBaseBeanId()); + got.add(q.getId()); } Set missing = new HashSet<>(wanted); diff --git a/src/test/scripts/prepare_test.py b/src/test/scripts/prepare_test.py index dbd8cf46e..8cb6f39ad 100644 --- a/src/test/scripts/prepare_test.py +++ b/src/test/scripts/prepare_test.py @@ -41,6 +41,7 @@ "log.list = SESSION WRITE READ INFO", "lucene.url = %s" % lucene_url, "lucene.populateBlockSize = 10000", + "lucene.searchBlockSize = 1000", "lucene.directory = %s/data/lucene" % subst["HOME"], "lucene.backlogHandlerIntervalSeconds = 60", "lucene.enqueuedRequestIntervalSeconds = 3", From d9720570a7b735506c304df54f4fef4aa17f46ea Mon Sep 17 00:00:00 2001 From: patrick-austin <61705287+patrick-austin@users.noreply.github.com> Date: Fri, 11 Mar 2022 13:54:34 +0000 Subject: [PATCH 3/6] Fix change to lucenePopulateBlockSize #277 --- src/main/java/org/icatproject/core/manager/PropertyHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/icatproject/core/manager/PropertyHandler.java b/src/main/java/org/icatproject/core/manager/PropertyHandler.java index 7c1e45a99..7f8416cf7 100644 --- a/src/main/java/org/icatproject/core/manager/PropertyHandler.java +++ b/src/main/java/org/icatproject/core/manager/PropertyHandler.java @@ -461,7 +461,7 @@ private void init() { luceneUrl = props.getURL("lucene.url"); formattedProps.add("lucene.url" + " " + luceneUrl); - lucenePopulateBlockSize = props.getPositiveInt("lucene.searchBlockSize"); + lucenePopulateBlockSize = props.getPositiveInt("lucene.populateBlockSize"); formattedProps.add("lucene.populateBlockSize" + " " + lucenePopulateBlockSize); luceneSearchBlockSize = props.getPositiveInt("lucene.searchBlockSize"); From 939e42101103b3e91c9ec09e7325ce2cdfa451f3 Mon Sep 17 00:00:00 2001 From: Patrick Austin Date: Thu, 17 Mar 2022 18:10:46 +0000 Subject: [PATCH 4/6] Add documentation to altered methods #277 --- .../core/manager/EntityBeanManager.java | 21 +++++++++ .../icatproject/core/manager/GateKeeper.java | 44 ++++++++++++------- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java index 44cbc8316..c5e18d93c 100644 --- a/src/main/java/org/icatproject/core/manager/EntityBeanManager.java +++ b/src/main/java/org/icatproject/core/manager/EntityBeanManager.java @@ -782,6 +782,27 @@ private void exportTable(String beanName, Set ids, OutputStream output, } } + /** + * 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 acceptedResults, List newResults, int maxCount, String userId, EntityManager manager, Class klass) throws IcatException { diff --git a/src/main/java/org/icatproject/core/manager/GateKeeper.java b/src/main/java/org/icatproject/core/manager/GateKeeper.java index ae12fe5c1..20de1f36c 100644 --- a/src/main/java/org/icatproject/core/manager/GateKeeper.java +++ b/src/main/java/org/icatproject/core/manager/GateKeeper.java @@ -165,9 +165,14 @@ public Set getPublicTables() { } /** - * @param userId The user making the READ request - * @param simpleName The name of the requested entity type - * @param manager + * 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. */ @@ -197,10 +202,12 @@ private List getRestrictions(String userId, String simpleName, EntityMan /** * 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 + * @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 getReadable(String userId, List beans, EntityManager manager) { @@ -229,10 +236,15 @@ public List getReadable(String userId, List bean } /** - * @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 + * 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 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. */ @@ -252,11 +264,13 @@ public Set getReadableIds(String userId, List entit } /** - * @param userId The user making the READ request - * @param entities The entities to check - * @param restrictions The restrictions applying to the entities - * @param manager - * @return Set of the ids that the user has read access to + * 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 getReadableIds(String userId, List entities, List restrictions, EntityManager manager) { From 6c9477a836be357467f3857c4c89dd82a11dabc6 Mon Sep 17 00:00:00 2001 From: Patrick Austin Date: Wed, 5 Oct 2022 11:11:01 +0100 Subject: [PATCH 5/6] Expand TestRS to cover getReadableIds change #277 --- .../org/icatproject/integration/TestRS.java | 353 +++++++++++++++--- 1 file changed, 292 insertions(+), 61 deletions(-) diff --git a/src/test/java/org/icatproject/integration/TestRS.java b/src/test/java/org/icatproject/integration/TestRS.java index 2e0c80f92..0350ff438 100644 --- a/src/test/java/org/icatproject/integration/TestRS.java +++ b/src/test/java/org/icatproject/integration/TestRS.java @@ -19,7 +19,9 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.InputStream; +import java.io.StringReader; import java.net.URI; +import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -88,14 +90,28 @@ public void clearSession() throws Exception { wSession.clearAuthz(); } - @Ignore("Test fails because of bug in eclipselink") - @Test - public void testDistinctBehaviour() throws Exception { + private Session rootSession() throws URISyntaxException, IcatException { ICAT icat = new ICAT(System.getProperty("serverUrl")); Map credentials = new HashMap<>(); credentials.put("username", "root"); credentials.put("password", "password"); Session session = icat.login("db", credentials); + return session; + } + + private Session piOneSession() throws URISyntaxException, IcatException { + ICAT icat = new ICAT(System.getProperty("serverUrl")); + Map credentials = new HashMap<>(); + credentials.put("username", "piOne"); + credentials.put("password", "piOne"); + Session session = icat.login("db", credentials); + return session; + } + + @Ignore("Test fails because of bug in eclipselink") + @Test + public void testDistinctBehaviour() throws Exception { + Session session = rootSession(); Path path = Paths.get(ClassLoader.class.getResource("/icat.port").toURI()); session.importMetaData(path, DuplicateAction.CHECK, Attributes.USER); @@ -115,97 +131,244 @@ public void TestJsoniseBean() throws Exception { DateFormat dft = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSXXX"); Session session = createAndPopulate(); - /* Expected: <[{"User":{"id":8148,"createId":"db/notroot","createTime":"2019-03-11T14:14:47.000Z","modId":"db/notroot","modTime":"2019-03-11T14:14:47.000Z","affiliation":"Unseen University","familyName":"Worblehat","fullName":"Dr. Horace Worblehat","givenName":"Horace","instrumentScientists":[],"investigationUsers":[],"name":"db/lib","studies":[],"userGroups":[]}}]> */ + /* + * Expected: <[{"User":{"id":8148,"createId":"db/notroot","createTime": + * "2019-03-11T14:14:47.000Z","modId":"db/notroot","modTime": + * "2019-03-11T14:14:47.000Z","affiliation":"Unseen University","familyName": + * "Worblehat","fullName":"Dr. Horace Worblehat","givenName":"Horace", + * "instrumentScientists":[],"investigationUsers":[],"name":"db/lib","studies":[ + * ],"userGroups":[]}}]> + */ JsonArray user_response = search(session, "SELECT u from User u WHERE u.name = 'db/lib'", 1); collector.checkThat(user_response.getJsonObject(0).containsKey("User"), is(true)); JsonObject user = user_response.getJsonObject(0).getJsonObject("User"); - collector.checkThat(user.getJsonNumber("id").isIntegral(), is(true)); // Check Integer conversion - collector.checkThat(user.getString("createId"), is("db/notroot")); // Check String conversion - - /* Expected: <[{"Facility":{"id":2852,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","applications":[],"datafileFormats":[],"datasetTypes":[],"daysUntilRelease":90,"facilityCycles":[],"instruments":[],"investigationTypes":[],"investigations":[],"name":"Test port facility","parameterTypes":[],"sampleTypes":[]}}]> */ + collector.checkThat(user.getJsonNumber("id").isIntegral(), is(true)); // Check Integer conversion + collector.checkThat(user.getString("createId"), is("db/notroot")); // Check String conversion + + /* + * Expected: <[{"Facility":{"id":2852,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","applications":[],"datafileFormats":[], + * "datasetTypes":[],"daysUntilRelease":90,"facilityCycles":[],"instruments":[], + * "investigationTypes":[],"investigations":[],"name":"Test port facility" + * ,"parameterTypes":[],"sampleTypes":[]}}]> + */ JsonArray fac_response = search(session, "SELECT f from Facility f WHERE f.name = 'Test port facility'", 1); collector.checkThat(fac_response.getJsonObject(0).containsKey("Facility"), is(true)); - /* Expected: <[{"Instrument":{"id":1449,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","fullName":"EDDI - Energy Dispersive Diffraction","instrumentScientists":[],"investigationInstruments":[],"name":"EDDI","pid":"ig:0815","shifts":[]}}]> */ + /* + * Expected: <[{"Instrument":{"id":1449,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","fullName":"EDDI - Energy Dispersive Diffraction" + * ,"instrumentScientists":[],"investigationInstruments":[],"name":"EDDI","pid": + * "ig:0815","shifts":[]}}]> + */ JsonArray inst_response = search(session, "SELECT i from Instrument i WHERE i.name = 'EDDI'", 1); collector.checkThat(inst_response.getJsonObject(0).containsKey("Instrument"), is(true)); - /* Expected: <[{"InvestigationType":{"id":3401,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","investigations":[],"name":"atype"}}]> */ + /* + * Expected: + * <[{"InvestigationType":{"id":3401,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","investigations":[],"name":"atype"}}]> + */ JsonArray it_response = search(session, "SELECT it from InvestigationType it WHERE it.name = 'atype'", 1); collector.checkThat(it_response.getJsonObject(0).containsKey("InvestigationType"), is(true)); - /* Expected: <[{"ParameterType":{"id":5373,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","applicableToDataCollection":false,"applicableToDatafile":true,"applicableToDataset":true,"applicableToInvestigation":true,"applicableToSample":false,"dataCollectionParameters":[],"datafileParameters":[],"datasetParameters":[],"enforced":false,"investigationParameters":[],"minimumNumericValue":73.4,"name":"temp","permissibleStringValues":[],"pid":"pt:25c","sampleParameters":[],"units":"degrees Kelvin","valueType":"NUMERIC","verified":false}}]> */ + /* + * Expected: <[{"ParameterType":{"id":5373,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","applicableToDataCollection":false, + * "applicableToDatafile":true,"applicableToDataset":true, + * "applicableToInvestigation":true,"applicableToSample":false, + * "dataCollectionParameters":[],"datafileParameters":[],"datasetParameters":[], + * "enforced":false,"investigationParameters":[],"minimumNumericValue":73.4, + * "name":"temp","permissibleStringValues":[],"pid":"pt:25c","sampleParameters": + * [],"units":"degrees Kelvin","valueType":"NUMERIC","verified":false}}]> + */ JsonArray pt_response = search(session, "SELECT pt from ParameterType pt WHERE pt.name = 'temp'", 1); collector.checkThat(pt_response.getJsonObject(0).containsKey("ParameterType"), is(true)); - collector.checkThat((Double) pt_response.getJsonObject(0).getJsonObject("ParameterType").getJsonNumber("minimumNumericValue").doubleValue(), is(73.4)); // Check Double conversion - collector.checkThat((Boolean) pt_response.getJsonObject(0).getJsonObject("ParameterType").getBoolean("enforced"), is(Boolean.FALSE)); // Check boolean conversion - collector.checkThat(pt_response.getJsonObject(0).getJsonObject("ParameterType").getJsonString("valueType").getString(), is("NUMERIC")); // Check ParameterValueType conversion - - /* Expected: <[{"Investigation":{"id":4814,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"endDate":"2010-12-31T23:59:59.000Z","investigationGroups":[],"investigationInstruments":[],"investigationUsers":[],"keywords":[],"name":"expt1","parameters":[],"publications":[],"samples":[],"shifts":[],"startDate":"2010-01-01T00:00:00.000Z","studyInvestigations":[],"title":"a title at the beginning","visitId":"zero"}},{"Investigation":{"id":4815,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"endDate":"2011-12-31T23:59:59.000Z","investigationGroups":[],"investigationInstruments":[],"investigationUsers":[],"keywords":[],"name":"expt1","parameters":[],"publications":[],"samples":[],"shifts":[],"startDate":"2011-01-01T00:00:00.000Z","studyInvestigations":[],"title":"a title in the middle","visitId":"one"}},{"Investigation":{"id":4816,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"endDate":"2012-12-31T23:59:59.000Z","investigationGroups":[],"investigationInstruments":[],"investigationUsers":[],"keywords":[],"name":"expt1","parameters":[],"publications":[],"samples":[],"shifts":[],"startDate":"2012-01-01T00:00:00.000Z","studyInvestigations":[],"title":"a title at the end","visitId":"two"}}]> */ + collector.checkThat((Double) pt_response.getJsonObject(0).getJsonObject("ParameterType") + .getJsonNumber("minimumNumericValue").doubleValue(), is(73.4)); // Check Double conversion + collector.checkThat( + (Boolean) pt_response.getJsonObject(0).getJsonObject("ParameterType").getBoolean("enforced"), + is(Boolean.FALSE)); // Check boolean conversion + collector.checkThat( + pt_response.getJsonObject(0).getJsonObject("ParameterType").getJsonString("valueType").getString(), + is("NUMERIC")); // Check ParameterValueType conversion + + /* + * Expected: <[{"Investigation":{"id":4814,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","datasets":[],"endDate":"2010-12-31T23:59:59.000Z" + * ,"investigationGroups":[],"investigationInstruments":[],"investigationUsers": + * [],"keywords":[],"name":"expt1","parameters":[],"publications":[],"samples":[ + * ],"shifts":[],"startDate":"2010-01-01T00:00:00.000Z","studyInvestigations":[] + * ,"title":"a title at the beginning","visitId":"zero"}},{"Investigation":{"id" + * :4815,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId" + * :"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"endDate": + * "2011-12-31T23:59:59.000Z","investigationGroups":[], + * "investigationInstruments":[],"investigationUsers":[],"keywords":[],"name": + * "expt1","parameters":[],"publications":[],"samples":[],"shifts":[], + * "startDate":"2011-01-01T00:00:00.000Z","studyInvestigations":[], + * "title":"a title in the middle","visitId":"one"}},{"Investigation":{"id":4816 + * ,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId": + * "db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"endDate": + * "2012-12-31T23:59:59.000Z","investigationGroups":[], + * "investigationInstruments":[],"investigationUsers":[],"keywords":[],"name": + * "expt1","parameters":[],"publications":[],"samples":[],"shifts":[], + * "startDate":"2012-01-01T00:00:00.000Z","studyInvestigations":[], + * "title":"a title at the end","visitId":"two"}}]> + */ JsonArray inv_response = search(session, "SELECT inv from Investigation inv WHERE inv.name = 'expt1'", 3); collector.checkThat(inv_response.getJsonObject(0).containsKey("Investigation"), is(true)); - /* Expected: <[{"InvestigationUser":{"id":4723,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","role":"troublemaker"}}]> */ - JsonArray invu_response = search(session, "SELECT invu from InvestigationUser invu WHERE invu.role = 'troublemaker'", 1); + /* + * Expected: + * <[{"InvestigationUser":{"id":4723,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","role":"troublemaker"}}]> + */ + JsonArray invu_response = search(session, + "SELECT invu from InvestigationUser invu WHERE invu.role = 'troublemaker'", 1); collector.checkThat(invu_response.getJsonObject(0).containsKey("InvestigationUser"), is(true)); - /* Expected: <[{"Shift":{"id":2995,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","comment":"waiting","endDate":"2013-12-31T22:59:59.000Z","startDate":"2013-12-31T11:00:00.000Z"}}]> */ + /* + * Expected: <[{"Shift":{"id":2995,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","comment":"waiting","endDate": + * "2013-12-31T22:59:59.000Z","startDate":"2013-12-31T11:00:00.000Z"}}]> + */ JsonArray shift_response = search(session, "SELECT shift from Shift shift WHERE shift.comment = 'waiting'", 1); collector.checkThat(shift_response.getJsonObject(0).containsKey("Shift"), is(true)); - /* Expected: <[{"SampleType":{"id":3220,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","molecularFormula":"C","name":"diamond","safetyInformation":"fairly harmless","samples":[]}}]> */ + /* + * Expected: <[{"SampleType":{"id":3220,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","molecularFormula":"C","name":"diamond", + * "safetyInformation":"fairly harmless","samples":[]}}]> + */ JsonArray st_response = search(session, "SELECT st from SampleType st WHERE st.name = 'diamond'", 1); collector.checkThat(st_response.getJsonObject(0).containsKey("SampleType"), is(true)); - /* Expected: <[{"Sample":{"id":2181,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"name":"Koh-I-Noor","parameters":[],"pid":"sdb:374717"}}]> */ + /* + * Expected: <[{"Sample":{"id":2181,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","datasets":[],"name":"Koh-I-Noor","parameters":[], + * "pid":"sdb:374717"}}]> + */ JsonArray s_response = search(session, "SELECT s from Sample s WHERE s.name = 'Koh-I-Noor'", 1); collector.checkThat(s_response.getJsonObject(0).containsKey("Sample"), is(true)); - /* Expected: <[{"InvestigationParameter":{"id":1123,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","stringValue":"green"}}]> */ - JsonArray invp_response = search(session, "SELECT invp from InvestigationParameter invp WHERE invp.stringValue = 'green'", 1); + /* + * Expected: + * <[{"InvestigationParameter":{"id":1123,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","stringValue":"green"}}]> + */ + JsonArray invp_response = search(session, + "SELECT invp from InvestigationParameter invp WHERE invp.stringValue = 'green'", 1); collector.checkThat(invp_response.size(), equalTo(1)); collector.checkThat(invp_response.getJsonObject(0).containsKey("InvestigationParameter"), is(true)); - /* Expected: <[{"DatasetType":{"id":1754,"createId":"db/notroot","createTime":"2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime":"2019-03-11T15:58:33.000Z","datasets":[],"name":"calibration"}}]> */ + /* + * Expected: <[{"DatasetType":{"id":1754,"createId":"db/notroot","createTime": + * "2019-03-11T15:58:33.000Z","modId":"db/notroot","modTime": + * "2019-03-11T15:58:33.000Z","datasets":[],"name":"calibration"}}]> + */ JsonArray dst_response = search(session, "SELECT dst from DatasetType dst WHERE dst.name = 'calibration'", 1); collector.checkThat(dst_response.getJsonObject(0).containsKey("DatasetType"), is(true)); - /* Expected: <[{"Dataset":{"id":8128,"createId":"db/notroot","createTime":"2019-03-12T11:40:26.000Z","modId":"db/notroot","modTime":"2019-03-12T11:40:26.000Z","complete":true,"dataCollectionDatasets":[],"datafiles":[],"description":"alpha","endDate":"2014-05-16T04:28:26.000Z","name":"ds1","parameters":[],"startDate":"2014-05-16T04:28:26.000Z"}}]> */ + /* + * Expected: <[{"Dataset":{"id":8128,"createId":"db/notroot","createTime": + * "2019-03-12T11:40:26.000Z","modId":"db/notroot","modTime": + * "2019-03-12T11:40:26.000Z","complete":true,"dataCollectionDatasets":[], + * "datafiles":[],"description":"alpha","endDate":"2014-05-16T04:28:26.000Z", + * "name":"ds1","parameters":[],"startDate":"2014-05-16T04:28:26.000Z"}}]> + */ JsonArray ds_response = search(session, "SELECT ds from Dataset ds WHERE ds.name = 'ds1'", 1); collector.checkThat(ds_response.getJsonObject(0).containsKey("Dataset"), is(true)); - collector.checkThat(dft.parse(ds_response.getJsonObject(0).getJsonObject("Dataset").getString("startDate")), isA(Date.class)); //Check Date conversion - - /* Expected: <[{"DatasetParameter":{"id":4632,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","stringValue":"green"}}]> */ - JsonArray dsp_response = search(session, "SELECT dsp from DatasetParameter dsp WHERE dsp.stringValue = 'green'", 1); + collector.checkThat(dft.parse(ds_response.getJsonObject(0).getJsonObject("Dataset").getString("startDate")), + isA(Date.class)); // Check Date conversion + + /* + * Expected: + * <[{"DatasetParameter":{"id":4632,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z","stringValue":"green"}}]> + */ + JsonArray dsp_response = search(session, "SELECT dsp from DatasetParameter dsp WHERE dsp.stringValue = 'green'", + 1); collector.checkThat(dsp_response.size(), equalTo(1)); collector.checkThat(dsp_response.getJsonObject(0).containsKey("DatasetParameter"), is(true)); - /* Expected: <[{"Datafile":{"id":15643,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","dataCollectionDatafiles":[],"destDatafiles":[],"fileSize":17,"name":"df2","parameters":[],"sourceDatafiles":[]}}]> */ + /* + * Expected: <[{"Datafile":{"id":15643,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z","dataCollectionDatafiles":[],"destDatafiles":[], + * "fileSize":17,"name":"df2","parameters":[],"sourceDatafiles":[]}}]> + */ JsonArray df_response = search(session, "SELECT df from Datafile df WHERE df.name = 'df2'", 1); collector.checkThat(df_response.size(), equalTo(1)); collector.checkThat(df_response.getJsonObject(0).containsKey("Datafile"), is(true)); - /* Expected: <[{"DatafileParameter":{"id":1938,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","stringValue":"green"}}]> */ - JsonArray dfp_response = search(session, "SELECT dfp from DatafileParameter dfp WHERE dfp.stringValue = 'green'", 1); + /* + * Expected: + * <[{"DatafileParameter":{"id":1938,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z","stringValue":"green"}}]> + */ + JsonArray dfp_response = search(session, + "SELECT dfp from DatafileParameter dfp WHERE dfp.stringValue = 'green'", 1); collector.checkThat(dfp_response.size(), equalTo(1)); collector.checkThat(dfp_response.getJsonObject(0).containsKey("DatafileParameter"), is(true)); - /* Expected: <[{"Application":{"id":2972,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","jobs":[],"name":"aprog","version":"1.2.3"}},{"Application":{"id":2973,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","jobs":[],"name":"aprog","version":"1.2.6"}}]> */ + /* + * Expected: <[{"Application":{"id":2972,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z","jobs":[],"name":"aprog","version":"1.2.3"}},{ + * "Application":{"id":2973,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z","jobs":[],"name":"aprog","version":"1.2.6"}}]> + */ JsonArray a_response = search(session, "SELECT a from Application a WHERE a.name = 'aprog'", 2); collector.checkThat(a_response.size(), equalTo(2)); collector.checkThat(a_response.getJsonObject(0).containsKey("Application"), is(true)); - /* Expected: <[{DataCollection":{"id":4485,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters":[]}},{"DataCollection":{"id":4486,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters":[]}},{"DataCollection":{"id":4487,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters":[]}}]> */ + /* + * Expected: + * <[{DataCollection":{"id":4485,"createId":"db/notroot","createTime":"2019-03- + * 12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33. + * 000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters":[]}},{"DataCollection":{"id":4486,"createId":"db + * /notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/ + * notroot","modTime":"2019-03-12T13:30:33. + * 000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters":[]}},{"DataCollection":{"id":4487,"createId":"db + * /notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/ + * notroot","modTime":"2019-03-12T13:30:33. + * 000Z","dataCollectionDatafiles":[],"dataCollectionDatasets":[],"jobsAsInput":[],"jobsAsOutput":[],"parameters + * ":[]}}]> + */ JsonArray dc_response = search(session, "SELECT dc from DataCollection dc", 3); collector.checkThat(dc_response.size(), equalTo(3)); collector.checkThat(dc_response.getJsonObject(0).containsKey("DataCollection"), is(true)); - /* Expected: <[{"DataCollectionDatafile":{"id":4362,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z"}},{"DataCollectionDatafile":{"id":4363,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z"}}]> */ + /* + * Expected: + * <[{"DataCollectionDatafile":{"id":4362,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z"}},{"DataCollectionDatafile":{"id":4363,"createId": + * "db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot", + * "modTime":"2019-03-12T13:30:33.000Z"}}]> + */ JsonArray dcdf_response = search(session, "SELECT dcdf from DataCollectionDatafile dcdf", 2); collector.checkThat(dcdf_response.getJsonObject(0).containsKey("DataCollectionDatafile"), is(true)); - /* Expected: <[{"Job":{"id":1634,"createId":"db/notroot","createTime":"2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime":"2019-03-12T13:30:33.000Z"}}]> */ + /* + * Expected: <[{"Job":{"id":1634,"createId":"db/notroot","createTime": + * "2019-03-12T13:30:33.000Z","modId":"db/notroot","modTime": + * "2019-03-12T13:30:33.000Z"}}]> + */ JsonArray j_response = search(session, "SELECT j from Job j", 1); collector.checkThat(j_response.getJsonObject(0).containsKey("Job"), is(true)); } @@ -329,6 +492,10 @@ public void testLuceneDatafiles() throws Exception { // Set text and parameters array = searchDatafiles(session, null, "df2", null, null, parameters, 20, 1); checkResultFromLuceneSearch(session, "df2", array, "Datafile", "name"); + + // Search with a user who should not see any results + Session piOneSession = piOneSession(); + searchDatafiles(piOneSession, null, null, null, null, null, 20, 0); } @Test @@ -374,6 +541,10 @@ public void testLuceneDatasets() throws Exception { array = searchDatasets(session, null, "gamma AND ds3", dft.parse("2014-05-16T05:09:03+0000"), dft.parse("2014-05-16T05:15:26+0000"), parameters, 20, 1); checkResultFromLuceneSearch(session, "gamma", array, "Dataset", "description"); + + // Search with a user who should not see any results + Session piOneSession = piOneSession(); + searchDatasets(piOneSession, null, null, null, null, null, 20, 0); } @Test @@ -439,6 +610,10 @@ public void testLuceneInvestigations() throws Exception { } catch (IcatException e) { assertEquals(IcatExceptionType.BAD_PARAMETER, e.getType()); } + + // Search with a user who should not see any results + Session piOneSession = piOneSession(); + searchInvestigations(piOneSession, null, null, null, null, null, null, null, 20, 0); } private void checkResultFromLuceneSearch(Session session, String val, JsonArray array, String ename, String field) @@ -517,9 +692,10 @@ public void testGet() throws Exception { long fid = search(session, "Facility.id", 1).getJsonNumber(0).longValueExact(); + String query = "Facility INCLUDE InvestigationType"; JsonObject fac = Json .createReader( - new ByteArrayInputStream(session.get("Facility INCLUDE InvestigationType", fid).getBytes())) + new ByteArrayInputStream(session.get(query, fid).getBytes())) .readObject().getJsonObject("Facility"); assertEquals("Test port facility", fac.getString("name")); @@ -533,6 +709,13 @@ public void testGet() throws Exception { } Collections.sort(names); assertEquals(Arrays.asList("atype", "btype"), names); + + // Search with a user who should not see any results + Session piOneSession = piOneSession(); + wSession.addRule(null, "Facility", "R"); + fac = Json.createReader(new StringReader(piOneSession.get(query, fid))).readObject().getJsonObject("Facility"); + its = fac.getJsonArray("investigationTypes"); + assertEquals(0, its.size()); } @Test @@ -583,11 +766,7 @@ public void testSearchWithNew() throws Exception { @Test public void testWait() throws Exception { - ICAT icat = new ICAT(System.getProperty("serverUrl")); - Map credentials = new HashMap<>(); - credentials.put("username", "root"); - credentials.put("password", "password"); - Session rootSession = icat.login("db", credentials); + Session rootSession = rootSession(); long t = System.currentTimeMillis(); rootSession.waitMillis(1000L); System.out.println(System.currentTimeMillis() - t); @@ -607,14 +786,15 @@ public void testSearch() throws Exception { JsonArray array; - JsonObject user = search(session, "SELECT u FROM User u WHERE u.name = 'db/lib'", 1).getJsonObject(0).getJsonObject("User"); + JsonObject user = search(session, "SELECT u FROM User u WHERE u.name = 'db/lib'", 1).getJsonObject(0) + .getJsonObject("User"); assertEquals("Horace", user.getString("givenName")); assertEquals("Worblehat", user.getString("familyName")); assertEquals("Unseen University", user.getString("affiliation")); String query = "SELECT inv FROM Investigation inv JOIN inv.shifts AS s " - + "WHERE s.instrument.pid = 'ig:0815' AND s.comment = 'beamtime' " - + "AND s.startDate <= '2014-01-01 12:00:00' AND s.endDate >= '2014-01-01 12:00:00'"; + + "WHERE s.instrument.pid = 'ig:0815' AND s.comment = 'beamtime' " + + "AND s.startDate <= '2014-01-01 12:00:00' AND s.endDate >= '2014-01-01 12:00:00'"; JsonObject inv = search(session, query, 1).getJsonObject(0).getJsonObject("Investigation"); assertEquals("expt1", inv.getString("name")); assertEquals("zero", inv.getString("visitId")); @@ -716,6 +896,14 @@ public void testSearch() throws Exception { Collections.sort(names); assertEquals(Arrays.asList("atype", "btype"), names); } + + // Search with a user who should not see any results + Session piOneSession = piOneSession(); + wSession.addRule(null, "Facility", "R"); + JsonObject searchResult = search(piOneSession, "Facility INCLUDE InvestigationType", 1).getJsonObject(0); + JsonArray investigationTypes = searchResult.getJsonObject("Facility").getJsonArray("investigationTypes"); + System.out.println(investigationTypes); + assertEquals(0, investigationTypes.size()); } @Test @@ -1071,7 +1259,7 @@ public void testWriteGood() throws Exception { JsonArray array = search(session, "SELECT it.name, it.facility.name FROM InvestigationType it WHERE it.id = " + newInvTypeId, 1) - .getJsonArray(0); + .getJsonArray(0); assertEquals("ztype", array.getString(0)); assertEquals("Test port facility", array.getString(1)); @@ -1533,14 +1721,65 @@ private void exportMetaDataDump(Map credentials) throws Exceptio Files.delete(dump2); } + @Test + public void exportMetaDataQueryUser() throws Exception { + Session rootSession = rootSession(); + Session piOneSession = piOneSession(); + Path path = Paths.get(ClassLoader.class.getResource("/icat.port").toURI()); + + // Get known configuration + rootSession.importMetaData(path, DuplicateAction.CHECK, Attributes.ALL); + String query = "Investigation INCLUDE Facility, Dataset"; + Path dump1 = Files.createTempFile("dump1", ".tmp"); + Path dump2 = Files.createTempFile("dump2", ".tmp"); + + // piOne should only be able to dump the Investigation, but not have R access to + // Dataset, Facility + wSession.addRule(null, "Investigation", "R"); + try (InputStream stream = piOneSession.exportMetaData(query, Attributes.USER)) { + Files.copy(stream, dump1, StandardCopyOption.REPLACE_EXISTING); + } + // piOne should now be able to dump all due to rules giving R access + wSession.addRule(null, "Facility", "R"); + wSession.addRule(null, "Dataset", "R"); + try (InputStream stream = piOneSession.exportMetaData(query, Attributes.USER)) { + Files.copy(stream, dump2, StandardCopyOption.REPLACE_EXISTING); + } + List restrictedLines = Files.readAllLines(dump1); + List permissiveLines = Files.readAllLines(dump2); + String restrictiveMessage = " appeared in export, but piOne use should not have access"; + String permissiveMessage = " did not appear in export, but piOne should have access"; + + boolean containsInvestigations = false; + for (String line : restrictedLines) { + System.out.println(line); + containsInvestigations = containsInvestigations || line.startsWith("Investigation"); + assertFalse("Dataset" + restrictiveMessage, line.startsWith("Dataset")); + assertFalse("Facility" + restrictiveMessage, line.startsWith("Facility")); + } + assertTrue("Investigation" + permissiveMessage, containsInvestigations); + + containsInvestigations = false; + boolean containsDatasets = false; + boolean containsFacilities = false; + for (String line : permissiveLines) { + System.out.println(line); + containsInvestigations = containsInvestigations || line.startsWith("Investigation"); + containsDatasets = containsDatasets || line.startsWith("Dataset"); + containsFacilities = containsFacilities || line.startsWith("Facility"); + } + assertTrue("Investigation" + permissiveMessage, containsInvestigations); + assertTrue("Dataset" + permissiveMessage, containsDatasets); + assertTrue("Facility" + permissiveMessage, containsFacilities); + + Files.delete(dump1); + Files.delete(dump2); + } + @Ignore("Test fails - appears brittle to differences in timezone") @Test public void exportMetaDataQuery() throws Exception { - ICAT icat = new ICAT(System.getProperty("serverUrl")); - Map credentials = new HashMap<>(); - credentials.put("username", "root"); - credentials.put("password", "password"); - Session session = icat.login("db", credentials); + Session session = rootSession(); Path path = Paths.get(ClassLoader.class.getResource("/icat.port").toURI()); // Get known configuration @@ -1571,11 +1810,7 @@ public void exportMetaDataQuery() throws Exception { @Test public void importMetaDataAllNotRoot() throws Exception { - ICAT icat = new ICAT(System.getProperty("serverUrl")); - Map credentials = new HashMap<>(); - credentials.put("username", "piOne"); - credentials.put("password", "piOne"); - Session session = icat.login("db", credentials); + Session session = piOneSession(); Path path = Paths.get(ClassLoader.class.getResource("/icat.port").toURI()); try { session.importMetaData(path, DuplicateAction.CHECK, Attributes.ALL); @@ -1586,11 +1821,7 @@ public void importMetaDataAllNotRoot() throws Exception { } private void importMetaData(Attributes attributes, String userName) throws Exception { - ICAT icat = new ICAT(System.getProperty("serverUrl")); - Map credentials = new HashMap<>(); - credentials.put("username", "root"); - credentials.put("password", "password"); - Session session = icat.login("db", credentials); + Session session = rootSession(); Path path = Paths.get(ClassLoader.class.getResource("/icat.port").toURI()); start = System.currentTimeMillis(); From 03213aaac476a860c38e813c0cb678ac49d57092 Mon Sep 17 00:00:00 2001 From: patrick-austin <61705287+patrick-austin@users.noreply.github.com> Date: Mon, 24 Oct 2022 17:26:25 +0100 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Viktor Bozhinov <45173816+VKTB@users.noreply.github.com> --- src/test/java/org/icatproject/integration/TestRS.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/icatproject/integration/TestRS.java b/src/test/java/org/icatproject/integration/TestRS.java index 0350ff438..42f1ea73f 100644 --- a/src/test/java/org/icatproject/integration/TestRS.java +++ b/src/test/java/org/icatproject/integration/TestRS.java @@ -95,8 +95,7 @@ private Session rootSession() throws URISyntaxException, IcatException { Map credentials = new HashMap<>(); credentials.put("username", "root"); credentials.put("password", "password"); - Session session = icat.login("db", credentials); - return session; + return icat.login("db", credentials); } private Session piOneSession() throws URISyntaxException, IcatException { @@ -104,8 +103,7 @@ private Session piOneSession() throws URISyntaxException, IcatException { Map credentials = new HashMap<>(); credentials.put("username", "piOne"); credentials.put("password", "piOne"); - Session session = icat.login("db", credentials); - return session; + return icat.login("db", credentials); } @Ignore("Test fails because of bug in eclipselink")