Skip to content

Make sure urlparams are correctly injected in global routes #14626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 2 commits into from
Mar 12, 2019
Merged

Make sure urlparams are correctly injected in global routes #14626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e8b033
Make sure urlParams are correctly injected in global routes
nickvergessen Mar 11, 2019
a3c9e5b
Add a message about disabled global routes
nickvergessen Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 11 additions & 90 deletions core/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@
['name' => 'contactsMenu#findOne', 'url' => '/contactsmenu/findOne', 'verb' => 'POST'],
['name' => 'WalledGarden#get', 'url' => '/204', 'verb' => 'GET'],
['name' => 'Search#search', 'url' => '/core/search', 'verb' => 'GET'],

// 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'],
Copy link
Member Author

@nickvergessen nickvergessen Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should also be possible, to move those to the respective apps, when at the same time we allow defining a root' => '/cloud' similar to OCS urls.
Just not sure how this breaks existing route generations (because all lowercase instead of CamelCase controller names, which is why the hacks in lib/private/AppFramework/Routing/RouteConfig.php were necessary)

['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'],
],
'ocs' => [
['root' => '/cloud', 'name' => 'OCS#getCapabilities', 'url' => '/capabilities', 'verb' => 'GET'],
Expand Down Expand Up @@ -109,93 +120,3 @@
// Routing
$this->create('core_ajax_update', '/core/ajax/update.php')
->actionInclude('core/ajax/update.php');

// File routes
$this->create('files.viewcontroller.showFile', '/f/{fileid}')->action(function($urlParams) {
$app = new \OCA\Files\AppInfo\Application($urlParams);
$app->dispatch('ViewController', 'index');
});

// Call routes
/**
* @suppress PhanUndeclaredClassConstant
* @suppress PhanUndeclaredClassMethod
*/
$this->create('spreed.pagecontroller.showCall', '/call/{token}')->action(function($urlParams) {
if (class_exists(\OCA\Spreed\AppInfo\Application::class, false)) {
$app = new \OCA\Spreed\AppInfo\Application($urlParams);
$app->dispatch('PageController', 'index');
} else {
throw new \OC\HintException('App spreed is not enabled');
}
});

// OCM routes
/**
* @suppress PhanUndeclaredClassConstant
* @suppress PhanUndeclaredClassMethod
*/
$this->create('cloud_federation_api.requesthandlercontroller.addShare', '/ocm/shares')->post()->action(function($urlParams) {
if (class_exists(\OCA\CloudFederationAPI\AppInfo\Application::class, false)) {
$app = new \OCA\CloudFederationAPI\AppInfo\Application($urlParams);
$app->dispatch('RequestHandlerController', 'addShare');
} else {
throw new \OC\HintException('Cloud Federation API not enabled');
}
});

/**
* @suppress PhanUndeclaredClassConstant
* @suppress PhanUndeclaredClassMethod
*/
$this->create('cloud_federation_api.requesthandlercontroller.receiveNotification', '/ocm/notifications')->post()->action(function($urlParams) {
if (class_exists(\OCA\CloudFederationAPI\AppInfo\Application::class, false)) {
$app = new \OCA\CloudFederationAPI\AppInfo\Application($urlParams);
$app->dispatch('RequestHandlerController', 'receiveNotification');
} else {
throw new \OC\HintException('Cloud Federation API not enabled');
}
});


// Sharing routes
$this->create('files_sharing.sharecontroller.showShare', '/s/{token}')->action(function($urlParams) {
if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) {
$app = new \OCA\Files_Sharing\AppInfo\Application($urlParams);
$app->dispatch('ShareController', 'showShare');
} else {
throw new \OC\HintException('App file sharing is not enabled');
}
});
$this->create('files_sharing.sharecontroller.authenticate', '/s/{token}/authenticate/{redirect}')->post()->action(function($urlParams) {
if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) {
$app = new \OCA\Files_Sharing\AppInfo\Application($urlParams);
$app->dispatch('ShareController', 'authenticate');
} else {
throw new \OC\HintException('App file sharing is not enabled');
}
});
$this->create('files_sharing.sharecontroller.showAuthenticate', '/s/{token}/authenticate/{redirect}')->get()->action(function($urlParams) {
if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) {
$app = new \OCA\Files_Sharing\AppInfo\Application($urlParams);
$app->dispatch('ShareController', 'showAuthenticate');
} else {
throw new \OC\HintException('App file sharing is not enabled');
}
});
$this->create('files_sharing.sharecontroller.downloadShare', '/s/{token}/download')->get()->action(function($urlParams) {
if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) {
$app = new \OCA\Files_Sharing\AppInfo\Application($urlParams);
$app->dispatch('ShareController', 'downloadShare');
} else {
throw new \OC\HintException('App file sharing is not enabled');
}
});
$this->create('files_sharing.publicpreview.directLink', '/s/{token}/preview')->get()->action(function($urlParams) {
if (class_exists(\OCA\Files_Sharing\AppInfo\Application::class, false)) {
$app = new \OCA\Files_Sharing\AppInfo\Application($urlParams);
$app->dispatch('PublicPreviewController', 'directLink');
} else {
throw new \OC\HintException('App file sharing is not enabled');
}
});
8 changes: 8 additions & 0 deletions lib/private/AppFramework/App.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

