Skip to content
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.

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

Support for CORS on OCS and Webdav APIs + domain whitelisting #28457

Merged
merged 1 commit into from
Aug 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
109 changes: 109 additions & 0 deletions apps/dav/lib/Connector/Sabre/CorsPlugin.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
<?php
/**
* @author Noveen Sachdeva <noveen.sachdeva@research.iiit.ac.in>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* 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, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OCA\DAV\Connector\Sabre;

use Sabre\HTTP\ResponseInterface;
use Sabre\HTTP\RequestInterface;

/**
* Class CorsPlugin is a plugin which adds CORS headers to the responses
*/
class CorsPlugin extends \Sabre\DAV\ServerPlugin {

/**
* Reference to main server object
*
* @var \Sabre\DAV\Server
*/
private $server;

/**
* Reference to logged in user's session
*
* @var \OCP\IUserSession
*/
private $userSession;

/**
* @param \OCP\IUserSession $userSession
*/
public function __construct(\OCP\IUserSession $userSession) {
$this->userSession = $userSession;
$this->extraHeaders['Access-Control-Allow-Headers'] = ["X-OC-Mtime", "OC-Checksum", "OC-Total-Length", "Depth", "Destination", "Overwrite"];
$this->extraHeaders['Access-Control-Allow-Methods'] = ["MOVE", "COPY"];
}

/**
* This initializes the plugin.
*
* This function is called by \Sabre\DAV\Server, after
* addPlugin is called.
*
* This method should set up the required event subscriptions.
*
* @param \Sabre\DAV\Server $server
* @return void
*/
public function initialize(\Sabre\DAV\Server $server) {
$this->server = $server;

$this->server->on('beforeMethod', [$this, 'setCorsHeaders']);
$this->server->on('beforeMethod:OPTIONS', [$this, 'setOptionsRequestHeaders']);
}

/**
* This method sets the cors headers for all requests
*
* @return void
*/
public function setCorsHeaders(RequestInterface $request, ResponseInterface $response) {
if ($request->getHeader('origin') !== null && !is_null($this->userSession->getUser())) {
$requesterDomain = $request->getHeader('origin');
$userId = $this->userSession->getUser()->getUID();
$response = \OC_Response::setCorsHeaders($userId, $requesterDomain, $response, null, $this->extraHeaders);
}
}

/**
* Handles the OPTIONS request
*
* @param RequestInterface $request
* @param ResponseInterface $response
*
* @return false
*/
public function setOptionsRequestHeaders(RequestInterface $request, ResponseInterface $response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just like this. But use $response for setting the headers, not \OC_Response as there is a risk of framework interference. Sabre has its own way to buffer the headers until the last moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't add headers using the $response->addHeader here, because all OPTIONS requests are unauthorized, I'll have to return false from this method.

If I don't return, I'll get a 401-Unauthorized

Copy link
Contributor

Choose a reason for hiding this comment

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

Response and unauthorized shouldn't matter. Whatever you set on $response, Sabre should set into the response even if you return false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you debug into the Sabre code to find out what it does ?

Copy link
Contributor Author

@noveens noveens Aug 24, 2017

Choose a reason for hiding this comment

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

@PVince81 ,

Can you check out my new approach?

I tried to find out why sabre doesn't set headers from the $response object after returning false, but couldn't find anything.

So, I made a method (https://github.com/owncloud/core/pull/28457/files#diff-f7c504a05e1213746f92d28316dcdf95R297) which would set PHP headers from a response object.
Hence, I called that method when I was returning (https://github.com/owncloud/core/pull/28457/files#diff-99f175128bced5bdb6cf1610acbc9c33R82)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's already better

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a unit test for this method ? I suggest to mock RequestInterface, ResponseInterface and $sapi.

$authorization = $request->getHeader('Authorization');
if ($authorization === null || $authorization === '') {
// Set the proper response
$response->setStatus(200);
$response = \OC_Response::setOptionsRequestHeaders($response, $this->extraHeaders);

// Since All OPTIONS requests are unauthorized, we will have to return false from here
// If we don't return false, due to no authorization, a 401-Unauthorized will be thrown
// Which we don't want here
// Hence this sendResponse
$this->server->sapi->sendResponse($response);
return false;
}
}
}
1 change: 1 addition & 0 deletions apps/dav/lib/Connector/Sabre/ServerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public function createServer($baseUri,
$server->setBaseUri($baseUri);

// Load plugins
$server->addPlugin(new \OCA\DAV\Connector\Sabre\CorsPlugin($this->userSession));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove it and only allow Cors on the new DAV endpoint which is "remote.php/dav". For files it's "remote.php/dav/files/$user/"

I assume that this is what you are targetting with your library ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How to?

Copy link
Contributor

Choose a reason for hiding this comment

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

there are two webdav endpoints: remote.php/webdav/path/to/file is the old one.
You can perform the same operations using remote.php/dav/files/$user/path/to/file.

The backend is slightly different as you might have noticed. And remote.php/dav is built to be more extensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library uses remote.php/webdav and not dav

Copy link
Contributor

Choose a reason for hiding this comment

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

let's leave it then

$server->addPlugin(new \OCA\DAV\Connector\Sabre\MaintenancePlugin($this->config));
$server->addPlugin(new \OCA\DAV\Connector\Sabre\ValidateRequestPlugin('webdav'));
$server->addPlugin(new \OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin($this->config));
Expand Down
2 changes: 2 additions & 0 deletions apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin;
use OCA\DAV\Connector\Sabre\CommentPropertiesPlugin;
use OCA\DAV\Connector\Sabre\CopyEtagHeaderPlugin;
use OCA\DAV\Connector\Sabre\CorsPlugin;
use OCA\DAV\Connector\Sabre\DavAclPlugin;
use OCA\DAV\Connector\Sabre\DummyGetResponsePlugin;
use OCA\DAV\Connector\Sabre\FakeLockerPlugin;
Expand Down Expand Up @@ -84,6 +85,7 @@ public function __construct(IRequest $request, $baseUri) {
$this->server->addPlugin(new MaintenancePlugin($config));
$this->server->addPlugin(new ValidateRequestPlugin('dav'));
$this->server->addPlugin(new BlockLegacyClientPlugin($config));
$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession()));
$authPlugin = new Plugin();
$authPlugin->addBackend(new PublicAuth());
$this->server->addPlugin($authPlugin);
Expand Down
3 changes: 3 additions & 0 deletions apps/provisioning_api/appinfo/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
\OC::$server->getLogger(),
\OC::$server->getTwoFactorAuthManager()
);

API::register('get', '/cloud/users', [$users, 'getUsers'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('post', '/cloud/users', [$users, 'addUser'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('get', '/cloud/users/{userid}', [$users, 'getUser'], 'provisioning_api', API::USER_AUTH);
Expand All @@ -60,6 +61,7 @@
\OC::$server->getUserSession(),
\OC::$server->getRequest()
);

API::register('get', '/cloud/groups', [$groups, 'getGroups'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('post', '/cloud/groups', [$groups, 'addGroup'], 'provisioning_api', API::SUBADMIN_AUTH);
API::register('get', '/cloud/groups/{groupid}', [$groups, 'getGroup'], 'provisioning_api', API::SUBADMIN_AUTH);
Expand All @@ -68,6 +70,7 @@

// Apps
$apps = new Apps(\OC::$server->getAppManager());

API::register('get', '/cloud/apps', [$apps, 'getApps'], 'provisioning_api', API::ADMIN_AUTH);
API::register('get', '/cloud/apps/{appid}', [$apps, 'getAppInfo'], 'provisioning_api', API::ADMIN_AUTH);
API::register('post', '/cloud/apps/{appid}', [$apps, 'enable'], 'provisioning_api', API::ADMIN_AUTH);
Expand Down
8 changes: 4 additions & 4 deletions apps/provisioning_api/lib/Users.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public function getUsers() {
}

if($offset === null) {
$offset = 0;
$offset = 0;
}

$users = [];
Expand Down Expand Up @@ -153,7 +153,7 @@ public function addUser() {
return new Result(null, 106, 'no group specified (required for subadmins)');
}
}

try {
$newUser = $this->userManager->createUser($userId, $password);
$this->logger->info('Successful addUser call with userid: '.$userId, ['app' => 'ocs_api']);
Expand Down Expand Up @@ -219,7 +219,7 @@ public function getUser($parameters) {
return new Result($data);
}

/**
/**
* edit users
*
* @param array $parameters
Expand Down Expand Up @@ -440,7 +440,7 @@ public function getUsersGroups($parameters) {
return new Result(null, 997);
}
}

}

/**
Expand Down
4 changes: 3 additions & 1 deletion core/Controller/CloudController.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public function __construct($appName, IRequest $request) {
/**
* @NoAdminRequired
* @NoCSRFRequired
* @CORS
*
* @return array
*/
Expand All @@ -49,7 +50,7 @@ public function getCapabilities() {
'string' => \OC_Util::getVersionString(),
'edition' => \OC_Util::getEditionString(),
];

$result['capabilities'] = \OC::$server->getCapabilitiesManager()->getCapabilities();

return ['data' => $result];
Expand All @@ -58,6 +59,7 @@ public function getCapabilities() {
/**
* @NoAdminRequired
* @NoCSRFRequired
* @CORS
*
* @return array
*/
Expand Down
3 changes: 2 additions & 1 deletion lib/private/AppFramework/DependencyInjection/DIContainer.php
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,8 @@ public function __construct($appName, $urlParams = []){
return new CORSMiddleware(
$c['Request'],
$c['ControllerMethodReflector'],
$c['OCP\IUserSession']
$c['OCP\IUserSession'],
$c['OCP\IConfig']
);
});

Expand Down
32 changes: 23 additions & 9 deletions lib/private/AppFramework/Middleware/Security/CORSMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
use OC\AppFramework\Utility\ControllerMethodReflector;
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
use OC\User\Session;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Middleware;
use OCP\IRequest;
use OCP\IUserSession;
use OCP\IConfig;

/**
* This middleware sets the correct CORS headers on a response if the
Expand All @@ -55,21 +56,29 @@ class CORSMiddleware extends Middleware {
private $reflector;

/**
* @var Session
* @var IUserSession
*/
private $session;

/**
* @var IConfig
*/
private $config;

/**
* @param IRequest $request
* @param ControllerMethodReflector $reflector
* @param Session $session
* @param IUserSession $session
* @param IConfig $config
*/
public function __construct(IRequest $request,
ControllerMethodReflector $reflector,
Session $session) {
IUserSession $session,
IConfig $config) {
$this->request = $request;
$this->reflector = $reflector;
$this->session = $session;
$this->config = $config;
}

/**
Expand Down Expand Up @@ -114,9 +123,17 @@ public function beforeController($controller, $methodName){
*/
public function afterController($controller, $methodName, Response $response){
// only react if its a CORS request and if the request sends origin and
$userId = null;
if (!is_null($this->session->getUser())) {
$userId = $this->session->getUser()->getUID();
}

if(isset($this->request->server['HTTP_ORIGIN']) &&
$this->reflector->hasAnnotation('CORS')) {
if($this->request->getHeader("Origin") !== null &&
$this->reflector->hasAnnotation('CORS') && !is_null($userId)) {

$requesterDomain = $this->request->getHeader("Origin");

\OC_Response::setCorsHeaders($userId, $requesterDomain, $response, $this->config);

// allow credentials headers must not be true or CSRF is possible
// otherwise
Expand All @@ -128,9 +145,6 @@ public function afterController($controller, $methodName, Response $response){
throw new SecurityException($msg);
}
}

$origin = $this->request->server['HTTP_ORIGIN'];
$response->addHeader('Access-Control-Allow-Origin', $origin);
}
return $response;
}
Expand Down
35 changes: 35 additions & 0 deletions lib/private/Route/Router.php
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,41 @@ public function match($url) {
}

$matcher = new UrlMatcher($this->root, $this->context);

if (\OC::$server->getRequest()->getMethod() === "OPTIONS") {
try {
// Checking whether the actual request (one which OPTIONS is pre-flight for)
// Is actually valid
$requestingMethod = \OC::$server->getRequest()->getHeader('Access-Control-Request-Method');
$tempContext = $this->context;
$tempContext->setMethod($requestingMethod);
$tempMatcher = new UrlMatcher($this->root, $tempContext);
$parameters = $tempMatcher->match($url);

// Reach here if it's valid
$response = new \OC\OCS\Result(null, 100, 'OPTIONS request successful');
$response = \OC_Response::setOptionsRequestHeaders($response);
\OC_API::respond($response, \OC_API::requestedFormat());
Copy link
Contributor

Choose a reason for hiding this comment

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

This will send an OCS format response even if the route itself isn't from an OCS API call.
Please check if there is a way to detect if the route is OCS. Is the matcher able to instantiate the target controller so we can check if it's instanceof OcsController ? If not, check if the base file is v1.php or v2.php ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a need for all that?
I just need to send a 200 status and nothing else is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

my concern is mostly whether the response contains XML stuff from OCS. If the response is completely empty then I'm fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's totally empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

great


// Return since no more processing for an OPTIONS request is required
return;
} catch (ResourceNotFoundException $e) {
if (substr($url, -1) !== '/') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to just rethrow, calling OPTIONS on the root path doesn't make much sense I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not calling options on the root path.
Here, I'm mocking the original request (which would be received after the OPTIONS request), hence, it would make sense to call on the root path.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, fine

// We allow links to apps/files? for backwards compatibility reasons
// However, since Symfony does not allow empty route names, the route
// we need to match is '/', so we need to append the '/' here.
try {
$parameters = $matcher->match($url . '/');
} catch (ResourceNotFoundException $newException) {
// If we still didn't match a route, we throw the original exception
throw $e;
}
} else {
throw $e;
}
}
}

try {
$parameters = $matcher->match($url);
} catch (ResourceNotFoundException $e) {
Expand Down
6 changes: 6 additions & 0 deletions lib/private/Settings/SettingsManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
use OC\Settings\Panels\Personal\Clients;
use OC\Settings\Panels\Personal\Version;
use OC\Settings\Panels\Personal\Tokens;
use OC\Settings\Panels\Personal\Cors;
use OC\Settings\Panels\Personal\Quota;
use OC\Settings\Panels\Admin\BackgroundJobs;
use OC\Settings\Panels\Admin\Certificates;
Expand Down Expand Up @@ -246,6 +247,7 @@ private function getBuiltInPanels($type) {
LegacyPersonal::class,
Version::class,
Tokens::class,
Cors::class,
Quota::class
];
}
Expand All @@ -268,6 +270,10 @@ public function getBuiltInPanel($className) {
Clients::class => new Clients($this->config, $this->defaults),
Version::class => new Version(),
Tokens::class => new Tokens(),
Cors::class => new Cors(
$this->userSession,
$this->urlGenerator,
$this->config),
Quota::class => new Quota($this->helper),
// Admin
BackgroundJobs::class => new BackgroundJobs($this->config),
Expand Down
Loading