Move excluded files to config array #29126

sharidas · 2017-09-29T14:59:17Z

Moving excluded files to config array.
Instead of populating in the ExcludeFile*
php files, we can have it in config.php.

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

Description

This change would help users to mention folders/files to be excluded during integrity check to the config.php. In the config.php it will be represented as an array.

Related Issue

#23186
https://github.com/owncloud/enterprise/issues/1874

Motivation and Context

Instead of hard coding the files/folders in the files, its better to move them to the config.php. Also this change would help the integrity check to exclude files/folders mentioned in the config.php

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.

tomneedham · 2017-10-03T11:06:07Z

should also include a sample entry in config.sample.php

tomneedham · 2017-10-03T11:03:35Z

lib/private/IntegrityCheck/Checker.php

 		}

+		//Exclude files which shouldn't fall for comparison
+		$excludeFiles = \OC::$server->getSystemConfig()->getValue('excludedFiles', '');


default should be an array since we use it in in_array later

DeepDiver1975 · 2017-10-05T19:37:48Z

config/config.sample.php

+ */
+'excludedFolders' =>
+	array (
+		0 => '/home/sujith/test/owncloud/data',


better use OC::$SERVERROOT

example

OC::$SERVERROOT . '/assets'

and we shall find good example folders - maybe two are enough.
/apps/my-theme might be a good example and /assets

DeepDiver1975 · 2017-10-05T19:39:59Z

config/config.sample.php

+	),
+/**
+ * Exclude file name patterns from the integrity checker command
+ */


isn't the pattern enough to also properly exclude folders and files from above?

not sure... maybe some files could have prefixes/suffixes ? I'd also prefer the simpler solution.

@sharidas Did anyone report such cases in the original ticket ?

@PVince81 No I didn't find any cases related to exclude filename patterns in the original ticket. I think we can stick on with exclude file names and remove exclude filename patterns from the PR.

DeepDiver1975 · 2017-10-05T19:40:55Z

lib/private/IntegrityCheck/Checker.php

 		}

+		//Exclude files which shouldn't fall for comparison
+		$excludeFiles = \OC::$server->getSystemConfig()->getValue('excludedFiles', []);


inject config instance - no use of \OC::$server inside any class please

DeepDiver1975 · 2017-10-05T19:41:21Z

lib/private/IntegrityCheck/Iterator/ExcludeFileByNameFilterIterator.php

 	}
+
+	protected function getExcludedFileNames() {
+		return \OC::$server->getConfig()->getSystemValue('excludedFiles', '');


keep proper defaults - basically just like before

DeepDiver1975 · 2017-10-05T19:42:20Z

lib/private/Setup.php

 			'overwrite.cli.url'	=> $request->getServerProtocol() . '://' . $request->getInsecureServerHost() . \OC::$WEBROOT,
 			'dbtype'			=> $dbType,
 			'version'			=> implode('.', \OCP\Util::getVersion()),
+			'excludedFolders'   => $excludeFolders,


I would no set them during setup. the defaults will be in our code base and people can extend/change the list if they like.

DeepDiver1975 · 2017-10-05T19:42:47Z

tests/lib/IntegrityCheck/CheckerTest.php

 		$this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/app/');
 	}

-	public function testWriteCoreSignatureWithUnmodifiedHtaccess() {


where did these tests go?

PVince81 · 2017-10-10T13:41:25Z

I heard from @Peter-Prochaska that it might not be a good idea to allow excluding whole folders, so not sure... See #29094 (comment)

PVince81 · 2017-10-18T15:15:14Z

Please only allow excluding files, not folders, as discussed with @Peter-Prochaska

mmattel · 2017-10-23T13:52:27Z

Maybe only a small detail to avoid naming confusion:
excludedFolders --> integrityExcludedFolders also valid for the others.

Reason: https://doc.owncloud.org/server/latest/admin_manual/configuration/server/excluded_blacklisted_files.html

sharidas · 2017-11-03T15:08:22Z

Updated this PR to exclude files only and not folders.

codecov · 2017-11-03T23:03:13Z

Codecov Report

Merging #29126 into master will increase coverage by 0.23%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #29126      +/-   ##
============================================
+ Coverage     60.84%   61.08%   +0.23%     
- Complexity    17238    17239       +1     
============================================
  Files          1032     1031       -1     
  Lines         57365    57145     -220     
============================================
+ Hits          34904    34907       +3     
+ Misses        22461    22238     -223

Impacted Files	Coverage Δ	Complexity Δ
lib/private/IntegrityCheck/Checker.php	`92.69% <100%> (+0.1%)`	`61 <0> (+1)`	⬆️

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 3da7b8e...bbf96e9. Read the comment docs.

mmattel · 2017-11-05T16:40:04Z

config/config.sample.php

+/**
+ * Exclude directories from the integrity checker command
+ */
+'excludedFiles' =>


Description text: Exclude directories --> Exclude files

Also see my comment above to avoid confusion:
This variable should be renamed from excludedFiles to integrityExcludedFiles

sharidas · 2017-11-06T07:08:57Z

Not sure how to make codecov happy for config.sample.php. Any pointers would be helpful. Thanks.

PVince81 · 2017-11-06T09:37:31Z

config/config.sample.php

no need for the numeric index

Updated the config.sample.php by removing numeric index.

sharidas · 2017-11-06T11:13:38Z

Backport PR to stable10 branch: #29460

PVince81 · 2017-11-06T13:37:20Z

config/config.sample.php

why camel case ? all of the other config entries are either dot separated, underscore separated or have no separator for all-lowercase. The latest trend is to use dots as separators.

Updated to integrity.excluded.files

PVince81 · 2017-11-06T13:39:23Z

@Peter-Prochaska second review ?

peterprochaska · 2017-11-07T09:53:17Z

lib/private/IntegrityCheck/Checker.php

 	 * @param X509 $certificate
 	 * @param RSA $privateKey
-	 * @return string
+	 * @return mixed


Why mixed? it's an array

@Peter-Prochaska Updated the PR.

Moving excluded files to config array. Instead of populating in the ExcludeFile* php files, we can have it in config.php. Signed-off-by: Sujith H <[email protected]>

mmattel · 2017-11-14T18:38:04Z

config/config.sample.php

 	),
