Skip to content

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 26, 2017

Description

Add webdav resource for public shared files and folders

Motivation and Context

DAV rules!

How Has This Been Tested?

  • share a file or folder publicly
  • open the dav url /remote.php/dav/public-files/${token}

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.

@DeepDiver1975
Copy link
Member Author

  • deleting a file does not work

@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #29369 into master will decrease coverage by 0.47%.
The diff coverage is 11.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #29369      +/-   ##
============================================
- Coverage     62.27%   61.79%   -0.48%     
- Complexity    18229    19112     +883     
============================================
  Files          1141     1096      -45     
  Lines         68119    61616    -6503     
  Branches       1230        0    -1230     
============================================
- Hits          42420    38077    -4343     
+ Misses        25336    23539    -1797     
+ Partials        363        0     -363
Flag Coverage Δ Complexity Δ
#javascript ? ?
#phpunit ? ?
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/DAV/PublicAuth.php 18.18% <ø> (ø) 5 <0> (ø) ⬇️
apps/dav/lib/Files/PublicFiles/ShareNode.php 0% <0%> (ø) 8 <8> (?)
lib/private/Files/Node/Node.php 84.05% <0%> (-1.24%) 63 <1> (+1)
apps/dav/lib/Files/PublicFiles/SharedFolder.php 0% <0%> (ø) 13 <13> (?)
apps/dav/lib/Files/PublicFiles/SharedFile.php 0% <0%> (ø) 10 <10> (?)
apps/dav/lib/RootCollection.php 100% <100%> (ø) 1 <0> (ø) ⬇️
apps/dav/lib/Server.php 53.33% <100%> (+2.25%) 24 <0> (ø) ⬇️
...ps/dav/lib/Files/PublicFiles/PublicSharingAuth.php 27.77% <27.77%> (ø) 8 <8> (?)
apps/dav/lib/Files/PublicFiles/RootCollection.php 27.77% <27.77%> (ø) 7 <7> (?)
lib/private/DB/PostgreSqlMigrator.php 0% <0%> (-100%) 6% <0%> (ø)
... and 191 more

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 df728bc...137b795. Read the comment docs.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code looks good so far, see some proposed additions.

* @inheritdoc
*/
function challenge(RequestInterface $request, ResponseInterface $response) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if purposefully left empty, please add a comment stating so


function __construct() {
$this->l10n = \OC::$server->getL10N('dav');
$this->shareManager = \OC::$server->getShareManager();
Copy link
Contributor

Choose a reason for hiding this comment

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

use DI ?

use Sabre\DAV\Collection;
use Sabre\DAV\INode;

class ShareNode extends Collection {
Copy link
Contributor

Choose a reason for hiding this comment

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

PHPDoc, what is this node about ?

$this->share = $share;
}
/**
* Returns an array with all the child nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest clarifying and saying we are returning both SharedFolder and SharedFile nodes depending on child types

use Sabre\DAVACL\IACL;

/**
* Class MetaFile
Copy link
Contributor

Choose a reason for hiding this comment

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

please adjust, smells like copy-pasted from another PR 😉

}

// function setName($name) {
// $this->file->setName($name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can forbid renaming share file here. This is because the name itself isn't visible anyway but someone might attempt to hack the API to try it out.

For local shares the file name is a received mount point. But here for link shares this is no mount point.

Throw Forbidden ?

use Sabre\DAVACL\IACL;

/**
* Class MetaFolder
Copy link
Contributor

Choose a reason for hiding this comment

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

phpdoc

return $this->folder->getName();
}

function getLastModified() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add getEtag() maybe.

We'll likely do a PROPFIND on this folder so it's good to get the etag to know if there were changes inside the folder.

Also add this to SharedFile.

Sounds like we might need a custom INode interface which has the getEtag method ? Or does Sabre retrieve the Etag differently ? (I forgot)

@PVince81 PVince81 modified the milestones: planned, development Nov 22, 2017
@DeepDiver1975
Copy link
Member Author

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost!

@ownclouders
Copy link
Contributor

Automated rebase failed! Please rebase your pull request manually via the command line.

Error:
Command '['git', 'rebase', 'base/master']' returned non-zero exit status 128.

@felixboehm felixboehm modified the milestones: development, planned Apr 10, 2018
@DeepDiver1975 DeepDiver1975 modified the milestones: development, triage Apr 25, 2018
@PVince81 PVince81 modified the milestones: triage, maybe some day May 22, 2018
@PVince81
Copy link
Contributor

Does it work with link password ?

This would obsolete the "public.php/webdav" endpoint which takes userid=$token and password=$password.

@DeepDiver1975
Copy link
Member Author

Does it work with link password ?

password handling in in here -
137b795

@PVince81
Copy link
Contributor

PVince81 commented Aug 1, 2018

  • consider implications on federated shares and OCM. I expect that we only need to change the Webdav URL exposed in the discovery (to be tested)

@PVince81
Copy link
Contributor

PVince81 commented Oct 9, 2018

reconsider for Phoenix ?

@PVince81
Copy link
Contributor

PVince81 commented May 6, 2019

Fixes #23269

@DeepDiver1975
Copy link
Member Author

As discussed in #35777 the master branch will from now on hold the ownCloud 10 codebase.

This PR targetted ownCloud 11 which is postponed to a far distant future.

Because of that I'm closing this PR and kindly ask you to re-submit this PR in a few days.

Thanks a lot for your patience

@DeepDiver1975 DeepDiver1975 deleted the dav-public-files branch July 30, 2019 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants