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

Exclude Directories #19235

Merged
merged 4 commits into from
Sep 2, 2016
Merged

Exclude Directories #19235

merged 4 commits into from
Sep 2, 2016

Conversation

mmattel
Copy link
Contributor

@mmattel mmattel commented Sep 22, 2015

Successor of PR #16534

Before mmattel/core
Now owncloud/core

Shoud fix CI start

@@ -61,6 +61,15 @@
exit();
}

if (\OC\Files\Filesystem::isForbiddenFileOrDir($fileName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it does not.
And we need this query here to display on the UI the text.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's about "upload" then? There you also get a generic error message only, right?
Maybe it would be a good idea to let the hook throw an exception with the corresponding error message if it is forbidden and then make sure that we display the error message from the exception instead of a generic one. You could use the HintException to provide a translated message. We do something similar with encryption errors.

It is probably more failure save instead of adding if statements all over the place and it is more generic since other errors can be handled the same way.

@oparoz
Copy link
Contributor

oparoz commented Sep 25, 2015

@mmattel @PVince81 @rperezb In order to test this properly, could we have some sort of acceptance test list with checkboxes? Would make it easier to know what's been tested and which corner cases we may have forgotten about.

@mmattel
Copy link
Contributor Author

mmattel commented Sep 26, 2015

Testscenario_1:
Focus: try to create a excluded directory name
Preperation: the name must NOT exist in the filesystem

Storage Task (t) Browser Webdav Creatable Result (r) Comment
local D x yes
local E x no (1)
local D x yes
local E x no (2)
SMB D x yes
SMB E x no (1)
SMB D x yes
SMB E x no (2)
  • t ... define a directory name to be excluded like fgh
    • E ... enable it config.php
    • D ... disable it in config.php (I just renamed it to .fgh and vice versa)
  • r ... y (yes), n (no), -- (n/a)

(1) ... A browser popup should appear that this name is excluded and can´t be used
(2) ... WebDav reports 'Forbidden'

  • Testscenario_1 sucessfully finished

Testscenario_2:
Focus: are the directories properly shown.
Preperation: have a directory already created with an excluded name which will then be checked against. This means, that the ecluded directory name already exists in the filesystem.

Storage Task (t) Browser Webdav Is_Visible Result (r) Comment
local D x yes
local E x no
local D x yes
local E x no
SMB D x yes
SMB E x no
SMB D x yes
SMB E x no
  • t ... define a directory name to be excluded like fgh
    • E ... enable it config.php
    • D ... disable it in config.php (I just renamed it to .fgh and vice versa)
  • r ... y (yes), n (no), -- (n/a)
  • Testscenario_2 sucessfully finished

Testscenario_3:
Focus: try to create an excluded directory name where the name DOES already exist in the filesystem
Preperation: like test 2

Storage Task (t) Browser Webdav Is_Creatable Result (r) Comment
local D x no same as Test2
local E x no same as Test2
local D x no (1)
local E x no (1)
SMB D x no same as Test2
SMB E x no same as Test2
SMB D x no (1)
SMB E x no (1)
  • t ... define a directory name to be excluded like fgh
    • E ... enable it config.php
    • D ... disable it in config.php (I just renamed it to .fgh and vice versa)
  • r ... y (yes), n (no), -- (n/a)

(1) ... A Fatal Error Message is created, the WebDav client also shows an error. But this is according to my tests not related to this PR. For details pls see #19101 (WebDav: create directory with an already existing directory name -> Fatal error)

  • Testscenario_3 sucessfully finished

Note : I wanted to use checkboxes in the table, but this did not work

@oparoz
Copy link
Contributor

oparoz commented Sep 26, 2015

Excellent, thank you :)

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 @oparoz I don't think we want this in 8.2. We are already in feature freeze.

@oparoz
Copy link
Contributor

oparoz commented Sep 28, 2015

I don't have a say in this ;)

@MorrisJobke MorrisJobke modified the milestones: 9.0-next, 8.2-current Sep 28, 2015
@MorrisJobke
Copy link
Contributor

@DeepDiver1975 @cmonteroluque enhancement -> 9.0

@ghost
Copy link

ghost commented Sep 30, 2015

@MorrisJobke ok

@DeepDiver1975
Copy link
Member

rebased

@PVince81
Copy link
Contributor

PVince81 commented Jan 8, 2016

@icewind1991 can you please comment on this PR ?

@DeepDiver1975
Copy link
Member

needs rebase

@ghost
Copy link

ghost commented Jan 16, 2016

Please also ping in owncloud-archive/documentation#1275 if this PR gets merged.

@PVince81
Copy link
Contributor

Rebased, not tested yet (files app still seems to work at the moment)

@DeepDiver1975
Copy link
Member

unit tests are badly falling apart - 89 failed tests
https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-19235/1/testReport/

@@ -1408,6 +1408,9 @@ public function getDirectoryContent($directory, $mimetype_filter = '') {
/**
* @var \OC\Files\FileInfo[] $files
*/
$files = array_filter(function(ICacheEntry $content) {
return (!\OC\Files\Filesystem::isForbiddenFileOrDir($content['path']));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should´nt that be (without testing):

$files = array_filter($content, function(ICacheEntry $c) {
                return (!\OC\Files\Filesystem::isForbiddenFileOrDir($c['path']));
            });

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, fixed

@PVince81
Copy link
Contributor

Did a quick test with the old Webdav and new Webdav endpoints and an excluded folder called "snapshots".

Seems to work so far: listing, deleting or overwriting the folder is denied.

@mmattel
Copy link
Contributor Author

mmattel commented Aug 24, 2016

Great !
I have written test cases, pls see above. If they pass then this PR should be ready. Sorry if I cant do, businesstrip...

@PVince81
Copy link
Contributor

businesstrip...

No time to kill on the plane ? (if applicable) 😉

Am thinking of maybe writing integration tests to automatically test this.
The browser test cases can be removed as the web UI uses Webdav already since 9.0 (except for uploads)

mmattel and others added 4 commits August 29, 2016 14:19
extended case: search at any place of the path given

adding case insensitiveness

added suggestions and improvements

removed unnecessary function parameter

new approach according the rethinking

fixed code and unit tests

update comments

improved code, added calls for trashbin copyFromStorage
extended case: search at any place of the path given

adding case insensitiveness

added suggestions and improvements

removed unnecessary function parameter

new approach according the rethinking

fixed code and unit tests

update comments

improved code, added calls for trashbin copyFromStorage
@mmattel
Copy link
Contributor Author

mmattel commented Sep 2, 2016

@PVince81 any news? would really appreciate to get that in

@PVince81
Copy link
Contributor

PVince81 commented Sep 2, 2016

Tested with many file operations on local storage and external storage and also subdirectories.
Sync client works too.
Any attempt to do any file operation on the excluded folder results in 403 Forbidden.

Exclusion works fine 👍

@PVince81 PVince81 merged commit 1826836 into master Sep 2, 2016
@PVince81 PVince81 deleted the exclude_directories_III branch September 2, 2016 10:16
@PVince81
Copy link
Contributor

PVince81 commented Sep 2, 2016

@PVince81
Copy link
Contributor

PVince81 commented Sep 2, 2016

@mmattel thanks a lot ! And sorry that it took so long to get in.

@mmattel
Copy link
Contributor Author

mmattel commented Sep 2, 2016

🎉 😃

mmattel added a commit to nextcloud/server that referenced this pull request Oct 5, 2016
… Storage use) owncloud/core/pull/19235

Adds the ability to define directories in config.php which will not be further processed (scanned, shown, created ect). These directories exist but will not handled in nextcloud.
Usecase: eg. filesystem which have the ability of snapshots have a defined naming for them. For details pls see also the documentation PR coming asap.
@JKawohl
Copy link
Contributor

JKawohl commented Feb 22, 2017

@PVince81 is this safe to patch into current 9.1 ?

@PVince81
Copy link
Contributor

@kawohl no, this is a huge change and I wouldn't recommend patching. This would need careful testing.

@JKawohl
Copy link
Contributor

JKawohl commented Feb 23, 2017

thanks!

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants