From bd95db76091f649b535f04bbec880af5e4d0cfba Mon Sep 17 00:00:00 2001 From: Ralf Becker Date: Wed, 19 May 2021 18:26:40 +0200 Subject: [PATCH] * AD: fix account-selection type "groupmembers" caused high load on AD also caching groups now on instance level instead of session --- api/src/Accounts.php | 55 ++++++++-- api/src/Accounts/Ads.php | 219 ++++++++++++++++++++------------------- 2 files changed, 159 insertions(+), 115 deletions(-) diff --git a/api/src/Accounts.php b/api/src/Accounts.php index 3753124cbe..ab2f108a66 100644 --- a/api/src/Accounts.php +++ b/api/src/Accounts.php @@ -246,7 +246,21 @@ class Accounts $account_search = &self::$cache['account_search']; $serial = self::cacheKey($param, $serial_unlimited); - if (isset($account_search[$serial])) + // cache list of all groups on instance level (not session) + if ($serial_unlimited === self::cacheKey(['type'=>'groups','active'=>true])) + { + $result = Cache::getCache($this->config['install_id'], __CLASS__, 'groups', function() use ($param) + { + return $this->backend->search($param); + }, [], self::READ_CACHE_TIMEOUT); + $this->total = count($result); + if (!empty($param['offset'])) + { + return array_slice($result, $param['start'], $param['offset']); + } + return $result; + } + elseif (isset($account_search[$serial])) { $this->total = $account_search[$serial]['total']; } @@ -840,7 +854,7 @@ class Accounts self::setup_cache(); $name_list = &self::$cache['name_list']; - if(@isset($name_list[$which][$name]) && $name_list[$which][$name]) + if (isset($name_list[$which][$name])) { return $name_list[$which][$name]; } @@ -1353,22 +1367,30 @@ class Accounts { //error_log(__METHOD__.'('.array2string($account_ids).')'); + $instance = self::getInstance(); + // instance-wide cache + $invalidate_groups = !$account_ids; if ($account_ids) { - $instance = self::getInstance(); - foreach((array)$account_ids as $account_id) { Cache::unsetCache($instance->config['install_id'], __CLASS__, 'account-'.$account_id); unset(self::$request_cache[$account_id]); + + if ($account_id < 0) $invalidate_groups = true; } } else { self::$request_cache = array(); } + // invalidate instance-wide all-groups cache + if ($invalidate_groups) + { + Cache::unsetCache($instance->config['install_id'], __CLASS__, 'groups'); + } // session-cache if (self::$cache) self::$cache = array(); @@ -1397,10 +1419,11 @@ class Accounts * * @param int $account_id * @param boolean $need_active =false true = 'members-active' required - * @return array + * @param boolean $return_not_cached =false true: return null if nothing is cached + * @return array or null if nothing is cached * @throws Exception\WrongParameter if no integer was passed as $account_id */ - static function cache_read($account_id, $need_active=false) + static function cache_read($account_id, bool $need_active=false, bool $return_not_cached=false) { if (!is_numeric($account_id)) throw new Exception\WrongParameter('Not an integer!'); @@ -1414,6 +1437,10 @@ class Accounts if (!isset($account)) // not in instance cache --> read from backend { + if ($return_not_cached) + { + return null; + } if (($account = $instance->backend->read($account_id))) { if ($instance->get_type($account_id) == 'u') @@ -1424,7 +1451,7 @@ class Accounts { if (!isset($account['members'])) $account['members'] = $instance->backend->members($account_id); } - Cache::setCache($instance->config['install_id'], __CLASS__, 'account-'.$account_id, $account, self::READ_CACHE_TIMEOUT); + $instance->cache_data($account_id, $account); } //error_log(__METHOD__."($account_id) read from backend ".array2string($account)); } @@ -1445,6 +1472,20 @@ class Accounts return $account; } + /** + * Cache account read by backend (incl. members or memberships!) + * + * Can be used by backends too, to inject accounts into the cache. + * + * @param int $account_id + * @param array $account + * @throws Exception\WrongParameter + */ + public function cache_data(int $account_id, array $account) + { + Cache::setCache($this->config['install_id'], __CLASS__, 'account-'.$account_id, $account, self::READ_CACHE_TIMEOUT); + } + /** * Internal functions not meant to use outside this class!!! */ diff --git a/api/src/Accounts/Ads.php b/api/src/Accounts/Ads.php index fa01bafaa4..dad69e237e 100644 --- a/api/src/Accounts/Ads.php +++ b/api/src/Accounts/Ads.php @@ -881,8 +881,6 @@ class Ads /** * Searches / lists accounts: users and/or groups * - * @todo sort and limit query on AD, PHP5.4 and AD support it - * * @param array with the following keys: * @param $param['type'] string/int 'accounts', 'groups', 'owngroups' (groups the user is a member of), 'both' * or integer group-id for a list of members of that group @@ -904,122 +902,99 @@ class Ads function search($param) { //error_log(__METHOD__.'('.json_encode($param).') '.function_backtrace()); - $account_search = []; // disabled, we have sorted&limited queries now &$this->cache['account_search']; - - // check if the query is cached - $serial = Api\Accounts::cacheKey($param, $unl_serial); - if (isset($account_search[$serial])) - { - $this->total = $account_search[$serial]['total']; - return $account_search[$serial]['data']; - } - // if it's a limited query, check if the unlimited query is cached $start = $param['start']; if (!($maxmatchs = $GLOBALS['egw_info']['user']['preferences']['common']['maxmatchs'])) $maxmatchs = 15; if (!($offset = $param['offset'])) $offset = $maxmatchs; - unset($param['start']); - unset($param['offset']); - if (isset($account_search[$unl_serial])) + $this->total = null; + $query = Api\Ldap::quote(strtolower($param['query'])); + + $accounts = array(); + if($param['type'] !== 'groups') { - $this->total = $account_search[$unl_serial]['total']; - $sortedAccounts = $account_search[$unl_serial]['data']; + if (!empty($query) && $query != '*') + { + switch($param['query_type']) + { + case 'all': + default: + $query = '*'.$query; + // fall-through + case 'start': + $query .= '*'; + // fall-through + case 'exact': + $filter = "(|(samaccountname=$query)(sn=$query)(cn=$query)(givenname=$query)(mail=$query))"; + break; + case 'firstname': + case 'lastname': + case 'lid': + case 'email': + static $to_ldap = array( + 'firstname' => 'givenname', + 'lastname' => 'sn', + 'lid' => 'samaccountname', + 'email' => 'mail', + ); + $filter = '('.$to_ldap[$param['query_type']].'=*'.$query.'*)'; + break; + } + } + if (is_numeric($param['type'])) + { + $membership_filter = '(|(memberOf='.$this->id2name((int)$param['type'], 'account_dn').')(PrimaryGroupId='.abs($param['type']).'))'; + $filter = $filter ? "(&$membership_filter$filter)" : $membership_filter; + } + foreach($this->filter($filter, 'u', self::$user_attributes, [], $param['active'], $param['order'].' '.$param['sort'], $start, $offset, $this->total) as $account_id => $data) + { + $account = $this->_ldap2user($data); + $account['account_fullname'] = Api\Accounts::format_username($account['account_lid'],$account['account_firstname'],$account['account_lastname'],$account['account_id']); + $accounts[$account_id] = $account; + } } - else // we need to run the unlimited query + if ($param['type'] === 'groups' || $param['type'] === 'both') { - $this->total = null; $query = Api\Ldap::quote(strtolower($param['query'])); - $accounts = array(); - if($param['type'] !== 'groups') + $filter = null; + if(!empty($query) && $query != '*') { - if (!empty($query) && $query != '*') + switch($param['query_type']) { - switch($param['query_type']) - { - case 'all': - default: - $query = '*'.$query; - // fall-through - case 'start': - $query .= '*'; - // fall-through - case 'exact': - $filter = "(|(samaccountname=$query)(sn=$query)(cn=$query)(givenname=$query)(mail=$query))"; - break; - case 'firstname': - case 'lastname': - case 'lid': - case 'email': - static $to_ldap = array( - 'firstname' => 'givenname', - 'lastname' => 'sn', - 'lid' => 'samaccountname', - 'email' => 'mail', - ); - $filter = '('.$to_ldap[$param['query_type']].'=*'.$query.'*)'; - break; - } - } - if (is_numeric($param['type'])) - { - $membership_filter = '(|(memberOf='.$this->id2name((int)$param['type'], 'account_dn').')(PrimaryGroupId='.abs($param['type']).'))'; - $filter = $filter ? "(&$membership_filter$filter)" : $membership_filter; - } - foreach($this->filter($filter, 'u', self::$user_attributes, [], $param['active'], $param['order'].' '.$param['sort'], $start, $offset, $this->total) as $account_id => $data) - { - $account = $this->_ldap2user($data); - $account['account_fullname'] = Api\Accounts::format_username($account['account_lid'],$account['account_firstname'],$account['account_lastname'],$account['account_id']); - $accounts[$account_id] = $account; + case 'all': + default: + $query = '*'.$query; + // fall-through + case 'start': + $query .= '*'; + // fall-through + case 'exact': + break; } + $filter = "(|(cn=$query)(description=$query))"; } - if ($param['type'] === 'groups' || $param['type'] === 'both') + foreach($this->filter($filter, 'g', self::$group_attributes) as $account_id => $data) { - $query = Api\Ldap::quote(strtolower($param['query'])); - - $filter = null; - if(!empty($query) && $query != '*') - { - switch($param['query_type']) - { - case 'all': - default: - $query = '*'.$query; - // fall-through - case 'start': - $query .= '*'; - // fall-through - case 'exact': - break; - } - $filter = "(|(cn=$query)(description=$query))"; - } - foreach($this->filter($filter, 'g', self::$group_attributes) as $account_id => $data) - { - $accounts[$account_id] = $this->_ldap2group($data); - } + $accounts[$account_id] = $this->_ldap2group($data); } - // sort the array - $this->_callback_sort = strtoupper($param['sort']); - $this->_callback_order = empty($param['order']) ? array('account_lid') : explode(',',$param['order']); - foreach($this->_callback_order as &$col) - { - if (substr($col, 0, 8) !== 'account_') $col = 'account_'.$col; - } - $sortedAccounts = $accounts; - uasort($sortedAccounts,array($this,'_sort_callback')); - $account_search[$unl_serial]['data'] = $sortedAccounts; - - $account_search[$unl_serial]['total'] = $this->total = $this->total ?? count($accounts); } + // sort the array + $this->_callback_sort = strtoupper($param['sort']); + $this->_callback_order = empty($param['order']) ? array('account_lid') : explode(',',$param['order']); + foreach($this->_callback_order as &$col) + { + if (substr($col, 0, 8) !== 'account_') $col = 'account_'.$col; + } + $sortedAccounts = $accounts; + uasort($sortedAccounts,array($this,'_sort_callback')); + + $this->total = $this->total ?? count($accounts); + // return only the wanted accounts reset($sortedAccounts); if(is_numeric($start) && is_numeric($offset)) { - $account_search[$serial]['data'] = array_slice($sortedAccounts, $start, $offset); - $account_search[$serial]['total'] = $this->total; - //error_log(__METHOD__.'('.array2string($param).") returning $offset/$this->total entries from $start ".array2string($account_search[$serial]['data'])); - return $account_search[$serial]['data']; + return array_slice($sortedAccounts, $start, $offset); } //error_log(__METHOD__.'('.array2string($param).') returning all '.array2string($sortedAccounts)); return $sortedAccounts; @@ -1093,15 +1068,39 @@ class Ads isset($GLOBALS['egw_info']['user']['account_id']) && // only use groupmembers, if we have a user context (stalls login otherwise!) (!isset($GLOBALS['egw_info']['user']['apps']['admin']))) { - $type_filter .= '(|'; - foreach($GLOBALS['egw']->accounts->memberships($GLOBALS['egw_info']['user']['account_id'],true) as $group_id) + // generate groupmembers filter once per session without any external calls (but groups) to NOT generate a deep recursion! + $type_filter .= Api\Cache::getSession(__CLASS__, 'groupmembers_filter', function() { - if (!in_array($group_id, $this->ignore_membership) && ($dn = Api\Accounts::id2name($group_id, 'account_dn'))) + $account_id = $GLOBALS['egw_info']['user']['account_id']; + $account = $this->filter(['objectSid' => $this->get_sid($account_id)], + false, ['memberOf','primaryGroupID','objectSid','samAccountType'])[$account_id]; + $filter = '(|'; + $primary_ignored = in_array(-$account['primarygroupid'][0], $this->ignore_membership); + foreach($account['memberof'] as $key => $dn) { - $type_filter .= '(memberOf='.$dn.')(primaryGroupID='.abs($group_id).')'; + if ($key === 'count') continue; + // if the primary group of the user is ignored (=Domain Users), we assume it's the case for everyone + if ($primary_ignored) + { + $filter .= '(memberOf='.$dn.')'; + continue; + } + if (!isset($groups)) + { + $groups = $this->frontend->search(['type' => 'groups']); + } + foreach($groups as $gid => $group) + { + if (($gid == -$account['primarygroupid'][0] || $dn === $group['account_dn']) && + !in_array($gid, $this->ignore_membership)) + { + $filter .= '(memberOf='.$group['account_dn'].')(primaryGroupID='.(-$gid).')'; + break; + } + } } - } - $type_filter .= ')'; + return $filter.')'; + }); } $type_filter .= ')'; if ($account_type === 'u') break; @@ -1167,7 +1166,7 @@ class Ads * All reading ADS queries are done throught this methods. * * @param string|array $attr_filter array with attribute => value pairs or filter string or empty - * @param string $account_type u = user, g = group, default null = try both + * @param string|false $account_type u = user, g = group, default null = try both, false: no type_filter! * @param array $attrs =null default return account_lid, else return raw values from ldap-query * @param array $accounts =array() array to add filtered accounts too, default empty array * @param bool $filter_expired =false true: filter out expired users @@ -1222,7 +1221,11 @@ class Ads $filter .= '('.$attr.'='.$this->adldap->utilities()->ldapSlashes($value).')'; } } - $filter .= $this->type_filter($account_type).')'; + if ($account_type !== false) + { + $filter .= $this->type_filter($account_type); + } + $filter .= ')'; } if (!($sri = ldap_search($ds=$this->ldap_connection(), $context=$this->ads_context(), $filter, $attrs ? $attrs : self::$default_attributes, null, null, null, null, $control))) @@ -1305,9 +1308,9 @@ class Ads * * Calls frontend which uses (cached) read method to fetch all data by account_id. * - * @param int $account_id numerica account_id + * @param int $account_id numeric account_id * @param string $which ='account_lid' type to convert to: account_lid (default), account_email, ... - * @return string/false converted value or false on error ($account_id not found) + * @return string|false converted value or false on error ($account_id not found) */ public function id2name($account_id, $which='account_lid') {