Merge pull request #988 from nextcloud/ocs_federation

Move OCSAuthAPI to AppFramework

3 files changed, 82 insertions, 64 deletions

diff --git a/apps/federation/appinfo/routes.php b/apps/federation/appinfo/routes.php
index 03acc60c682..b9515812a01 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
index a22de155d4c..6cd3b1890ef 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,18 +91,20 @@ class OCSAuthAPI {
 	}
 
 	/**
+	 * @NoCSRFRequired
+	 * @PublicPage
+	 *
 	 * request received to ask remote server for a shared secret
 	 *
-	 * @return \OC_OCS_Result
+	 * @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']);
-			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 +115,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,23 +135,24 @@ 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
+	 * @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']);
-			return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN);
+			throw new OCSForbiddenException();
 		}
 
 		if ($this->isValidToken($url, $token) === false) {
@@ -156,7 +161,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 +170,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
index 7861e917ff8..2b231b4fca0 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,21 +91,19 @@ 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';
 
-		$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);
 		$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 +113,19 @@ class OCSAuthAPITest extends TestCase {
 			$this->jobList->expects($this->never())->method('remove');
 		}
 
-		$result = $this->ocsAuthApi->requestSharedSecret();
-		$this->assertSame($expected, $result->getStatusCode());
+		try {
+			$this->ocsAuthApi->requestSharedSecret($url, $token);
+			$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,20 +134,18 @@ 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';
 
-		$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 +161,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 +175,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($url, $token);
+			$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],
 		];
 	}