Skip to content
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

Disable validation of transmission checksums by default #4638 #4663

Merged
merged 1 commit into from
Apr 21, 2016

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Apr 12, 2016

The server may send invalid checksums when files change on disk,
see owncloud/core#23783.

Introduces a new validateChecksumOnDownload configuration and
branding option.

@ckamm ckamm added this to the 2.2.0-current milestone Apr 12, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @dragotin, @danimo and @ogoffart to be potential reviewers

@ckamm ckamm force-pushed the disabletransmissionchecksum branch 2 times, most recently from 5aecbdf to 352f5d2 Compare April 14, 2016 07:41
* Add checksums/supportedTypes and checksums/preferredUploadType
  capabilities. The default is that no checksum types are supported.

* Remove the transmissionChecksum config option. Servers must now
  use the capabilities to indicate that they are fine with the
  client sending checksums.

Note: This intentionally breaks brandings that overrode
Theme::transmissionChecksum. The override must be removed and the
server's capabilities must be adjusted to include the new values.
@ckamm ckamm force-pushed the disabletransmissionchecksum branch from 352f5d2 to ea40e31 Compare April 15, 2016 08:59
@@ -17,6 +17,7 @@
#include "syncfileitem.h"
#include "propagatorjobs.h"
#include "account.h"
#include "configfile.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we don't make dependency between the sync engine and the config file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since if it's for a debugging purpose, maybe an environment variable would work. But I think we could also leave it out. Who is this debug option for? Developers can as well comment the code that does the checking.

@ogoffart
Copy link
Contributor

I did not see the validateChecksumOnDownload

Should we really get rid of the branding option? branding who enables this option do it because their server is capable? Or is it assumed that the branding would also have their own server?

@dragotin
Copy link
Contributor

@ogoffart yes, there is only one branding using that. And they agreed on setting the server capability.

@guruz guruz added the p2-high Escalation, on top of current planning, release blocker label Apr 20, 2016
@ckamm
Copy link
Contributor Author

ckamm commented Apr 21, 2016

@dragotin @ogoffart Could I ask you to potentially fix and merge this? I won't get around to it before Monday, unfortunately!

@moscicki
Copy link
Contributor

Didn't we agree that this option is not needed? And to use capabilities and not the configuration file?

#4638 (comment)

@dragotin
Copy link
Contributor

Yes, we did, I guess that is exactly what Christian means.

@ogoffart ogoffart assigned ogoffart and unassigned dragotin Apr 21, 2016
@ogoffart ogoffart merged commit ea40e31 into owncloud:master Apr 21, 2016
ogoffart added a commit that referenced this pull request Apr 21, 2016
Added in previous commit from pull request #4663

As discussed, we do not need this option so no need to introduce
a new dependency on the config file in the sync engine
ogoffart added a commit that referenced this pull request Apr 21, 2016
Disable validation of transmission checksums by default
@ckamm
Copy link
Contributor Author

ckamm commented Apr 21, 2016

@dragotin @moscicki I didn't update this pull request description as the discussion in the ticket unfolded. The patch ea40e31 removes the config and branding options, as should be expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-high Escalation, on top of current planning, release blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants