From 1406d0bc5cea8881ed472442b31b082988709d2c Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 4 Oct 2018 17:25:09 +0200 Subject: [PATCH 1/4] Add initial tests for PageController::getServer2ServerProperties method While reviewing this method as part of a software audit, I noticed a couple of areas where the method could, potentially, be streamlined, and improved. This commit adds the initial set of tests which make those improvements possible. --- tests/unit/controller/PageControllerTest.php | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/unit/controller/PageControllerTest.php b/tests/unit/controller/PageControllerTest.php index 625a3ea6c2..0ae758be7b 100644 --- a/tests/unit/controller/PageControllerTest.php +++ b/tests/unit/controller/PageControllerTest.php @@ -169,6 +169,39 @@ public function testPublicIndexWithFileToken() { $this->assertEquals($template->getRedirectURL(), $response->getRedirectURL()); } + /** + * @return array + */ + public function getServer2ServerPropertiesDataProvider() { + return [ + ['I am a password', 'yes', true, 'true'], + ['', 'yes', true, 'false'], + [null, 'yes', true, 'false'], + ]; + } + + /** + * @throws \ReflectionException + * @dataProvider getServer2ServerPropertiesDataProvider + */ + public function testGetServer2ServerProperties( + $password, + $server2ServerSharingEnabled, + $expectedSharing, + $expectedPasswordProtected + ) { + $this->mockGetSharePassword($password); + $this->mockGetAppValue($server2ServerSharingEnabled); + + $reflection = new \ReflectionClass(\get_class($this->controller)); + $method = $reflection->getMethod('getServer2ServerProperties'); + $method->setAccessible(true); + list($server2ServerSharing, $passwordProtected) = $method->invokeArgs($this->controller, []); + + $this->assertSame($expectedSharing, $server2ServerSharing, 'Incorrect server to server sharing result returned'); + $this->assertSame($expectedPasswordProtected, $passwordProtected, 'Incorrect password protected result returned'); + } + public function testErrorPage() { $message = 'Not found!'; $code = Http::STATUS_NOT_FOUND; From 766a1be3a803cf02353c6c646f388861afaa21f1 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 4 Oct 2018 17:27:29 +0200 Subject: [PATCH 2/4] Simplify the server2ServerSharing evaluation $server2ServerSharing doesn't need to be instantiated, as the comparison natively returns true or false. As a result, returning the result of the comparison can be done when initialising the returned array. --- controller/pagecontroller.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/controller/pagecontroller.php b/controller/pagecontroller.php index 14192f0c50..51b0d580d4 100644 --- a/controller/pagecontroller.php +++ b/controller/pagecontroller.php @@ -239,10 +239,12 @@ private function getServer2ServerProperties() { $server2ServerSharing = $this->appConfig->getAppValue( 'files_sharing', 'outgoing_server2server_share_enabled', 'yes' ); - $server2ServerSharing = ($server2ServerSharing === 'yes') ? true : false; $password = $this->environment->getSharePassword(); $passwordProtected = ($password) ? 'true' : 'false'; - return [$server2ServerSharing, $passwordProtected]; + return [ + ($server2ServerSharing === 'yes'), + $passwordProtected + ]; } } From b58ff854a373cb851cd813ff4f7c512552031a66 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 4 Oct 2018 17:39:21 +0200 Subject: [PATCH 3/4] Simplify the evaluation of the share password While there's nothing wrong with how it's currently done, I feel that it's overly verbose, and can be simplified, as in this commit. By doing so, it should make it that much easier to read and maintain. --- controller/pagecontroller.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controller/pagecontroller.php b/controller/pagecontroller.php index 51b0d580d4..dae500d08e 100644 --- a/controller/pagecontroller.php +++ b/controller/pagecontroller.php @@ -239,12 +239,10 @@ private function getServer2ServerProperties() { $server2ServerSharing = $this->appConfig->getAppValue( 'files_sharing', 'outgoing_server2server_share_enabled', 'yes' ); - $password = $this->environment->getSharePassword(); - $passwordProtected = ($password) ? 'true' : 'false'; return [ ($server2ServerSharing === 'yes'), - $passwordProtected + ($this->environment->getSharePassword()) ? 'true' : 'false' ]; } } From 3faef27f87bf233a5d035f1a55b1f983fc3ec8b9 Mon Sep 17 00:00:00 2001 From: Matthew Setter Date: Thu, 4 Oct 2018 17:40:28 +0200 Subject: [PATCH 4/4] Improve readability of getAppValue call Potentially not necessary, but having the function arguments listed underneath each other seemed to be that much easier to read. Perhaps it's just me, but it seems worth doing. --- controller/pagecontroller.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/controller/pagecontroller.php b/controller/pagecontroller.php index dae500d08e..18ea2551ba 100644 --- a/controller/pagecontroller.php +++ b/controller/pagecontroller.php @@ -237,7 +237,9 @@ private function showPublicPage($token) { */ private function getServer2ServerProperties() { $server2ServerSharing = $this->appConfig->getAppValue( - 'files_sharing', 'outgoing_server2server_share_enabled', 'yes' + 'files_sharing', + 'outgoing_server2server_share_enabled', + 'yes' ); return [