Fix new routing in settings etc

Also prefix resources
Unify the prefix handling
Handle urls with and without slash

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

diff --git a/apps/settings/appinfo/routes.php b/apps/settings/appinfo/routes.php
index f0cc30084eb6c2b1068d14ecba4aba2d7270a8c1..213ca05b16b2e3e8d9be0104f420bcb2a81c031f 100644 (file)
--- a/apps/settings/appinfo/routes.php
+++ b/apps/settings/appinfo/routes.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -31,68 +34,61 @@
  *
  */
 
-namespace OCA\Settings;
-
-use OCA\Settings\AppInfo\Application;
-
-/** @var Application $application */
-$application = \OC::$server->query(Application::class);
-$this->useCollection('root');
-$application->registerRoutes($this, [
+return [
        'resources' => [
-               'AuthSettings' => ['url' => '/settings/personal/authtokens'],
+               'AuthSettings' => ['url' => '/settings/personal/authtokens' , 'root' => ''],
        ],
        'routes' => [
-               ['name' => 'AuthSettings#wipe', 'url' => '/settings/personal/authtokens/wipe/{id}', 'verb' => 'POST'],
+               ['name' => 'AuthSettings#wipe', 'url' => '/settings/personal/authtokens/wipe/{id}', 'verb' => 'POST' , 'root' => ''],
 
-               ['name' => 'MailSettings#setMailSettings', 'url' => '/settings/admin/mailsettings', 'verb' => 'POST'],
-               ['name' => 'MailSettings#storeCredentials', 'url' => '/settings/admin/mailsettings/credentials', 'verb' => 'POST'],
-               ['name' => 'MailSettings#sendTestMail', 'url' => '/settings/admin/mailtest', 'verb' => 'POST'],
-               ['name' => 'Encryption#startMigration', 'url' => '/settings/admin/startmigration', 'verb' => 'POST'],
+               ['name' => 'MailSettings#setMailSettings', 'url' => '/settings/admin/mailsettings', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'MailSettings#storeCredentials', 'url' => '/settings/admin/mailsettings/credentials', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'MailSettings#sendTestMail', 'url' => '/settings/admin/mailtest', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'Encryption#startMigration', 'url' => '/settings/admin/startmigration', 'verb' => 'POST' , 'root' => ''],
 
-               ['name' => 'AppSettings#listCategories', 'url' => '/settings/apps/categories', 'verb' => 'GET'],
-               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps', 'verb' => 'GET'],
-               ['name' => 'AppSettings#listApps', 'url' => '/settings/apps/list', 'verb' => 'GET'],
-               ['name' => 'AppSettings#enableApp', 'url' => '/settings/apps/enable/{appId}', 'verb' => 'GET'],
-               ['name' => 'AppSettings#enableApp', 'url' => '/settings/apps/enable/{appId}', 'verb' => 'POST'],
-               ['name' => 'AppSettings#enableApps', 'url' => '/settings/apps/enable', 'verb' => 'POST'],
-               ['name' => 'AppSettings#disableApp', 'url' => '/settings/apps/disable/{appId}', 'verb' => 'GET'],
-               ['name' => 'AppSettings#disableApps', 'url' => '/settings/apps/disable', 'verb' => 'POST'],
-               ['name' => 'AppSettings#updateApp', 'url' => '/settings/apps/update/{appId}', 'verb' => 'GET'],
-               ['name' => 'AppSettings#uninstallApp', 'url' => '/settings/apps/uninstall/{appId}', 'verb' => 'GET'],
-               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps/{category}', 'verb' => 'GET', 'defaults' => ['category' => '']],
-               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps/{category}/{id}', 'verb' => 'GET', 'defaults' => ['category' => '', 'id' => '']],
-               ['name' => 'AppSettings#force', 'url' => '/settings/apps/force', 'verb' => 'POST'],
+               ['name' => 'AppSettings#listCategories', 'url' => '/settings/apps/categories', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#listApps', 'url' => '/settings/apps/list', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#enableApp', 'url' => '/settings/apps/enable/{appId}', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#enableApp', 'url' => '/settings/apps/enable/{appId}', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'AppSettings#enableApps', 'url' => '/settings/apps/enable', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'AppSettings#disableApp', 'url' => '/settings/apps/disable/{appId}', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#disableApps', 'url' => '/settings/apps/disable', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'AppSettings#updateApp', 'url' => '/settings/apps/update/{appId}', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#uninstallApp', 'url' => '/settings/apps/uninstall/{appId}', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps/{category}', 'verb' => 'GET', 'defaults' => ['category' => ''] , 'root' => ''],
+               ['name' => 'AppSettings#viewApps', 'url' => '/settings/apps/{category}/{id}', 'verb' => 'GET', 'defaults' => ['category' => '', 'id' => ''] , 'root' => ''],
+               ['name' => 'AppSettings#force', 'url' => '/settings/apps/force', 'verb' => 'POST' , 'root' => ''],
 
-               ['name' => 'Users#setDisplayName', 'url' => '/settings/users/{username}/displayName', 'verb' => 'POST'],
-               ['name' => 'Users#setEMailAddress', 'url' => '/settings/users/{id}/mailAddress', 'verb' => 'PUT'],
-               ['name' => 'Users#setUserSettings', 'url' => '/settings/users/{username}/settings', 'verb' => 'PUT'],
-               ['name' => 'Users#getVerificationCode', 'url' => '/settings/users/{account}/verify', 'verb' => 'GET'],
-               ['name' => 'Users#usersList', 'url' => '/settings/users', 'verb' => 'GET'],
-               ['name' => 'Users#usersListByGroup', 'url' => '/settings/users/{group}', 'verb' => 'GET', 'requirements' => ['group' => '.+']],
-               ['name' => 'Users#setPreference', 'url' => '/settings/users/preferences/{key}', 'verb' => 'POST'],
-               ['name' => 'LogSettings#setLogLevel', 'url' => '/settings/admin/log/level', 'verb' => 'POST'],
-               ['name' => 'LogSettings#getEntries', 'url' => '/settings/admin/log/entries', 'verb' => 'GET'],
-               ['name' => 'LogSettings#download', 'url' => '/settings/admin/log/download', 'verb' => 'GET'],
-               ['name' => 'CheckSetup#check', 'url' => '/settings/ajax/checksetup', 'verb' => 'GET'],
-               ['name' => 'CheckSetup#getFailedIntegrityCheckFiles', 'url' => '/settings/integrity/failed', 'verb' => 'GET'],
-               ['name' => 'CheckSetup#rescanFailedIntegrityCheck', 'url' => '/settings/integrity/rescan', 'verb' => 'GET'],
-               ['name' => 'Certificate#addPersonalRootCertificate', 'url' => '/settings/personal/certificate', 'verb' => 'POST'],
-               ['name' => 'Certificate#removePersonalRootCertificate', 'url' => '/settings/personal/certificate/{certificateIdentifier}', 'verb' => 'DELETE'],
-               ['name' => 'Certificate#addSystemRootCertificate', 'url' => '/settings/admin/certificate', 'verb' => 'POST'],
-               ['name' => 'Certificate#removeSystemRootCertificate', 'url' => '/settings/admin/certificate/{certificateIdentifier}', 'verb' => 'DELETE'],
-               ['name' => 'PersonalSettings#index', 'url' => '/settings/user/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'personal-info']],
-               ['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'server']],
-               ['name' => 'AdminSettings#form', 'url' => '/settings/admin/{section}', 'verb' => 'GET'],
-               ['name' => 'ChangePassword#changePersonalPassword', 'url' => '/settings/personal/changepassword', 'verb' => 'POST'],
-               ['name' => 'ChangePassword#changeUserPassword', 'url' => '/settings/users/changepassword', 'verb' => 'POST'],
-               ['name' => 'TwoFactorSettings#index', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'GET'],
-               ['name' => 'TwoFactorSettings#update', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'PUT'],
+               ['name' => 'Users#setDisplayName', 'url' => '/settings/users/{username}/displayName', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'Users#setEMailAddress', 'url' => '/settings/users/{id}/mailAddress', 'verb' => 'PUT' , 'root' => ''],
+               ['name' => 'Users#setUserSettings', 'url' => '/settings/users/{username}/settings', 'verb' => 'PUT' , 'root' => ''],
+               ['name' => 'Users#getVerificationCode', 'url' => '/settings/users/{account}/verify', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'Users#usersList', 'url' => '/settings/users', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'Users#usersListByGroup', 'url' => '/settings/users/{group}', 'verb' => 'GET', 'requirements' => ['group' => '.+'] , 'root' => ''],
+               ['name' => 'Users#setPreference', 'url' => '/settings/users/preferences/{key}', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'LogSettings#setLogLevel', 'url' => '/settings/admin/log/level', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'LogSettings#getEntries', 'url' => '/settings/admin/log/entries', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'LogSettings#download', 'url' => '/settings/admin/log/download', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'CheckSetup#check', 'url' => '/settings/ajax/checksetup', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'CheckSetup#getFailedIntegrityCheckFiles', 'url' => '/settings/integrity/failed', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'CheckSetup#rescanFailedIntegrityCheck', 'url' => '/settings/integrity/rescan', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'Certificate#addPersonalRootCertificate', 'url' => '/settings/personal/certificate', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'Certificate#removePersonalRootCertificate', 'url' => '/settings/personal/certificate/{certificateIdentifier}', 'verb' => 'DELETE' , 'root' => ''],
+               ['name' => 'Certificate#addSystemRootCertificate', 'url' => '/settings/admin/certificate', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'Certificate#removeSystemRootCertificate', 'url' => '/settings/admin/certificate/{certificateIdentifier}', 'verb' => 'DELETE' , 'root' => ''],
+               ['name' => 'PersonalSettings#index', 'url' => '/settings/user/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'personal-info'] , 'root' => ''],
+               ['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'server'] , 'root' => ''],
+               ['name' => 'AdminSettings#form', 'url' => '/settings/admin/{section}', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'ChangePassword#changePersonalPassword', 'url' => '/settings/personal/changepassword', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'ChangePassword#changeUserPassword', 'url' => '/settings/users/changepassword', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'TwoFactorSettings#index', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'TwoFactorSettings#update', 'url' => '/settings/api/admin/twofactorauth', 'verb' => 'PUT' , 'root' => ''],
 
-               ['name' => 'Help#help', 'url' => '/settings/help/{mode}', 'verb' => 'GET', 'defaults' => ['mode' => '']],
+               ['name' => 'Help#help', 'url' => '/settings/help/{mode}', 'verb' => 'GET', 'defaults' => ['mode' => ''] , 'root' => ''],
 
-               ['name' => 'WebAuthn#startRegistration', 'url' => '/settings/api/personal/webauthn/registration', 'verb' => 'GET'],
-               ['name' => 'WebAuthn#finishRegistration', 'url' => '/settings/api/personal/webauthn/registration', 'verb' => 'POST'],
-               ['name' => 'WebAuthn#deleteRegistration', 'url' => '/settings/api/personal/webauthn/registration/{id}', 'verb' => 'DELETE'],
+               ['name' => 'WebAuthn#startRegistration', 'url' => '/settings/api/personal/webauthn/registration', 'verb' => 'GET' , 'root' => ''],
+               ['name' => 'WebAuthn#finishRegistration', 'url' => '/settings/api/personal/webauthn/registration', 'verb' => 'POST' , 'root' => ''],
+               ['name' => 'WebAuthn#deleteRegistration', 'url' => '/settings/api/personal/webauthn/registration/{id}', 'verb' => 'DELETE' , 'root' => ''],
        ]
-]);
+];

diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php
index 2f2d51f6e1af9f1026b1699c10a99c1c9a2523b9..957dcba453d607502794c2ac9aaf381b2f744d3d 100644 (file)
--- a/lib/private/AppFramework/Routing/RouteConfig.php
+++ b/lib/private/AppFramework/Routing/RouteConfig.php
@@ -60,6 +60,7 @@ class RouteConfig {
                'core',
                'files_sharing',
                'files',
+               'settings',
                'spreed',
        ];
 
@@ -82,10 +83,10 @@ class RouteConfig {
        public function register() {
 
                // parse simple
-               $this->processSimpleRoutes($this->routes);
+               $this->processIndexRoutes($this->routes);
 
                // parse resources
-               $this->processResources($this->routes);
+               $this->processIndexResources($this->routes);
 
                /*
                 * OCS routes go into a different collection
@@ -114,7 +115,7 @@ class RouteConfig {
         * @param array $routes
         * @throws \UnexpectedValueException
         */
-       private function processSimpleRoutes(array $routes): void {
+       private function processIndexRoutes(array $routes): void {
                $simpleRoutes = $routes['routes'] ?? [];
                foreach ($simpleRoutes as $simpleRoute) {
                        $this->processRoute($simpleRoute);
@@ -124,14 +125,9 @@ class RouteConfig {
        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, $this->rootUrlApps, true)) {
-                       // Only allow root URLS for some apps
-                       $root = $defaultRoot;
-               }
+               $root = $this->buildRootPrefix($route, $routeNamePrefix);
 
-               $url = $root . $route['url'];
+               $url = $root . '/' . ltrim($route['url'], '/');
                $verb = strtoupper($route['verb'] ?? 'GET');
 
                $split = explode('#', $name, 2);
@@ -176,44 +172,7 @@ class RouteConfig {
         * @param array $routes
         */
        private function processOCSResources(array $routes): void {
-               // declaration of all restful actions
-               $actions = [
-                       ['name' => 'index', 'verb' => 'GET', 'on-collection' => true],
-                       ['name' => 'show', 'verb' => 'GET'],
-                       ['name' => 'create', 'verb' => 'POST', 'on-collection' => true],
-                       ['name' => 'update', 'verb' => 'PUT'],
-                       ['name' => 'destroy', 'verb' => 'DELETE'],
-               ];
-
-               $resources = $routes['ocs-resources'] ?? [];
-               foreach ($resources as $resource => $config) {
-                       $root = $config['root'] ?? '/apps/' . $this->appName;
-
-                       // the url parameter used as id to the resource
-                       foreach ($actions as $action) {
-                               $url = $root . $config['url'];
-                               $method = $action['name'];
-                               $verb = strtoupper($action['verb'] ?? 'GET');
-                               $collectionAction = $action['on-collection'] ?? false;
-                               if (!$collectionAction) {
-                                       $url .= '/{id}';
-                               }
-                               if (isset($action['url-postfix'])) {
-                                       $url .= '/' . $action['url-postfix'];
-                               }
-
-                               $controller = $resource;
-
-                               $controllerName = $this->buildControllerName($controller);
-                               $actionName = $this->buildActionName($method);
-
-                               $routeName = 'ocs.' . $this->appName . '.' . strtolower($resource) . '.' . strtolower($method);
-
-                               $this->router->create($routeName, $url)->method($verb)->action(
-                                       new RouteActionHandler($this->container, $controllerName, $actionName)
-                               );
-                       }
-               }
+               $this->processResources($routes['ocs-resources'] ?? [], 'ocs.');
        }
 
        /**
@@ -226,7 +185,22 @@ class RouteConfig {
         *
         * @param array $routes
         */
-       private function processResources(array $routes): void {
+       private function processIndexResources(array $routes): void {
+               $this->processResources($routes['resources'] ?? []);
+       }
+
+       /**
+        * For a given name and url restful routes are created:
+        *  - index
+        *  - show
+        *  - create
+        *  - update
+        *  - destroy
+        *
+        * @param array $resources
+        * @param string $routeNamePrefix
+        */
+       protected function processResources(array $resources, string $routeNamePrefix = ''): void {
                // declaration of all restful actions
                $actions = [
                        ['name' => 'index', 'verb' => 'GET', 'on-collection' => true],
@@ -236,13 +210,14 @@ class RouteConfig {
                        ['name' => 'destroy', 'verb' => 'DELETE'],
                ];
 
-               $resources = $routes['resources'] ?? [];
                foreach ($resources as $resource => $config) {
+                       $root = $this->buildRootPrefix($config, $routeNamePrefix);
 
                        // the url parameter used as id to the resource
                        foreach ($actions as $action) {
-                               $url = $config['url'];
+                               $url = $root . '/' . ltrim($config['url'], '/');
                                $method = $action['name'];
+
                                $verb = strtoupper($action['verb'] ?? 'GET');
                                $collectionAction = $action['on-collection'] ?? false;
                                if (!$collectionAction) {
@@ -257,7 +232,7 @@ class RouteConfig {
                                $controllerName = $this->buildControllerName($controller);
                                $actionName = $this->buildActionName($method);
 
-                               $routeName = $this->appName . '.' . strtolower($resource) . '.' . strtolower($method);
+                               $routeName = $routeNamePrefix . $this->appName . '.' . strtolower($resource) . '.' . strtolower($method);
 
                                $this->router->create($routeName, $url)->method($verb)->action(
                                        new RouteActionHandler($this->container, $controllerName, $actionName)
@@ -266,6 +241,23 @@ class RouteConfig {
                }
        }
 
+       private function buildRootPrefix(array $route, string $routeNamePrefix): string {
+               $defaultRoot = $this->appName === 'core' ? '' : '/apps/' . $this->appName;
+               $root = $route['root'] ?? $defaultRoot;
+
+               if ($routeNamePrefix !== '') {
+                       // In OCS all apps are whitelisted
+                       return $root;
+               }
+
+               if (!\in_array($this->appName, $this->rootUrlApps, true)) {
+                       // Only allow root URLS for some apps
+                       return  $defaultRoot;
+               }
+
+               return $root;
+       }
+
        /**
         * Based on a given route name the controller name is generated
         * @param string $controller

diff --git a/tests/lib/AppFramework/Routing/RoutingTest.php b/tests/lib/AppFramework/Routing/RoutingTest.php
index f9cd181468fd0bde4f7cb4f55a73fac8696c8741..34aaff82310d855f54acf63b1c9ae3e4828fe707 100644 (file)
--- a/tests/lib/AppFramework/Routing/RoutingTest.php
+++ b/tests/lib/AppFramework/Routing/RoutingTest.php
@@ -194,13 +194,13 @@ class RoutingTest extends \Test\TestCase {
        public function testResource() {
                $routes = ['resources' => ['account' => ['url' => '/accounts']]];
 
-               $this->assertResource($routes, 'account', '/accounts', 'AccountController', 'id');
+               $this->assertResource($routes, 'account', '/apps/app1/accounts', 'AccountController', 'id');
        }
 
        public function testResourceWithUnderScoreName() {
                $routes = ['resources' => ['admin_accounts' => ['url' => '/admin/accounts']]];
 
-               $this->assertResource($routes, 'admin_accounts', '/admin/accounts', 'AdminAccountsController', 'id');
+               $this->assertResource($routes, 'admin_accounts', '/apps/app1/admin/accounts', 'AdminAccountsController', 'id');
        }
 
        private function assertSimpleRoute($routes, $name, $verb, $url, $controllerName, $actionName, array $requirements = [], array $defaults = [], $postfix = '', $allowRootUrl = false): void {