wbrawner/server
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #10628 from nextcloud/feature/10154/app-directory-permission-check

Browse source 
Adds a permission check for app directories
This commit is contained in:
Morris Jobke 2018-08-20 17:08:18 +02:00 committed by GitHub
commit 37869d9b2f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 217 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
core/js/setupchecks.js
View file

@ -316,6 +316,23 @@
type: OC.SetupChecks.MESSAGE_TYPE_WARNING
});
}
if(data.appDirsWithDifferentOwner.length > 0) {
var appDirsWithDifferentOwner = data.appDirsWithDifferentOwner.reduce(
function(appDirsWithDifferentOwner, directory) {
return appDirsWithDifferentOwner + '<li>' + directory + '</li>';
},
''
);
messages.push({
msg: t('core', 'Some app directories are owned by a different user than the web server one. ' +
'This may be the case if apps have been installed manually. ' +
'Check the permissions of the following app directories:')
+ '<ul>' + appDirsWithDifferentOwner + '</ul>',
type: OC.SetupChecks.MESSAGE_TYPE_WARNING
});
}
} else {
messages.push({
msg: t('core', 'Error occurred while checking server setup'),

80
core/js/tests/specs/setupchecksSpec.js
View file

@ -170,7 +170,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -217,7 +218,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -265,7 +267,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -311,7 +314,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -355,7 +359,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -368,6 +373,53 @@ describe('OC.SetupChecks tests', function() {
});
});
it('should return a warning if there are app directories with wrong permissions', function(done) {
var async = OC.SetupChecks.checkSetup();
suite.server.requests[0].respond(
200,
{
'Content-Type': 'application/json',
},
JSON.stringify({
hasFileinfoInstalled: true,
isGetenvServerWorking: true,
isReadOnlyConfig: false,
hasWorkingFileLocking: true,
hasValidTransactionIsolationLevel: true,
suggestedOverwriteCliURL: '',
isUrandomAvailable: true,
securityDocs: 'https://docs.owncloud.org/myDocs.html',
serverHasInternetConnection: true,
isMemcacheConfigured: true,
forwardedForHeadersWorking: true,
isCorrectMemcachedPHPModuleInstalled: true,
hasPassedCodeIntegrityCheck: true,
isOpcacheProperlySetup: true,
hasOpcacheLoaded: true,
isSettimelimitAvailable: true,
hasFreeTypeSupport: true,
missingIndexes: [],
outdatedCaches: [],
cronErrors: [],
cronInfo: {
diffInSeconds: 0
},
appDirsWithDifferentOwner: [
'/some/path'
]
})
);
async.done(function( data, s, x ){
expect(data).toEqual([{
msg: 'Some app directories are owned by a different user than the web server one. This may be the case if apps have been installed manually. Check the permissions of the following app directories:<ul><li>/some/path</li></ul>',
type: OC.SetupChecks.MESSAGE_TYPE_WARNING
}]);
done();
});
});
it('should return an error if the forwarded for headers are not working', function(done) {
var async = OC.SetupChecks.checkSetup();
@ -399,7 +451,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -443,7 +496,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -508,7 +562,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -553,7 +608,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -598,7 +654,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);
@ -643,7 +700,8 @@ describe('OC.SetupChecks tests', function() {
cronErrors: [],
cronInfo: {
diffInSeconds: 0
}
},
appDirsWithDifferentOwner: []
})
);

53
settings/Controller/CheckSetupController.php
View file

@ -31,9 +31,11 @@
namespace OC\Settings\Controller;
use bantu\IniGetWrapper\IniGetWrapper;
use DirectoryIterator;
use Doctrine\DBAL\DBALException;
use Doctrine\DBAL\Platforms\SqlitePlatform;
use GuzzleHttp\Exception\ClientException;
use OC;
use OC\AppFramework\Http;
use OC\DB\Connection;
use OC\DB\MissingIndexInformation;
@ -529,6 +531,54 @@ Raw output
return function_exists('opcache_get_status');
}
/**
* Iterates through the configured app roots and
* tests if the subdirectories are owned by the same user than the current user.
*
* @return array
*/
protected function getAppDirsWithDifferentOwner(): array {
$currentUser = posix_getpwuid(posix_getuid());
$appDirsWithDifferentOwner = [];
foreach (OC::$APPSROOTS as $appRoot) {
if ($appRoot['writable'] === true) {
$appDirsWithDifferentOwner = array_merge(
$appDirsWithDifferentOwner,
$this->getAppDirsWithDifferentOwnerForAppRoot($currentUser, $appRoot)
);
}
}
sort($appDirsWithDifferentOwner);
return $appDirsWithDifferentOwner;
}
/**
* Tests if the directories for one apps directory are writable by the current user.
*
* @param array $currentUser The current user
* @param array $appRoot The app root config
* @return string[] The none writable directory paths inside the app root
*/
private function getAppDirsWithDifferentOwnerForAppRoot(array $currentUser, array $appRoot): array {
$appDirsWithDifferentOwner = [];
$appsPath = $appRoot['path'];
$appsDir = new DirectoryIterator($appRoot['path']);
foreach ($appsDir as $fileInfo) {
if ($fileInfo->isDir() && !$fileInfo->isDot()) {
$absAppPath = $appsPath . DIRECTORY_SEPARATOR . $fileInfo->getFilename();
$appDirUser = posix_getpwuid(fileowner($absAppPath));
if ($appDirUser !== $currentUser) {
$appDirsWithDifferentOwner[] = $absAppPath . DIRECTORY_SEPARATOR . $fileInfo->getFilename();
}
}
}
return $appDirsWithDifferentOwner;
}
/**
* @return DataResponse
*/
@ -565,7 +615,8 @@ Raw output
'isSqliteUsed' => $this->isSqliteUsed(),
'databaseConversionDocumentation' => $this->urlGenerator->linkToDocs('admin-db-conversion'),
'isPhpMailerUsed' => $this->isPhpMailerUsed(),
'mailSettingsDocumentation' => $this->urlGenerator->getAbsoluteURL('index.php/settings/admin')
'mailSettingsDocumentation' => $this->urlGenerator->getAbsoluteURL('index.php/settings/admin'),
'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(),
]
);
}

