Disable trashbin and public webdav API by default #36124

VicDeo · 2019-08-30T12:54:43Z

Description

A config switch to enable Trashbin and webdav API

system config switch dav.enable.tech_preview added
pipeline.libsonnet and .drone.yml adjusted to enable the switch when running litmus tests
acceptance test steps added to enable/disable the switch
enable the switch during acceptance test scenarios that are testing the new "tech preview" APIs
when Then steps using the new trashbin API are being done to check the results of previous When actions, then enable the switch (if not already enabled), do the check, disable the switch (if we enabled it). That allows webUITrashbin tests to run with the switch off for the webUI trashbin actions, and for the subsequent checks to work using the new trashbin API. Thus we know that the old webUI trashbin behavior is working with the switch off.
add acceptance tests to confirm that the tech preview trashbin WebDAV API is non-responsive when the switch is off.
add acceptance tests to confirm that the tech preview public link WebDAV API is non-responsive when the switch is off.

Related Issue

Fixes Config switch to disable tech preview APIs #36118

Motivation and Context

We don't want to expose tech preview APIs in 10.3 by default

How Has This Been Tested?

not tested yet

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

phil-davis · 2019-08-30T13:05:59Z

Heads-up: it will need acceptance tests adjusted, so that the relevant scenarios set dav.enable.tech_preview at the start of the scenario.

codecov · 2019-08-30T14:37:39Z

Codecov Report

Merging #36124 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36124   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7403     7403           
  Branches     1308     1308           
=======================================
  Hits         3998     3998           
  Misses       3019     3019           
  Partials      386      386

Flag	Coverage Δ
#javascript	`54% <ø> (ø)`	⬆️

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 98922f1...f5bdbe4. Read the comment docs.

codecov · 2019-08-30T14:37:40Z

Codecov Report

Merging #36124 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36124   +/-   ##
=======================================
  Coverage   54.01%   54.01%           
=======================================
  Files          63       63           
  Lines        7404     7404           
  Branches     1309     1309           
=======================================
  Hits         3999     3999           
  Misses       3019     3019           
  Partials      386      386

Flag	Coverage Δ
#javascript	`54.01% <ø> (ø)`	⬆️

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 ce786ac...51a5850. Read the comment docs.

.drone.yml

phil-davis · 2019-09-03T12:00:43Z

@VicDeo or anybody

I delete a file using the webUI. I view it in "deleted files" on the webUI.

Then I do a "simple" PROPFIND to see what is in the trashbin:

curl http://admin:admin@172.17.0.1:8080/remote.php/dav/trash-bin/admin -X PROPFIND
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"><d:response><d:href>/remote.php/dav/trash-bin/admin/</d:href><d:propstat><d:prop><d:resourcetype><d:collection/></d:resourcetype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response><d:response><d:href>/remote.php/dav/trash-bin/admin/2147485656</d:href><d:propstat><d:prop><d:getlastmodified>Tue, 03 Sep 2019 11:46:56 GMT</d:getlastmodified><d:getcontentlength>3</d:getcontentlength><d:resourcetype/><d:getetag>&quot;407931bd258b410c9734a3536ee58a0f&quot;</d:getetag><d:getcontenttype>application/octet-stream</d:getcontenttype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat><d:propstat><d:prop><d:quota-used-bytes/><d:quota-available-bytes/></d:prop><d:status>HTTP/1.1 404 Not Found</d:status></d:propstat></d:response></d:multistatus>

There is a file with file id 2147485656 (I did not bother doing enough in the propfind to see its file name, original location etc) (when I dump some content of this sort of listing in acceptance tests, I see that file ids get returned but not the file names...)

I turn off the switch:

php occ config:system:set dav.enable.tech_preview --value true --type boolean

I do the PROPFIND again and get the same response. Maybe that is OK? Is that PROPFIND part of the trashbin API that should switch off?

Then I use the file id to delete the file (whatever its file name might be):

curl http://admin:admin@172.17.0.1:8080/remote.php/dav/trash-bin/admin/2147485656 -X DELETE

It worked. The file has been deleted.. I go to the webUI and refresh the "deleted files" page and the file is gone.

I did not expect that to work. I thought that the remote.php/dav/trash-bin/user/fileid endpoint would be disabled.

I am trying to make test scenarios that confirm that the endpoint(s) are disabled, but, for example, this one still seems to be enabled.

???

phil-davis · 2019-09-03T14:56:35Z

I first cheated and found out a file id in the trashbin:

curl http://admin:admin@172.17.0.1:8080/remote.php/dav/trash-bin/admin -X PROPFIND
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns"><d:response><d:href>/remote.php/dav/trash-bin/admin/</d:href><d:propstat><d:prop><d:resourcetype><d:collection/></d:resourcetype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat></d:response><d:response><d:href>/remote.php/dav/trash-bin/admin/2147483890</d:href><d:propstat><d:prop><d:getlastmodified>Tue, 03 Sep 2019 14:46:09 GMT</d:getlastmodified><d:getcontentlength>4</d:getcontentlength><d:resourcetype/><d:getetag>&quot;cb090cdd29f42193fb063f0a6402991c&quot;</d:getetag><d:getcontenttype>application/octet-stream</d:getcontenttype></d:prop><d:status>HTTP/1.1 200 OK</d:status></d:propstat><d:propstat><d:prop><d:quota-used-bytes/><d:quota-available-bytes/></d:prop><d:status>HTTP/1.1 404 Not Found</d:status></d:propstat></d:response></d:multistatus>

Then switch off the tech preview by checking out this branch:

git checkout feature/36118

Then try the PROPFIND again:

curl http://admin:admin@172.17.0.1:8080/remote.php/dav/trash-bin/admin -X PROPFIND
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>

Looks good.

And try directly using a file id that I know is in the trashbin:

curl http://admin:admin@172.17.0.1:8080/remote.php/dav/trash-bin/admin/2147483890 -X DELETE
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>File not found: trash-bin in 'root'</s:message>
</d:error>

Good stuff - it does not delete.

tests/acceptance/features/bootstrap/TrashbinContext.php

phil-davis · 2019-09-05T07:50:34Z

I am working on a few more tests - will push another commit in a while.
Some drone matrix entries have also "hung" like https://drone.owncloud.com/owncloud/core/20561/35/1
May as well just let drone trigger again when I push.

phil-davis · 2019-09-05T09:13:52Z

tests/acceptance/features/apiShareManagementBasic/createShare.feature

@@ -269,10 +270,10 @@ Feature: sharing
      | uid_file_owner         | user0                |
      | uid_owner              | user0                |
      | name                   |                      |
-    And the public should be able to download the range "bytes=1-7" of file "/parent.txt" from inside the last public shared folder using the old public WebDAV API and the content should be "wnCloud"


I refactored various tests that did "download range" Then steps, because the "download range" thing was only a way to a short bit of content to check. Better to do an ordinary full download of a file with short content.
(We did this sort of refactoring in other places already)

phil-davis · 2019-09-05T09:15:51Z

tests/acceptance/features/apiShareOperations/uploadToShare.feature

-    And uploading a file should not work using the new public WebDAV API
+
+  @public_link_share-feature-required
+  Scenario: Uploading file to a public read-only share folder does not work


I split these kind of scenario into 2 scenarios. That allows the first scenario for the old public WebDAV API to run completely with the new public WebDAV API disabled. IMO we do want to know that the old public WebDAV API works properly with the new public WebDAV API disabled.

phil-davis · 2019-09-05T09:19:12Z

tests/acceptance/features/bootstrap/PublicWebDavContext.php

@@ -323,10 +329,21 @@ public function checkLastPublicSharedFileDownload(
 	public function checkLastPublicSharedFileWithPasswordDownload(
 		$publicWebDAVAPIVersion, $password, $expectedContent
 	) {
+		if ($publicWebDAVAPIVersion === "new") {


For various Then steps that already use the new public WebDAV API to do their checks, I enable it, do the API call(s), then disable it. This allows the scenario(s) which use these steps to run with the API disabled during the scenario setup and When steps. That helps in testing that the When steps operate correctly with the tech preview APIs disabled.

phil-davis

I am finished! This looks good to me. I have asked @individual-it to review the acceptance test changes. He should be in an airport somewhere!
@PVince81 please review.

PVince81

Looks good to me 👍

individual-it

tests code looks good

VicDeo self-assigned this Aug 30, 2019

VicDeo added the 2 - Developing label Aug 30, 2019

VicDeo added this to the development milestone Aug 30, 2019

VicDeo added the p1-urgent Critical issue, need to consider hotfix with just that issue label Aug 30, 2019

VicDeo force-pushed the feature/36118 branch from f5bdbe4 to 4d5113e Compare September 2, 2019 09:54

phil-davis reviewed Sep 2, 2019

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

VicDeo force-pushed the feature/36118 branch 4 times, most recently from 16bed51 to 645afd3 Compare September 2, 2019 17:47

phil-davis force-pushed the feature/36118 branch from 645afd3 to 81b821e Compare September 3, 2019 10:35

phil-davis mentioned this pull request Sep 3, 2019

[WIP] Feature/36118 tests #36141

Closed

14 tasks

VicDeo added 3 - To Review and removed 2 - Developing labels Sep 3, 2019

phil-davis force-pushed the feature/36118 branch from 7ee4234 to ad6a32b Compare September 4, 2019 12:09

VicDeo commented Sep 4, 2019

View reviewed changes

tests/acceptance/features/bootstrap/TrashbinContext.php Outdated Show resolved Hide resolved

phil-davis force-pushed the feature/36118 branch from ad6a32b to 497bc56 Compare September 4, 2019 15:19

VicDeo requested a review from PVince81 September 5, 2019 06:25

davitol mentioned this pull request Sep 5, 2019

10.3 - Changelog test Results #36142

Closed

13 tasks

VicDeo and others added 2 commits September 5, 2019 14:54

Disable trashbin and public webdav API by default

101f250

Trashbin API acceptance tests for when the tech_preview switch is off

51a5850

phil-davis force-pushed the feature/36118 branch from 5c380ad to 51a5850 Compare September 5, 2019 09:09

phil-davis requested a review from individual-it September 5, 2019 09:11

phil-davis reviewed Sep 5, 2019

View reviewed changes

PVince81 approved these changes Sep 5, 2019

View reviewed changes

individual-it reviewed Sep 5, 2019

View reviewed changes

phil-davis added 4 - To release and removed 3 - To Review labels Sep 5, 2019

PVince81 mentioned this pull request Sep 5, 2019

Add setup about tech preview APIs in server core owncloud/web#1898

Merged

9 tasks

phil-davis merged commit ae9ee7a into master Sep 5, 2019

delete-merged-branch bot deleted the feature/36118 branch September 5, 2019 10:20

phil-davis added the release:10.3.0 label Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable trashbin and public webdav API by default #36124

Disable trashbin and public webdav API by default #36124

VicDeo commented Aug 30, 2019 •

edited by phil-davis

Loading

phil-davis commented Aug 30, 2019

codecov bot commented Aug 30, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading

phil-davis commented Sep 3, 2019 •

edited

Loading

phil-davis commented Sep 3, 2019

phil-davis commented Sep 5, 2019 •

edited

Loading

phil-davis Sep 5, 2019

phil-davis Sep 5, 2019

phil-davis Sep 5, 2019

phil-davis left a comment

PVince81 left a comment

individual-it left a comment

Disable trashbin and public webdav API by default #36124

Disable trashbin and public webdav API by default #36124

Conversation

VicDeo commented Aug 30, 2019 • edited by phil-davis Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

phil-davis commented Aug 30, 2019

codecov bot commented Aug 30, 2019

Codecov Report

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

phil-davis commented Sep 3, 2019 • edited Loading

phil-davis commented Sep 3, 2019

phil-davis commented Sep 5, 2019 • edited Loading

phil-davis Sep 5, 2019

Choose a reason for hiding this comment

phil-davis Sep 5, 2019

Choose a reason for hiding this comment

phil-davis Sep 5, 2019

Choose a reason for hiding this comment

phil-davis left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

individual-it left a comment

Choose a reason for hiding this comment

VicDeo commented Aug 30, 2019 •

edited by phil-davis

Loading

codecov bot commented Aug 30, 2019 •

edited

Loading

phil-davis commented Sep 3, 2019 •

edited

Loading

phil-davis commented Sep 5, 2019 •

edited

Loading