Output:

> ldapsearch -H ldap://localhost:3389 -b 'dc=example,dc=com' -s sub -x '(|(foouaouoaeuo=bar)(dc=example))'
# extended LDIF
#
# LDAPv3
# base <dc=example,dc=com> with scope subtree
# filter: (|(foouaouoaeuo=bar)(dc=example))
# requesting: ALL
#
# example.com
dn: dc=example,dc=com
objectClass: top
objectClass: domain
dc: example
description: dc=example,dc=com
# search result
search: 2
result: 0 Success
text: Invalid attribute in filter - results may not be complete.
# numResponses: 2
# numEntries: 1

This function could be replaced with the following lines. Is it intented ?

slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &current_text);
if (current_text == NULL) {
    slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, warning_text);

}

The text is added in all index function, could it be done at an upper level ?
For example in do_search with something like:

if ((config_get_verify_filter_schema() == FILTER_POLICY_PROTECT) &&
     (r == FILTER_SCHEMA_FAILURE)) {
       slapi_pblock_set_result_text_if_empty(pb, "Invalid attribute in filter - results may not be complete.");
}

This function could be replaced with the following lines. Is it intented ?
slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &current_text);
if (current_text == NULL) {
slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, warning_text);
}

It's intended. It saves entry into the giant case-switch, and it avoids the allocation of the pointer on the stack. It's much cleaner to have it this way IMO.

The text is added in all index function, could it be done at an upper level ?
For example in do_search with something like:
if ((config_get_verify_filter_schema() == FILTER_POLICY_PROTECT) &&
(r == FILTER_SCHEMA_FAILURE)) {
slapi_pblock_set_result_text_if_empty(pb, "Invalid attribute in filter - results may not be complete.");
}

We don't have access to the pb in the schema check, so that eliminates it from an earlier message setup. We also need to consider a search we filter-test-threshold, such as "(&(uid=foo)(notexist=bar))". The results here "aren't" affected because we shortcut and filter tested.

It's only when we return idl_alloc(0) from an index do we need to indicate that the results may not be as intended. Yes that means that the semantics of "(&(notexist=bar)(uid=foo))" and "(&(uid=food)(notexist=bar))" will be different, but that's just the nature of how we apply queries, and we only need to flag that the results aren't as expected in the former.

@firstyear, I do not fully agree with your point.
Using an unknown attribute is dangerous and the admin/client should be aware of it. This even if the server manages to handle optimally the request. Ordering component, look-throughtlimit, require-index, filter-threshold... are internals mechanism to protect the server but when the server detects a risky request it should log/return a warning.

Just to avoid adding the text in many places I suggested a change like the following, that do not need the pblock during schema check:

diff --git a/ldap/servers/slapd/search.c b/ldap/servers/slapd/search.c
index 6cdb27601..2f1ae09aa 100644
--- a/ldap/servers/slapd/search.c
+++ b/ldap/servers/slapd/search.c
@@ -219,6 +219,13 @@ do_search(Slapi_PBlock *pb)
         log_search_access(pb, base, scope, "???", errtxt);
         send_ldap_result(pb, err, NULL, errtxt, 0, NULL);
         goto free_and_return;
+    } else if ((config_get_verify_filter_schema() == FILTER_POLICY_PROTECT) &&
+            (r == FILTER_SCHEMA_WARNING)) {
+        char *current_text;
+        slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &current_text);
+        if (current_text == NULL) {
+            slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, "Invalid attribute in filter - results may not be complete.");
+        }
     }
     /* attributes */

Yeah, actually, you're right. It should always be returned.

I'll update the patch :)

Thanks!

Okay, updated to always send the message when protect is enabled. :)

@firstyear thanks for the changes.
However I think the rebase went wrong, it contains tons of changes and not the ones from this ticket :(
Am I missing something ?

Wow, yeah, something did go wrong. I'll investigate next week, at a conference at the moment.

Okay, I think this fixes it @tbordaz :)

@firstyear, the fix sets the result text into slapi_filter_schema_check so it requires to change the interface to pass 'pb' to the function.
I was suggesting to set result text a step above in do_search (testing slapi_filter_schema_check returned value == FILTER_SCHEMA_WARNING) where 'pb' is already available.
What is the benefit of setting it into slapi_filter_schema_check vs do_search ?

Note that do_search already sets returned text but only if slapi_filter_schema_check returns FILTER_SCHEMA_FAILURE

The benefit is keeping the process "all together" making it logically easier to find when we wonder what is going on in the schema check. I find it nicer to say "okay here is the check process and how the message gets there" rather than layering the message addition in a seperate area to the check.

thanks @firstyear for the explanation, it makes sense. You have my ACK

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3862

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.