+/**
+ * Exclude directories from the integrity checker command
+ */


@PVince81 we still have the description text mismatch stating that we exclude directories while we exclude files ...
This inconsistancy needs to be fixed and backported

@PVince81 just seen that is has been fixed ...

lock · 2019-08-01T23:01:39Z

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 self-assigned this Sep 29, 2017

sharidas added the 2 - Developing label Sep 29, 2017

sharidas added this to the development milestone Sep 29, 2017

ghost mentioned this pull request Sep 30, 2017

exclude the temporary directory from the integrity check #29079

Closed

sharidas force-pushed the exclude-files-and-folders branch from af39124 to a8ac310 Compare October 2, 2017 07:20

sharidas added 3 - To Review and removed 2 - Developing labels Oct 3, 2017

sharidas changed the title ~~[WIP] Move excluded files/folders to config array~~ Move excluded files/folders to config array Oct 3, 2017

sharidas requested review from DeepDiver1975 and PVince81 October 3, 2017 09:27

sharidas changed the title ~~Move excluded files/folders to config array~~ [WIP] Move excluded files/folders to config array Oct 3, 2017

tomneedham reviewed Oct 3, 2017

View reviewed changes

sharidas force-pushed the exclude-files-and-folders branch from a8ac310 to 675fe9b Compare October 3, 2017 11:29

sharidas changed the title ~~[WIP] Move excluded files/folders to config array~~ Move excluded files/folders to config array Oct 4, 2017

DeepDiver1975 requested changes Oct 5, 2017

View reviewed changes

sharidas force-pushed the exclude-files-and-folders branch from 675fe9b to 5db4b07 Compare November 3, 2017 14:40

mmattel reviewed Nov 5, 2017

View reviewed changes

sharidas force-pushed the exclude-files-and-folders branch 2 times, most recently from 365aff4 to ba236b6 Compare November 6, 2017 05:03

sharidas changed the title ~~Move excluded files/folders to config array~~ Move excluded files to config array Nov 6, 2017

sharidas force-pushed the exclude-files-and-folders branch from ba236b6 to 5e729d7 Compare November 6, 2017 08:16

PVince81 reviewed Nov 6, 2017

View reviewed changes

sharidas force-pushed the exclude-files-and-folders branch from 5e729d7 to 5b26179 Compare November 6, 2017 09:45

PVince81 reviewed Nov 6, 2017

View reviewed changes

sharidas force-pushed the exclude-files-and-folders branch from 5b26179 to d5eff56 Compare November 6, 2017 14:04

peterprochaska suggested changes Nov 7, 2017

View reviewed changes

Move excluded files to config array

bbf96e9

Moving excluded files to config array. Instead of populating in the ExcludeFile* php files, we can have it in config.php. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the exclude-files-and-folders branch from d5eff56 to bbf96e9 Compare November 7, 2017 10:18

peterprochaska approved these changes Nov 8, 2017

View reviewed changes

PVince81 merged commit 94e2727 into master Nov 8, 2017

PVince81 deleted the exclude-files-and-folders branch November 8, 2017 08:42

phil-davis mentioned this pull request Nov 8, 2017

[stable10] Move excluded files to config array #29460

Merged

9 tasks

mmattel reviewed Nov 14, 2017

View reviewed changes

mmattel mentioned this pull request Dec 18, 2017

Description text in integrity.excluded.files in config.php.sample is wrong #29880

Closed

PVince81 mentioned this pull request Dec 18, 2017

Integrity Check: Move excluded files/folders into config array #23186

Closed

ownclouders added the status/STALE label Dec 20, 2017

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

Move excluded files to config array #29126

Move excluded files to config array #29126

Uh oh!

Conversation

sharidas commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

tomneedham commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Oct 10, 2017

Uh oh!

PVince81 commented Oct 18, 2017

Uh oh!

mmattel commented Oct 23, 2017

Uh oh!

sharidas commented Nov 3, 2017

Uh oh!

codecov bot commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

sharidas commented Sep 29, 2017 •

edited

Loading

codecov bot commented Nov 3, 2017 •

edited

Loading