From 58efd22cdefed368c97ef235f2abb66ac4aec234 Mon Sep 17 00:00:00 2001 From: tbordaz Date: Thu, 5 Sep 2024 10:19:59 +0200 Subject: [PATCH] Issue 6307 - Wrong set of entries returned for some search filters (#6308) Bug description: When the server returns an entry to a search it checks both access and matching of the filter. When evaluating a '!' (NOT) logical expression the server, in a first phase evaluates ONLY the right to access the related component (and its subcomponents). Then in a second phase verifies the matching. If the related component is a OR, in the first phase it evaluates access AND matching, this even if the call was to evaluate only access. This result in incoherent results. Fix description: Make sure that when the function vattr_test_filter_list_or is called to only check access, it does not evaluate the matching. fixes: #6307 Reviewed by: Pierre Rogier (Thanks !!) --- .../tests/suites/filter/filter_test.py | 116 ++++++++++++++++++ ldap/servers/slapd/filterentry.c | 11 ++ 2 files changed, 127 insertions(+) diff --git a/dirsrvtests/tests/suites/filter/filter_test.py b/dirsrvtests/tests/suites/filter/filter_test.py index b17846c01..e1d02f133 100644 --- a/dirsrvtests/tests/suites/filter/filter_test.py +++ b/dirsrvtests/tests/suites/filter/filter_test.py @@ -10,12 +10,14 @@ import logging import pytest import time +from ldap import SCOPE_SUBTREE from lib389.dirsrv_log import DirsrvAccessLog from lib389.tasks import * from lib389.backend import Backends, Backend from lib389.dbgen import dbgen_users, dbgen_groups from lib389.topologies import topology_st from lib389._constants import PASSWORD, DEFAULT_SUFFIX, DN_DM, SUFFIX, DN_CONFIG_LDBM +from lib389.idm.user import UserAccount, UserAccounts from lib389.utils import * pytestmark = pytest.mark.tier1 @@ -432,6 +434,120 @@ def test_match_large_valueset(topology_st): assert len(entries) == groups_number assert (etime < 5) +def test_filter_not_operator(topology_st, request): + """Test ldapsearch with scope one gives only single entry + + :id: b3711e02-7e76-444d-82f3-495c6dadd97f + :setup: Standalone instance + :steps: + 1. Creating user1..user9 + 2. Adding specific 'telephonenumber' to the users + 3. Check returned set (5 users) with the first filter + 4. Check returned set (4 users) with the second filter + :expectedresults: + 1. This should pass + 2. This should pass + 3. This should pass + 4. This should pass + """ + + topology_st.standalone.start() + # Creating Users + log.info('Create users from user1 to user9') + users = UserAccounts(topology_st.standalone, "ou=people,%s" % DEFAULT_SUFFIX, rdn=None) + + for user in ['user1', + 'user2', + 'user3', + 'user4', + 'user5', + 'user6', + 'user7', + 'user8', + 'user9']: + users.create(properties={ + 'mail': f'{user}@redhat.com', + 'uid': user, + 'givenName': user.title(), + 'cn': f'bit {user}', + 'sn': user.title(), + 'manager': f'uid={user},{SUFFIX}', + 'userpassword': PW_DM, + 'homeDirectory': '/home/' + user, + 'uidNumber': '1000', + 'gidNumber': '2000', + }) + # Adding specific values to the users + log.info('Adding telephonenumber values') + user = UserAccount(topology_st.standalone, 'uid=user1, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234', '2345']) + + user = UserAccount(topology_st.standalone, 'uid=user2, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234', '4567']) + + user = UserAccount(topology_st.standalone, 'uid=user3, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234', '4567']) + + user = UserAccount(topology_st.standalone, 'uid=user4, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234']) + + user = UserAccount(topology_st.standalone, 'uid=user5, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['2345']) + + user = UserAccount(topology_st.standalone, 'uid=user6, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['3456']) + + user = UserAccount(topology_st.standalone, 'uid=user7, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['4567']) + + user = UserAccount(topology_st.standalone, 'uid=user8, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234']) + + user = UserAccount(topology_st.standalone, 'uid=user9, ou=people, %s' % DEFAULT_SUFFIX) + user.add('telephonenumber', ['1234', '4567']) + + # Do a first test of filter containing a NOT + # and check the expected values are retrieved + log.info('Search with filter containing NOT') + log.info('expect user2, user3, user6, user8 and user9') + filter1 = "(|(telephoneNumber=3456)(&(telephoneNumber=1234)(!(|(uid=user1)(uid=user4)))))" + entries = topology_st.standalone.search_s(DEFAULT_SUFFIX, SCOPE_SUBTREE, filter1) + uids = [] + for entry in entries: + assert entry.hasAttr('uid') + uids.append(entry.getValue('uid')) + + assert len(uids) == 5 + for uid in [b'user2', b'user3', b'user6', b'user8', b'user9']: + assert uid in uids + + # Do a second test of filter containing a NOT + # and check the expected values are retrieved + log.info('Search with a second filter containing NOT') + log.info('expect user2, user3, user8 and user9') + filter1 = "(|(&(telephoneNumber=1234)(!(|(uid=user1)(uid=user4)))))" + entries = topology_st.standalone.search_s(DEFAULT_SUFFIX, SCOPE_SUBTREE, filter1) + uids = [] + for entry in entries: + assert entry.hasAttr('uid') + uids.append(entry.getValue('uid')) + + assert len(uids) == 4 + for uid in [b'user2', b'user3', b'user8', b'user9']: + assert uid in uids + + def fin(): + """ + Deletes entries after the test. + """ + for user in users.list(): + pass + user.delete() + + + request.addfinalizer(fin) + + if __name__ == '__main__': # Run isolated # -s for DEBUG mode diff --git a/ldap/servers/slapd/filterentry.c b/ldap/servers/slapd/filterentry.c index d2c7e3082..f5604161d 100644 --- a/ldap/servers/slapd/filterentry.c +++ b/ldap/servers/slapd/filterentry.c @@ -948,14 +948,17 @@ slapi_vattr_filter_test_ext_internal( break; case LDAP_FILTER_NOT: + slapi_log_err(SLAPI_LOG_FILTER, "vattr_test_filter_list_NOT", "=>\n"); rc = slapi_vattr_filter_test_ext_internal(pb, e, f->f_not, verify_access, only_check_access, access_check_done); if (verify_access && only_check_access) { /* dont play with access control return codes * do not negate return code */ + slapi_log_err(SLAPI_LOG_FILTER, "vattr_test_filter_list_NOT only check access", "<= %d\n", rc); break; } if (rc > 0) { /* an error occurred or access denied, don't negate */ + slapi_log_err(SLAPI_LOG_FILTER, "vattr_test_filter_list_NOT slapi_vattr_filter_test_ext_internal fails", "<= %d\n", rc); break; } if (verify_access) { @@ -980,6 +983,7 @@ slapi_vattr_filter_test_ext_internal( /* filter verification only, no error */ rc = (rc == 0) ? -1 : 0; } + slapi_log_err(SLAPI_LOG_FILTER, "vattr_test_filter_list_NOT", "<= %d\n", rc); break; default: @@ -1084,6 +1088,13 @@ vattr_test_filter_list_or( continue; } } + /* we are not evaluating if the entry matches + * but only that we have access to ALL components + * so check the next one + */ + if (only_check_access) { + continue; + } /* now check if filter matches */ /* * We can NOT skip this because we need to know if the item we matched on -- 2.47.1