79
tests/Settings/Controller/CheckSetupControllerTest.php
View file

@ -21,6 +21,7 @@
namespace Tests\Settings\Controller;
use OC;
use OC\DB\Connection;
use OC\Settings\Controller\CheckSetupController;
use OCP\AppFramework\Http;
@ -44,6 +45,7 @@ use OC\IntegrityCheck\Checker;
/**
* Class CheckSetupControllerTest
*
* @backupStaticAttributes
* @package Tests\Settings\Controller
*/
class CheckSetupControllerTest extends TestCase {
@ -74,6 +76,13 @@ class CheckSetupControllerTest extends TestCase {
/** @var IDateTimeFormatter|\PHPUnit_Framework_MockObject_MockObject */
private $dateTimeFormatter;
/**
* Holds a list of directories created during tests.
*
* @var array
*/
private $dirsToRemove = [];
public function setUp() {
parent::setUp();
@ -135,9 +144,23 @@ class CheckSetupControllerTest extends TestCase {
'isSqliteUsed',
'isPhpMailerUsed',
'hasOpcacheLoaded',
'getAppDirsWithDifferentOwner',
])->getMock();
}
/**
* Removes directories created during tests.
*
* @after
* @return void
*/
public function removeTestDirectories() {
foreach ($this->dirsToRemove as $dirToRemove) {
rmdir($dirToRemove);
}
$this->dirsToRemove = [];
}
public function testIsInternetConnectionWorkingDisabledViaConfig() {
$this->config->expects($this->once())
->method('getSystemValue')
@ -425,6 +448,11 @@ class CheckSetupControllerTest extends TestCase {
->method('hasPassedCheck')
->willReturn(true);
$this->checkSetupController
->expects($this->once())
->method('getAppDirsWithDifferentOwner')
->willReturn([]);
$expected = new DataResponse(
[
'isGetenvServerWorking' => true,
@ -465,6 +493,7 @@ class CheckSetupControllerTest extends TestCase {
'missingIndexes' => [],
'isPhpMailerUsed' => false,
'mailSettingsDocumentation' => 'https://server/index.php/settings/admin',
'appDirsWithDifferentOwner' => [],
]
);
$this->assertEquals($expected, $this->checkSetupController->check());
@ -571,6 +600,56 @@ class CheckSetupControllerTest extends TestCase {
$this->assertSame('', $this->invokePrivate($this->checkSetupController, 'isUsedTlsLibOutdated'));
}
/**
* Setups a temp directory and some subdirectories.
* Then calls the 'getAppDirsWithDifferentOwner' method.
* The result is expected to be empty since
* there are no directories with different owners than the current user.
*
* @return void
*/
public function testAppDirectoryOwnersOk() {
$tempDir = tempnam(sys_get_temp_dir(), 'apps') . 'dir';
mkdir($tempDir);
mkdir($tempDir . DIRECTORY_SEPARATOR . 'app1');
mkdir($tempDir . DIRECTORY_SEPARATOR . 'app2');
$this->dirsToRemove[] = $tempDir . DIRECTORY_SEPARATOR . 'app1';
$this->dirsToRemove[] = $tempDir . DIRECTORY_SEPARATOR . 'app2';
$this->dirsToRemove[] = $tempDir;
OC::$APPSROOTS = [
[
'path' => $tempDir,
'url' => '/apps',
'writable' => true,
],
];
$this->assertSame(
[],
$this->invokePrivate($this->checkSetupController, 'getAppDirsWithDifferentOwner')
);
}
/**
* Calls the check for a none existing app root that is marked as not writable.
* It's expected that no error happens since the check shouldn't apply.
*
* @return void
*/
public function testAppDirectoryOwnersNotWritable() {
$tempDir = tempnam(sys_get_temp_dir(), 'apps') . 'dir';
OC::$APPSROOTS = [
[
'path' => $tempDir,
'url' => '/apps',
'writable' => false,
],
];
$this->assertSame(
[],
$this->invokePrivate($this->checkSetupController, 'getAppDirsWithDifferentOwner')
);
}
public function testIsBuggyNss400() {
$this->config->expects($this->any())
->method('getSystemValue')