Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IQSS/10705: Refactor permission filter query in indexing #10706

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+65 −198
Open

IQSS/10705: Refactor permission filter query in indexing #10706

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd82437
refactor/fix avoidJoin logic
qqmyers Jun 27, 2024
0331585
bug fix - default to */All groups for superuser
qqmyers Jul 3, 2024
1aa2e64
drop extra line from merge
qqmyers Jul 19, 2024
d9488a7
add logging to allow comparing old and new result
qqmyers Jul 19, 2024
86ea279
Merge remote-tracking branch 'IQSS/develop' into QDR-refactor_avoidJo…
qqmyers Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
263 changes: 65 additions & 198 deletions src/main/java/edu/harvard/iq/dataverse/search/SearchServiceBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import jakarta.inject.Inject;
import jakarta.inject.Named;
import jakarta.persistence.NoResultException;

import org.apache.commons.lang3.StringUtils;
import org.apache.solr.client.solrj.SolrQuery;
import org.apache.solr.client.solrj.SolrQuery.SortClause;
import org.apache.solr.client.solrj.SolrServerException;
Expand All @@ -52,6 +54,8 @@ public class SearchServiceBean {

private static final Logger logger = Logger.getLogger(SearchServiceBean.class.getCanonicalName());

private static final String ALL_GROUPS = "*";

/**
* We're trying to make the SearchServiceBean lean, mean, and fast, with as
* few injections of EJBs as possible.
Expand Down Expand Up @@ -182,6 +186,7 @@ public SolrQueryResponse search(

SolrQuery solrQuery = new SolrQuery();
query = SearchUtil.sanitizeQuery(query);

solrQuery.setQuery(query);
if (sortField != null) {
// is it ok not to specify any sort? - there are cases where we
Expand Down Expand Up @@ -323,24 +328,13 @@ public SolrQueryResponse search(
}
}

//I'm not sure if just adding null here is good for hte permissions system... i think it needs something
if(dataverses != null) {
for(Dataverse dataverse : dataverses) {
// -----------------------------------
// PERMISSION FILTER QUERY
// -----------------------------------
String permissionFilterQuery = this.getPermissionFilterQuery(dataverseRequest, solrQuery, dataverse, onlyDatatRelatedToMe, addFacets);
if (permissionFilterQuery != null) {
solrQuery.addFilterQuery(permissionFilterQuery);
}
}
} else {
String permissionFilterQuery = this.getPermissionFilterQuery(dataverseRequest, solrQuery, null, onlyDatatRelatedToMe, addFacets);
if (permissionFilterQuery != null) {
solrQuery.addFilterQuery(permissionFilterQuery);
}
// -----------------------------------
// PERMISSION FILTER QUERY
// -----------------------------------
String permissionFilterQuery = this.getPermissionFilterQuery(dataverseRequest, solrQuery, onlyDatatRelatedToMe, addFacets);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dataverse param was never used in getPermissionFilterQuery - that must have been dropped at some point.

if (permissionFilterQuery != null) {
solrQuery.addFilterQuery(permissionFilterQuery);
}


/**
* @todo: do sanity checking... throw error if negative
Expand Down Expand Up @@ -994,7 +988,7 @@ public String getCapitalizedName(String name) {
*
* @return
*/
private String getPermissionFilterQuery(DataverseRequest dataverseRequest, SolrQuery solrQuery, Dataverse dataverse, boolean onlyDatatRelatedToMe, boolean addFacets) {
private String getPermissionFilterQuery(DataverseRequest dataverseRequest, SolrQuery solrQuery, boolean onlyDatatRelatedToMe, boolean addFacets) {

User user = dataverseRequest.getUser();
if (user == null) {
Expand All @@ -1003,226 +997,99 @@ private String getPermissionFilterQuery(DataverseRequest dataverseRequest, SolrQ
if (solrQuery == null) {
throw new NullPointerException("solrQuery cannot be null");
}
/**
* @todo For people who are not logged in, should we show stuff indexed
* with "AllUsers" group or not? If so, uncomment the allUsersString
* stuff below.
*/
// String allUsersString = IndexServiceBean.getGroupPrefix() + AllUsers.get().getAlias();
// String publicOnly = "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":(" + IndexServiceBean.getPublicGroupString() + " OR " + allUsersString + ")";
String publicOnly = "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":(" + IndexServiceBean.getPublicGroupString() + ")";
// String publicOnly = "{!join from=" + SearchFields.GROUPS + " to=" + SearchFields.PERMS + "}id:" + IndexServiceBean.getPublicGroupString();
// initialize to public only to be safe
String dangerZoneNoSolrJoin = null;


if (user instanceof PrivateUrlUser) {
user = GuestUser.get();
}

AuthenticatedUser au = null;
ArrayList<String> groupList = new ArrayList<String>();
AuthenticatedUser au = null;
Set<Group> groups;

if (user instanceof GuestUser) {
// Yes, GuestUser may be part of one or more groups; such as IP Groups.
groups = groupService.collectAncestors(groupService.groupsFor(dataverseRequest));
} else {
if (!(user instanceof AuthenticatedUser)) {
logger.severe("Should never reach here. A User must be an AuthenticatedUser or a Guest");
throw new IllegalStateException("A User must be an AuthenticatedUser or a Guest");
}

boolean avoidJoin = FeatureFlags.AVOID_EXPENSIVE_SOLR_JOIN.enabled();

if (user instanceof AuthenticatedUser) {
au = (AuthenticatedUser) user;

// ----------------------------------------------------
// (3) Is this a Super User?
// Is this a Super User?
// If so, they can see everything
// ----------------------------------------------------
if (au.isSuperuser()) {
// Somewhat dangerous because this user (a superuser) will be able
// to see everything in Solr with no regard to permissions. But it's
// been this way since Dataverse 4.0. So relax. :)

return dangerZoneNoSolrJoin;
return buildPermissionFilterQuery(avoidJoin, ALL_GROUPS);
}

// ----------------------------------------------------
// (4) User is logged in AND onlyDatatRelatedToMe == true
// User is logged in AND onlyDatatRelatedToMe == true
// Yes, give back everything -> the settings will be in
// the filterqueries given to search
// the filterqueries given to search
// ----------------------------------------------------
if (onlyDatatRelatedToMe == true) {
if (systemConfig.myDataDoesNotUsePermissionDocs()) {
logger.fine("old 4.2 behavior: MyData is not using Solr permission docs");
return dangerZoneNoSolrJoin;
return buildPermissionFilterQuery(avoidJoin, ALL_GROUPS);
} else {
// fall-through
logger.fine("new post-4.2 behavior: MyData is using Solr permission docs");
}
}

// ----------------------------------------------------
// (5) Work with Authenticated User who is not a Superuser
// Work with Authenticated User who is not a Superuser
// ----------------------------------------------------

groups = groupService.collectAncestors(groupService.groupsFor(dataverseRequest));
groupList.add(IndexServiceBean.getGroupPerUserPrefix() + au.getId());
}

if (FeatureFlags.AVOID_EXPENSIVE_SOLR_JOIN.enabled()) {
/**
* Instead of doing a super expensive join, we will rely on the
* new boolean field PublicObject:true for public objects. This field
* is indexed on the content document itself, rather than a permission
* document. An additional join will be added only for any extra,
* more restricted groups that the user may be part of.
* **Note the experimental nature of this optimization**.
*/
StringBuilder sb = new StringBuilder();
StringBuilder sbgroups = new StringBuilder();

// All users, guests and authenticated, should see all the
// documents marked as publicObject_b:true, at least:
sb.append(SearchFields.PUBLIC_OBJECT + ":" + true);
// In addition to the user referenced directly, we will also
// add joins on all the non-public groups that may exist for the
// user:

// One or more groups *may* also be available for this user. Once again,
// do note that Guest users may be part of some groups, such as
// IP groups.

int groupCounter = 0;
// Authenticated users and GuestUser may be part of one or more groups; such
// as IP Groups.
groups = groupService.collectAncestors(groupService.groupsFor(dataverseRequest));

// An AuthenticatedUser should also be able to see all the content
// on which they have direct permissions:
if (au != null) {
groupCounter++;
sbgroups.append(IndexServiceBean.getGroupPerUserPrefix() + au.getId());
}

// In addition to the user referenced directly, we will also
// add joins on all the non-public groups that may exist for the
// user:
for (Group group : groups) {
String groupAlias = group.getAlias();
if (groupAlias != null && !groupAlias.isEmpty() && !groupAlias.startsWith("builtIn")) {
groupCounter++;
if (groupCounter > 1) {
sbgroups.append(" OR ");
}
sbgroups.append(IndexServiceBean.getGroupPrefix() + groupAlias);
}
}

if (groupCounter > 1) {
// If there is more than one group, the parentheses must be added:
sbgroups.insert(0, "(");
sbgroups.append(")");
}

if (groupCounter > 0) {
// If there are any groups for this user, an extra join must be
// added to the query, and the extra sub-query must be added to
// the combined Solr query:
sb.append(" OR {!join from=" + SearchFields.DEFINITION_POINT + " to=id v=$q1}");
// Add the subquery to the combined Solr query:
solrQuery.setParam("q1", SearchFields.DISCOVERABLE_BY + ":" + sbgroups.toString());
logger.info("The sub-query q1 set to " + SearchFields.DISCOVERABLE_BY + ":" + sbgroups.toString());
}

String ret = sb.toString();
logger.fine("Returning experimental query: " + ret);
return ret;
}

// END OF EXPERIMENTAL OPTIMIZATION

// Old, un-optimized way of handling permissions.
// Largely left intact, minus the lookups that have already been performed
// above.

// ----------------------------------------------------
// (1) Is this a GuestUser?
// ----------------------------------------------------
if (user instanceof GuestUser) {

StringBuilder sb = new StringBuilder();

String groupsFromProviders = "";
for (Group group : groups) {
logger.fine("found group " + group.getIdentifier() + " with alias " + group.getAlias());
String groupAlias = group.getAlias();
if (groupAlias != null && !groupAlias.isEmpty()) {
sb.append(" OR ");
// i.e. group_builtIn/all-users, ip/ipGroup3
sb.append(IndexServiceBean.getGroupPrefix()).append(groupAlias);
}
}
groupsFromProviders = sb.toString();
logger.fine("groupsFromProviders:" + groupsFromProviders);
String guestWithGroups = "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":(" + IndexServiceBean.getPublicGroupString() + groupsFromProviders + ")";
logger.fine(guestWithGroups);
return guestWithGroups;
}

// ----------------------------------------------------
// (5) Work with Authenticated User who is not a Superuser
// ----------------------------------------------------
// It was already confirmed, that if the user is not GuestUser, we
// have an AuthenticatedUser au which is not null.
/**
* @todo all this code needs cleanup and clarification.
*/
/**
* Every AuthenticatedUser is part of a "User Private Group" (UGP), a
* concept we borrow from RHEL:
* https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Deployment_Guide/ch-Managing_Users_and_Groups.html#s2-users-groups-private-groups
*/
/**
* @todo rename this from publicPlusUserPrivateGroup. Confusing
*/
// safe default: public only
String publicPlusUserPrivateGroup = publicOnly;
// + (onlyDatatRelatedToMe ? "" : (publicOnly + " OR "))
// + "{!join from=" + SearchFields.GROUPS + " to=" + SearchFields.PERMS + "}id:" + IndexServiceBean.getGroupPerUserPrefix() + au.getId() + ")";

// /**
// * @todo add onlyDatatRelatedToMe option into the experimental JOIN
// * before enabling it.
// */
/**
* From a search perspective, we don't care about if the group was
* created within one dataverse or another. We just want a list of *all*
* the groups the user is part of. We are greedy. We want all BuiltIn
* Groups, Shibboleth Groups, IP Groups, "system" groups, everything.
*
* A JOIN on "permission documents" will determine if the user can find
* a given "content document" (dataset version, etc) in Solr.
*/
String groupsFromProviders = "";
StringBuilder sb = new StringBuilder();
for (Group group : groups) {
logger.fine("found group " + group.getIdentifier() + " with alias " + group.getAlias());
String groupAlias = group.getAlias();
if (groupAlias != null && !groupAlias.isEmpty()) {
sb.append(" OR ");
// i.e. group_builtIn/all-users, group_builtIn/authenticated-users, group_1-explictGroup1, group_shib/2
sb.append(IndexServiceBean.getGroupPrefix() + groupAlias);
if (groupAlias != null && !groupAlias.isEmpty() && (!avoidJoin || !groupAlias.startsWith("builtIn"))) {
groupList.add(IndexServiceBean.getGroupPrefix() + groupAlias);
}
}
groupsFromProviders = sb.toString();

logger.fine(groupsFromProviders);
if (true) {
/**
* @todo get rid of "experimental" in name
*/
String experimentalJoin = "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":(" + IndexServiceBean.getPublicGroupString() + " OR " + IndexServiceBean.getGroupPerUserPrefix() + au.getId() + groupsFromProviders + ")";
publicPlusUserPrivateGroup = experimentalJoin;
if (!avoidJoin) {
// Add the public group
groupList.add(0, IndexServiceBean.getPublicGroupString());
}

String groupString = null;
//If we have additional groups, format them correctly into a search string, with parens if there is more than one
if (groupList.size() > 1) {
groupString = "(" + StringUtils.join(groupList, " OR ") + ")";
} else if (groupList.size() == 1) {
groupString = groupList.get(0);
}

//permissionFilterQuery = publicPlusUserPrivateGroup;
logger.fine(publicPlusUserPrivateGroup);

return publicPlusUserPrivateGroup;

logger.fine("Groups: " + groupString);
String permissionQuery = buildPermissionFilterQuery(avoidJoin, groupString);
logger.fine("Permission Query: " + permissionQuery);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the query from the existing code line 1203 should be the same in all cases, whether the avoidJoin flag is set or not.

return permissionQuery;
}

private String buildPermissionFilterQuery(boolean avoidJoin, String permissionFilterGroups) {
String query = (avoidJoin&& !isAllGroups(permissionFilterGroups)) ? SearchFields.PUBLIC_OBJECT + ":" + true : "";
if (permissionFilterGroups != null && !isAllGroups(permissionFilterGroups)) {
if (!query.isEmpty()) {
query = "(" + query + " OR " + "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":" + permissionFilterGroups + ")";
} else {
query = "{!join from=" + SearchFields.DEFINITION_POINT + " to=id}" + SearchFields.DISCOVERABLE_BY + ":" + permissionFilterGroups;
}
}
return query;
}

private boolean isAllGroups(String groups) {
return (groups!=null &&groups.equals(ALL_GROUPS));
}
}

Loading