From 16ff207465335b624e67b9a9d0dae00ef23cd45c Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Aug 2016 14:25:59 +0200 Subject: [PATCH 1/2] Move OCSAuthAPI to AppFramework * Convert class * Convert routes * Convert tests --- apps/federation/appinfo/routes.php | 16 ++++- .../OCSAuthAPIController.php} | 44 ++++++++----- .../OCSAuthAPIControllerTest.php} | 65 ++++++++++--------- 3 files changed, 76 insertions(+), 49 deletions(-) rename apps/federation/lib/{API/OCSAuthAPI.php => Controller/OCSAuthAPIController.php} (85%) rename apps/federation/tests/{API/OCSAuthAPITest.php => Controller/OCSAuthAPIControllerTest.php} (79%) diff --git a/apps/federation/appinfo/routes.php b/apps/federation/appinfo/routes.php index 03acc60c68..b9515812a0 100644 --- a/apps/federation/appinfo/routes.php +++ b/apps/federation/appinfo/routes.php @@ -41,8 +41,18 @@ $application->registerRoutes( 'url' => '/auto-add-servers', 'verb' => 'POST' ], - ] + ], + 'ocs' => [ + [ + 'name' => 'OCSAuthAPI#getSharedSecret', + 'url' => '/api/v1/shared-secret', + 'verb' => 'GET', + ], + [ + 'name' => 'OCSAuthAPI#requestSharedSecret', + 'url' => '/api/v1/request-shared-secret', + 'verb' => 'POST', + ], + ], ] ); - -$application->registerOCSApi(); diff --git a/apps/federation/lib/API/OCSAuthAPI.php b/apps/federation/lib/Controller/OCSAuthAPIController.php similarity index 85% rename from apps/federation/lib/API/OCSAuthAPI.php rename to apps/federation/lib/Controller/OCSAuthAPIController.php index a22de155d4..68e0f8b271 100644 --- a/apps/federation/lib/API/OCSAuthAPI.php +++ b/apps/federation/lib/Controller/OCSAuthAPIController.php @@ -25,11 +25,13 @@ */ -namespace OCA\Federation\API; +namespace OCA\Federation\Controller; use OCA\Federation\DbHandler; use OCA\Federation\TrustedServers; use OCP\AppFramework\Http; +use OCP\AppFramework\OCS\OCSForbiddenException; +use OCP\AppFramework\OCSController; use OCP\BackgroundJob\IJobList; use OCP\ILogger; use OCP\IRequest; @@ -40,12 +42,9 @@ use OCP\Security\ISecureRandom; * * OCS API end-points to exchange shared secret between two connected ownClouds * - * @package OCA\Federation\API + * @package OCA\Federation\Controller */ -class OCSAuthAPI { - - /** @var IRequest */ - private $request; +class OCSAuthAPIController extends OCSController{ /** @var ISecureRandom */ private $secureRandom; @@ -65,6 +64,7 @@ class OCSAuthAPI { /** * OCSAuthAPI constructor. * + * @param string $appName * @param IRequest $request * @param ISecureRandom $secureRandom * @param IJobList $jobList @@ -73,6 +73,7 @@ class OCSAuthAPI { * @param ILogger $logger */ public function __construct( + $appName, IRequest $request, ISecureRandom $secureRandom, IJobList $jobList, @@ -80,7 +81,8 @@ class OCSAuthAPI { DbHandler $dbHandler, ILogger $logger ) { - $this->request = $request; + parent::__construct($appName, $request); + $this->secureRandom = $secureRandom; $this->jobList = $jobList; $this->trustedServers = $trustedServers; @@ -89,9 +91,13 @@ class OCSAuthAPI { } /** + * @NoCSRFRequired + * @PublicPage + * * request received to ask remote server for a shared secret * - * @return \OC_OCS_Result + * @return Http\DataResponse + * @throws OCSForbiddenException */ public function requestSharedSecret() { @@ -100,7 +106,7 @@ class OCSAuthAPI { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']); - return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); + throw new OCSForbiddenException(); } // if both server initiated the exchange of the shared secret the greater @@ -111,7 +117,7 @@ class OCSAuthAPI { 'remote server (' . $url . ') presented lower token. We will initiate the exchange of the shared secret.', ['app' => 'federation'] ); - return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); + throw new OCSForbiddenException(); } // we ask for the shared secret so we no longer have to ask the other server @@ -131,14 +137,17 @@ class OCSAuthAPI { ] ); - return new \OC_OCS_Result(null, Http::STATUS_OK); - + return new Http\DataResponse(); } /** + * @NoCSRFRequired + * @PublicPage + * * create shared secret and return it * - * @return \OC_OCS_Result + * @return Http\DataResponse + * @throws OCSForbiddenException */ public function getSharedSecret() { @@ -147,7 +156,7 @@ class OCSAuthAPI { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']); - return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); + throw new OCSForbiddenException(); } if ($this->isValidToken($url, $token) === false) { @@ -156,7 +165,7 @@ class OCSAuthAPI { 'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret', ['app' => 'federation'] ); - return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); + throw new OCSForbiddenException(); } $sharedSecret = $this->secureRandom->generate(32); @@ -165,8 +174,9 @@ class OCSAuthAPI { // reset token after the exchange of the shared secret was successful $this->dbHandler->addToken($url, ''); - return new \OC_OCS_Result(['sharedSecret' => $sharedSecret], Http::STATUS_OK); - + return new Http\DataResponse([ + 'sharedSecret' => $sharedSecret + ]); } protected function isValidToken($url, $token) { diff --git a/apps/federation/tests/API/OCSAuthAPITest.php b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php similarity index 79% rename from apps/federation/tests/API/OCSAuthAPITest.php rename to apps/federation/tests/Controller/OCSAuthAPIControllerTest.php index 7861e917ff..ba10ce57c3 100644 --- a/apps/federation/tests/API/OCSAuthAPITest.php +++ b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php @@ -22,20 +22,21 @@ */ -namespace OCA\Federation\Tests\API; +namespace OCA\Federation\Tests\Controller; use OC\BackgroundJob\JobList; -use OCA\Federation\API\OCSAuthAPI; +use OCA\Federation\Controller\OCSAuthAPIController; use OCA\Federation\DbHandler; use OCA\Federation\TrustedServers; use OCP\AppFramework\Http; +use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\ILogger; use OCP\IRequest; use OCP\Security\ISecureRandom; use Test\TestCase; -class OCSAuthAPITest extends TestCase { +class OCSAuthAPIControllerTest extends TestCase { /** @var \PHPUnit_Framework_MockObject_MockObject | IRequest */ private $request; @@ -55,14 +56,14 @@ class OCSAuthAPITest extends TestCase { /** @var \PHPUnit_Framework_MockObject_MockObject | ILogger */ private $logger; - /** @var OCSAuthApi */ + /** @var OCSAuthAPIController */ private $ocsAuthApi; public function setUp() { parent::setUp(); - $this->request = $this->getMock('OCP\IRequest'); - $this->secureRandom = $this->getMock('OCP\Security\ISecureRandom'); + $this->request = $this->getMockBuilder('OCP\IRequest')->getMock(); + $this->secureRandom = $this->getMockBuilder('OCP\Security\ISecureRandom')->getMock(); $this->trustedServers = $this->getMockBuilder('OCA\Federation\TrustedServers') ->disableOriginalConstructor()->getMock(); $this->dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler') @@ -72,7 +73,8 @@ class OCSAuthAPITest extends TestCase { $this->logger = $this->getMockBuilder('OCP\ILogger') ->disableOriginalConstructor()->getMock(); - $this->ocsAuthApi = new OCSAuthAPI( + $this->ocsAuthApi = new OCSAuthAPIController( + 'federation', $this->request, $this->secureRandom, $this->jobList, @@ -89,9 +91,9 @@ class OCSAuthAPITest extends TestCase { * @param string $token * @param string $localToken * @param bool $isTrustedServer - * @param int $expected + * @param bool $ok */ - public function testRequestSharedSecret($token, $localToken, $isTrustedServer, $expected) { + public function testRequestSharedSecret($token, $localToken, $isTrustedServer, $ok) { $url = 'url'; @@ -103,7 +105,7 @@ class OCSAuthAPITest extends TestCase { $this->dbHandler->expects($this->any()) ->method('getToken')->with($url)->willReturn($localToken); - if ($expected === Http::STATUS_OK) { + if ($ok) { $this->jobList->expects($this->once())->method('add') ->with('OCA\Federation\BackgroundJob\GetSharedSecret', ['url' => $url, 'token' => $token]); $this->jobList->expects($this->once())->method('remove') @@ -113,15 +115,19 @@ class OCSAuthAPITest extends TestCase { $this->jobList->expects($this->never())->method('remove'); } - $result = $this->ocsAuthApi->requestSharedSecret(); - $this->assertSame($expected, $result->getStatusCode()); + try { + $result = $this->ocsAuthApi->requestSharedSecret(); + $this->assertTrue($ok); + } catch (OCSForbiddenException $e) { + $this->assertFalse($ok); + } } public function dataTestRequestSharedSecret() { return [ - ['token2', 'token1', true, Http::STATUS_OK], - ['token1', 'token2', false, Http::STATUS_FORBIDDEN], - ['token1', 'token2', true, Http::STATUS_FORBIDDEN], + ['token2', 'token1', true, true], + ['token1', 'token2', false, false], + ['token1', 'token2', true, false], ]; } @@ -130,9 +136,9 @@ class OCSAuthAPITest extends TestCase { * * @param bool $isTrustedServer * @param bool $isValidToken - * @param int $expected + * @param bool $ok */ - public function testGetSharedSecret($isTrustedServer, $isValidToken, $expected) { + public function testGetSharedSecret($isTrustedServer, $isValidToken, $ok) { $url = 'url'; $token = 'token'; @@ -140,10 +146,11 @@ class OCSAuthAPITest extends TestCase { $this->request->expects($this->at(0))->method('getParam')->with('url')->willReturn($url); $this->request->expects($this->at(1))->method('getParam')->with('token')->willReturn($token); - /** @var OCSAuthAPI | \PHPUnit_Framework_MockObject_MockObject $ocsAuthApi */ - $ocsAuthApi = $this->getMockBuilder('OCA\Federation\API\OCSAuthAPI') + /** @var OCSAuthAPIController | \PHPUnit_Framework_MockObject_MockObject $ocsAuthApi */ + $ocsAuthApi = $this->getMockBuilder('OCA\Federation\Controller\OCSAuthAPIController') ->setConstructorArgs( [ + 'federation', $this->request, $this->secureRandom, $this->jobList, @@ -159,7 +166,7 @@ class OCSAuthAPITest extends TestCase { $ocsAuthApi->expects($this->any()) ->method('isValidToken')->with($url, $token)->willReturn($isValidToken); - if($expected === Http::STATUS_OK) { + if($ok) { $this->secureRandom->expects($this->once())->method('generate')->with(32) ->willReturn('secret'); $this->trustedServers->expects($this->once()) @@ -173,22 +180,22 @@ class OCSAuthAPITest extends TestCase { $this->dbHandler->expects($this->never())->method('addToken'); } - $result = $ocsAuthApi->getSharedSecret(); - - $this->assertSame($expected, $result->getStatusCode()); - - if ($expected === Http::STATUS_OK) { + try { + $result = $ocsAuthApi->getSharedSecret(); + $this->assertTrue($ok); $data = $result->getData(); $this->assertSame('secret', $data['sharedSecret']); + } catch (OCSForbiddenException $e) { + $this->assertFalse($ok); } } public function dataTestGetSharedSecret() { return [ - [true, true, Http::STATUS_OK], - [false, true, Http::STATUS_FORBIDDEN], - [true, false, Http::STATUS_FORBIDDEN], - [false, false, Http::STATUS_FORBIDDEN], + [true, true, true], + [false, true, false], + [true, false, false], + [false, false, false], ]; } From b6520827f704b304b0df6e0a618f28b96a3e04c7 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Aug 2016 21:41:55 +0200 Subject: [PATCH 2/2] Use function parameters --- .../lib/Controller/OCSAuthAPIController.php | 16 ++++++---------- .../Controller/OCSAuthAPIControllerTest.php | 9 ++------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php index 68e0f8b271..6cd3b1890e 100644 --- a/apps/federation/lib/Controller/OCSAuthAPIController.php +++ b/apps/federation/lib/Controller/OCSAuthAPIController.php @@ -96,14 +96,12 @@ class OCSAuthAPIController extends OCSController{ * * request received to ask remote server for a shared secret * + * @param string $url + * @param string $token * @return Http\DataResponse * @throws OCSForbiddenException */ - public function requestSharedSecret() { - - $url = $this->request->getParam('url'); - $token = $this->request->getParam('token'); - + public function requestSharedSecret($url, $token) { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']); throw new OCSForbiddenException(); @@ -146,14 +144,12 @@ class OCSAuthAPIController extends OCSController{ * * create shared secret and return it * + * @param string $url + * @param string $token * @return Http\DataResponse * @throws OCSForbiddenException */ - public function getSharedSecret() { - - $url = $this->request->getParam('url'); - $token = $this->request->getParam('token'); - + public function getSharedSecret($url, $token) { if ($this->trustedServers->isTrustedServer($url) === false) { $this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']); throw new OCSForbiddenException(); diff --git a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php index ba10ce57c3..2b231b4fca 100644 --- a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php +++ b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php @@ -97,8 +97,6 @@ class OCSAuthAPIControllerTest extends TestCase { $url = 'url'; - $this->request->expects($this->at(0))->method('getParam')->with('url')->willReturn($url); - $this->request->expects($this->at(1))->method('getParam')->with('token')->willReturn($token); $this->trustedServers ->expects($this->once()) ->method('isTrustedServer')->with($url)->willReturn($isTrustedServer); @@ -116,7 +114,7 @@ class OCSAuthAPIControllerTest extends TestCase { } try { - $result = $this->ocsAuthApi->requestSharedSecret(); + $this->ocsAuthApi->requestSharedSecret($url, $token); $this->assertTrue($ok); } catch (OCSForbiddenException $e) { $this->assertFalse($ok); @@ -143,9 +141,6 @@ class OCSAuthAPIControllerTest extends TestCase { $url = 'url'; $token = 'token'; - $this->request->expects($this->at(0))->method('getParam')->with('url')->willReturn($url); - $this->request->expects($this->at(1))->method('getParam')->with('token')->willReturn($token); - /** @var OCSAuthAPIController | \PHPUnit_Framework_MockObject_MockObject $ocsAuthApi */ $ocsAuthApi = $this->getMockBuilder('OCA\Federation\Controller\OCSAuthAPIController') ->setConstructorArgs( @@ -181,7 +176,7 @@ class OCSAuthAPIControllerTest extends TestCase { } try { - $result = $ocsAuthApi->getSharedSecret(); + $result = $ocsAuthApi->getSharedSecret($url, $token); $this->assertTrue($ok); $data = $result->getData(); $this->assertSame('secret', $data['sharedSecret']);