X Tutup
Skip to content

Update Encryption.php to fix issue #34599 "encrypted remote storage"#36546

Merged
micbar merged 2 commits intorelease-10.4.0from
stream_read-wrapper
Jan 31, 2020
Merged

Update Encryption.php to fix issue #34599 "encrypted remote storage"#36546
micbar merged 2 commits intorelease-10.4.0from
stream_read-wrapper

Conversation

@phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 9, 2019 •
edited
Loading

Description

This is an updated rebase of PR #36179 with the commits squashed.

The commit still has @martink-p as author, so that will be good for giving proper credit to the author.

It adds a wrapper function to "stream_read" which reads (and checks) until the required block size is available or there is no remaining data.

Issue

Motivation and Context

This pull request fixes an issue with encrypted external WebDAV storage and was verified as a solution with nextcloud issue nextcloud/server#9792

How Has This Been Tested?

Text from #36179 :

  • test environment: running owncloud docker system with external WebDAV storage at Strato HiDrive
  • test case 1: several files. uploaded, checked encryption, downloaded, checked decryption.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@phil-davis phil-davis self-assigned this Dec 9, 2019
@phil-davis phil-davis mentioned this pull request Dec 9, 2019
Closed
@phil-davis
Copy link
Contributor Author

phil-davis commented Dec 10, 2019

Various things related to notifications are failing when encryption is on (user and/or master key)

https://drone.owncloud.com/owncloud/encryption/1026/114/16 - user key webUISharingNotifications

--- Failed scenarios:

    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/notificationLink.feature:16
    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/shareWithGroups.feature:21
    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/shareWithUsers.feature:18
    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/shareWithUsers.feature:28
    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/shareWithUsers.feature:36
    /var/www/owncloud/testrunner/tests/acceptance/features/webUISharingNotifications/shareWithUsers.feature:47

6 scenarios (6 failed)
67 steps (49 passed, 6 failed, 12 skipped)

and the same for master-key at https://drone.owncloud.com/owncloud/encryption/1026/89/16

https://drone.owncloud.com/owncloud/encryption/1026/61/14 - user key apiSharingNotifications

--- Failed scenarios:

    /var/www/owncloud/testrunner/tests/acceptance/features/apiSharingNotifications/sharingNotifications.feature:23
    /var/www/owncloud/testrunner/tests/acceptance/features/apiSharingNotifications/sharingNotifications.feature:35
    /var/www/owncloud/testrunner/tests/acceptance/features/apiSharingNotifications/sharingNotifications.feature:69
    /var/www/owncloud/testrunner/tests/acceptance/features/apiSharingNotifications/sharingNotifications.feature:94
    /var/www/owncloud/testrunner/tests/acceptance/features/apiSharingNotifications/sharingNotifications.feature:133

9 scenarios (4 passed, 5 failed)
122 steps (105 passed, 5 failed, 12 skipped)

and the same at https://drone.owncloud.com/owncloud/encryption/1026/36/14 - master key apiSharingNotifications

@codecov
Copy link

codecov bot commented Dec 10, 2019 •
edited
Loading

Codecov Report

Merging #36546 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36546      +/-   ##
============================================
+ Coverage      64.7%    64.7%   +<.01%     
- Complexity    19130    19133       +3     
============================================
  Files          1270     1270              
  Lines         74854    74863       +9     
  Branches       1327     1327              
============================================
+ Hits          48434    48443       +9     
  Misses        26029    26029              
  Partials        391      391
Flag Coverage Δ Complexity Δ
#javascript 54.09% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.88% <100%> (ø) 19133 <3> (+3) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Stream/Encryption.php 94.53% <100%> (+0.28%) 56 <3> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 422e4e3...e681b10. Read the comment docs.

@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 10, 2020

Rebased just now. After some changes to CI in encryption today, I will run encryption CI again against this...

@mmattel
Copy link
Contributor

mmattel commented Jan 22, 2020

Hi @phil-davis do you have an update on this?

@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 24, 2020 •
edited
Loading

Rebased again just now.
Tests are running again with encryption - see owncloud/encryption#166

@mmattel
Copy link
Contributor

mmattel commented Jan 24, 2020

Seems that CI is failing with errors creating some images 😢

@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 24, 2020

https://drone.owncloud.com/owncloud/core/22911/22/7
It's just stupid CI - lately the Oracle docker image has often "hung" when being fetched from docker. install-core waits up to 10 minutes for Oracle to become available and then gives up.
I don't really care about the core pipelines that failed!

@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 30, 2020 •
edited
Loading

I found the issue with CI - owncloud/encryption#166 (comment) - and actually all the tests pass fine with encryption enabled.

@phil-davis phil-davis marked this pull request as ready for review January 30, 2020 04:08
@phil-davis phil-davis requested a review from micbar January 30, 2020 04:08
@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 30, 2020

@micbar I added a changelog.

From a QA point-of-view this does not break anything.

Please get an appropriate person to do a final code review. then IMO this could be merged.

micbar
micbar suggested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Changelog title exceeds 80 chars.
Otherwise 👍

martink-p and others added 2 commits January 30, 2020 14:09
@martink-p @phil-davis
Update Encryption.php
6598d0a 
Update Encryption.php

Update Encryption.php

Update Encryption.php
@phil-davis
Add changelog for stream_read fix
e681b10
@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 30, 2020

Changelog title exceeds 80 chars.

fixed

(I thought that the 80 char limit would be for the text after the Bugfix: keyword - but it is the length of the whole line)

@phil-davis phil-davis requested a review from micbar January 30, 2020 08:25
@phil-davis
Copy link
Contributor Author

phil-davis commented Jan 30, 2020

@micbar CI has passed - ready for review again.

@micbar micbar changed the base branch from master to release-10.4.0 January 31, 2020 08:49
@micbar micbar merged commit 6ff7a7e into release-10.4.0 Jan 31, 2020
@delete-merged-branch delete-merged-branch bot deleted the stream_read-wrapper branch January 31, 2020 08:51
@phil-davis phil-davis mentioned this pull request Jan 31, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@micbar micbar micbar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@phil-davis phil-davis

Labels

3 - To Review QA:team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@phil-davis @mmattel @micbar @martink-p
X Tutup