Fix issues with directory service cache build (#13968)

This fixes the following issues:

    Directory service cache query could return duplicate entries
    Cache query result keys were out of sync with normal user.query and group.query
    Detection of id_type_both was broken

Test coverage is added for the above through the following:

    We were already using set of names returned from cache. Compare set length with original list length.
    Grant privilege for user.query and group.query for our AD privilege tests. Since it's a limited user the output will pass through schema validation and raise error if they get out of sync with expected.
    Check id_type_both is set when using RID backend.

There is still an outstanding issue that new schema has broken account queries for directory services as a limited user account. This will cause (2) to fail until the underlying schema issue is fixed.

src/middlewared/middlewared/plugins/directoryservices_/util_cache.py

@@ -1,6 +1,5 @@
 import enum
 import os
-import typing
 
 from collections import defaultdict
 from collections.abc import Iterable
@@ -33,7 +32,6 @@
 from threading import Lock
 from uuid import uuid4
 
-
 # Update progress of job every nth user / group, we expect possibly hundreds to
 # a few thousand users and groups, but some edge cases where they number in
 # tens of thousands. Percentage complete is not updated when generating
@@ -157,6 +155,8 @@ def _add_sid_info_to_entries(
             entry['id_type'] = idmap_entry['id_type']
             if dom_by_sid:
                 entry['domain_info'] = dom_by_sid[idmap_entry['sid'].rsplit('-', 1)[0]]
+            else:
+                entry['domain_info'] = None
 
         to_remove.reverse()
         for idx in to_remove:
@@ -236,6 +236,10 @@ def fill_cache(
         ):
             # Now iterate members of 100 for insertion
             for u in users:
+                if u['domain_info']:
+                    id_type_both = u['domain_info']['idmap_backend'] in ('AUTORID', 'RID')
+                else:
+                    id_type_both = False
 
                 user_data = u['nss']
                 entry = {
@@ -257,11 +261,14 @@ def fill_cache(
                     'attributes': {},
                     'groups': [],
                     'sshpubkey': None,
+                    'immutable': True,
+                    'two_factor_auth_configured': False,
                     'local': False,
-                    'id_type_both': u['id_type'] == 'BOTH',
+                    'id_type_both': id_type_both,
                     'nt_name': user_data.pw_name,
                     'smb': u['sid'] is not None,
                     'sid': u['sid'],
+                    'roles': []
                 }
 
                 if user_count % LOG_CACHE_ENTRY_INTERVAL == 0:
@@ -280,6 +287,11 @@ def fill_cache(
             dom_by_sid
         ):
             for g in groups:
+                if g['domain_info']:
+                    id_type_both = g['domain_info']['idmap_backend'] in ('AUTORID', 'RID')
+                else:
+                    id_type_both = False
+
                 group_data = g['nss']
                 entry = {
                     'id': BASE_SYNTHETIC_DATASTORE_ID + group_data.gr_gid,
@@ -291,10 +303,11 @@ def fill_cache(
                     'sudo_commands_nopasswd': [],
                     'users': [],
                     'local': False,
-                    'id_type_both': g['id_type'] == 'BOTH',
+                    'id_type_both': id_type_both,
                     'nt_name': group_data.gr_name,
                     'smb': g['sid'] is not None,
                     'sid': g['sid'],
+                    'roles': []
                 }
 
                 if group_count % LOG_CACHE_ENTRY_INTERVAL == 0:
@@ -371,4 +384,4 @@ def query_cache_entries(
     options: dict
 ) -> list:
     with get_tdb_handle(DSCacheFile[id_type.name].value, CACHE_OPTIONS) as handle:
-        return filter_list(handle.entries(include_keys=False), filters, options)
+        return filter_list(handle.entries(include_keys=False, key_prefix='ID_'), filters, options)

src/middlewared/middlewared/plugins/idmap.py

@@ -607,7 +607,7 @@ async def idmap_conf_to_client_config(self, data):
     async def query(self, filters, options):
         extra = options.get("extra", {})
         more_info = extra.get("additional_information", [])
-        ret = await super().query(filters, options)
+        ret = await super().query()
         if 'DOMAIN_INFO' in more_info:
             for entry in ret:
                 try:
@@ -622,7 +622,7 @@ async def query(self, filters, options):
 
                 entry.update({'domain_info': domain_info})
 
-        return ret
+        return filter_list(ret, filters, options)
 
     @accepts(Dict(
         'idmap_domain_create',
@@ -1188,6 +1188,9 @@ async def synthetic_user(self, passwd, sid):
             'sshpubkey': None,
             'local': False,
             'id_type_both': id_type_both,
+            'roles': [],
+            'two_factor_auth_configured': False,
+            'immutable': True,
             'smb': True,
             'sid': sid
         }
@@ -1214,6 +1217,7 @@ async def synthetic_group(self, grp, sid):
             'users': [],
             'local': False,
             'id_type_both': id_type_both,
+            'roles': [],
             'smb': True,
             'sid': sid
         }

src/middlewared/middlewared/utils/tdb.py

@@ -204,7 +204,7 @@ def clear(self) -> None:
         """
         self.hdl.clear()
 
-    def entries(self, include_keys: bool = True) -> Iterable[dict]:
+    def entries(self, include_keys: bool = True, key_prefix: str = None) -> Iterable[dict]:
         """
         Iterate entries in TDB file:
 
@@ -221,6 +221,9 @@ def entries(self, include_keys: bool = True) -> Iterable[dict]:
             if self.keys_null_terminated:
                 tdb_key = tdb_key[:-1]
 
+            if key_prefix and not tdb_key.startswith(key_prefix):
+                continue
+
             tdb_val = self.get(tdb_key)
             if include_keys:
                 yield {

tests/api2/test_030_activedirectory.py

@@ -340,14 +340,22 @@ def test_10_account_privilege_authentication(request, set_product_type):
 
     with active_directory(dns_timeout=15):
         call("system.general.update", {"ds_auth": True})
+        nusers = call("user.query", [["local", "=", False]], {"count": True})
+        assert nusers > 0
+        ngroups = call("group.query", [["local", "=", False]], {"count": True})
+        assert ngroups > 0
         try:
             # RID 513 is constant for "Domain Users"
             domain_sid = call("idmap.domain_info", AD_DOMAIN.split(".")[0])['sid']
             with privilege({
                 "name": "AD privilege",
                 "local_groups": [],
                 "ds_groups": [f"{domain_sid}-513"],
-                "allowlist": [{"method": "CALL", "resource": "system.info"}],
+                "allowlist": [
+                    {"method": "CALL", "resource": "system.info"},
+                    {"method": "CALL", "resource": "user.query"},
+                    {"method": "CALL", "resource": "group.query"},
+                ],
                 "web_shell": False,
             }):
                 with client(auth=(f"limiteduser@{AD_DOMAIN}", ADPASSWORD)) as c:
@@ -357,6 +365,9 @@ def test_10_account_privilege_authentication(request, set_product_type):
                     assert 'DIRECTORY_SERVICE' in me['account_attributes']
                     assert 'ACTIVE_DIRECTORY' in me['account_attributes']
 
+                    assert len(c.call("user.query", [["local", "=", False]])) == nusers
+                    assert len(c.call("group.query", [["local", "=", False]])) == ngroups
+
                 assert "system.info" in methods
                 assert "pool.create" not in methods
 

tests/api2/test_040_ad_user_group_cache.py

@@ -28,15 +28,21 @@ def do_ad_connection(request):
         if cache_fill_job['state'] == 'RUNNING':
             call('core.job_wait', cache_fill_job['id'], job=True)
 
-        users = set([x['username'] for x in call(
+        users = [x['username'] for x in call(
             'user.query', [['local', '=', False]],
-        )])
+        )]
 
-        groups = set([x['name'] for x in call(
+        set_users = set(users)
+        assert len(set_users) == len(users)
+
+        groups = [x['name'] for x in call(
             'group.query', [['local', '=', False]],
-        )])
+        )]
+
+        set_groups = set(groups)
+        assert len(set_groups) == len(groups)
 
-        yield ad | {'users': users, 'groups': groups}
+        yield ad | {'users': set_users, 'groups': set_groups}
 
 
 def get_ad_user_and_group(ad_connection):
@@ -132,6 +138,12 @@ def test_check_lazy_initialization_of_users_and_groups_by_name(do_ad_connection)
 
     ad_user, ad_group = get_ad_user_and_group(do_ad_connection)
 
+    assert ad_user['id_type_both'] is True
+    assert ad_user['immutable'] is True
+    assert ad_user['local'] is False
+    assert ad_group['id_type_both'] is True
+    assert ad_group['local'] is False
+
     cache_names = set([x['username'] for x in call(
         'user.query', [['local', '=', False]],
     )])