From 0d656a9cbfb52b16a6f58fc6039364e15f9acd34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Fri, 23 Feb 2018 11:43:59 +0100 Subject: [PATCH 1/2] fixes #117: in case the token is no longer valid we properly need to handle the exception --- lib/AuthModule.php | 4 +- lib/Sabre/OAuth2.php | 18 +++-- tests/Unit/Sabre/OAuth2Test.php | 118 +++++++++++++++++++------------- 3 files changed, 87 insertions(+), 53 deletions(-) diff --git a/lib/AuthModule.php b/lib/AuthModule.php index 3e2dda33..ed55edce 100644 --- a/lib/AuthModule.php +++ b/lib/AuthModule.php @@ -23,6 +23,7 @@ use OCA\OAuth2\Db\AccessToken; use OCA\OAuth2\Db\AccessTokenMapper; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\Authentication\IAuthModule; use OCP\IRequest; use OCP\IUser; @@ -71,7 +72,6 @@ public function getUserPassword(IRequest $request) { /** * @param string $bearerToken * @return null|IUser - * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException */ public function authToken($bearerToken) { $app = new Application(); @@ -89,6 +89,8 @@ public function authToken($bearerToken) { } } catch (DoesNotExistException $exception) { return null; + } catch (MultipleObjectsReturnedException $e) { + return null; } /** @var IUserManager $userManager */ diff --git a/lib/Sabre/OAuth2.php b/lib/Sabre/OAuth2.php index fb594353..6b4d87fd 100644 --- a/lib/Sabre/OAuth2.php +++ b/lib/Sabre/OAuth2.php @@ -84,7 +84,7 @@ public function __construct(ISession $session, * @return bool True if the user initially authenticated via DAV, false otherwise. */ private function isDavAuthenticated($username) { - return !is_null($this->session->get(self::DAV_AUTHENTICATED)) && + return $this->session->get(self::DAV_AUTHENTICATED) !== null && $this->session->get(self::DAV_AUTHENTICATED) === $username; } @@ -96,6 +96,7 @@ private function isDavAuthenticated($username) { * * @param string $bearerToken The Bearer token. * @return string|false The full principal url, if the token is valid, false otherwise. + * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException */ protected function validateBearerToken($bearerToken) { if ($this->userSession->isLoggedIn() && @@ -112,19 +113,24 @@ protected function validateBearerToken($bearerToken) { \OC_Util::setupFS($userId); $this->session->close(); return $this->principalPrefix . $userId; - } else { - \OC_Util::setupFS(); //login hooks may need early access to the filesystem + } + + \OC_Util::setupFS(); //login hooks may need early access to the filesystem + try { if ($this->userSession->tryAuthModuleLogin($this->request)) { $userId = $this->userSession->getUser()->getUID(); \OC_Util::setupFS($userId); $this->session->set(self::DAV_AUTHENTICATED, $userId); $this->session->close(); return $this->principalPrefix . $userId; - } else { - $this->session->close(); - return false; } + + $this->session->close(); + return false; + } catch (\Exception $ex) { + $this->session->close(); + return false; } } } diff --git a/tests/Unit/Sabre/OAuth2Test.php b/tests/Unit/Sabre/OAuth2Test.php index 768d56a1..3dae8a91 100644 --- a/tests/Unit/Sabre/OAuth2Test.php +++ b/tests/Unit/Sabre/OAuth2Test.php @@ -27,13 +27,20 @@ use OCA\OAuth2\Db\Client; use OCA\OAuth2\Db\ClientMapper; use OCA\OAuth2\Sabre\OAuth2; -use OCA\remote_user_auth\Auth; use OCP\IRequest; use OCP\ISession; use OCP\IUser; use PHPUnit_Framework_MockObject_MockObject; use Test\TestCase; +use OC\Session\Memory; +use OC\User\User; +/** + * Class OAuth2Test + * + * @package OCA\OAuth2\Tests\Unit\Sabre + * @group DB + */ class OAuth2Test extends TestCase { /** @var IRequest | PHPUnit_Framework_MockObject_MockObject $request */ @@ -47,26 +54,33 @@ class OAuth2Test extends TestCase { /** @var ClientMapper $clientMapper */ private $clientMapper; - /** @var AccessTokenMapper */ private $accessTokenMapper; - /** @var Client */ private $client; - /** @var AccessToken */ private $accessToken; + /** @var IUser | PHPUnit_Framework_MockObject_MockObject */ + private $user; + /** @var ISession | PHPUnit_Framework_MockObject_MockObject */ + private $session; public function setUp() { parent::setUp(); - $this->request = $this->createMock('\OCP\IRequest'); + $this->request = $this->createMock(IRequest::class); + $this->session = $this->createMock(Memory::class); + + $this->user = $this->createMock(User::class); + $this->user->expects($this->any()) + ->method('getUID') + ->willReturn($this->userId); $app = new Application(); $container = $app->getContainer(); - $this->clientMapper = $container->query('OCA\OAuth2\Db\ClientMapper'); - $this->accessTokenMapper = $container->query('OCA\OAuth2\Db\AccessTokenMapper'); + $this->clientMapper = $container->query(ClientMapper::class); + $this->accessTokenMapper = $container->query(AccessTokenMapper::class); $client = new Client(); $client->setIdentifier('NXCy3M3a6FM9pecVyUZuGF62AJVJaCfmkYz7us4yr4QZqVzMIkVZUf1v2IzvsFZa'); @@ -84,6 +98,13 @@ public function setUp() { $this->accessToken = $this->accessTokenMapper->insert($accessToken); } + public function providesBearerTokenData() { + return [ + [true], + [false] + ]; + } + protected function tearDown() { parent::tearDown(); @@ -94,115 +115,120 @@ protected function tearDown() { public function testIsDavAuthenticated() { // User has not initially authenticated via DAV /** @var ISession | PHPUnit_Framework_MockObject_MockObject $session */ - $session = $this->createMock('\OC\Session\Memory'); + $session = $this->createMock(Memory::class); $session->expects($this->any()) ->method('get') ->with($this->equalTo(OAuth2::DAV_AUTHENTICATED)) - ->will($this->returnValue(null)); + ->willReturn(null); /** @var Session | PHPUnit_Framework_MockObject_MockObject $userSession */ - $userSession = $this->createMock('\OC\User\Session'); + $userSession = $this->createMock(Session::class); /** @var AuthModule | PHPUnit_Framework_MockObject_MockObject $authModule */ $authModule = $this->createMock(AuthModule::class); $oAuth2 = new OAuth2($session, $userSession, $this->request, $authModule, $this->principalPrefix); $this->assertFalse( - $this->invokePrivate( + static::invokePrivate( $oAuth2, 'isDavAuthenticated', [$this->userId]) ); // User has initially authenticated via DAV - $session = $this->createMock('\OC\Session\Memory'); + $session = $this->createMock(Memory::class); $session->expects($this->any()) ->method('get') ->with($this->equalTo(OAuth2::DAV_AUTHENTICATED)) - ->will($this->returnValue($this->userId)); - $userSession = $this->createMock('\OC\User\Session'); + ->willReturn($this->userId); + $userSession = $this->createMock(Session::class); /** @var AuthModule | PHPUnit_Framework_MockObject_MockObject $authModule */ $authModule = $this->createMock(AuthModule::class); $oAuth2 = new OAuth2($session, $userSession, $this->request, $authModule, $this->principalPrefix); $this->assertTrue( - $this->invokePrivate( + static::invokePrivate( $oAuth2, 'isDavAuthenticated', [$this->userId]) ); } - public function testValidateBearerToken() { - /** @var ISession | PHPUnit_Framework_MockObject_MockObject $session */ - $session = $this->createMock('\OC\Session\Memory'); - $user = $this->createMock('\OC\User\User'); - $user->expects($this->any()) - ->method('getUID') - ->will($this->returnValue($this->userId)); - - // Failing Login + /** + * @dataProvider providesBearerTokenData + * @param bool $invalidToken + */ + public function testValidateBearerTokenFailedLogin($invalidToken) { /** @var Session | PHPUnit_Framework_MockObject_MockObject $userSession */ - $userSession = $this->createMock('\OC\User\Session'); + $userSession = $this->createMock(Session::class); $userSession->expects($this->any()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($this->user); $userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(false)); - $userSession->expects($this->once()) - ->method('tryAuthModuleLogin') - ->will($this->returnValue(false)); + ->willReturn(false); + $this->session->expects($this->once())->method('close'); + if ($invalidToken) { + $userSession->expects($this->once()) + ->method('tryAuthModuleLogin') + ->willReturn(false); + } else { + $userSession->expects($this->once()) + ->method('tryAuthModuleLogin') + ->willThrowException(new \Exception('Invalid token')); + } /** @var AuthModule | PHPUnit_Framework_MockObject_MockObject $authModule */ $authModule = $this->createMock(AuthModule::class); - $oAuth2 = new OAuth2($session, $userSession, $this->request, $authModule, $this->principalPrefix); + $oAuth2 = new OAuth2($this->session, $userSession, $this->request, $authModule, $this->principalPrefix); $this->assertFalse( - $this->invokePrivate( + static::invokePrivate( $oAuth2, 'validateBearerToken', [$this->accessToken->getToken()]) ); + } + public function testValidateBearerToken() { // Successful login - $userSession = $this->createMock('\OC\User\Session'); + /** @var Session | PHPUnit_Framework_MockObject_MockObject $userSession */ + $userSession = $this->createMock(Session::class); $userSession->expects($this->any()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($this->user); $userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(false)); + ->willReturn(false); $userSession->expects($this->once()) ->method('tryAuthModuleLogin') - ->will($this->returnValue(true)); + ->willReturn(true); /** @var AuthModule | PHPUnit_Framework_MockObject_MockObject $authModule */ $authModule = $this->createMock(AuthModule::class); - $oAuth2 = new OAuth2($session, $userSession, $this->request, $authModule, $this->principalPrefix); + $oAuth2 = new OAuth2($this->session, $userSession, $this->request, $authModule, $this->principalPrefix); $this->assertEquals( $this->principalPrefix . $this->userId, - $this->invokePrivate( + static::invokePrivate( $oAuth2, 'validateBearerToken', [$this->accessToken->getToken()]) ); // User has initially authenticated via DAV - $session = $this->createMock('\OC\Session\Memory'); - $session->expects($this->any()) + $this->session->expects($this->any()) ->method('get') ->with($this->equalTo(OAuth2::DAV_AUTHENTICATED)) - ->will($this->returnValue($this->userId)); - $userSession = $this->createMock('\OC\User\Session'); + ->willReturn($this->userId); + $userSession = $this->createMock(Session::class); $userSession->expects($this->any()) ->method('getUser') - ->will($this->returnValue($user)); + ->willReturn($this->user); $userSession->expects($this->once()) ->method('isLoggedIn') - ->will($this->returnValue(true)); + ->willReturn(true); $john = $this->createMock(IUser::class); /** @var AuthModule | PHPUnit_Framework_MockObject_MockObject $authModule */ $authModule = $this->createMock(AuthModule::class); $authModule->expects($this->once())->method('authToken')->willReturn($john); - $oAuth2 = new OAuth2($session, $userSession, $this->request, $authModule, $this->principalPrefix); + $oAuth2 = new OAuth2($this->session, $userSession, $this->request, $authModule, $this->principalPrefix); $this->assertEquals( $this->principalPrefix . $this->userId, - $this->invokePrivate( + static::invokePrivate( $oAuth2, 'validateBearerToken', [$this->accessToken->getToken()]) From 79d24db2a72c5ca5bdaaad16911cfaeb13efa62e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Fri, 23 Feb 2018 12:17:00 +0100 Subject: [PATCH 2/2] Fix travis - testing correct php versions on correct branches --- .travis.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6246514b..23cf1414 100755 --- a/.travis.yml +++ b/.travis.yml @@ -10,7 +10,7 @@ php: env: global: - - CORE_BRANCH=master + - CORE_BRANCH=stable10 - APP_NAME=oauth2 matrix: - DB=sqlite @@ -25,8 +25,10 @@ matrix: env: DB=pgsql - php: 5.6 env: DB=mysqlmb4 - - php: 7.0 - env: DB=sqlite CORE_BRANCH=stable10 + - php: 7.1 + env: DB=sqlite CORE_BRANCH=master + - php: 7.2 + env: DB=sqlite CORE_BRANCH=master allow_failures: - php: nightly fast_finish: true