-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Move excluded files to config array #29126
Conversation
af39124
to
a8ac310
Compare
should also include a sample entry in config.sample.php |
@@ -398,13 +398,21 @@ private function verify($signaturePath, $basePath, $certificateCN, $force = fals | |||
throw new InvalidSignatureException('Signature could not get verified.'); | |||
} | |||
|
|||
//Exclude files which shouldn't fall for comparison | |||
$excludeFiles = \OC::$server->getSystemConfig()->getValue('excludedFiles', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default should be an array since we use it in in_array
later
a8ac310
to
675fe9b
Compare
config/config.sample.php
Outdated
'excludedFolders' => | ||
array ( | ||
0 => '/home/sujith/test/owncloud/data', | ||
1 => '/home/sujith/test/owncloud/themes', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use OC::$SERVERROOT
example
OC::$SERVERROOT . '/assets'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we shall find good example folders - maybe two are enough.
/apps/my-theme might be a good example and /assets
config/config.sample.php
Outdated
/** | ||
* Exclude file name patterns from the integrity checker command | ||
*/ | ||
'excludedFilenamePatterns' => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the pattern enough to also properly exclude folders and files from above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -398,13 +398,21 @@ private function verify($signaturePath, $basePath, $certificateCN, $force = fals | |||
throw new InvalidSignatureException('Signature could not get verified.'); | |||
} | |||
|
|||
//Exclude files which shouldn't fall for comparison | |||
$excludeFiles = \OC::$server->getSystemConfig()->getValue('excludedFiles', []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inject config instance - no use of \OC::$server inside any class please
@@ -78,4 +85,12 @@ public function accept() { | |||
|
|||
return true; | |||
} | |||
|
|||
protected function getExcludedFileNames() { | |||
return \OC::$server->getConfig()->getSystemValue('excludedFiles', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep proper defaults - basically just like before
lib/private/Setup.php
Outdated
@@ -317,6 +340,9 @@ public function install($options) { | |||
'overwrite.cli.url' => $request->getServerProtocol() . '://' . $request->getInsecureServerHost() . \OC::$WEBROOT, | |||
'dbtype' => $dbType, | |||
'version' => implode('.', \OCP\Util::getVersion()), | |||
'excludedFolders' => $excludeFolders, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -497,114 +497,6 @@ public function testWriteCoreSignature() { | |||
$this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/app/'); | |||
} | |||
|
|||
public function testWriteCoreSignatureWithUnmodifiedHtaccess() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did these tests go?
I heard from @Peter-Prochaska that it might not be a good idea to allow excluding whole folders, so not sure... See #29094 (comment) |
|
Maybe only a small detail to avoid naming confusion: |
675fe9b
to
5db4b07
Compare
Updated this PR to exclude files only and not folders. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
config/config.sample.php
Outdated
* Exclude directories from the integrity checker command | ||
*/ | ||
'excludedFiles' => | ||
array ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description text: Exclude directories --> Exclude files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see my comment above to avoid confusion:
This variable should be renamed from excludedFiles
to integrityExcludedFiles
365aff4
to
ba236b6
Compare
Not sure how to make codecov happy for config.sample.php. Any pointers would be helpful. Thanks. |
ba236b6
to
5e729d7
Compare
config/config.sample.php
Outdated
*/ | ||
'integrityExcludedFiles' => | ||
array ( | ||
0 => '.DS_Store', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the numeric index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the config.sample.php by removing numeric index.
5e729d7
to
5b26179
Compare
Backport PR to stable10 branch: #29460 |
config/config.sample.php
Outdated
@@ -1210,6 +1210,25 @@ | |||
'~snapshot', | |||
), | |||
/** | |||
* Exclude directories from the integrity checker command | |||
*/ | |||
'integrityExcludedFiles' => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to integrity.excluded.files
@Peter-Prochaska second review ? |
5b26179
to
d5eff56
Compare
@@ -230,7 +230,7 @@ private function generateHashes(\RecursiveIteratorIterator $iterator, | |||
* @param array $hashes | |||
* @param X509 $certificate | |||
* @param RSA $privateKey | |||
* @return string | |||
* @return mixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why mixed? it's an array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 <sharidasan@owncloud.com>
d5eff56
to
bbf96e9
Compare
@@ -1210,6 +1210,18 @@ | |||
'~snapshot', | |||
), | |||
/** | |||
* Exclude directories from the integrity checker command | |||
*/ | |||
'integrity.excluded.files' => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PVince81 just seen that is has been fixed ...
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. |
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 sharidasan@owncloud.com
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
Checklist: