Adding new Symfony events for zipfile download and public link share #29885

sharidas · 2017-12-18T14:48:39Z

Adding new Symfony events, to check, if creating
shares or downloading zips are blocked by any
apps or not.

Signed-off-by: Sujith H [email protected]

Description

Adding new Symfony events to check if any apps revoked the premission to download zip files or creating public links.

Related Issue

Motivation and Context

Adding new symfony events which can be used by the apps to see if creating public links for files/folders are allowed or not. And also see if downloading zip files are allowed or not.

How Has This Been Tested?

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.

codecov · 2017-12-18T16:20:57Z

Codecov Report

Merging #29885 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #29885      +/-   ##
============================================
+ Coverage     58.24%   58.26%   +0.01%     
- Complexity    18519    18525       +6     
============================================
  Files          1093     1093              
  Lines         63711    63771      +60     
============================================
+ Hits          37108    37155      +47     
- Misses        26603    26616      +13

Impacted Files	Coverage Δ	Complexity Δ
...src/apps/files_sharing/lib/API/OCSShareWrapper.php	`0% <0%> (ø)`	`6% <0%> (ø)`	⬇️
drone/src/lib/private/legacy/files.php	`0% <0%> (ø)`	`65% <0%> (+3%)`	⬆️
...rone/src/apps/files_sharing/lib/API/Share20OCS.php	`91.89% <0%> (+0.26%)`	`150% <0%> (+1%)`	⬆️
drone/src/lib/private/legacy/helper.php	`75.16% <0%> (+2.53%)`	`118% <0%> (+2%)`	⬆️

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 46ff4ee...7e977b4. Read the comment docs.

PVince81

See comments above.

Note that for ZIP download I think there are at least 3 cases:

URL with "files" with only one file => direct download
URL with "files" and multiple files with full path => download all of them as ZIP
URL with "dir" containing base dir and "files" containing a list of files inside that dir => prepend dir to all files and download as ZIP
URL with "dir" without "files" => download said folder as ZIP

this is from memory, please check the code

PVince81 · 2017-12-22T09:55:53Z

apps/files_sharing/lib/API/Share20OCS.php

@@ -103,6 +107,7 @@ public function __construct(
 		$this->l = $l10n;
 		$this->config = $config;
 		$this->additionalInfoField = $this->config->getAppValue('core', 'user_additional_info_field', '');
+		$this->eventDispatcher = \OC::$server->getEventDispatcher();


why not inject it in the constructor like the other 9 services ?

PVince81 · 2017-12-22T09:57:48Z

apps/files_sharing/lib/API/Share20OCS.php

+		$event->setArgument('name', $name);
+		$event->setArgument('run', true);
+		//Dispatch an event to see if creating shares is blocked by any app
+		$this->eventDispatcher->dispatch('file.beforecreateShare', $event);


share.beforeCreate ?

Please also add a share.afterCreate if possible for the sake of completeness.

And also catch exception here and transform it to an OCS Result with the exception message. I expect that listeners will throw exceptions in case of denied access.

PVince81 · 2017-12-22T09:59:24Z

lib/private/legacy/files.php

@@ -139,6 +139,13 @@ public static function get($dir, $files, $params = null) {
 				}
 			}

+			//Dispatch an event to see if any apps have problem with download
+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['files' => $files]);
+			OC::$server->getEventDispatcher()->dispatch('file.beforecreatezip', $event);


in your other PR you used camel case for event names, please do so here as well.

PVince81 · 2017-12-22T10:00:55Z

lib/private/legacy/files.php

@@ -139,6 +139,13 @@ public static function get($dir, $files, $params = null) {
 				}
 			}

+			//Dispatch an event to see if any apps have problem with download
+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['files' => $files]);


Please double check what happens if "$dir" is set with a single value and multiple "$files" are set. It is likely that we also need to pass $dir. The alternative is to have a loop that prepends the directory to all given $files entries.

Here is the snapshot I took from IDE:

Basically during testing I have used a directory and a file. Above is the screenshot of the get method with single dir newfolder and multiple files: welcome.txt and rename_testfile.txt. The newfolder and the files reside in the home folder of the user. The $dir is "/" in the above screenshot. I tried to create a folder inside newfolder and then created few files inside newfolder. In this case also $dir remains "/".

PVince81 · 2017-12-22T10:02:41Z

lib/private/legacy/files.php

+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['files' => $files]);
+			OC::$server->getEventDispatcher()->dispatch('file.beforecreatezip', $event);
+			if ($event->hasArgument('run') and ($event->getArgument('run') === false) and ($event->hasArgument('message'))) {
+				throw new Exception();


A generic exception without a message will confuse the user. Maybe throw ForbiddenException instead with a message "Access denied".

In general I'd rather expect event listeners to directly throw an exception from inside with a proper message, so the exception itself would already be forwarded by not catching it in the dispatch() call.

PVince81 · 2017-12-22T10:03:33Z

apps/files_sharing/lib/API/Share20OCS.php

+		//Dispatch an event to see if creating shares is blocked by any app
+		$this->eventDispatcher->dispatch('file.beforecreateShare', $event);
+		if ($event->getArgument('run') === false) {
+			return new \OC\OCS\Result(null, 404, $this->l->t('No permission to create share'));


no permission is a 403 forbidden, not 404 not found

sharidas · 2017-12-22T13:18:15Z

Made the following changes:

injected the event dispatcher to the constructor.
used camel case for event name
Replaced generic exception with ForbiddedException
Added $dir to the event array
Added after event to make sure the creation of share is successful.
@PVince81 Hope the new change looks better.

PVince81

better, still some changes needed

PVince81 · 2017-12-22T14:25:12Z

apps/files_sharing/lib/API/Share20OCS.php

@@ -466,12 +484,15 @@ public function createShare() {

 		$share->getNode()->unlock(\OCP\Lock\ILockingProvider::LOCK_SHARED);

+		$event->setArgument('output', $output);


create a new event instance, don't resend the same one

PVince81 · 2017-12-22T14:26:15Z

apps/files_sharing/tests/ApiTest.php

@@ -115,6 +116,10 @@ private function createOCS($request, $userId) {
 				return vsprintf($text, $parameters);
 			}));

+		\OC::$server->getEventDispatcher()->addListener('share.beforeCreate', function (GenericEvent $event) {
+			$event->setArgument('run', true);


is this really needed ? "run" is true by default

This was added because the tests were failing. It was using false value from Share20OCSTest.php. That's the reason I set run value to true here.

Strange. Do you mean one test is influencing another ? This shouldn't happen. We need to find a way to reset the event dispatcher in tearDown to avoid test side effects.

PVince81 · 2017-12-22T14:27:43Z

lib/private/legacy/files.php

+			//Dispatch an event to see if any apps have problem with download
+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['dir' => $dir, 'files' => $files]);
+			OC::$server->getEventDispatcher()->dispatch('file.beforeCreateZip', $event);
+			if ($event->hasArgument('run') and ($event->getArgument('run') === false) and ($event->hasArgument('message'))) {


Make sure the "run" argument is created by you and set to "true" by default like you did for share creation.
Any reason you chose to not simply use the same pattern like for share creation ?

rename "message" to "errorMessage"

sharidas · 2017-12-22T14:53:29Z

Updated the changes:

made sure that run is set in the files.php
Created new instance of event for after share event
The reason for making run to true is addressed at : Adding new Symfony events for zipfile download and public link share #29885 (comment)

PVince81 · 2018-01-08T14:08:17Z

lib/private/legacy/files.php

+			//Dispatch an event to see if any apps have problem with download
+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['dir' => $dir, 'files' => $files, 'run' => true]);
+			OC::$server->getEventDispatcher()->dispatch('file.beforeCreateZip', $event);
+			if (($event->getArgument('run') === false) and ($event->hasArgument('errorMessage'))) {


this should be ||, not and. If run is false and no error message was supplied, we should still stop running.

PVince81 · 2018-01-08T14:08:40Z

lib/private/legacy/files.php

@@ -167,6 +174,9 @@ public static function get($dir, $files, $params = null) {
 			$streamer->finalize();
 			set_time_limit($executionTime);
 			self::unlockAllTheFiles($dir, $files, $getType, $view, $filename);
+			$event = new \Symfony\Component\EventDispatcher\GenericEvent(null, ['result' => 'success']);


please also pass dir and files arguments here

Adding new Symfony events, to check, if creating shares or downloading zips are blocked by any apps or not. Signed-off-by: Sujith H <[email protected]>

sharidas · 2018-01-09T10:10:30Z

@PVince81 Let me know if this change looks ok.

sharidas · 2018-01-09T11:48:54Z

Created backport to stable10 PR: #30067

PVince81

👍

lock · 2019-08-01T12:02:11Z

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.

sharidas added the 2 - Developing label Dec 18, 2017

sharidas self-assigned this Dec 18, 2017

sharidas added this to the development milestone Dec 18, 2017

sharidas force-pushed the add-symfony-events-part2 branch from 199790f to 74627de Compare December 18, 2017 16:20

sharidas force-pushed the add-symfony-events-part2 branch 25 times, most recently from 89c8df4 to fecaf0b Compare December 19, 2017 14:09

sharidas removed the 2 - Developing label Dec 21, 2017

sharidas requested a review from PVince81 December 21, 2017 13:03

sharidas changed the title ~~Adding new Symfony events~~ Adding new Symfony events for zipfile download and public link share Dec 21, 2017

sharidas force-pushed the add-symfony-events-part2 branch from de74508 to 738162e Compare December 21, 2017 14:26

PVince81 suggested changes Dec 22, 2017

View reviewed changes

sharidas force-pushed the add-symfony-events-part2 branch 2 times, most recently from dabb82e to 6e0724c Compare December 22, 2017 13:12

PVince81 suggested changes Dec 22, 2017

View reviewed changes

sharidas force-pushed the add-symfony-events-part2 branch from 6e0724c to 89c1dc3 Compare December 22, 2017 14:50

sharidas force-pushed the add-symfony-events-part2 branch from 89c1dc3 to ddffd96 Compare December 22, 2017 14:59

ownclouders added the status/STALE label Dec 30, 2017

sharidas force-pushed the add-symfony-events-part2 branch from ddffd96 to f656be5 Compare January 2, 2018 06:54

ownclouders removed the status/STALE label Jan 2, 2018

PVince81 reviewed Jan 8, 2018

View reviewed changes

sharidas force-pushed the add-symfony-events-part2 branch 3 times, most recently from 92c20e2 to 1abe239 Compare January 9, 2018 03:45

Adding new Symfony events for zipfile download and public link share

7e977b4

Adding new Symfony events, to check, if creating shares or downloading zips are blocked by any apps or not. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the add-symfony-events-part2 branch from 1abe239 to 7e977b4 Compare January 9, 2018 07:52

PVince81 approved these changes Jan 10, 2018

View reviewed changes

PVince81 merged commit f1a45d1 into master Jan 10, 2018

PVince81 deleted the add-symfony-events-part2 branch January 10, 2018 16:06

ownclouders mentioned this pull request Apr 25, 2018

Symfony events for user preference changes #31264

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new Symfony events for zipfile download and public link share #29885

Adding new Symfony events for zipfile download and public link share #29885

sharidas commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading

PVince81 left a comment

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

sharidas Dec 22, 2017

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

sharidas commented Dec 22, 2017

PVince81 left a comment

PVince81 Dec 22, 2017

PVince81 Dec 22, 2017

sharidas Dec 22, 2017

PVince81 Jan 8, 2018

PVince81 Dec 22, 2017

sharidas commented Dec 22, 2017

PVince81 Jan 8, 2018

PVince81 Jan 8, 2018

sharidas commented Jan 9, 2018

sharidas commented Jan 9, 2018

PVince81 left a comment

lock bot commented Aug 1, 2019

		@@ -466,12 +484,15 @@ public function createShare() {

		$share->getNode()->unlock(\OCP\Lock\ILockingProvider::LOCK_SHARED);

		$event->setArgument('output', $output);

Adding new Symfony events for zipfile download and public link share #29885

Adding new Symfony events for zipfile download and public link share #29885

Conversation

sharidas commented Dec 18, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Dec 18, 2017 • edited Loading

Codecov Report

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Dec 22, 2017

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Jan 9, 2018

sharidas commented Jan 9, 2018

PVince81 left a comment

Choose a reason for hiding this comment

lock bot commented Aug 1, 2019

sharidas commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading