Add logs in case some users are ignored and aren't returned #104
Conversation
| $this->access->connection->writeToCache($cachekey, $ldap_users); | ||
| return $ldap_users; | ||
| $this->access->connection->writeToCache($cachekey, $ldap_users2); | ||
| return $ldap_users2; |
There was a problem hiding this comment.
It would be nice to find a way to both:
- return just the "good" subset of users (like here); and
- return the count of all users in this "batch" $ldap_users_count (or a boolean to say if $limit users were found)
So that the caller can know when this may not be the last "batch".
There was a problem hiding this comment.
It doesn't seem possible. Core asks for 500 users and LDAP is returning less. Returning more information from LDAP is useless at the moment because that info won't be used by core.
On the other hand, core needs to handle other backends. The condition to decide whether keep fetching users or not is pretty clear: if core asks for 500 users and 500 users are returned, there could be more users, otherwise it's the last bunch. This way we don't need to count how many users there are, since this is time-consuming specially for LDAP (we need to fetch all the entries)
In addition, if someone asks core for 500 users it should return 500 users. This means that if the backend returns less, core should ask for more. For LDAP, LDAP will need to fetch the next 500 users even though we might only need a few of them (I don't think we can change the page size during a search) which will require complex handling.
There was a problem hiding this comment.
I guess that if this method gets 500 users and only 498 of them are "good" then it could query again for the next users, and put the first 2 good ones into the array to return to the caller.
But then when it is called in future with some offset, it needs to know how to adjust that to also skip the "non-good" users that were in previous batches.
And of course this whole thing can be a problem if users are being added or deleted on the LDAP server between query batches - then where the oC end thinks it is up to will be out-of-alignment with LDAPs current set of users.
There was a problem hiding this comment.
with the account table getUsers is only used during sync and during login via saml / shibboleth. for the sync case the OutOfBoundsExceptions are a better way to notify core of preblems with accounts. For the saml login this also works, so I'll close this PR.
|
@jvillafanez finish this ? add review label ? |
|
nothing more to add |
lib/User_LDAP.php
Outdated
| $ldap_users_count = count($ldap_users); | ||
| $ldap_users2_count = count($ldap_users2); | ||
| if ($ldap_users_count !== $ldap_users2_count) { | ||
| \OCP\Util::writeLog('user_ldap', "getUsers: fetched $ldap_users_count users but returned $ldap_users2_count", \OCP\Util::ERROR); |
There was a problem hiding this comment.
I guess we need better message here - i doubt that anybody can do anything reasonable based on this message.
Me personally have no clue on what to do ..... 😕
There was a problem hiding this comment.
The new log from the lib/Access file should also pop up, which should help to identify the missing entries. Anyway, I'm opened to suggestions.
There was a problem hiding this comment.
'LDAP unable to find user mappings for all the users returned by the search'
|
@jvillafanez can you rebase and update the comment and then we can get this in :) |
|
@DeepDiver1975 new review. And looks like it needs a rebase @jvillafanez |
|
Rebased |
|
@DeepDiver1975 @tomneedham second review ? |
|
Rebasing.. |
|
During rebase I realised that the getUsers method has been rewritten - @jvillafanez can you reevaluate whether this PR is still necessary and if so where the code should now reside? |
| @@ -651,6 +651,8 @@ private function ldap2ownCloudNames($ldapObjects, $isUsers) { | |||
| ? $ldapObject[$sndAttribute][0] : ''; | |||
| $this->cacheUserDisplayName($ocName, $nameByLDAP, $sndName); | |||
| } | |||
| } else { | |||
| Util::writeLog('user_ldap', "Setting the owncloud name for LDAP entry {$ldapObject['dn'][0]} isn't possible", Util::ERROR); | |||
There was a problem hiding this comment.
logging now done when the OutOfBounds exception in https://github.com/owncloud/user_ldap/blob/master/lib/User/UserEntry.php#L106-L110 is finally logged. Also see owncloud/core#28805 to allow skipping of "broken" users during sync
Log more errors for owncloud/core#28039