From 3f8168b6e5c0fc14709b713d9ca4943f9df70273 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 23 Mar 2020 16:33:11 +0100 Subject: [PATCH 01/11] Allow some apps to have root URLs in their own routing file Signed-off-by: Joas Schilling --- apps/cloud_federation_api/appinfo/routes.php | 36 ++++++ apps/files/appinfo/routes.php | 7 + apps/files_sharing/appinfo/routes.php | 31 +++++ core/routes.php | 12 -- .../AppFramework/Routing/RouteConfig.php | 121 +++++++----------- lib/private/Route/Router.php | 1 - .../AuthPublicShareController.php | 4 +- 7 files changed, 121 insertions(+), 91 deletions(-) create mode 100644 apps/cloud_federation_api/appinfo/routes.php diff --git a/apps/cloud_federation_api/appinfo/routes.php b/apps/cloud_federation_api/appinfo/routes.php new file mode 100644 index 0000000000..49ea115530 --- /dev/null +++ b/apps/cloud_federation_api/appinfo/routes.php @@ -0,0 +1,36 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +return [ + [ + 'name' => 'RequestHandler#addShare', + 'url' => '/ocm/shares', + 'verb' => 'POST', + 'root' => '', + ], + [ + 'name' => 'RequestHandler#receiveNotification', + 'url' => '/ocm/notifications', + 'verb' => 'POST', + 'root' => '', + ], +]; diff --git a/apps/files/appinfo/routes.php b/apps/files/appinfo/routes.php index 825249b087..01d1e14054 100644 --- a/apps/files/appinfo/routes.php +++ b/apps/files/appinfo/routes.php @@ -41,6 +41,13 @@ $application->registerRoutes( $this, [ 'routes' => [ + [ + 'name' => 'View#showFile', + 'url' => '/f/{fileid}', + 'verb' => 'GET', + 'root' => '', + ], + [ 'name' => 'API#getThumbnail', 'url' => '/api/v1/thumbnail/{x}/{y}/{file}', diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 1346e0c689..a4edada738 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -30,6 +30,37 @@ return [ 'ExternalShares' => ['url' => '/api/externalShares'], ], 'routes' => [ + [ + 'name' => 'Share#showShare', + 'url' => '/s/{token}', + 'verb' => 'GET', + 'root' => '', + ], + [ + 'name' => 'Share#showAuthenticate', + 'url' => '/s/{token}/authenticate/{redirect}', + 'verb' => 'GET', + 'root' => '', + ], + [ + 'name' => 'Share#authenticate', + 'url' => '/s/{token}/authenticate/{redirect}', + 'verb' => 'POST', + 'root' => '', + ], + [ + 'name' => 'Share#downloadShare', + 'url' => '/s/{token}/download', + 'verb' => 'GET', + 'root' => '', + ], + [ + 'name' => 'PublicPreview#directLink', + 'url' => '/s/{token}/preview', + 'verb' => 'GET', + 'root' => '', + ], + [ 'name' => 'externalShares#testRemote', 'url' => '/testremote', diff --git a/core/routes.php b/core/routes.php index 8d03be05bb..3d30b12e39 100644 --- a/core/routes.php +++ b/core/routes.php @@ -89,18 +89,6 @@ $application->registerRoutes($this, [ // Logins for passwordless auth ['name' => 'WebAuthn#startAuthentication', 'url' => 'login/webauthn/start', 'verb' => 'POST'], ['name' => 'WebAuthn#finishAuthentication', 'url' => 'login/webauthn/finish', 'verb' => 'POST'], - - // Legacy routes that need to be globally available while they are handled by an app - ['name' => 'viewcontroller#showFile', 'url' => '/f/{fileid}', 'verb' => 'GET', 'app' => 'files'], - ['name' => 'sharecontroller#showShare', 'url' => '/s/{token}', 'verb' => 'GET', 'app' => 'files_sharing'], - ['name' => 'sharecontroller#showAuthenticate', 'url' => '/s/{token}/authenticate/{redirect}', 'verb' => 'GET', 'app' => 'files_sharing'], - ['name' => 'sharecontroller#authenticate', 'url' => '/s/{token}/authenticate/{redirect}', 'verb' => 'POST', 'app' => 'files_sharing'], - ['name' => 'sharecontroller#downloadShare', 'url' => '/s/{token}/download', 'verb' => 'GET', 'app' => 'files_sharing'], - ['name' => 'publicpreview#directLink', 'url' => '/s/{token}/preview', 'verb' => 'GET', 'app' => 'files_sharing'], - ['name' => 'requesthandlercontroller#addShare', 'url' => '/ocm/shares', 'verb' => 'POST', 'app' => 'cloud_federation_api'], - ['name' => 'requesthandlercontroller#receiveNotification', 'url' => '/ocm/notifications', 'verb' => 'POST', 'app' => 'cloud_federation_api'], - ['name' => 'pagecontroller#showCall', 'url' => '/call/{token}', 'verb' => 'GET', 'app' => 'spreed'], - ['name' => 'pagecontroller#authenticatePassword', 'url' => '/call/{token}', 'verb' => 'POST', 'app' => 'spreed'], ], 'ocs' => [ ['root' => '/cloud', 'name' => 'OCS#getCapabilities', 'url' => '/capabilities', 'verb' => 'GET'], diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index eb9991fbe6..58e677dd01 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -56,6 +56,14 @@ class RouteConfig { /** @var string[] */ private $controllerNameCache = []; + public const ROOT_URL_APPS = [ + 'cloud_federation_api', + 'core', + 'files_sharing', + 'files', + 'spreed', + ]; + /** * @param \OC\AppFramework\DependencyInjection\DIContainer $container * @param \OCP\Route\IRouter $router @@ -98,42 +106,7 @@ class RouteConfig { private function processOCS(array $routes): void { $ocsRoutes = $routes['ocs'] ?? []; foreach ($ocsRoutes as $ocsRoute) { - $name = $ocsRoute['name']; - $postfix = $ocsRoute['postfix'] ?? ''; - $root = $ocsRoute['root'] ?? '/apps/' . $this->appName; - - $url = $root . $ocsRoute['url']; - $verb = strtoupper($ocsRoute['verb'] ?? 'GET'); - - $split = explode('#', $name, 2); - if (count($split) !== 2) { - throw new \UnexpectedValueException('Invalid route name'); - } - list($controller, $action) = $split; - - $controllerName = $this->buildControllerName($controller); - $actionName = $this->buildActionName($action); - - $routeName = 'ocs.' . $this->appName . '.' . $controller . '.' . $action . $postfix; - - // register the route - $handler = new RouteActionHandler($this->container, $controllerName, $actionName); - - $router = $this->router->create($routeName, $url) - ->method($verb) - ->action($handler); - - // optionally register requirements for route. This is used to - // tell the route parser how url parameters should be matched - if (array_key_exists('requirements', $ocsRoute)) { - $router->requirements($ocsRoute['requirements']); - } - - // optionally register defaults for route. This is used to - // tell the route parser how url parameters should be default valued - if (array_key_exists('defaults', $ocsRoute)) { - $router->defaults($ocsRoute['defaults']); - } + $this->processRoute($ocsRoute, 'ocs.'); } } @@ -145,53 +118,51 @@ class RouteConfig { private function processSimpleRoutes(array $routes): void { $simpleRoutes = $routes['routes'] ?? []; foreach ($simpleRoutes as $simpleRoute) { - $name = $simpleRoute['name']; - $postfix = $simpleRoute['postfix'] ?? ''; + $this->processRoute($simpleRoute); + } + } - $url = $simpleRoute['url']; - $verb = strtoupper($simpleRoute['verb'] ?? 'GET'); + protected function processRoute(array $route, string $routeNamePrefix = ''): void { + $name = $route['name']; + $postfix = $route['postfix'] ?? ''; + $defaultRoot = $this->appName === 'core' ? '' : '/apps/' . $this->appName; + $root = $route['root'] ?? $defaultRoot; + if ($routeNamePrefix === '' && !\in_array($this->appName, self::ROOT_URL_APPS, true)) { + // Only allow root URLS for some apps + $root = $defaultRoot; + } - $split = explode('#', $name, 2); - if (count($split) !== 2) { - throw new \UnexpectedValueException('Invalid route name'); - } - list($controller, $action) = $split; + $url = $root . $route['url']; + $verb = strtoupper($route['verb'] ?? 'GET'); - $controllerName = $this->buildControllerName($controller); - $actionName = $this->buildActionName($action); - $appName = $simpleRoute['app'] ?? $this->appName; + $split = explode('#', $name, 2); + if (count($split) !== 2) { + throw new \UnexpectedValueException('Invalid route name'); + } + list($controller, $action) = $split; - if (isset($simpleRoute['app'])) { - // Legacy routes that need to be globally available while they are handled by an app - // E.g. '/f/{id}', '/s/{token}', '/call/{token}', … - $controllerName = str_replace('controllerController', 'Controller', $controllerName); - if ($controllerName === 'PublicpreviewController') { - $controllerName = 'PublicPreviewController'; - } elseif ($controllerName === 'RequesthandlerController') { - $controllerName = 'RequestHandlerController'; - } - $controllerName = App::buildAppNamespace($appName) . '\\Controller\\' . $controllerName; - } + $controllerName = $this->buildControllerName($controller); + $actionName = $this->buildActionName($action); - $routeName = $appName . '.' . $controller . '.' . $action . $postfix; + $routeName = $routeNamePrefix . $this->appName . '.' . $controller . '.' . $action . $postfix; - // register the route - $handler = new RouteActionHandler($this->container, $controllerName, $actionName); - $router = $this->router->create($routeName, $url) - ->method($verb) - ->action($handler); + // register the route + $handler = new RouteActionHandler($this->container, $controllerName, $actionName); - // optionally register requirements for route. This is used to - // tell the route parser how url parameters should be matched - if (array_key_exists('requirements', $simpleRoute)) { - $router->requirements($simpleRoute['requirements']); - } + $router = $this->router->create($routeName, $url) + ->method($verb) + ->action($handler); - // optionally register defaults for route. This is used to - // tell the route parser how url parameters should be default valued - if (array_key_exists('defaults', $simpleRoute)) { - $router->defaults($simpleRoute['defaults']); - } + // optionally register requirements for route. This is used to + // tell the route parser how url parameters should be matched + if(array_key_exists('requirements', $route)) { + $router->requirements($route['requirements']); + } + + // optionally register defaults for route. This is used to + // tell the route parser how url parameters should be default valued + if(array_key_exists('defaults', $route)) { + $router->defaults($route['defaults']); } } diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index ccb1578f8d..9c6e69908e 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -151,7 +151,6 @@ class Router implements IRouter { $this->useCollection($app); $this->requireRouteFile($file, $app); $collection = $this->getCollection($app); - $collection->addPrefix('/apps/' . $app); $this->root->addCollection($collection); // Also add the OCS collection diff --git a/lib/public/AppFramework/AuthPublicShareController.php b/lib/public/AppFramework/AuthPublicShareController.php index 1170819d5e..989067bdf4 100644 --- a/lib/public/AppFramework/AuthPublicShareController.php +++ b/lib/public/AppFramework/AuthPublicShareController.php @@ -165,9 +165,7 @@ abstract class AuthPublicShareController extends PublicShareController { private function getRoute(string $function): string { $app = strtolower($this->appName); $class = (new \ReflectionClass($this))->getShortName(); - if ($this->appName === 'files_sharing') { - $class = strtolower($class); - } elseif (substr($class, -10) === 'Controller') { + if (substr($class, -10) === 'Controller') { $class = substr($class, 0, -10); } return $app .'.'. $class .'.'. $function; From aad16c8508b11c54cd87122456642b8e20c774d3 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 23 Mar 2020 17:00:41 +0100 Subject: [PATCH 02/11] Fix creation of legacy routes Signed-off-by: Joas Schilling --- lib/private/Route/Router.php | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 9c6e69908e..075b81224a 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -335,6 +335,7 @@ class Router implements IRouter { if ($absolute === false) { $referenceType = UrlGenerator::ABSOLUTE_PATH; } + $name = $this->fixLegacyRootName($name); return $this->getGenerator()->generate($name, $parameters, $referenceType); } catch (RouteNotFoundException $e) { $this->logger->logException($e); @@ -342,6 +343,34 @@ class Router implements IRouter { } } + protected function fixLegacyRootName(string $routeName): string { + if ($routeName === 'files.viewcontroller.showFile') { + return 'files.View.showFile'; + } + if ($routeName === 'files_sharing.sharecontroller.showShare') { + return 'files_sharing.Share.showShare'; + } + if ($routeName === 'files_sharing.sharecontroller.showAuthenticate') { + return 'files_sharing.Share.showAuthenticate'; + } + if ($routeName === 'files_sharing.sharecontroller.authenticate') { + return 'files_sharing.Share.authenticate'; + } + if ($routeName === 'files_sharing.sharecontroller.downloadShare') { + return 'files_sharing.Share.downloadShare'; + } + if ($routeName === 'files_sharing.publicpreview.directLink') { + return 'files_sharing.PublicPreview.directLink'; + } + if ($routeName === 'cloud_federation_api.requesthandlercontroller.addShare') { + return 'cloud_federation_api.RequestHandler.addShare'; + } + if ($routeName === 'cloud_federation_api.requesthandlercontroller.receiveNotification') { + return 'cloud_federation_api.RequestHandler.receiveNotification'; + } + return $routeName; + } + /** * To isolate the variable scope used inside the $file it is required in it's own method * From f93d55eebd216e59f200da2bf46cd332bc17bedc Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 14 Apr 2020 17:24:29 +0200 Subject: [PATCH 03/11] PHP CS fixes Signed-off-by: Joas Schilling --- apps/cloud_federation_api/appinfo/routes.php | 1 + lib/private/AppFramework/Routing/RouteConfig.php | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/cloud_federation_api/appinfo/routes.php b/apps/cloud_federation_api/appinfo/routes.php index 49ea115530..9aa98315c3 100644 --- a/apps/cloud_federation_api/appinfo/routes.php +++ b/apps/cloud_federation_api/appinfo/routes.php @@ -1,4 +1,5 @@ diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index 58e677dd01..af745a35fc 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -33,7 +33,6 @@ declare(strict_types=1); namespace OC\AppFramework\Routing; use OC\AppFramework\DependencyInjection\DIContainer; -use OCP\AppFramework\App; use OCP\Route\IRouter; /** @@ -155,13 +154,13 @@ class RouteConfig { // optionally register requirements for route. This is used to // tell the route parser how url parameters should be matched - if(array_key_exists('requirements', $route)) { + if (array_key_exists('requirements', $route)) { $router->requirements($route['requirements']); } // optionally register defaults for route. This is used to // tell the route parser how url parameters should be default valued - if(array_key_exists('defaults', $route)) { + if (array_key_exists('defaults', $route)) { $router->defaults($route['defaults']); } } From 250467e842a0856a84e9962ed6ef476a2e3ccfef Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 14 Apr 2020 17:53:15 +0200 Subject: [PATCH 04/11] Extend tests for root url Signed-off-by: Joas Schilling --- .../AppFramework/Routing/RouteConfig.php | 4 +- .../lib/AppFramework/Routing/RoutingTest.php | 137 +++++++----------- 2 files changed, 52 insertions(+), 89 deletions(-) diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index af745a35fc..2f2d51f6e1 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -55,7 +55,7 @@ class RouteConfig { /** @var string[] */ private $controllerNameCache = []; - public const ROOT_URL_APPS = [ + protected $rootUrlApps = [ 'cloud_federation_api', 'core', 'files_sharing', @@ -126,7 +126,7 @@ class RouteConfig { $postfix = $route['postfix'] ?? ''; $defaultRoot = $this->appName === 'core' ? '' : '/apps/' . $this->appName; $root = $route['root'] ?? $defaultRoot; - if ($routeNamePrefix === '' && !\in_array($this->appName, self::ROOT_URL_APPS, true)) { + if ($routeNamePrefix === '' && !\in_array($this->appName, $this->rootUrlApps, true)) { // Only allow root URLS for some apps $root = $defaultRoot; } diff --git a/tests/lib/AppFramework/Routing/RoutingTest.php b/tests/lib/AppFramework/Routing/RoutingTest.php index 1aef757720..f9cd181468 100644 --- a/tests/lib/AppFramework/Routing/RoutingTest.php +++ b/tests/lib/AppFramework/Routing/RoutingTest.php @@ -5,6 +5,7 @@ namespace Test\AppFramework\Routing; use OC\AppFramework\DependencyInjection\DIContainer; use OC\AppFramework\Routing\RouteActionHandler; use OC\AppFramework\Routing\RouteConfig; +use OC\Route\Route; use OC\Route\Router; use OCP\ILogger; use OCP\Route\IRouter; @@ -16,7 +17,15 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open', 'verb' => 'GET'] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'GET', '/folders/{folderId}/open', 'FoldersController', 'open'); + $this->assertSimpleRoute($routes, 'folders.open', 'GET', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open'); + } + + public function testSimpleRouteWithUnderScoreNames() { + $routes = ['routes' => [ + ['name' => 'admin_folders#open_current', 'url' => '/folders/{folderId}/open', 'verb' => 'delete', 'root' => ''] + ]]; + + $this->assertSimpleRoute($routes, 'admin_folders.open_current', 'DELETE', '/folders/{folderId}/open', 'AdminFoldersController', 'openCurrent', [], [], '', true); } public function testSimpleOCSRoute() { @@ -33,7 +42,7 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open'] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'GET', '/folders/{folderId}/open', 'FoldersController', 'open'); + $this->assertSimpleRoute($routes, 'folders.open', 'GET', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open'); } public function testSimpleOCSRouteWithMissingVerb() { @@ -50,7 +59,7 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete'] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/folders/{folderId}/open', 'FoldersController', 'open'); + $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open'); } public function testSimpleOCSRouteWithLowercaseVerb() { @@ -67,7 +76,7 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete', 'requirements' => ['something']] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/folders/{folderId}/open', 'FoldersController', 'open', ['something']); + $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open', ['something']); } public function testSimpleOCSRouteWithRequirements() { @@ -84,7 +93,7 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete', [], 'defaults' => ['param' => 'foobar']] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/folders/{folderId}/open', 'FoldersController', 'open', [], ['param' => 'foobar']); + $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open', [], ['param' => 'foobar']); } @@ -102,7 +111,7 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders#open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete', 'postfix' => '_something'] ]]; - $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/folders/{folderId}/open', 'FoldersController', 'open', [], [], '_something'); + $this->assertSimpleRoute($routes, 'folders.open', 'DELETE', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open', [], [], '_something'); } public function testSimpleOCSRouteWithPostfix() { @@ -114,7 +123,7 @@ class RoutingTest extends \Test\TestCase { $this->assertSimpleOCSRoute($routes, 'folders.open', 'DELETE', '/apps/app1/folders/{folderId}/open', 'FoldersController', 'open', [], [], '_something'); } - + public function testSimpleRouteWithBrokenName() { $this->expectException(\UnexpectedValueException::class); @@ -122,10 +131,10 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders_open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete'] ]]; - // router mock - $router = $this->getMockBuilder('\OC\Route\Router') - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // load route configuration @@ -135,7 +144,7 @@ class RoutingTest extends \Test\TestCase { $config->register(); } - + public function testSimpleOCSRouteWithBrokenName() { $this->expectException(\UnexpectedValueException::class); @@ -143,10 +152,10 @@ class RoutingTest extends \Test\TestCase { ['name' => 'folders_open', 'url' => '/folders/{folderId}/open', 'verb' => 'delete'] ]]; - // router mock - $router = $this->getMockBuilder('\OC\Route\Router') - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // load route configuration @@ -156,14 +165,6 @@ class RoutingTest extends \Test\TestCase { $config->register(); } - public function testSimpleRouteWithUnderScoreNames() { - $routes = ['routes' => [ - ['name' => 'admin_folders#open_current', 'url' => '/folders/{folderId}/open', 'verb' => 'delete'] - ]]; - - $this->assertSimpleRoute($routes, 'admin_folders.open_current', 'DELETE', '/folders/{folderId}/open', 'AdminFoldersController', 'openCurrent'); - } - public function testSimpleOCSRouteWithUnderScoreNames() { $routes = ['ocs' => [ ['name' => 'admin_folders#open_current', 'url' => '/folders/{folderId}/open', 'verb' => 'delete'] @@ -202,14 +203,7 @@ class RoutingTest extends \Test\TestCase { $this->assertResource($routes, 'admin_accounts', '/admin/accounts', 'AdminAccountsController', 'id'); } - /** - * @param string $name - * @param string $verb - * @param string $url - * @param string $controllerName - * @param string $actionName - */ - private function assertSimpleRoute($routes, $name, $verb, $url, $controllerName, $actionName, array $requirements=[], array $defaults=[], $postfix='') { + private function assertSimpleRoute($routes, $name, $verb, $url, $controllerName, $actionName, array $requirements = [], array $defaults = [], $postfix = '', $allowRootUrl = false): void { if ($postfix) { $name .= $postfix; } @@ -218,10 +212,10 @@ class RoutingTest extends \Test\TestCase { $container = new DIContainer('app1'); $route = $this->mockRoute($container, $verb, $controllerName, $actionName, $requirements, $defaults); - // router mock - $router = $this->getMockBuilder('\OC\Route\Router') - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // we expect create to be called once: @@ -233,6 +227,9 @@ class RoutingTest extends \Test\TestCase { // load route configuration $config = new RouteConfig($container, $router, $routes); + if ($allowRootUrl) { + self::invokePrivate($config, 'rootUrlApps', [['app1']]); + } $config->register(); } @@ -265,10 +262,10 @@ class RoutingTest extends \Test\TestCase { $container = new DIContainer('app1'); $route = $this->mockRoute($container, $verb, $controllerName, $actionName, $requirements, $defaults); - // router mock - $router = $this->getMockBuilder('\OC\Route\Router') - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // we expect create to be called once: @@ -294,8 +291,8 @@ class RoutingTest extends \Test\TestCase { private function assertOCSResource($yaml, $resourceName, $url, $controllerName, $paramName): void { /** @var IRouter|MockObject $router */ $router = $this->getMockBuilder(Router::class) - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // route mocks @@ -352,10 +349,10 @@ class RoutingTest extends \Test\TestCase { * @param string $paramName */ private function assertResource($yaml, $resourceName, $url, $controllerName, $paramName) { - // router mock - $router = $this->getMockBuilder('\OC\Route\Router') - ->setMethods(['create']) - ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->onlyMethods(['create']) + ->setConstructorArgs([$this->createMock(ILogger::class)]) ->getMock(); // route mocks @@ -412,7 +409,7 @@ class RoutingTest extends \Test\TestCase { * @param string $actionName * @param array $requirements * @param array $defaults - * @return \PHPUnit_Framework_MockObject_MockObject + * @return MockObject */ private function mockRoute( DIContainer $container, @@ -422,25 +419,25 @@ class RoutingTest extends \Test\TestCase { array $requirements=[], array $defaults=[] ) { - $route = $this->getMockBuilder('\OC\Route\Route') - ->setMethods(['method', 'action', 'requirements', 'defaults']) + $route = $this->getMockBuilder(Route::class) + ->onlyMethods(['method', 'action', 'requirements', 'defaults']) ->disableOriginalConstructor() ->getMock(); $route - ->expects($this->exactly(1)) + ->expects($this->once()) ->method('method') ->with($this->equalTo($verb)) ->willReturn($route); $route - ->expects($this->exactly(1)) + ->expects($this->once()) ->method('action') ->with($this->equalTo(new RouteActionHandler($container, $controllerName, $actionName))) ->willReturn($route); if (count($requirements) > 0) { $route - ->expects($this->exactly(1)) + ->expects($this->once()) ->method('requirements') ->with($this->equalTo($requirements)) ->willReturn($route); @@ -448,7 +445,7 @@ class RoutingTest extends \Test\TestCase { if (count($defaults) > 0) { $route - ->expects($this->exactly(1)) + ->expects($this->once()) ->method('defaults') ->with($this->equalTo($defaults)) ->willReturn($route); @@ -457,37 +454,3 @@ class RoutingTest extends \Test\TestCase { return $route; } } - -/* -# -# sample routes.yaml for ownCloud -# -# the section simple describes one route - -routes: - - name: folders#open - url: /folders/{folderId}/open - verb: GET - # controller: name.split()[0] - # action: name.split()[1] - -# for a resource following actions will be generated: -# - index -# - create -# - show -# - update -# - destroy -# - new -resources: - accounts: - url: /accounts - - folders: - url: /accounts/{accountId}/folders - # actions can be used to define additional actions on the resource - actions: - - name: validate - verb: GET - on-collection: false - - * */ From 708b4991d9039543130b569dd845ed8d8cd1a68c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 14 Apr 2020 18:24:02 +0200 Subject: [PATCH 05/11] Fix legacy routes Signed-off-by: Joas Schilling --- apps/files/appinfo/routes.php | 4 ++-- apps/files_external/appinfo/routes.php | 6 +++--- apps/files_versions/appinfo/routes.php | 6 +++--- apps/user_ldap/appinfo/routes.php | 14 +++++++------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/apps/files/appinfo/routes.php b/apps/files/appinfo/routes.php index 01d1e14054..d304ceb7b2 100644 --- a/apps/files/appinfo/routes.php +++ b/apps/files/appinfo/routes.php @@ -153,7 +153,7 @@ $application->registerRoutes( /** @var $this \OC\Route\Router */ -$this->create('files_ajax_download', 'ajax/download.php') +$this->create('files_ajax_download', 'apps/files/ajax/download.php') ->actionInclude('files/ajax/download.php'); -$this->create('files_ajax_list', 'ajax/list.php') +$this->create('files_ajax_list', 'apps/files/ajax/list.php') ->actionInclude('files/ajax/list.php'); diff --git a/apps/files_external/appinfo/routes.php b/apps/files_external/appinfo/routes.php index c504110210..17484594a4 100644 --- a/apps/files_external/appinfo/routes.php +++ b/apps/files_external/appinfo/routes.php @@ -60,11 +60,11 @@ ] ); -$this->create('files_external_oauth1', 'ajax/oauth1.php') +$this->create('files_external_oauth1', 'apps/files_external/ajax/oauth1.php') ->actionInclude('files_external/ajax/oauth1.php'); -$this->create('files_external_oauth2', 'ajax/oauth2.php') +$this->create('files_external_oauth2', 'apps/files_external/ajax/oauth2.php') ->actionInclude('files_external/ajax/oauth2.php'); -$this->create('files_external_list_applicable', '/applicable') +$this->create('files_external_list_applicable', '/apps/files_external/applicable') ->actionInclude('files_external/ajax/applicable.php'); diff --git a/apps/files_versions/appinfo/routes.php b/apps/files_versions/appinfo/routes.php index 7252ddcd22..35a746777d 100644 --- a/apps/files_versions/appinfo/routes.php +++ b/apps/files_versions/appinfo/routes.php @@ -40,9 +40,9 @@ $application->registerRoutes($this, [ ]); /** @var $this \OCP\Route\IRouter */ -$this->create('files_versions_download', 'download.php') +$this->create('files_versions_download', 'apps/files_versions/download.php') ->actionInclude('files_versions/download.php'); -$this->create('files_versions_ajax_getVersions', 'ajax/getVersions.php') +$this->create('files_versions_ajax_getVersions', 'apps/files_versions/ajax/getVersions.php') ->actionInclude('files_versions/ajax/getVersions.php'); -$this->create('files_versions_ajax_rollbackVersion', 'ajax/rollbackVersion.php') +$this->create('files_versions_ajax_rollbackVersion', 'apps/files_versions/ajax/rollbackVersion.php') ->actionInclude('files_versions/ajax/rollbackVersion.php'); diff --git a/apps/user_ldap/appinfo/routes.php b/apps/user_ldap/appinfo/routes.php index d7955446a4..d3ff28a3f1 100644 --- a/apps/user_ldap/appinfo/routes.php +++ b/apps/user_ldap/appinfo/routes.php @@ -28,19 +28,19 @@ declare(strict_types=1); */ /** @var $this \OCP\Route\IRouter */ -$this->create('user_ldap_ajax_clearMappings', 'ajax/clearMappings.php') +$this->create('user_ldap_ajax_clearMappings', 'apps/user_ldap/ajax/clearMappings.php') ->actionInclude('user_ldap/ajax/clearMappings.php'); -$this->create('user_ldap_ajax_deleteConfiguration', 'ajax/deleteConfiguration.php') +$this->create('user_ldap_ajax_deleteConfiguration', 'apps/user_ldap/ajax/deleteConfiguration.php') ->actionInclude('user_ldap/ajax/deleteConfiguration.php'); -$this->create('user_ldap_ajax_getConfiguration', 'ajax/getConfiguration.php') +$this->create('user_ldap_ajax_getConfiguration', 'apps/user_ldap/ajax/getConfiguration.php') ->actionInclude('user_ldap/ajax/getConfiguration.php'); -$this->create('user_ldap_ajax_getNewServerConfigPrefix', 'ajax/getNewServerConfigPrefix.php') +$this->create('user_ldap_ajax_getNewServerConfigPrefix', 'apps/user_ldap/ajax/getNewServerConfigPrefix.php') ->actionInclude('user_ldap/ajax/getNewServerConfigPrefix.php'); -$this->create('user_ldap_ajax_setConfiguration', 'ajax/setConfiguration.php') +$this->create('user_ldap_ajax_setConfiguration', 'apps/user_ldap/ajax/setConfiguration.php') ->actionInclude('user_ldap/ajax/setConfiguration.php'); -$this->create('user_ldap_ajax_testConfiguration', 'ajax/testConfiguration.php') +$this->create('user_ldap_ajax_testConfiguration', 'apps/user_ldap/ajax/testConfiguration.php') ->actionInclude('user_ldap/ajax/testConfiguration.php'); -$this->create('user_ldap_ajax_wizard', 'ajax/wizard.php') +$this->create('user_ldap_ajax_wizard', 'apps/user_ldap/ajax/wizard.php') ->actionInclude('user_ldap/ajax/wizard.php'); $application = new \OCP\AppFramework\App('user_ldap'); From e45c87cd2e7cb6c818f7a99b51afd1f16606f930 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 15 Apr 2020 14:06:23 +0200 Subject: [PATCH 06/11] Fix creating the share controller Signed-off-by: Joas Schilling --- .../files_sharing/lib/AppInfo/Application.php | 73 +------------------ 1 file changed, 3 insertions(+), 70 deletions(-) diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index 34db7441da..23db5e3ce3 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -80,48 +80,10 @@ class Application extends App { $mountProviderCollection = $server->getMountProviderCollection(); $notifications = $server->getNotificationManager(); - /** - * Controllers - */ - $container->registerService('ShareController', function (SimpleContainer $c) use ($server) { - $federatedSharingApp = new \OCA\FederatedFileSharing\AppInfo\Application(); - return new ShareController( - $c->query('AppName'), - $c->query('Request'), - $server->getConfig(), - $server->getURLGenerator(), - $server->getUserManager(), - $server->getLogger(), - $server->getActivityManager(), - $server->getShareManager(), - $server->getSession(), - $server->getPreviewManager(), - $server->getRootFolder(), - $federatedSharingApp->getFederatedShareProvider(), - $server->getEventDispatcher(), - $server->getL10N($c->query('AppName')), - $server->query(Defaults::class) - ); - }); - $container->registerService('ExternalSharesController', function (SimpleContainer $c) { - return new ExternalSharesController( - $c->query('AppName'), - $c->query('Request'), - $c->query('ExternalManager'), - $c->query('HttpClientService') - ); - }); - /** * Core class wrappers */ - $container->registerService('HttpClientService', function (SimpleContainer $c) use ($server) { - return $server->getHTTPClientService(); - }); - $container->registerService(ICloudIdManager::class, function (SimpleContainer $c) use ($server) { - return $server->getCloudIdManager(); - }); - $container->registerService('ExternalManager', function (SimpleContainer $c) use ($server) { + $container->registerService(Manager::class, function (SimpleContainer $c) use ($server) { $user = $server->getUserSession()->getUser(); $uid = $user ? $user->getUID() : null; return new \OCA\Files_Sharing\External\Manager( @@ -138,43 +100,14 @@ class Application extends App { $uid ); }); - $container->registerAlias(Manager::class, 'ExternalManager'); /** * Middleware */ - $container->registerService('SharingCheckMiddleware', function (SimpleContainer $c) use ($server) { - return new SharingCheckMiddleware( - $c->query('AppName'), - $server->getConfig(), - $server->getAppManager(), - $server->query(IControllerMethodReflector::class), - $server->getShareManager(), - $server->getRequest() - ); - }); - - $container->registerService(ShareInfoMiddleware::class, function () use ($server) { - return new ShareInfoMiddleware( - $server->getShareManager() - ); - }); - - // Execute middlewares - $container->registerMiddleWare('SharingCheckMiddleware'); + $container->registerMiddleWare(SharingCheckMiddleware::class); $container->registerMiddleWare(OCSShareAPIMiddleware::class); $container->registerMiddleWare(ShareInfoMiddleware::class); - $container->registerService('MountProvider', function (IContainer $c) { - /** @var \OCP\IServerContainer $server */ - $server = $c->query('ServerContainer'); - return new MountProvider( - $server->getConfig(), - $server->getShareManager(), - $server->getLogger() - ); - }); - $container->registerService('ExternalMountProvider', function (IContainer $c) { /** @var \OCP\IServerContainer $server */ $server = $c->query('ServerContainer'); @@ -205,7 +138,7 @@ class Application extends App { } protected function registerMountProviders(IMountProviderCollection $mountProviderCollection) { - $mountProviderCollection->registerProvider($this->getContainer()->query('MountProvider')); + $mountProviderCollection->registerProvider($this->getContainer()->query(MountProvider::class)); $mountProviderCollection->registerProvider($this->getContainer()->query('ExternalMountProvider')); } From 9654a924abb76aae5299780f3ae3868d2244c90c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Apr 2020 11:17:01 +0200 Subject: [PATCH 07/11] Fix route definitions of cloud_federation_api Signed-off-by: Joas Schilling --- apps/cloud_federation_api/appinfo/routes.php | 24 +++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/apps/cloud_federation_api/appinfo/routes.php b/apps/cloud_federation_api/appinfo/routes.php index 9aa98315c3..8dbc0e3e93 100644 --- a/apps/cloud_federation_api/appinfo/routes.php +++ b/apps/cloud_federation_api/appinfo/routes.php @@ -22,16 +22,18 @@ declare(strict_types=1); */ return [ - [ - 'name' => 'RequestHandler#addShare', - 'url' => '/ocm/shares', - 'verb' => 'POST', - 'root' => '', - ], - [ - 'name' => 'RequestHandler#receiveNotification', - 'url' => '/ocm/notifications', - 'verb' => 'POST', - 'root' => '', + 'routes' => [ + [ + 'name' => 'RequestHandler#addShare', + 'url' => '/ocm/shares', + 'verb' => 'POST', + 'root' => '', + ], + [ + 'name' => 'RequestHandler#receiveNotification', + 'url' => '/ocm/notifications', + 'verb' => 'POST', + 'root' => '', + ], ], ]; From 031b6656d1e67fae4af5a0aeab69c668c3cd586d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Apr 2020 11:19:32 +0200 Subject: [PATCH 08/11] Reduce noise in test output Signed-off-by: Joas Schilling --- apps/testing/appinfo/app.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/testing/appinfo/app.php b/apps/testing/appinfo/app.php index 48d8da8c20..428899df69 100644 --- a/apps/testing/appinfo/app.php +++ b/apps/testing/appinfo/app.php @@ -1,4 +1,5 @@ query(\OCA\Testing\AppInfo\Application::class); From 6332d7c0b3e3dddfdc6f0a77c63bd2684dda3b69 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Apr 2020 11:19:49 +0200 Subject: [PATCH 09/11] Make sure the cloud_federation_api routes are loaded Signed-off-by: Joas Schilling --- ocm-provider/index.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ocm-provider/index.php b/ocm-provider/index.php index e1aa7f2b03..1b90fd5366 100644 --- a/ocm-provider/index.php +++ b/ocm-provider/index.php @@ -29,6 +29,8 @@ $server = \OC::$server; $isEnabled = $server->getAppManager()->isEnabledForUser('cloud_federation_api'); if ($isEnabled) { + // Make sure the routes are loaded + \OC_App::loadApp('cloud_federation_api'); $capabilities = new OCA\CloudFederationAPI\Capabilities($server->getURLGenerator()); header('Content-Type: application/json'); echo json_encode($capabilities->getCapabilities()['ocm']); From 685d7b06374fdac72541f002c14c9359b34a2608 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Apr 2020 11:20:26 +0200 Subject: [PATCH 10/11] Directly use the class to get the service Signed-off-by: Joas Schilling --- apps/files_sharing/lib/AppInfo/Application.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index 23db5e3ce3..bb9da59dc1 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -114,7 +114,7 @@ class Application extends App { return new \OCA\Files_Sharing\External\MountProvider( $server->getDatabaseConnection(), function () use ($c) { - return $c->query('ExternalManager'); + return $c->query(Manager::class); }, $server->getCloudIdManager() ); From 1b93d5f1db6e2c9931f62c96c2dd60b7a2251ce9 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Apr 2020 11:31:37 +0200 Subject: [PATCH 11/11] PHP CS fixes Signed-off-by: Joas Schilling --- apps/files_sharing/lib/AppInfo/Application.php | 5 ----- apps/testing/appinfo/app.php | 1 + 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index bb9da59dc1..c7198c12cf 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -34,8 +34,6 @@ namespace OCA\Files_Sharing\AppInfo; use OC\AppFramework\Utility\SimpleContainer; use OCA\Files_Sharing\Capabilities; -use OCA\Files_Sharing\Controller\ExternalSharesController; -use OCA\Files_Sharing\Controller\ShareController; use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\Listener\LoadAdditionalListener; use OCA\Files_Sharing\Listener\LoadSidebarListener; @@ -51,10 +49,7 @@ use OCA\Files_Sharing\Notification\Notifier; use OCA\Files\Event\LoadAdditionalScriptsEvent; use OCA\Files\Event\LoadSidebar; use OCP\AppFramework\App; -use OCP\AppFramework\Utility\IControllerMethodReflector; -use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; -use OCP\Federation\ICloudIdManager; use OCP\Files\Config\IMountProviderCollection; use OCP\Group\Events\UserAddedEvent; use OCP\IContainer; diff --git a/apps/testing/appinfo/app.php b/apps/testing/appinfo/app.php index 428899df69..e588ce4743 100644 --- a/apps/testing/appinfo/app.php +++ b/apps/testing/appinfo/app.php @@ -1,4 +1,5 @@