fix rendering of multiple checksums

[butonic]

Hey OC10, long time no see...

I am implementing checksum support in ocis and wondered why oc10 is not rendiring multiple checksums as descriped in the original pr: #22199

<oc:checksums>
  <oc:checksum>TYPE1:CHECKSUM1</oc:checksum>
  <oc:checksum>TYPE2:CHECKSUM2</oc:checksum>
</oc:checksum>

The root cause is that at the time there was only a single checksum and it was expected to be comma seperated as can be read in #21997 (comment)

When multiple checksums got implemented they were seperated by ' ' in the database. leading to this output, which was however untested:

<oc:checksums>
  <oc:checksum>TYPE1:CHECKSUM1 TYPE2:CHECKSUM2 TYPE3:CHECKSUM3</oc:checksum>
</oc:checksum>

😭

b985261#diff-c76e89d5d2c586430906ac3c09dab6fb41a261ddc475febbcc6c79fd85872faeR25 would add tests but with multiple checksums in one row. This is part of #27097

but webdav only returned one checksum: 05e11ce

still digging for reasons where this went unnoticed ...

the current line in the tests was introduced in 014530b which was part of #27345

This was the change that exposed all hashes via webdav: https://github.com/owncloud/core/pull/27345/files#diff-1d6ed4804160da213bc58248990a6936622ce53100a23535d207dba153f7e759

The PR also cemented the wrong rendering.

@dragotin @micbar @IljaN what do we do about this?

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[butonic]

@felix-schwarz It seems that the ios-sdk overwrites the item checksums with the ones from the last oc:checksum property in Line 196 of

https://github.com/owncloud/ios-sdk/blob/538ef354ccc460bc3cba66f1844313ba71f1bcb8/ownCloudSDK/Item/OCItem%2BOCXMLObjectCreation.m#L177-L199

so ... fixing this would make ios clients not pick up the correct checksum, right?

[butonic]

@michaelstingl the android client does not use checksums, yet: owncloud/android#1840 right?

[butonic]

@TheOneRing The desktop client parses a byte array QByteArray findBestChecksum(const QByteArray &checksums):
https://github.com/owncloud/client/blob/3ef9835f8bed0ca36e67dd79abed10acba63f77d/src/common/checksums.cpp#L145-L160

It tries to grab the best hash until the next whitespace or the end of the line: https://github.com/owncloud/client/blob/3ef9835f8bed0ca36e67dd79abed10acba63f77d/src/common/checksums.cpp#L157

This should work when parsing the OC-Checksum header in https://github.com/owncloud/client/blob/ba2339a9865d9fd1e9400a673900f34c3c254d65/src/libsync/propagatedownload.cpp#L745

But when parsing the propfind response the code checks the checksums (plural s) property and passes it in as a whole: https://github.com/owncloud/client/blob/8ecd87fed6a81eab46229d83a45aab481f781780/src/libsync/discoveryphase.cpp#L409-L410

Wouldn't that pass in all of <oc:checksum>TYPE1:CHECKSUM1 TYPE2:CHECKSUM2 TYPE3:CHECKSUM3</oc:checksum>? which would work when more thin one checksum is returned. But when that string contains only one hash, eg: <oc:checksum>sha1:h3xc0d3</oc:checksum> findBestChecksum would return sha1:h3xc0d3</oc:checksum> ... this seems to be a bug, right?

[michaelstingl]

@michaelstingl the android client does not use checksums, yet: owncloud/android#1840 right?

yes, exactly

[butonic]

To summarize:

• android does not use checksums
• ios does but would only keep the last <oc:checksum> element value of the <oc:checksums> tag.
• the desktop client would pick up the closing tag of </oc:checksum> as part of the checksum

I fear we need to stay bug compatible with old clients.

We could add a user agent switch ...

[felix-schwarz]

@butonic

@felix-schwarz It seems that the ios-sdk overwrites the item checksums with the ones from the last oc:checksum property in Line 196 of

https://github.com/owncloud/ios-sdk/blob/538ef354ccc460bc3cba66f1844313ba71f1bcb8/ownCloudSDK/Item/OCItem%2BOCXMLObjectCreation.m#L177-L199

so ... fixing this would make ios clients not pick up the correct checksum, right?

The SDK won't overwrite previously parsed checksums, but re-use them and append new ones:

https://github.com/owncloud/ios-sdk/blob/538ef354ccc460bc3cba66f1844313ba71f1bcb8/ownCloudSDK/Item/OCItem%2BOCXMLObjectCreation.m#L184

NSMutableArray<OCChecksum *> *checksums = (item.checksums!=nil) ? // Does this item already have (a) checksum(s)?
	[[NSMutableArray alloc] initWithArray:item.checksums] :  // Yes -> Create a new array, but pre-populate with existing checksums
	[[NSMutableArray alloc] initWithCapacity:checksumStrings.count]; // No -> Create a new array and pre-allocate enough space in advance, so no reallocation of the backing storage is needed as items are added
…
// Parse checksums
{
	…
	// add parsed checksums to the local checksums array
	[checksums addObject:checksum];
}
…
item.checksums = checksums; // store the checksums into the item

• ios does but would only keep the last <oc:checksum> element value of the <oc:checksums> tag.

No, as shown above, it'd use all of them and should just work with that change.

[dragotin]

Let's stay bug compatible. Do not merge this.