Determine scenarios where DlsFlsValveImpl can be short circuited

[cwperks]

In #5184, logic was added to short circuit the DlsFlsValveImpl in the case where the action being performed was a cluster action. Initially, the logic applied to all cluster actions but an exception had to be made for mget to fix the ./gradlew dlicDlsflsTest --tests DlsTermLookupQueryTest.testMGet_1337 test.

I'm opening up this issue to come to a consensus on which scenarios the DlsFlsValveImpl can be short-circuited. Currently, the logic only applies to indices: actions.

There's some additional conversation on an old PR as well: #4937

[nibix]

The filter level DLS implementation rewrites mget requests. This happens here in DlsFilterLevelActionHandler:

security/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

Lines 319 to 384 in 75f03c7

	private boolean handle(MultiGetRequest multiGetRequest, StoredContext ctx) {
	if (documentAllowlist != null) {
	documentAllowlist.applyTo(threadContext);
	}
	Map<String, Set<String>> idsGroupedByIndex = multiGetRequest.getItems()
	.stream()
	.collect(Collectors.groupingBy((item) -> item.index(), Collectors.mapping((item) -> item.id(), Collectors.toSet())));
	Set<String> indices = idsGroupedByIndex.keySet();
	SearchRequest searchRequest = new SearchRequest(indices.toArray(new String[indices.size()]));
	BoolQueryBuilder query;
	if (indices.size() == 1) {
	Set<String> ids = idsGroupedByIndex.get(indices.iterator().next());
	query = QueryBuilders.boolQuery()
	.must(QueryBuilders.idsQuery().addIds(ids.toArray(new String[ids.size()])))
	.must(filterLevelQueryBuilder);
	} else {
	BoolQueryBuilder mgetQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);
	for (Map.Entry<String, Set<String>> entry : idsGroupedByIndex.entrySet()) {
	BoolQueryBuilder indexQuery = QueryBuilders.boolQuery()
	.must(QueryBuilders.termQuery("_index", entry.getKey()))
	.must(QueryBuilders.idsQuery().addIds(entry.getValue().toArray(new String[entry.getValue().size()])));
	mgetQuery.should(indexQuery);
	}
	query = QueryBuilders.boolQuery().must(mgetQuery).must(filterLevelQueryBuilder);
	}
	searchRequest.source(SearchSourceBuilder.searchSource().query(query));
	nodeClient.search(searchRequest, new ActionListener<SearchResponse>() {
	@Override
	public void onResponse(SearchResponse response) {
	try {
	ctx.restore();
	List<MultiGetItemResponse> itemResponses = new ArrayList<>(response.getHits().getHits().length);
	for (SearchHit hit : response.getHits().getHits()) {
	itemResponses.add(new MultiGetItemResponse(new GetResponse(searchHitToGetResult(hit)), null));
	}
	@SuppressWarnings("unchecked")
	ActionListener<MultiGetResponse> multiGetListener = (ActionListener<MultiGetResponse>) listener;
	multiGetListener.onResponse(
	new MultiGetResponse(itemResponses.toArray(new MultiGetItemResponse[itemResponses.size()]))
	);
	} catch (Exception e) {
	listener.onFailure(e);
	}
	}
	@Override
	public void onFailure(Exception e) {
	listener.onFailure(e);
	}
	});
	return false;
	}

The DlsFilterLevelActionHandler is called via DlsFlsValve, so it makes sense that it needs to handle mget requests.

Still, DlsFilterLevelActionHandler has its own logic to decide which requests are relevant to it or not:

security/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

Lines 90 to 115 in 75f03c7

	if (threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE) != null) {
	return true;
	}
	String action = context.getAction();
	ActionRequest request = context.getRequest();
	if (action.startsWith("cluster:")
	|| action.startsWith("indices:admin/template/")
	|| action.startsWith("indices:admin/index_template/")) {
	return true;
	}
	if (action.startsWith(SearchScrollAction.NAME)) {
	return true;
	}
	if (action.equals(SearchTemplateAction.NAME) || action.equals(MultiSearchTemplateAction.NAME)) {
	// Let it pass; DLS will be handled on a lower level
	return true;
	}
	if (request instanceof MultiSearchRequest) {
	// Let it pass; DLS will be handled on a lower level
	return true;
	}

With the changes in DlsFlsValve, quite a few of these conditions never get true. One might want to check whether this should be somehow refactored.

[cwperks]

Still, DlsFilterLevelActionHandler has its own logic to decide which requests are relevant to it or not:

security/src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

With the changes in DlsFlsValve, quite a few of these conditions never get true. One might want to check whether this should be somehow refactored.

This should certainly be revisited to remove the redundancy. Looks like quite a few lines could be removed from there.