New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converter::updateCard unconditionally *adds* image to user vCard #26242

Closed
shtrom opened this Issue Sep 28, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@shtrom
Contributor

shtrom commented Sep 28, 2016

Steps to reproduce

  1. use the user_ldap plugin (which updates the avatar from LDAP after each login: https://github.com/owncloud/user_ldap/blob/master/lib/User/User.php#L474-L483)
  2. set a user image in LDAP
  3. watch as the PHOTO field is repeatedly added to the user's vCard blob in oc_cards, until the database can hold no more (triggering doctrine/dbal#2523 in passing)

Expected behaviour

Only one PHOTO field is present in the vCard

Actual behaviour

Duplicate PHOTO fields keep getting added to the user's vCard

Cause

Converter::updateCard unconditionally adds the given photo to the vCard, rather than replace it:

if($this->propertyNeedsUpdate($vCard, 'PHOTO', $image)) {
$vCard->add('PHOTO', $image->data(), ['ENCODING' => 'b', 'TYPE' => $image->mimeType()]);
$updated = true;
}
; probably a missing unset($vCard->PHOTO);?

Server configuration

Operating system: OpenBSD 6.0

Web server: apache-httpd-2.4.23

Database: MySQL (mariadb-server-10.0.25p0v1)

PHP version: php-5.6.23p0

ownCloud version: (see ownCloud admin page) 9.0.2

Updated from an older ownCloud or fresh install: updated

Where did you install ownCloud from: OpenBSD ports

shtrom added a commit to shtrom/owncloud-core that referenced this issue Sep 28, 2016

Unset user PHOTO before setting new one in OCA\DAV\CardDAV\Converter:…
…:updateCard

Signed-off-by: Olivier Mehani <shtrom@ssji.net>

#26242

@DeepDiver1975 DeepDiver1975 added this to the 9.0.6 milestone Sep 29, 2016

@DeepDiver1975 DeepDiver1975 added the bug label Sep 29, 2016

DeepDiver1975 added a commit that referenced this issue Sep 29, 2016

Unset user PHOTO before setting new one in OCA\DAV\CardDAV\Converter:…
…:updateCard (#26243)

Signed-off-by: Olivier Mehani <shtrom@ssji.net>

#26242

DeepDiver1975 added a commit that referenced this issue Sep 29, 2016

[stable9.1] Unset user PHOTO before setting new one in OCA\DAV\CardDA…
…V\Converter::updateCard (#26243)

Signed-off-by: Olivier Mehani <shtrom@ssji.net>

#26242

DeepDiver1975 added a commit that referenced this issue Sep 30, 2016

[stable9.1] Unset user PHOTO before setting new one in OCA\DAV\CardDA…
…V\Converter::updateCard (#26243) (#26246)

Signed-off-by: Olivier Mehani <shtrom@ssji.net>

#26242
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Oct 4, 2016

Member

all prs got merged - THX

Member

DeepDiver1975 commented Oct 4, 2016

all prs got merged - THX

@fmkaiser

This comment has been minimized.

Show comment
Hide comment
@fmkaiser

fmkaiser Jan 20, 2017

@DeepDiver1975 sorry to re-open this, but it appears the fix was not backported to 9.0 despite being mentioned in the release notes.

We just stumbled across this issue again in 9.0.6 EE.

fmkaiser commented Jan 20, 2017

@DeepDiver1975 sorry to re-open this, but it appears the fix was not backported to 9.0 despite being mentioned in the release notes.

We just stumbled across this issue again in 9.0.6 EE.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 20, 2017

Member

This is true - just a sec ....

Member

DeepDiver1975 commented Jan 20, 2017

This is true - just a sec ....

DeepDiver1975 added a commit that referenced this issue Jan 20, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 20, 2017

Member

@fmkaiser here we go - please test - THX

#26991

Member

DeepDiver1975 commented Jan 20, 2017

@fmkaiser here we go - please test - THX

#26991

@fmkaiser

This comment has been minimized.

Show comment
Hide comment
@fmkaiser

fmkaiser Jan 20, 2017

@DeepDiver1975 I applied the fix on our servers (on top of 9.0.6 EE).

Looks good so far, but I'd like to watch it for a bit longer and will come back to you on Monday.

Our case was a bit differnet than described in the initial report.
Same outcome - oc_cards.carddata grows with more and more PHOTO objects until MySQL max_allowed_packet is reached, then the user can't log in anymore.
However while we do use LDAP, we don't have any user images in it, the users that were affected uploaded their avatar in ownCloud.

My guess is that the avatar somehow still gets written to the DB on LDAP sync, so it is likely the same bug.

fmkaiser commented Jan 20, 2017

@DeepDiver1975 I applied the fix on our servers (on top of 9.0.6 EE).

Looks good so far, but I'd like to watch it for a bit longer and will come back to you on Monday.

Our case was a bit differnet than described in the initial report.
Same outcome - oc_cards.carddata grows with more and more PHOTO objects until MySQL max_allowed_packet is reached, then the user can't log in anymore.
However while we do use LDAP, we don't have any user images in it, the users that were affected uploaded their avatar in ownCloud.

My guess is that the avatar somehow still gets written to the DB on LDAP sync, so it is likely the same bug.

@fmkaiser

This comment has been minimized.

Show comment
Hide comment
@fmkaiser

fmkaiser Jan 23, 2017

@DeepDiver1975 looks good still, thanks again for the quick reaction!

fmkaiser commented Jan 23, 2017

@DeepDiver1975 looks good still, thanks again for the quick reaction!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment