A new exception to suppress exception logs #30771

sharidas · 2018-03-13T15:56:02Z

There are scenarios where an app would have
done the job and the logger would have logged.
But the main task would throw an exception.
Since its logged gracefully we need not throw
exception in the log. This change would help
for the same.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

There can be situations where we need not throw the exceptions to the logger. For example when an antivirus app had detected the file cannot be uploaded and logged gracefully, it would be nice to exclude the exception logged.

Related Issue

#30751

Motivation and Context

There can be situations where we need not throw the exceptions to the logger. For example when an antivirus app had detected the file cannot be uploaded and logged gracefully, it would be nice to exclude the exception logged.

How Has This Been Tested?

Uploaded the eicar test file and the exception is not logged.

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 · 2018-03-13T16:17:42Z

Codecov Report

Merging #30771 into master will increase coverage by 0.14%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master   #30771      +/-   ##
============================================
+ Coverage     62.18%   62.32%   +0.14%     
- Complexity    18214    18217       +3     
============================================
  Files          1143     1143              
  Lines         68210    68216       +6     
  Branches       1232     1232              
============================================
+ Hits          42416    42516     +100     
+ Misses        25433    25339      -94     
  Partials        361      361

Flag	Coverage Δ	Complexity Δ
#javascript	`52.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.49% <71.42%> (+0.15%)`	`18217 <0> (+3)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
.../dav/lib/Connector/Sabre/ExceptionLoggerPlugin.php	`96.87% <100%> (+0.2%)`	`9 <0> (+1)`	⬆️
apps/dav/lib/Connector/Sabre/File.php	`83.57% <50%> (-0.25%)`	`108 <0> (+1)`
apps/dav/lib/Connector/Sabre/Directory.php	`70.62% <66.66%> (+0.37%)`	`74 <0> (+1)`	⬆️
lib/private/DB/MDB2SchemaManager.php	`91.22% <0%> (+5.26%)`	`17% <0%> (ø)`	⬇️
...eryBuilder/ExpressionBuilder/ExpressionBuilder.php	`95.89% <0%> (+5.47%)`	`20% <0%> (ø)`	⬇️
lib/private/DB/ConnectionFactory.php	`82.89% <0%> (+7.89%)`	`21% <0%> (ø)`	⬇️
lib/private/DB/AdapterSqlite.php	`83.33% <0%> (+83.33%)`	`7% <0%> (ø)`	⬇️
lib/private/Repair/SqliteAutoincrement.php	`85.18% <0%> (+85.18%)`	`9% <0%> (ø)`	⬇️
lib/private/DB/OCSqlitePlatform.php	`100% <0%> (+100%)`	`5% <0%> (ø)`	⬇️
lib/private/DB/SQLiteSessionInit.php	`100% <0%> (+100%)`	`4% <0%> (ø)`	⬇️
... and 1 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 c30c94d...f38b692. Read the comment docs.

VicDeo · 2018-03-22T19:26:21Z

lib/public/Files/ExcludeForbiddenException.php

+
+/**
+ * Exception for invalid content
+ * @since 6.0.0


10.0.8 or 10.1.0 ? :unsure:

VicDeo · 2018-03-22T19:26:46Z

lib/public/Files/ExcludeForbiddenException.php

+
+/**
+ * Public interface of ownCloud for apps to use.
+ * Files/InvalidContentException class


VicDeo · 2018-03-22T19:37:18Z

lib/public/Files/ExcludeForbiddenException.php

+ * Exception for invalid content
+ * @since 6.0.0
+ */
+class ExcludeForbiddenException extends \Exception {}


What HTTP status will get a desktop client in this case, something like 50x? Imho this exception should extend ForbiddenException... and files_ antivirus should throw it with $retry=true https://github.com/owncloud/core/blob/master/lib/public/Files/ForbiddenException.php#L43

VicDeo · 2018-03-22T19:43:25Z

apps/dav/lib/Connector/Sabre/File.php

@@ -601,6 +602,10 @@ private function needsPartFile($storage) {
 	 * @throws \Sabre\DAV\Exception
 	 */
 	private function convertToSabreException(\Exception $e) {
+		if ($e instanceof ExcludeForbiddenException) {
+			// the file content is not permitted
+			throw new UnsupportedMediaType($e->getMessage(), 0, $e);


it's not that critical to have exactly UnsupportedMediaType exception here. Actually back then I was choosing between 415 Unsupported Media Type and 418 I'm a teapot. And some people consider 415 status for infected files to be a bug nowadays, see owncloud/files_antivirus#177 ;)
If 403 is properly escalates exception message to the frontend we can just rethrow ExcludeForbiddenException (subclassed from ForbiddenException as requested above)

VicDeo · 2018-03-26T13:58:52Z

lib/public/Files/ExcludeForbiddenException.php

+namespace OCP\Files;
+
+/**
+ * Exception for invalid content


@sharidas Class description needs to be updated too. 'ForbiddenException with suppressed logging' or so.

@VicDeo Done.

VicDeo · 2018-03-27T16:57:38Z

Works as expected. There are no extra entries in log, Antivirus message is properly escalated to UI.

VicDeo · 2018-03-27T17:02:13Z

@PVince81 please consider backport to stable10 or files_antivirus will need a separate branch to support stable10

PVince81 · 2018-03-28T08:41:32Z

apps/dav/lib/Connector/Sabre/Directory.php

@@ -175,7 +176,11 @@ public function createFile($name, $data = null) {
 		} catch (InvalidPathException $ex) {
 			throw new InvalidPath($ex->getMessage());
 		} catch (ForbiddenException $ex) {
-			throw new Forbidden($ex->getMessage(), $ex->getRetry());
+			if ($ex->getPrevious() instanceof ExcludeForbiddenException) {
+				throw new ExcludeForbiddenException($ex->getMessage(), $ex->getRetry(), $ex);


this feels rather hacky. we can't create a subclass for every possible exception that we don't want to be logged.

or rename this exception with a name like "FileContentNotAllowedException" which is not semantically about "prevent logging" but about the actual error which is about disallowed file contents

Ok, renaming ExcludeForbidenException to FileContentNotAllowedException.
@VicDeo I will update owncloud/files_antivirus#219 accordingly.

PVince81 · 2018-04-03T10:09:17Z

lib/public/Files/FileContentNotAllowedException.php

+
+/**
+ * FileContentNotAllowed to suppress logging.
+ * @since 10.1.0


make that 10.0.8 if you think we can backport in time

PVince81

👍

There are scenarios where an app would have done the job and the logger would have logged. But the main task would throw an exception. Since its logged gracefully we need not throw exception in the log. This change would help for the same. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2018-04-03T11:02:33Z

Updated the PR as requested.

sharidas · 2018-04-03T11:09:44Z

Backport PR: #30991

lock · 2019-07-31T14:01:59Z

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 Mar 13, 2018

sharidas added this to the development milestone Mar 13, 2018

sharidas self-assigned this Mar 13, 2018

sharidas mentioned this pull request Mar 13, 2018

[files_antivirus] Upload of Eicar throws Sabre\Exception\UnsupportedMediaType #30751

Closed

sharidas changed the title ~~[WIP] A new exception to suppress exception logs~~ A new exception to suppress exception logs Mar 16, 2018

sharidas added 3 - To Review and removed 2 - Developing labels Mar 16, 2018

VicDeo requested changes Mar 22, 2018

View reviewed changes

sharidas force-pushed the suppress-exception-log branch 3 times, most recently from ff729e0 to bdd10d1 Compare March 23, 2018 10:31

VicDeo requested changes Mar 26, 2018

View reviewed changes

sharidas force-pushed the suppress-exception-log branch from bdd10d1 to 2bd142b Compare March 26, 2018 14:17

VicDeo approved these changes Mar 27, 2018

View reviewed changes

VicDeo mentioned this pull request Mar 27, 2018

A new exception to exclude from logging owncloud/files_antivirus#219

Merged

PVince81 reviewed Mar 28, 2018

View reviewed changes

sharidas force-pushed the suppress-exception-log branch from 2bd142b to f46b787 Compare March 28, 2018 15:15

PVince81 reviewed Apr 3, 2018

View reviewed changes

PVince81 approved these changes Apr 3, 2018

View reviewed changes

PVince81 added the backport-request label Apr 3, 2018

sharidas force-pushed the suppress-exception-log branch from f46b787 to f38b692 Compare April 3, 2018 11:02

sharidas mentioned this pull request Apr 3, 2018

[stable10] A new exception to suppress exception logs #30991

Merged

10 tasks

sharidas merged commit c59a9d1 into master Apr 3, 2018

sharidas deleted the suppress-exception-log branch April 3, 2018 12:07

phil-davis removed the backport-request label Apr 3, 2018

This was referenced Apr 18, 2018

Convert FileContentNotAllowedException into DAVForbiddenException to … #31190

Merged

[stable10] Convert FileContentNotAllowedException into DAVForbiddenException to … #31260

Merged

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A new exception to suppress exception logs #30771

A new exception to suppress exception logs #30771

sharidas commented Mar 13, 2018

codecov bot commented Mar 13, 2018 •

edited

Loading

VicDeo Mar 22, 2018

VicDeo Mar 22, 2018

VicDeo Mar 22, 2018

VicDeo Mar 22, 2018 •

edited

Loading

VicDeo Mar 26, 2018 •

edited

Loading

sharidas Mar 26, 2018

VicDeo commented Mar 27, 2018

VicDeo commented Mar 27, 2018

PVince81 Mar 28, 2018

sharidas Mar 28, 2018

PVince81 Apr 3, 2018

PVince81 left a comment

sharidas commented Apr 3, 2018

sharidas commented Apr 3, 2018

lock bot commented Jul 31, 2019

A new exception to suppress exception logs #30771

A new exception to suppress exception logs #30771

Conversation

sharidas commented Mar 13, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Mar 13, 2018 • edited Loading

Codecov Report

VicDeo Mar 22, 2018

Choose a reason for hiding this comment

VicDeo Mar 22, 2018

Choose a reason for hiding this comment

VicDeo Mar 22, 2018

Choose a reason for hiding this comment

VicDeo Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

VicDeo Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

sharidas Mar 26, 2018

Choose a reason for hiding this comment

VicDeo commented Mar 27, 2018

VicDeo commented Mar 27, 2018

PVince81 Mar 28, 2018

Choose a reason for hiding this comment

sharidas Mar 28, 2018

Choose a reason for hiding this comment

PVince81 Apr 3, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Apr 3, 2018

sharidas commented Apr 3, 2018

lock bot commented Jul 31, 2019

codecov bot commented Mar 13, 2018 •

edited

Loading

VicDeo Mar 22, 2018 •

edited

Loading

VicDeo Mar 26, 2018 •

edited

Loading