Commit e8b4d4bc authored by Roeland Jago Douma's avatar Roeland Jago Douma Committed by Thomas Müller
Browse files

OCS routes to appframework (#24886)

* Allow registering of OCS routes with the appframework

* Make the router handle OCS AppFramework Routes

* Make the OCS endpoint handle the new OCS AppFramework routes

* Move Sharees api over to use OCSController

* Move federation api over to use OCSController - fixes #20771

* Inject router into url generator and make tests db independent

* Fixing typo

* Fixing unit tests in federation and sharing related to changes in ocs routes

* Default response format for OCS is xml

* Handle LoginException in ocs calls

* Fix comment

* Fix webroot in router

* Adding undefined field in SessionMiddleware

* Federation handshake calls do no require any authentication

* Adding integration test to ensure that federation handshake behaves properly
parent a84fba41
......@@ -40,8 +40,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();
......@@ -102,41 +102,7 @@ class Application extends \OCP\AppFramework\App {
private function registerMiddleware() {
$container = $this->getContainer();
$container->registerMiddleware('addServerMiddleware');
}
/**
* register OCS API Calls
*/
public function registerOCSApi() {
$container = $this->getContainer();
$server = $container->getServer();
$auth = new OCSAuthAPI(
$server->getRequest(),
$server->getSecureRandom(),
$server->getJobList(),
$container->query('TrustedServers'),
$container->query('DbHandler'),
$server->getLogger()
);
API::register('get',
'/apps/federation/api/v1/shared-secret',
[$auth, 'getSharedSecret'],
'federation',
API::GUEST_AUTH
);
API::register('post',
'/apps/federation/api/v1/request-shared-secret',
[$auth, 'requestSharedSecret'],
'federation',
API::GUEST_AUTH
);
$container->registerMiddleWare('addServerMiddleware');
}
/**
......
......@@ -23,28 +23,25 @@
*
*/
namespace OCA\Federation\API;
namespace OCA\Federation\Controller;
use OCA\Federation\DbHandler;
use OCA\Federation\TrustedServers;
use OCP\AppFramework\Http;
use OCP\AppFramework\OCSController;
use OCP\BackgroundJob\IJobList;
use OCP\ILogger;
use OCP\IRequest;
use OCP\Security\ISecureRandom;
/**
* Class OCSAuthAPI
* Class OCSAuthAPIController
*
* 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;
......@@ -64,6 +61,7 @@ class OCSAuthAPI {
/**
* OCSAuthAPI constructor.
*
* @param string $appName
* @param IRequest $request
* @param ISecureRandom $secureRandom
* @param IJobList $jobList
......@@ -72,6 +70,7 @@ class OCSAuthAPI {
* @param ILogger $logger
*/
public function __construct(
$appName,
IRequest $request,
ISecureRandom $secureRandom,
IJobList $jobList,
......@@ -79,7 +78,8 @@ class OCSAuthAPI {
DbHandler $dbHandler,
ILogger $logger
) {
$this->request = $request;
parent::__construct($appName, $request);
$this->secureRandom = $secureRandom;
$this->jobList = $jobList;
$this->trustedServers = $trustedServers;
......@@ -88,18 +88,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 array()
*/
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);
return ['statuscode' => Http::STATUS_FORBIDDEN];
}
// if both server initiated the exchange of the shared secret the greater
......@@ -110,7 +112,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);
return ['statuscode' => Http::STATUS_FORBIDDEN];
}
// we ask for the shared secret so we no longer have to ask the other server
......@@ -130,23 +132,24 @@ class OCSAuthAPI {
]
);
return new \OC_OCS_Result(null, Http::STATUS_OK);
return ['statuscode' => Http::STATUS_OK];
}
/**
* @NoCSRFRequired
* @PublicPage
*
* create shared secret and return it
*
* @return \OC_OCS_Result
* @param string $url
* @param string $token
* @return array
*/
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);
return ['statuscode' => Http::STATUS_FORBIDDEN];
}
if ($this->isValidToken($url, $token) === false) {
......@@ -155,7 +158,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);
return ['statuscode' => Http::STATUS_FORBIDDEN];
}
$sharedSecret = $this->secureRandom->generate(32);
......@@ -164,8 +167,8 @@ 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 ['statuscode' => Http::STATUS_OK,
'data' => ['sharedSecret' => $sharedSecret]];
}
protected function isValidToken($url, $token) {
......
......@@ -25,7 +25,7 @@ namespace OCA\Federation\Tests\API;
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;
......@@ -54,24 +54,25 @@ 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->createMock('OCP\IRequest');
$this->secureRandom = $this->createMock('OCP\Security\ISecureRandom');
$this->trustedServers = $this->getMockBuilder('OCA\Federation\TrustedServers')
$this->request = $this->createMock(IRequest::class);
$this->secureRandom = $this->createMock(ISecureRandom::class);
$this->trustedServers = $this->getMockBuilder(TrustedServers::class)
->disableOriginalConstructor()->getMock();
$this->dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')
$this->dbHandler = $this->getMockBuilder(DbHandler::class)
->disableOriginalConstructor()->getMock();
$this->jobList = $this->getMockBuilder('OC\BackgroundJob\JobList')
$this->jobList = $this->getMockBuilder(JobList::class)
->disableOriginalConstructor()->getMock();
$this->logger = $this->getMockBuilder('OCP\ILogger')
$this->logger = $this->getMockBuilder(ILogger::class)
->disableOriginalConstructor()->getMock();
$this->ocsAuthApi = new OCSAuthAPI(
$this->ocsAuthApi = new OCSAuthAPIController(
'federation',
$this->request,
$this->secureRandom,
$this->jobList,
......@@ -94,8 +95,6 @@ class OCSAuthAPITest 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);
......@@ -112,8 +111,8 @@ class OCSAuthAPITest extends TestCase {
$this->jobList->expects($this->never())->method('remove');
}
$result = $this->ocsAuthApi->requestSharedSecret();
$this->assertSame($expected, $result->getStatusCode());
$result = $this->ocsAuthApi->requestSharedSecret($url, $token);
$this->assertSame($expected, $result['statuscode']);
}
public function dataTestRequestSharedSecret() {
......@@ -136,13 +135,11 @@ class OCSAuthAPITest 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 OCSAuthAPI | \PHPUnit_Framework_MockObject_MockObject $ocsAuthApi */
$ocsAuthApi = $this->getMockBuilder('OCA\Federation\API\OCSAuthAPI')
/** @var OCSAuthAPIController | \PHPUnit_Framework_MockObject_MockObject $ocsAuthApi */
$ocsAuthApi = $this->getMockBuilder(OCSAuthAPIController::class)
->setConstructorArgs(
[
'federation',
$this->request,
$this->secureRandom,
$this->jobList,
......@@ -172,12 +169,12 @@ class OCSAuthAPITest extends TestCase {
$this->dbHandler->expects($this->never())->method('addToken');
}
$result = $ocsAuthApi->getSharedSecret();
$result = $ocsAuthApi->getSharedSecret($url, $token);
$this->assertSame($expected, $result->getStatusCode());
$this->assertSame($expected, $result['statuscode']);
if ($expected === Http::STATUS_OK) {
$data = $result->getData();
$data = $result['data'];
$this->assertSame('secret', $data['sharedSecret']);
}
}
......
......@@ -39,6 +39,13 @@ $application->registerRoutes($this, [
'verb' => 'GET'
],
],
'ocs' => [
[
'name' => 'sharees#search',
'url' => '/api/v1/sharees',
'verb' => 'GET',
]
]
]);
/** @var $this \OCP\Route\IRouter */
......@@ -116,20 +123,3 @@ API::register('delete',
'/apps/files_sharing/api/v1/remote_shares/{id}',
['\OCA\Files_Sharing\API\Remote', 'unshare'],
'files_sharing');
$sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(),
\OC::$server->getUserManager(),
\OC::$server->getContactsManager(),
\OC::$server->getConfig(),
\OC::$server->getUserSession(),
\OC::$server->getURLGenerator(),
\OC::$server->getRequest(),
\OC::$server->getLogger(),
\OC::$server->getShareManager());
API::register('get',
'/apps/files_sharing/api/v1/sharees',
[$sharees, 'search'],
'files_sharing', API::USER_AUTH);
......@@ -21,9 +21,12 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\Files_Sharing\API;
namespace OCA\Files_Sharing\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCSController;
use OCP\Contacts\IManager;
use OCP\IGroup;
use OCP\IGroupManager;
......@@ -36,7 +39,7 @@ use OCP\IUserSession;
use OCP\IURLGenerator;
use OCP\Share;
class Sharees {
class ShareesController extends OCSController {
/** @var IGroupManager */
protected $groupManager;
......@@ -102,15 +105,18 @@ class Sharees {
* @param ILogger $logger
* @param \OCP\Share\IManager $shareManager
*/
public function __construct(IGroupManager $groupManager,
IUserManager $userManager,
IManager $contactsManager,
IConfig $config,
IUserSession $userSession,
IURLGenerator $urlGenerator,
IRequest $request,
ILogger $logger,
\OCP\Share\IManager $shareManager) {
public function __construct($appName,
IRequest $request,
IGroupManager $groupManager,
IUserManager $userManager,
IManager $contactsManager,
IConfig $config,
IUserSession $userSession,
IURLGenerator $urlGenerator,
ILogger $logger,
\OCP\Share\IManager $shareManager) {
parent::__construct($appName, $request);
$this->groupManager = $groupManager;
$this->userManager = $userManager;
$this->contactsManager = $contactsManager;
......@@ -400,19 +406,24 @@ class Sharees {
}
/**
* @return \OC_OCS_Result
* @NoAdminRequired
* @NoCSRFRequired
*
* @param string $search
* @param string $itemType
* @param int $page
* @param int $perPage
* @return array|DataResponse
*/
public function search() {
$search = isset($_GET['search']) ? (string) $_GET['search'] : '';
$itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null;
$page = isset($_GET['page']) ? (int) $_GET['page'] : 1;
$perPage = isset($_GET['perPage']) ? (int) $_GET['perPage'] : 200;
public function search($search = '', $itemType = null, $page = 1, $perPage = 200) {
if ($perPage <= 0) {
return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid perPage argument');
return [ 'statuscode' => Http::STATUS_BAD_REQUEST,
'message' => 'Invalid perPage argument'];
}
if ($page <= 0) {
return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid page');
return [ 'statuscode' => Http::STATUS_BAD_REQUEST,
'message' => 'Invalid page'];
}
$shareTypes = [
......@@ -470,12 +481,13 @@ class Sharees {
* @param array $shareTypes
* @param int $page
* @param int $perPage
* @return \OC_OCS_Result
* @return DataResponse|array
*/
protected function searchSharees($search, $itemType, array $shareTypes, $page, $perPage) {
// Verify arguments
if ($itemType === null) {
return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Missing itemType');
return [ 'statuscode' => Http::STATUS_BAD_REQUEST,
'message' => 'Missing itemType'];
}
// Get users
......@@ -493,8 +505,7 @@ class Sharees {
$this->getRemote($search);
}
$response = new \OC_OCS_Result($this->result);
$response->setItemsPerPage($perPage);
$response = new DataResponse(['data' => $this->result]);
if (sizeof($this->reachedEndFor) < 3) {
$response->addHeader('Link', $this->getPaginationLink($page, [
......
......@@ -24,9 +24,19 @@
namespace OCA\Files_Sharing\Tests\API;
use OCA\Files_Sharing\API\Sharees;
use OCA\Files_Sharing\Controller\ShareesController;
use OCA\Files_Sharing\Tests\TestCase;
use OCP\AppFramework\Http;
use OCP\Contacts\IManager;
use OCP\IConfig;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Share;
/**
......@@ -37,7 +47,7 @@ use OCP\Share;
* @package OCA\Files_Sharing\Tests\API
*/
class ShareesTest extends TestCase {
/** @var Sharees */
/** @var ShareesController */
protected $sharees;
/** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject */
......@@ -61,39 +71,40 @@ class ShareesTest extends TestCase {
protected function setUp() {
parent::setUp();
$this->userManager = $this->getMockBuilder('OCP\IUserManager')
$this->userManager = $this->getMockBuilder(IUserManager::class)
->disableOriginalConstructor()
->getMock();
$this->groupManager = $this->getMockBuilder('OCP\IGroupManager')
$this->groupManager = $this->getMockBuilder(IGroupManager::class)
->disableOriginalConstructor()
->getMock();
$this->contactsManager = $this->getMockBuilder('OCP\Contacts\IManager')
$this->contactsManager = $this->getMockBuilder(IManager::class)
->disableOriginalConstructor()
->getMock();
$this->session = $this->getMockBuilder('OCP\IUserSession')
$this->session = $this->getMockBuilder(IUserSession::class)
->disableOriginalConstructor()
->getMock();
$this->request = $this->getMockBuilder('OCP\IRequest')
$this->request = $this->getMockBuilder(IRequest::class)
->disableOriginalConstructor()
->getMock();
$this->shareManager = $this->getMockBuilder('OCP\Share\IManager')
$this->shareManager = $this->getMockBuilder(\OCP\Share\IManager::class)
->disableOriginalConstructor()
->getMock();
$this->sharees = new Sharees(
$this->sharees = new ShareesController(
'files_sharing',
$this->request,
$this->groupManager,
$this->userManager,
$this->contactsManager,
$this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder(IConfig::class)->disableOriginalConstructor()->getMock(),
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->request,
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder(IURLGenerator::class)->disableOriginalConstructor()->getMock(),
$this->getMockBuilder(ILogger::class)->disableOriginalConstructor()->getMock(),
$this->shareManager
);
}
......@@ -104,7 +115,7 @@ class ShareesTest extends TestCase {
* @return \OCP\IUser|\PHPUnit_Framework_MockObject_MockObject
*/
protected function getUserMock($uid, $displayName) {
$user = $this->getMockBuilder('OCP\IUser')
$user = $this->getMockBuilder(IUser::class)
->disableOriginalConstructor()
->getMock();
......@@ -124,7 +135,7 @@ class ShareesTest extends TestCase {
* @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject
*/
protected function getGroupMock($gid) {
$group = $this->getMockBuilder('OCP\IGroup')
$group = $this->getMockBuilder(IGroup::class)
->disableOriginalConstructor()
->getMock();
......@@ -1118,107 +1129,16 @@ class ShareesTest extends TestCase {
];
}
/**
* @dataProvider dataSearch
*
* @param array $getData
* @param string $apiSetting
* @param string $enumSetting
* @param bool $remoteSharingEnabled
* @param string $search
* @param string $itemType
* @param array $shareTypes
* @param int $page
* @param int $perPage
* @param bool $shareWithGroupOnly
* @param bool $shareeEnumeration
* @param bool $allowGroupSharing
*/
public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly, $shareeEnumeration, $allowGroupSharing) {
$oldGet = $_GET;
$_GET = $getData;
$config = $this->getMockBuilder('OCP\IConfig')
->disableOriginalConstructor()
->getMock();
$config->expects($this->exactly(2))
->method('getAppValue')
->with('core', $this->anything(), $this->anything())
->willReturnMap([
['core', 'shareapi_only_share_with_group_members', 'no', $apiSetting],
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', $enumSetting],
]);
$this->shareManager->expects($this->once())
->method('allowGroupSharing')
->willReturn($allowGroupSharing);
$sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees')
->setConstructorArgs([
$this->groupManager,
$this->userManager,
$this->contactsManager,
$config,
$this->session,
$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
$this->shareManager
])
->setMethods(['searchSharees', 'isRemoteSharingAllowed'])
->getMock();
$sharees->expects($this->once())
->method('searchSharees')
->with($search, $itemType, $shareTypes, $page, $perPage)
->willReturnCallback(function