search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.37k stars 2.06k forks source link

Avatar gets updated at every login with LDAP #25529

Open PVince81 opened 8 years ago

PVince81 commented 8 years ago

Steps

  1. Setup LDAP with avatars and email address
  2. Do a curl operation with basic auth
  3. Debug into the avatar code

    Expected result

Avatar only updated once

Actual result

Avatar updated for every login. This causes file operations like getDirectoryContents to delete all avatars and re-set the new one.

This should be debounced and done only once an hour or so, or whichever LDAP TTL is set.

This can make the connection slower for clients that do not support sessions, like Windows Webdav mounts or Linux file managers.

@jvillafanez @DeepDiver1975

PVince81 commented 8 years ago

CC @butonic

PVince81 commented 8 years ago

Stack leading there:

0  OC\Avatar->remove() /srv/www/htdocs/owncloud/lib/private/avatar.php:144
1  OC\Avatar->set() /srv/www/htdocs/owncloud/lib/private/avatar.php:126
2  OCA\user_ldap\lib\user\User->setOwnCloudAvatar() /srv/www/htdocs/owncloud/apps/user_ldap/lib/user/user.php:522
3  OCA\user_ldap\lib\user\User->updateAvatar() /srv/www/htdocs/owncloud/apps/user_ldap/lib/user/user.php:494
4  OCA\user_ldap\lib\user\User->updateAvatarPostLogin() /srv/www/htdocs/owncloud/apps/user_ldap/lib/user/user.php:476
5  call_user_func:{/srv/www/htdocs/owncloud/lib/private/hook.php:105}() /srv/www/htdocs/owncloud/lib/private/hook.php:105
6  OC_Hook::emit() /srv/www/htdocs/owncloud/lib/private/hook.php:105
7  OC\Server->OC\{closure}() /srv/www/htdocs/owncloud/lib/private/server.php:237
8  call_user_func_array:{/srv/www/htdocs/owncloud/lib/private/hooks/emittertrait.php:98}() /srv/www/htdocs/owncloud/lib/private/hooks/emittertrait.php:98
9  OC\Hooks\BasicEmitter->emit() /srv/www/htdocs/owncloud/lib/private/hooks/emittertrait.php:98
10 OC\Hooks\PublicEmitter->emit() /srv/www/htdocs/owncloud/lib/private/hooks/publicemitter.php:32
11 OC\User\Session->login() /srv/www/htdocs/owncloud/lib/private/user/session.php:225
12 OCA\DAV\Connector\Sabre\Auth->validateUserPass() /srv/www/htdocs/owncloud/apps/dav/lib/connector/sabre/auth.php:106
13 Sabre\DAV\Auth\Backend\AbstractBasic->check() /srv/www/htdocs/owncloud/3rdparty/sabre/dav/lib/DAV/Auth/Backend/AbstractBasic.php:105
14 OCA\DAV\Connector\Sabre\Auth->auth() /srv/www/htdocs/owncloud/apps/dav/lib/connector/sabre/auth.php:220
15 OCA\DAV\Connector\Sabre\Auth->check() /srv/www/htdocs/owncloud/apps/dav/lib/connector/sabre/auth.php:127
16 Sabre\DAV\Auth\Plugin->beforeMethod() /srv/www/htdocs/owncloud/3rdparty/sabre/dav/lib/DAV/Auth/Plugin.php:166
17 call_user_func_array:{/srv/www/htdocs/owncloud/3rdparty/sabre/event/lib/EventEmitterTrait.php:105}() /srv/www/htdocs/owncloud/3rdparty/sabre/event/lib/EventEmitterTrait.php:105
18 Sabre\Event\EventEmitter->emit() /srv/www/htdocs/owncloud/3rdparty/sabre/event/lib/EventEmitterTrait.php:105
19 Sabre\DAV\Server->invokeMethod() /srv/www/htdocs/owncloud/3rdparty/sabre/dav/lib/DAV/Server.php:446
20 Sabre\DAV\Server->exec() /srv/www/htdocs/owncloud/3rdparty/sabre/dav/lib/DAV/Server.php:248
21 require_once()  /srv/www/htdocs/owncloud/apps/dav/appinfo/v1/webdav.php:55
22 {main}          /srv/www/htdocs/owncloud/remote.php:138
PVince81 commented 7 years ago

might be obsoleted when we moved the avatar out of the data folder, needs rechecking

jvillafanez commented 7 years ago

The avatar should be updated at each login. Update doesn't mean set, which is what is happening.

The process should be: fetch - compare against what we have - set new image if needed.

PVince81 commented 6 years ago

Likely obsoleted by user:sync

butonic commented 6 years ago

still fetched from ldap on every login ... don't know about the updating though ...

PVince81 commented 6 years ago

Is this a core thing and would apply to all user backends or specific to user_ldap ?

ownclouders commented 6 years ago

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

PVince81 commented 6 years ago

@tomneedham did you spot anything during your recent trip through avatar code ?

tomneedham commented 6 years ago

https://github.com/owncloud/core/blob/ce4a435ed27db6f1ad4d123271898ea89be99744/lib/private/Avatar.php#L103 we should do some checking here. At the moment this would probably cause it to be set everytime whereas we could do some comparisons. Maybe even in the ldap app check if it is different.

butonic commented 6 years ago

so yes. during login the avatar will be fetched from ldap to update the avatar. hm but this means that using windows net mount will hammer the ldap server ...

I actually would prefer to add an option to force app passwords when accessing webdav. that way ldap serlers won't be hammered. Hm, we still need to check if the user is allowed to login. Otherwise a user might still be able to use his app password even while his account has been disabled.

Maybe introduce a dedicated check for this?

tomneedham commented 6 years ago

token login (used for app passwords right?) has a dedicated isEnabled check https://github.com/owncloud/core/blob/f02931c9d584a3cb2bb87d600241e0e5cd5a0f2e/lib/private/User/Session.php#L543

butonic commented 6 years ago

@tomneedham but does that walk all the way to the ldap server? I don't think so ... and as @PVince81 said we might want to debounce it to prevent hammering the ldap server.

tomneedham commented 6 years ago

isEnabled is a flag in owncloud - not supplied by the user backend? confused.

butonic commented 6 years ago

nope: https://github.com/owncloud/core/blob/2378fa213e5cb2e60640d3ab444f21a67c145608/lib/private/User/User.php#L370-L372

comes from the account table.

Disabling accounts in ldap is not straight forward, because it can be done in multiple ways.

Currently, the sync detecs accounts that are in oc but that vanished in ldap and either deletes or disables them. We should detect that between user syncs ... well single user sync could also help. But a cheap periodic check would be better imo. Depends on policies I guess.

butonic commented 6 years ago

it is worse. the validate session call triggers a checkPassword call when the token credentials are checked (every 5 min) ... which in the ldap app not only registers a hook to update the avatar but it will also request the avatar picture from ldap. this happens for the webdav calls, as well as the notification ajax calls.

checkPassword actually fetches all attributes necessary to fill the UserEntry ... that includes the avatar. effectively sycing the user metadata every 5 min https://github.com/owncloud/core/blob/8c44ae60fb4a7a5ed0b0844ee1d3a2151fc2f1cb/lib/private/User/Session.php#L712-L752

soo ther is some debounce in place ... hmmm

PVince81 commented 6 years ago

moving to backlog