Check if request is sent from official ownCloud client
There are authentication backends such as Shibboleth that do send no Basic Auth credentials for DAV requests. This means that the ownCloud DAV backend would consider these requests coming from an untrusted source and require higher levels of security checks. (e.g. a CSRF check) While an elegant solution would rely on authenticating via token (so that one can properly ensure that the request came indeed from a trusted client) this is a okay'ish workaround for this problem until we have something more reliable in the authentication code.
This commit is contained in:
parent
4b3af9dfe7
commit
cc8c0b6a90
|
@ -30,6 +30,7 @@
|
|||
namespace OCA\DAV\Connector\Sabre;
|
||||
|
||||
use Exception;
|
||||
use OC\AppFramework\Http\Request;
|
||||
use OCP\IRequest;
|
||||
use OCP\ISession;
|
||||
use OCP\IUserSession;
|
||||
|
@ -48,6 +49,8 @@ class Auth extends AbstractBasic {
|
|||
private $userSession;
|
||||
/** @var IRequest */
|
||||
private $request;
|
||||
/** @var string */
|
||||
private $currentUser;
|
||||
|
||||
/**
|
||||
* @param ISession $session
|
||||
|
@ -130,7 +133,46 @@ class Auth extends AbstractBasic {
|
|||
$msg = $e->getMessage();
|
||||
throw new ServiceUnavailable("$class: $msg");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks whether a CSRF check is required on the request
|
||||
*
|
||||
* @return bool
|
||||
*/
|
||||
private function requiresCSRFCheck() {
|
||||
// GET requires no check at all
|
||||
if($this->request->getMethod() === 'GET') {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Official ownCloud clients require no checks
|
||||
if($this->request->isUserAgent([
|
||||
Request::USER_AGENT_OWNCLOUD_DESKTOP,
|
||||
Request::USER_AGENT_OWNCLOUD_ANDROID,
|
||||
Request::USER_AGENT_OWNCLOUD_IOS,
|
||||
])) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// If not logged-in no check is required
|
||||
if(!$this->userSession->isLoggedIn()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// POST always requires a check
|
||||
if($this->request->getMethod() === 'POST') {
|
||||
return true;
|
||||
}
|
||||
|
||||
// If logged-in AND DAV authenticated no check is required
|
||||
if($this->userSession->isLoggedIn() &&
|
||||
$this->isDavAuthenticated($this->userSession->getUser()->getUID())) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* @param RequestInterface $request
|
||||
|
@ -139,27 +181,33 @@ class Auth extends AbstractBasic {
|
|||
* @throws NotAuthenticated
|
||||
*/
|
||||
private function auth(RequestInterface $request, ResponseInterface $response) {
|
||||
// If request is not GET and not authenticated via WebDAV a requesttoken is required
|
||||
if($this->userSession->isLoggedIn() &&
|
||||
$this->request->getMethod() !== 'GET' &&
|
||||
!$this->isDavAuthenticated($this->userSession->getUser()->getUID())) {
|
||||
if(!$this->request->passesCSRFCheck()) {
|
||||
$forcedLogout = false;
|
||||
if(!$this->request->passesCSRFCheck() &&
|
||||
$this->requiresCSRFCheck()) {
|
||||
// In case of a fail with POST we need to recheck the credentials
|
||||
if($this->request->getMethod() === 'POST') {
|
||||
$forcedLogout = true;
|
||||
} else {
|
||||
$response->setStatus(401);
|
||||
throw new \Sabre\DAV\Exception\NotAuthenticated('CSRF check not passed.');
|
||||
}
|
||||
}
|
||||
|
||||
if (\OC_User::handleApacheAuth() ||
|
||||
//Fix for broken webdav clients
|
||||
($this->userSession->isLoggedIn() && is_null($this->session->get(self::DAV_AUTHENTICATED))) ||
|
||||
//Well behaved clients that only send the cookie are allowed
|
||||
($this->userSession->isLoggedIn() && $this->session->get(self::DAV_AUTHENTICATED) === $this->userSession->getUser()->getUID() && $request->getHeader('Authorization') === null)
|
||||
) {
|
||||
$user = $this->userSession->getUser()->getUID();
|
||||
\OC_Util::setupFS($user);
|
||||
$this->currentUser = $user;
|
||||
$this->session->close();
|
||||
return [true, $this->principalPrefix . $user];
|
||||
if($forcedLogout) {
|
||||
$this->userSession->logout();
|
||||
} else {
|
||||
if (\OC_User::handleApacheAuth() ||
|
||||
//Fix for broken webdav clients
|
||||
($this->userSession->isLoggedIn() && is_null($this->session->get(self::DAV_AUTHENTICATED))) ||
|
||||
//Well behaved clients that only send the cookie are allowed
|
||||
($this->userSession->isLoggedIn() && $this->session->get(self::DAV_AUTHENTICATED) === $this->userSession->getUser()->getUID() && $request->getHeader('Authorization') === null)
|
||||
) {
|
||||
$user = $this->userSession->getUser()->getUID();
|
||||
\OC_Util::setupFS($user);
|
||||
$this->currentUser = $user;
|
||||
$this->session->close();
|
||||
return [true, $this->principalPrefix . $user];
|
||||
}
|
||||
}
|
||||
|
||||
if (!$this->userSession->isLoggedIn() && in_array('XMLHttpRequest', explode(',', $request->getHeader('X-Requested-With')))) {
|
||||
|
|
|
@ -198,10 +198,7 @@ class Auth extends TestCase {
|
|||
$this->assertFalse($this->invokePrivate($this->auth, 'validateUserPass', ['MyTestUser', 'MyTestPassword']));
|
||||
}
|
||||
|
||||
/**
|
||||
* @expectedException \Sabre\DAV\Exception\NotAuthenticated
|
||||
* @expectedExceptionMessage CSRF check not passed.
|
||||
*/
|
||||
|
||||
public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet() {
|
||||
$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
|
||||
->disableOriginalConstructor()
|
||||
|
@ -210,27 +207,182 @@ class Auth extends TestCase {
|
|||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->will($this->returnValue(true));
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('getMethod')
|
||||
->willReturn('POST');
|
||||
$this->session
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue(null));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->once())
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('MyWrongDavUser'));
|
||||
$this->userSession
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->method('passesCSRFCheck')
|
||||
->willReturn(false);
|
||||
|
||||
$expectedResponse = [
|
||||
false,
|
||||
"No 'Authorization: Basic' header found. Either the client didn't send one, or the server is mis-configured",
|
||||
];
|
||||
$response = $this->auth->check($request, $response);
|
||||
$this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response);
|
||||
$this->assertSame($expectedResponse, $response);
|
||||
}
|
||||
|
||||
public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAuthenticated() {
|
||||
$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->willReturn(true);
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('getMethod')
|
||||
->willReturn('PROPFIND');
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('isUserAgent')
|
||||
->with([
|
||||
'/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/',
|
||||
'/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/',
|
||||
'/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/',
|
||||
])
|
||||
->willReturn(false);
|
||||
$this->session
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue('LoggedInUser'));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('LoggedInUser'));
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->method('passesCSRFCheck')
|
||||
->willReturn(false);
|
||||
$this->auth->check($request, $response);
|
||||
}
|
||||
|
||||
/**
|
||||
* @expectedException \Sabre\DAV\Exception\NotAuthenticated
|
||||
* @expectedExceptionMessage CSRF check not passed.
|
||||
*/
|
||||
public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDavAuthenticated() {
|
||||
$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->willReturn(true);
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('getMethod')
|
||||
->willReturn('PROPFIND');
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('isUserAgent')
|
||||
->with([
|
||||
'/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/',
|
||||
'/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/',
|
||||
'/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/',
|
||||
])
|
||||
->willReturn(false);
|
||||
$this->session
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue('AnotherUser'));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('LoggedInUser'));
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->method('passesCSRFCheck')
|
||||
->willReturn(false);
|
||||
$this->auth->check($request, $response);
|
||||
}
|
||||
|
||||
public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDesktopClient() {
|
||||
$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->will($this->returnValue(true));
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('getMethod')
|
||||
->willReturn('POST');
|
||||
$this->request
|
||||
->expects($this->any())
|
||||
->method('isUserAgent')
|
||||
->with([
|
||||
'/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/',
|
||||
'/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/',
|
||||
'/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/',
|
||||
])
|
||||
->willReturn(true);
|
||||
$this->session
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue(null));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('MyWrongDavUser'));
|
||||
$this->userSession
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->method('passesCSRFCheck')
|
||||
->willReturn(false);
|
||||
|
||||
$this->auth->check($request, $response);
|
||||
}
|
||||
|
||||
public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForGet() {
|
||||
|
@ -241,26 +393,26 @@ class Auth extends TestCase {
|
|||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->exactly(2))
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->will($this->returnValue(true));
|
||||
$this->session
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue(null));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->once())
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('MyWrongDavUser'));
|
||||
$this->userSession
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->expects($this->any())
|
||||
->method('getMethod')
|
||||
->willReturn('GET');
|
||||
|
||||
|
@ -268,7 +420,6 @@ class Auth extends TestCase {
|
|||
$this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response);
|
||||
}
|
||||
|
||||
|
||||
public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet() {
|
||||
$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
|
||||
->disableOriginalConstructor()
|
||||
|
@ -277,22 +428,22 @@ class Auth extends TestCase {
|
|||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->userSession
|
||||
->expects($this->exactly(2))
|
||||
->expects($this->any())
|
||||
->method('isLoggedIn')
|
||||
->will($this->returnValue(true));
|
||||
$this->session
|
||||
->expects($this->exactly(2))
|
||||
->expects($this->any())
|
||||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue(null));
|
||||
$user = $this->getMockBuilder('\OCP\IUser')
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$user->expects($this->exactly(2))
|
||||
$user->expects($this->any())
|
||||
->method('getUID')
|
||||
->will($this->returnValue('MyWrongDavUser'));
|
||||
$this->userSession
|
||||
->expects($this->exactly(2))
|
||||
->expects($this->any())
|
||||
->method('getUser')
|
||||
->will($this->returnValue($user));
|
||||
$this->request
|
||||
|
@ -368,6 +519,10 @@ class Auth extends TestCase {
|
|||
->method('get')
|
||||
->with('AUTHENTICATED_TO_DAV_BACKEND')
|
||||
->will($this->returnValue('MyTestUser'));
|
||||
$this->request
|
||||
->expects($this->once())
|
||||
->method('getMethod')
|
||||
->willReturn('GET');
|
||||
$httpRequest
|
||||
->expects($this->atLeastOnce())
|
||||
->method('getHeader')
|
||||
|
|
|
@ -59,6 +59,9 @@ class Request implements \ArrayAccess, \Countable, IRequest {
|
|||
// Android Chrome user agent: https://developers.google.com/chrome/mobile/docs/user-agent
|
||||
const USER_AGENT_ANDROID_MOBILE_CHROME = '#Android.*Chrome/[.0-9]*#';
|
||||
const USER_AGENT_FREEBOX = '#^Mozilla/5\.0$#';
|
||||
const USER_AGENT_OWNCLOUD_IOS = '/^Mozilla\/5\.0 \(iOS\) ownCloud\-iOS.*$/';
|
||||
const USER_AGENT_OWNCLOUD_ANDROID = '/^Mozilla\/5\.0 \(Android\) ownCloud\-android.*$/';
|
||||
const USER_AGENT_OWNCLOUD_DESKTOP = '/^Mozilla\/5\.0 \([A-Za-z ]+\) (mirall|csyncoC)\/.*$/';
|
||||
const REGEX_LOCALHOST = '/^(127\.0\.0\.1|localhost)$/';
|
||||
|
||||
protected $inputStream;
|
||||
|
|
Loading…
Reference in New Issue