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.

Sign up for GitHub

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

Jump to bottom

Fix new DAV quota check target path #28160

Merged
merged 4 commits into from
Jun 29, 2017
Merged

Fix new DAV quota check target path #28160

merged 4 commits into from
Jun 29, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jun 19, 2017 •
edited
Loading

Description

Fixes the following:

  • use correct actual path in QuotaPlugin when coming from the new DAV endpoint
  • add quota check when moving FutureFile using chunked upload to new DAV endpoint

Also adds quota test hypercube to make sure we didn't forget anything, from owncloud/QA#461

Related Issue

Fixes #28159
Fixes owncloud/QA#461

Motivation and Context

One must not be able to bypass quotas

How Has This Been Tested?

Automated tests.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

TODO:

  • integration tests (test first!)
  • the actual fix

cc @SergioBertolinSG

@PVince81 PVince81 added this to the 10.0.3 milestone Jun 19, 2017
@PVince81 PVince81 self-assigned this Jun 19, 2017
@PVince81 PVince81 mentioned this pull request Jun 19, 2017
Closed
@SergioBertolinSG SergioBertolinSG self-requested a review June 20, 2017 06:53
@PVince81
Copy link
Contributor Author

@SergioBertolinSG it's too early for review but thanks for confirming the two cases I added.

@PVince81 PVince81 mentioned this pull request Jun 22, 2017
Closed
@PVince81
Copy link
Contributor Author

I'm done, please review:

@SergioBertolinSG please review my test changes and additions.

@DeepDiver1975 please review the QuotaPlugin additions which are related to new dav. I know they're not ideal but I'm aiming for a backportable fix here. (we could use getNodeForPath for a cleaner fix)

@PVince81
Copy link
Contributor Author

I'll address the failing unit tests

@PVince81
Copy link
Contributor Author

@SergioBertolinSG I have now added overwrite upload test cases for everything to make sure all is covered.

Now QuotaPlugin was switched to always use getNodeForPath instead of using path trickery. The automated tests confirm that it still works.

@DeepDiver1975 @jvillafanez please review

@PVince81
Copy link
Contributor Author

Rebased to restart unstable CI env

@SergioBertolinSG
Copy link
Contributor

👍 for the integration tests part.

@PVince81 PVince81 mentioned this pull request Jun 23, 2017
Closed
@PVince81
Copy link
Contributor Author

rebased due to Jenkins' sudden death

@PVince81
Copy link
Contributor Author

Aha, finally a legit failure:

23:20:56     /var/lib/jenkins/workspace/owncloud-core_core_PR-28160-GE6VZMPMYVCIQXV7U5Z2R262UNHGEWXD3YK3XVDVFGZDMDFAFJXQ/tests/integration/features/checksums.feature:145

PVince81
PVince81 commented Jun 26, 2017
View reviewed changes
tests/integration/features/bootstrap/WebDav.php
@@ -742,7 +897,7 @@ public function userMovesNewChunkFileWithIdToMychunkedfile($user, $id, $dest)
{
$source = '/uploads/' . $user . '/' . $id . '/.file';
$destination = substr($this->baseUrl, 0, -4) . $this->getDavFilesPath($user) . $dest;
$this->makeDavRequest($user, 'MOVE', $source, [
$this->response = $this->makeDavRequest($user, 'MOVE', $source, [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like storing the response here makes the checksum test fail, weird...

@PVince81
Copy link
Contributor Author

Fixed checksum test by making it check the HTTP status earlier.

@jvillafanez
Copy link
Member

👍

@PVince81
Copy link
Contributor Author

@owncloud-bot I hate you

10:05:08 Fire up the mysql docker
10:05:09 Waiting for MySQL(utf8mb4) initialisation ...
10:06:05 ...........................
10:06:06 MySQL(utf8mb4)  is up.
10:06:06 Installing ....
10:06:06 ownCloud is not installed - only a limited number of commands are available
10:06:11 Error while trying to create admin user: Failed to connect to the database: An exception occured in driver: SQLSTATE[HY000] [1045] Access denied for user 'oc_autotest4'@'172.17.0.1' (using password: YES)
10:06:11  -> 
10:06:11 Kill the docker cc8c93196e8ade792a9dc1db92eb226b25bdfb0b17fa570f03be2a5d5df0f424
10:06:11 cc8c93196e8ade792a9dc1db92eb226b25bdfb0b17fa570f03be2a5d5df0f424
10:06:11 cc8c93196e8ade792a9dc1db92eb226b25bdfb0b17fa570f03be2a5d5df0f424
10:06:11 Makefile:179: recipe for target 'test-php' failed
10:06:11 make: *** [test-php] Error 1

Vincent Petry added 3 commits June 27, 2017 15:02
Fix quota checks for new endpoint
622e2d5 
- use correct actual path in `QuotaPlugin` when coming from the new DAV endpoint
- add quota check when moving FutureFile using chunked upload to new DAV endpoint
Use getNodeForPath in QuotaPlugin for more accurate path
f7748f4
Fix checksum test to check http status earlier
c6e1a85 
Due to a small change in the Webdav test steps, the response of the last
Webdav call is stored now, so we need to check the HTTP status earlier.
@PVince81
Copy link
Contributor Author

anotherrrrr rrrrebase

@PVince81 PVince81 merged commit 2548f08 into master Jun 29, 2017
@PVince81 PVince81 deleted the fix-newdav-share-quota branch June 29, 2017 15:22
@PVince81 PVince81 mentioned this pull request Jun 29, 2017
Merged
@PVince81
Copy link
Contributor Author

stable10: #28261

@PVince81 PVince81 mentioned this pull request Aug 7, 2017
Closed
@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SergioBertolinSG SergioBertolinSG SergioBertolinSG approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@jvillafanez jvillafanez Awaiting requested review from jvillafanez

Assignees

@PVince81 PVince81

Labels
3 - To Review sev1-critical
Projects
None yet
Milestone
10.0.3
3 participants
@PVince81 @SergioBertolinSG @jvillafanez