use OC\AppFramework\Http\Dispatcher;
use OC\AppFramework\DependencyInjection\DIContainer;
use OC\HintException;
use OCP\AppFramework\Http;
use OCP\AppFramework\QueryException;
use OCP\AppFramework\Http\ICallbackResponse;
Expand Down Expand Up @@ -81,6 +82,7 @@ public static function buildAppNamespace(string $appId, string $topNamespace='OC
* @param string $methodName the method that you want to call
* @param DIContainer $container an instance of a pimple container.
* @param array $urlParams list of URL parameters (optional)
* @throws HintException
*/
public static function main(string $controllerName, string $methodName, DIContainer $container, array $urlParams = null) {
if (!is_null($urlParams)) {
Expand All @@ -94,6 +96,12 @@ public static function main(string $controllerName, string $methodName, DIContai
try {
$controller = $container->query($controllerName);
} catch(QueryException $e) {
if (strpos($controllerName, '\\Controller\\') !== false) {
// This is from a global registered app route that is not enabled.
[/*OC(A)*/, $app, /* Controller/Name*/] = explode('\\', $controllerName, 3);
throw new HintException('App ' . strtolower($app) . ' is not enabled');
Copy link
Member Author

@nickvergessen nickvergessen Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inaccurate for cloud_federation_api as it shows cloudfederationapi instead.
Worth adding another hack? however it's pretty useless anyway.

}

if ($appName === 'core') {
$appNameSpace = 'OC\\Core';
} else if ($appName === 'settings') {
Expand Down
16 changes: 15 additions & 1 deletion lib/private/AppFramework/Routing/RouteConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
namespace OC\AppFramework\Routing;

use OC\AppFramework\DependencyInjection\DIContainer;
use OCP\AppFramework\App;
use OCP\Route\IRouter;

/**
Expand Down Expand Up @@ -155,8 +156,21 @@ private function processSimpleRoutes(array $routes): void {

$controllerName = $this->buildControllerName($controller);
$actionName = $this->buildActionName($action);
$appName = $simpleRoute['app'] ?? $this->appName;

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';
} else if ($controllerName === 'RequesthandlerController') {
$controllerName = 'RequestHandlerController';
}
$controllerName = App::buildAppNamespace($appName) . '\\Controller\\' . $controllerName;
}

$routeName = $this->appName . '.' . $controller . '.' . $action . $postfix;
$routeName = $appName . '.' . $controller . '.' . $action . $postfix;

// register the route
$handler = new RouteActionHandler($this->container, $controllerName, $actionName);
Expand Down