From 131df248ef29f2dd1d78a1dd43b1fe09a65b3a48 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Wed, 12 Apr 2017 21:25:22 +0300 Subject: [PATCH 1/9] Catch session already closed exception in destructor --- lib/private/Session/CryptoSessionData.php | 7 ++++++- lib/private/Session/Internal.php | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php index 4e0b852cb3..31fcea4a7a 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/CryptoSessionData.php @@ -64,7 +64,12 @@ class CryptoSessionData implements \ArrayAccess, ISession { * Close session if class gets destructed */ public function __destruct() { - $this->close(); + try { + $this->close(); + } catch (SessionNotAvailableException $e){ + // This exception can occur if session is already closed + // So it is safe to ignore it and let the garbage collector to proceed + } } protected function initializeSession() { diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index 22878154c0..72af5727a5 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -151,7 +151,7 @@ class Internal extends Session { */ private function validateSession() { if ($this->sessionClosed) { - throw new \Exception('Session has been closed - no further changes to the session are allowed'); + throw new SessionNotAvailableException('Session has been closed - no further changes to the session are allowed'); } } } From ac0c21f4a7473396677ab5eb461c660532a1558e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 25 Apr 2017 17:11:57 +0200 Subject: [PATCH 2/9] Trigger change when a user is enabled/disabled Signed-off-by: Joas Schilling --- lib/private/User/User.php | 6 ++++- tests/lib/User/UserTest.php | 50 ++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/lib/private/User/User.php b/lib/private/User/User.php index a3be0c24bb..f55807bc76 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -342,9 +342,13 @@ class User implements IUser { * @param bool $enabled */ public function setEnabled($enabled) { + $oldStatus = $this->isEnabled(); $this->enabled = $enabled; $enabled = ($enabled) ? 'true' : 'false'; - $this->config->setUserValue($this->uid, 'core', 'enabled', $enabled); + if ($oldStatus !== $this->enabled) { + $this->triggerChange('enabled', $enabled); + $this->config->setUserValue($this->uid, 'core', 'enabled', $enabled); + } } /** diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 5fc07b692f..b53d07b0d4 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -705,7 +705,55 @@ class UserTest extends TestCase { 'false' ); - $user = new User('foo', $backend, null, $config); + $user = $this->getMockBuilder(User::class) + ->setConstructorArgs([ + 'foo', + $backend, + null, + $config, + ]) + ->setMethods(['isEnabled', 'triggerChange']) + ->getMock(); + + $user->expects($this->once()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->once()) + ->method('triggerChange') + ->with( + 'enabled', + 'false' + ); + + $user->setEnabled(false); + } + + public function testSetDisabledAlreadyDisabled() { + /** + * @var Backend | \PHPUnit_Framework_MockObject_MockObject $backend + */ + $backend = $this->createMock(\Test\Util\User\Dummy::class); + + $config = $this->createMock(IConfig::class); + $config->expects($this->never()) + ->method('setUserValue'); + + $user = $this->getMockBuilder(User::class) + ->setConstructorArgs([ + 'foo', + $backend, + null, + $config, + ]) + ->setMethods(['isEnabled', 'triggerChange']) + ->getMock(); + + $user->expects($this->once()) + ->method('isEnabled') + ->willReturn(false); + $user->expects($this->never()) + ->method('triggerChange'); + $user->setEnabled(false); } From 5ee445c54bbd0f29bfc46dce62ee43a127f483aa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 25 Apr 2017 17:18:56 +0200 Subject: [PATCH 3/9] Audit log for enabling/disabling a user Signed-off-by: Joas Schilling --- apps/admin_audit/lib/actions/usermanagement.php | 17 +++++++++++++++++ apps/admin_audit/lib/auditlogger.php | 1 + 2 files changed, 18 insertions(+) diff --git a/apps/admin_audit/lib/actions/usermanagement.php b/apps/admin_audit/lib/actions/usermanagement.php index 925d8b0a71..0ee192d9a3 100644 --- a/apps/admin_audit/lib/actions/usermanagement.php +++ b/apps/admin_audit/lib/actions/usermanagement.php @@ -60,6 +60,23 @@ class UserManagement extends Action { ); } + /** + * Log enabling of users + * + * @param array $params + */ + public function change(array $params) { + if ($params['feature'] === 'enabled') { + $this->log( + $params['value'] === 'true' ? 'User enabled: "%s"' : 'User disabled: "%s"', + ['user' => $params['user']->getUID()], + [ + 'user', + ] + ); + } + } + /** * Logs changing of the user scope * diff --git a/apps/admin_audit/lib/auditlogger.php b/apps/admin_audit/lib/auditlogger.php index a01fec6301..4e1909c647 100644 --- a/apps/admin_audit/lib/auditlogger.php +++ b/apps/admin_audit/lib/auditlogger.php @@ -90,6 +90,7 @@ class AuditLogger { Util::connectHook('OC_User', 'post_createUser', $userActions, 'create'); Util::connectHook('OC_User', 'post_deleteUser', $userActions, 'delete'); + Util::connectHook('OC_User', 'changeUser', $userActions, 'change'); $this->userSession->listen('\OC\User', 'postSetPassword', [$userActions, 'setPassword']); } From 6a16df728858de1a021d27c1406c2cf1dfd86784 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Mon, 24 Apr 2017 21:11:48 +0200 Subject: [PATCH 4/9] Add new auth flow This implements the basics for the new app-password based authentication flow for our clients. The current implementation tries to keep it as simple as possible and works the following way: 1. Unauthenticated client opens `/index.php/login/flow` 2. User will be asked whether they want to grant access to the client 3. If accepted the user has the chance to do so using existing App Token or automatically generate an app password. If the user chooses to use an existing app token then that one will simply be redirected to the `nc://` protocol handler. While we can improve on that in the future, I think keeping this smaller at the moment has its advantages. Also, in the near future we have to think about an automatic migration endpoint so there's that anyways :-) If the user chooses to use the regular login the following happens: 1. A session state token is written to the session 2. User is redirected to the login page 3. If successfully authenticated they will be redirected to a page redirecting to the POST controller 4. The POST controller will check if the CSRF token as well as the state token is correct, if yes the user will be redirected to the `nc://` protocol handler. This approach is quite simple but also allows to be extended in the future. One could for example allow external websites to consume this authentication endpoint as well. Signed-off-by: Lukas Reschke --- core/Controller/ClientFlowLoginController.php | 236 ++++++++++ core/css/login/authpicker.css | 5 + core/js/login/authpicker.js | 13 + core/js/login/redirect.js | 3 + core/routes.php | 3 + core/templates/loginflow/authpicker.php | 57 +++ core/templates/loginflow/redirect.php | 34 ++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../ClientFlowLoginControllerTest.php | 408 ++++++++++++++++++ 10 files changed, 761 insertions(+) create mode 100644 core/Controller/ClientFlowLoginController.php create mode 100644 core/css/login/authpicker.css create mode 100644 core/js/login/authpicker.js create mode 100644 core/js/login/redirect.js create mode 100644 core/templates/loginflow/authpicker.php create mode 100644 core/templates/loginflow/redirect.php create mode 100644 tests/Core/Controller/ClientFlowLoginControllerTest.php diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php new file mode 100644 index 0000000000..891910b8d0 --- /dev/null +++ b/core/Controller/ClientFlowLoginController.php @@ -0,0 +1,236 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ + +namespace OC\Core\Controller; + +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\PasswordlessTokenException; +use OC\Authentication\Token\IProvider; +use OC\Authentication\Token\IToken; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Http\TemplateResponse; +use OCP\Defaults; +use OCP\IL10N; +use OCP\IRequest; +use OCP\ISession; +use OCP\IURLGenerator; +use OCP\IUserSession; +use OCP\Security\ISecureRandom; +use OCP\Session\Exceptions\SessionNotAvailableException; + +class ClientFlowLoginController extends Controller { + /** @var IUserSession */ + private $userSession; + /** @var IL10N */ + private $l10n; + /** @var Defaults */ + private $defaults; + /** @var ISession */ + private $session; + /** @var IProvider */ + private $tokenProvider; + /** @var ISecureRandom */ + private $random; + /** @var IURLGenerator */ + private $urlGenerator; + + const stateName = 'client.flow.state.token'; + + /** + * @param string $appName + * @param IRequest $request + * @param IUserSession $userSession + * @param IL10N $l10n + * @param Defaults $defaults + * @param ISession $session + * @param IProvider $tokenProvider + * @param ISecureRandom $random + * @param IURLGenerator $urlGenerator + */ + public function __construct($appName, + IRequest $request, + IUserSession $userSession, + IL10N $l10n, + Defaults $defaults, + ISession $session, + IProvider $tokenProvider, + ISecureRandom $random, + IURLGenerator $urlGenerator) { + parent::__construct($appName, $request); + $this->userSession = $userSession; + $this->l10n = $l10n; + $this->defaults = $defaults; + $this->session = $session; + $this->tokenProvider = $tokenProvider; + $this->random = $random; + $this->urlGenerator = $urlGenerator; + } + + /** + * @return string + */ + private function getClientName() { + return $this->request->getHeader('USER_AGENT') !== null ? $this->request->getHeader('USER_AGENT') : 'unknown'; + } + + /** + * @param string $stateToken + * @return bool + */ + private function isValidToken($stateToken) { + $currentToken = $this->session->get(self::stateName); + if(!is_string($stateToken) || !is_string($currentToken)) { + return false; + } + return hash_equals($currentToken, $stateToken); + } + + /** + * @return TemplateResponse + */ + private function stateTokenForbiddenResponse() { + $response = new TemplateResponse( + $this->appName, + '403', + [ + 'file' => $this->l10n->t('State token does not match'), + ], + 'guest' + ); + $response->setStatus(Http::STATUS_FORBIDDEN); + return $response; + } + + /** + * @PublicPage + * @NoCSRFRequired + * @UseSession + * + * @return TemplateResponse + */ + public function showAuthPickerPage() { + if($this->userSession->isLoggedIn()) { + return new TemplateResponse( + $this->appName, + '403', + [ + 'file' => $this->l10n->t('Auth flow can only be started unauthenticated.'), + ], + 'guest' + ); + } + + $stateToken = $this->random->generate( + 64, + ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS + ); + $this->session->set(self::stateName, $stateToken); + + return new TemplateResponse( + $this->appName, + 'loginflow/authpicker', + [ + 'client' => $this->getClientName(), + 'instanceName' => $this->defaults->getName(), + 'urlGenerator' => $this->urlGenerator, + 'stateToken' => $stateToken, + 'serverHost' => $this->request->getServerHost(), + ], + 'guest' + ); + } + + /** + * @NoAdminRequired + * @NoCSRFRequired + * @UseSession + * + * @param string $stateToken + * @return TemplateResponse + */ + public function redirectPage($stateToken = '') { + if(!$this->isValidToken($stateToken)) { + return $this->stateTokenForbiddenResponse(); + } + + return new TemplateResponse( + $this->appName, + 'loginflow/redirect', + [ + 'urlGenerator' => $this->urlGenerator, + 'stateToken' => $stateToken, + ], + 'empty' + ); + } + + /** + * @NoAdminRequired + * @UseSession + * + * @param string $stateToken + * @return Http\RedirectResponse|Response + */ + public function generateAppPassword($stateToken) { + $this->session->remove(self::stateName); + if(!$this->isValidToken($stateToken)) { + return $this->stateTokenForbiddenResponse(); + } + + try { + $sessionId = $this->session->getId(); + } catch (SessionNotAvailableException $ex) { + $response = new Response(); + $response->setStatus(Http::STATUS_FORBIDDEN); + return $response; + } + + try { + $sessionToken = $this->tokenProvider->getToken($sessionId); + $loginName = $sessionToken->getLoginName(); + try { + $password = $this->tokenProvider->getPassword($sessionToken, $sessionId); + } catch (PasswordlessTokenException $ex) { + $password = null; + } + } catch (InvalidTokenException $ex) { + $response = new Response(); + $response->setStatus(Http::STATUS_FORBIDDEN); + return $response; + } + + $token = $this->random->generate(60); + $this->tokenProvider->generateToken( + $token, + $this->userSession->getUser()->getUID(), + $loginName, + $password, + $this->getClientName(), + IToken::PERMANENT_TOKEN, + IToken::DO_NOT_REMEMBER + ); + + return new Http\RedirectResponse('nc://' . urlencode($loginName) . ':' . urlencode($token) . '@' . $this->request->getServerHost()); + } + +} diff --git a/core/css/login/authpicker.css b/core/css/login/authpicker.css new file mode 100644 index 0000000000..d0f32173d2 --- /dev/null +++ b/core/css/login/authpicker.css @@ -0,0 +1,5 @@ +.picker-window { + background: rgba(255,255,255,0.3); + border-radius: 3px; + margin-bottom:20px; +} \ No newline at end of file diff --git a/core/js/login/authpicker.js b/core/js/login/authpicker.js new file mode 100644 index 0000000000..6d8a6bb416 --- /dev/null +++ b/core/js/login/authpicker.js @@ -0,0 +1,13 @@ +jQuery(document).ready(function() { + $('#app-token-login').click(function (e) { + e.preventDefault(); + $(this).addClass('hidden'); + $('#redirect-link').addClass('hidden'); + $('#app-token-login-field').removeClass('hidden'); + }); + + $('#submit-app-token-login').click(function(e) { + e.preventDefault(); + window.location.href = 'nc://' + encodeURIComponent($('#user').val()) + ':' + encodeURIComponent($('#password').val()) + '@' + encodeURIComponent($('#serverHost').val()); + }); +}); diff --git a/core/js/login/redirect.js b/core/js/login/redirect.js new file mode 100644 index 0000000000..ea214feab2 --- /dev/null +++ b/core/js/login/redirect.js @@ -0,0 +1,3 @@ +jQuery(document).ready(function() { + $('#submit-redirect-form').trigger('click'); +}); diff --git a/core/routes.php b/core/routes.php index 7a2d9d750c..93a098c596 100644 --- a/core/routes.php +++ b/core/routes.php @@ -49,6 +49,9 @@ $application->registerRoutes($this, [ ['name' => 'login#confirmPassword', 'url' => '/login/confirm', 'verb' => 'POST'], ['name' => 'login#showLoginForm', 'url' => '/login', 'verb' => 'GET'], ['name' => 'login#logout', 'url' => '/logout', 'verb' => 'GET'], + ['name' => 'ClientFlowLogin#showAuthPickerPage', 'url' => '/login/flow', 'verb' => 'GET'], + ['name' => 'ClientFlowLogin#redirectPage', 'url' => '/login/flow/redirect', 'verb' => 'GET'], + ['name' => 'ClientFlowLogin#generateAppPassword', 'url' => '/login/flow', 'verb' => 'POST'], ['name' => 'TwoFactorChallenge#selectChallenge', 'url' => '/login/selectchallenge', 'verb' => 'GET'], ['name' => 'TwoFactorChallenge#showChallenge', 'url' => '/login/challenge/{challengeProviderId}', 'verb' => 'GET'], ['name' => 'TwoFactorChallenge#solveChallenge', 'url' => '/login/challenge/{challengeProviderId}', 'verb' => 'POST'], diff --git a/core/templates/loginflow/authpicker.php b/core/templates/loginflow/authpicker.php new file mode 100644 index 0000000000..c5eb6cb316 --- /dev/null +++ b/core/templates/loginflow/authpicker.php @@ -0,0 +1,57 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ + +script('core', 'login/authpicker'); +style('core', 'login/authpicker'); + +/** @var array $_ */ +/** @var \OCP\IURLGenerator $urlGenerator */ +$urlGenerator = $_['urlGenerator']; +?> + +
+

+ t('You are about to grant "%s" access to your %s account.', [$_['client'], $_['instanceName']])) ?> +

+ +
+ + + + +
+ +t('Alternative login using app token')) ?> diff --git a/core/templates/loginflow/redirect.php b/core/templates/loginflow/redirect.php new file mode 100644 index 0000000000..544dcab831 --- /dev/null +++ b/core/templates/loginflow/redirect.php @@ -0,0 +1,34 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ +script('core', 'login/redirect'); + +/** @var array $_ */ +/** @var \OCP\IURLGenerator $urlGenerator */ +$urlGenerator = $_['urlGenerator']; +?> + +t('Redirecting…')) ?> + +
+ + + +
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 0d5f067779..516ac7c823 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -444,6 +444,7 @@ return array( 'OC\\Core\\Command\\User\\ResetPassword' => $baseDir . '/core/Command/User/ResetPassword.php', 'OC\\Core\\Command\\User\\Setting' => $baseDir . '/core/Command/User/Setting.php', 'OC\\Core\\Controller\\AvatarController' => $baseDir . '/core/Controller/AvatarController.php', + 'OC\\Core\\Controller\\ClientFlowLoginController' => $baseDir . '/core/Controller/ClientFlowLoginController.php', 'OC\\Core\\Controller\\CssController' => $baseDir . '/core/Controller/CssController.php', 'OC\\Core\\Controller\\JsController' => $baseDir . '/core/Controller/JsController.php', 'OC\\Core\\Controller\\LoginController' => $baseDir . '/core/Controller/LoginController.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 82c31c24a2..5cb12a4b64 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -474,6 +474,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Core\\Command\\User\\ResetPassword' => __DIR__ . '/../../..' . '/core/Command/User/ResetPassword.php', 'OC\\Core\\Command\\User\\Setting' => __DIR__ . '/../../..' . '/core/Command/User/Setting.php', 'OC\\Core\\Controller\\AvatarController' => __DIR__ . '/../../..' . '/core/Controller/AvatarController.php', + 'OC\\Core\\Controller\\ClientFlowLoginController' => __DIR__ . '/../../..' . '/core/Controller/ClientFlowLoginController.php', 'OC\\Core\\Controller\\CssController' => __DIR__ . '/../../..' . '/core/Controller/CssController.php', 'OC\\Core\\Controller\\JsController' => __DIR__ . '/../../..' . '/core/Controller/JsController.php', 'OC\\Core\\Controller\\LoginController' => __DIR__ . '/../../..' . '/core/Controller/LoginController.php', diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php new file mode 100644 index 0000000000..afaca3daca --- /dev/null +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -0,0 +1,408 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ + +namespace Tests\Core\Controller; + +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\PasswordlessTokenException; +use OC\Authentication\Token\IProvider; +use OC\Authentication\Token\IToken; +use OC\Core\Controller\ClientFlowLoginController; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\TemplateResponse; +use OCP\Defaults; +use OCP\IL10N; +use OCP\IRequest; +use OCP\ISession; +use OCP\IURLGenerator; +use OCP\IUser; +use OCP\IUserSession; +use OCP\Security\ISecureRandom; +use OCP\Session\Exceptions\SessionNotAvailableException; +use Test\TestCase; + +class ClientFlowLoginControllerTest extends TestCase { + /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ + private $request; + /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */ + private $userSession; + /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */ + private $l10n; + /** @var Defaults|\PHPUnit_Framework_MockObject_MockObject */ + private $defaults; + /** @var ISession|\PHPUnit_Framework_MockObject_MockObject */ + private $session; + /** @var IProvider|\PHPUnit_Framework_MockObject_MockObject */ + private $tokenProvider; + /** @var ISecureRandom|\PHPUnit_Framework_MockObject_MockObject */ + private $random; + /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ + private $urlGenerator; + /** @var ClientFlowLoginController */ + private $clientFlowLoginController; + + public function setUp() { + parent::setUp(); + + $this->request = $this->createMock(IRequest::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->l10n = $this->createMock(IL10N::class); + $this->l10n + ->expects($this->any()) + ->method('t') + ->will($this->returnCallback(function($text, $parameters = array()) { + return vsprintf($text, $parameters); + })); + $this->defaults = $this->createMock(Defaults::class); + $this->session = $this->createMock(ISession::class); + $this->tokenProvider = $this->createMock(IProvider::class); + $this->random = $this->createMock(ISecureRandom::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + + $this->clientFlowLoginController = new ClientFlowLoginController( + 'core', + $this->request, + $this->userSession, + $this->l10n, + $this->defaults, + $this->session, + $this->tokenProvider, + $this->random, + $this->urlGenerator + ); + } + + public function testShowAuthPickerPageNotAuthenticated() { + $this->userSession + ->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(true); + + $expected = new TemplateResponse( + 'core', + '403', + [ + 'file' => 'Auth flow can only be started unauthenticated.', + ], + 'guest' + ); + $this->assertEquals($expected, $this->clientFlowLoginController->showAuthPickerPage()); + } + + public function testShowAuthPickerPage() { + $this->userSession + ->expects($this->once()) + ->method('isLoggedIn') + ->willReturn(false); + $this->random + ->expects($this->once()) + ->method('generate') + ->with( + 64, + ISecureRandom::CHAR_LOWER.ISecureRandom::CHAR_UPPER.ISecureRandom::CHAR_DIGITS + ) + ->willReturn('StateToken'); + $this->session + ->expects($this->once()) + ->method('set') + ->with('client.flow.state.token', 'StateToken'); + $this->request + ->expects($this->exactly(2)) + ->method('getHeader') + ->with('USER_AGENT') + ->willReturn('Mac OS X Sync Client'); + $this->defaults + ->expects($this->once()) + ->method('getName') + ->willReturn('ExampleCloud'); + $this->request + ->expects($this->once()) + ->method('getServerHost') + ->willReturn('example.com'); + + $expected = new TemplateResponse( + 'core', + 'loginflow/authpicker', + [ + 'client' => 'Mac OS X Sync Client', + 'instanceName' => 'ExampleCloud', + 'urlGenerator' => $this->urlGenerator, + 'stateToken' => 'StateToken', + 'serverHost' => 'example.com', + ], + 'guest' + ); + $this->assertEquals($expected, $this->clientFlowLoginController->showAuthPickerPage()); + } + + public function testRedirectPageWithInvalidToken() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('OtherToken'); + + $expected = new TemplateResponse( + 'core', + '403', + [ + 'file' => 'State token does not match', + ], + 'guest' + ); + $expected->setStatus(Http::STATUS_FORBIDDEN); + $this->assertEquals($expected, $this->clientFlowLoginController->redirectPage('MyStateToken')); + } + + public function testRedirectPageWithoutToken() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn(null); + + $expected = new TemplateResponse( + 'core', + '403', + [ + 'file' => 'State token does not match', + ], + 'guest' + ); + $expected->setStatus(Http::STATUS_FORBIDDEN); + $this->assertEquals($expected, $this->clientFlowLoginController->redirectPage('MyStateToken')); + } + + public function testRedirectPage() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('MyStateToken'); + + $expected = new TemplateResponse( + 'core', + 'loginflow/redirect', + [ + 'urlGenerator' => $this->urlGenerator, + 'stateToken' => 'MyStateToken', + ], + 'empty' + ); + $this->assertEquals($expected, $this->clientFlowLoginController->redirectPage('MyStateToken')); + } + + public function testGenerateAppPasswordWithInvalidToken() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('OtherToken'); + $this->session + ->expects($this->once()) + ->method('remove') + ->with('client.flow.state.token'); + + $expected = new TemplateResponse( + 'core', + '403', + [ + 'file' => 'State token does not match', + ], + 'guest' + ); + $expected->setStatus(Http::STATUS_FORBIDDEN); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); + } + + public function testGenerateAppPasswordWithSessionNotAvailableException() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('MyStateToken'); + $this->session + ->expects($this->once()) + ->method('remove') + ->with('client.flow.state.token'); + $this->session + ->expects($this->once()) + ->method('getId') + ->willThrowException(new SessionNotAvailableException()); + + $expected = new Http\Response(); + $expected->setStatus(Http::STATUS_FORBIDDEN); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); + } + + public function testGenerateAppPasswordWithInvalidTokenException() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('MyStateToken'); + $this->session + ->expects($this->once()) + ->method('remove') + ->with('client.flow.state.token'); + $this->session + ->expects($this->once()) + ->method('getId') + ->willReturn('SessionId'); + $this->tokenProvider + ->expects($this->once()) + ->method('getToken') + ->with('SessionId') + ->willThrowException(new InvalidTokenException()); + + $expected = new Http\Response(); + $expected->setStatus(Http::STATUS_FORBIDDEN); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); + } + + public function testGeneratePasswordWithPassword() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('MyStateToken'); + $this->session + ->expects($this->once()) + ->method('remove') + ->with('client.flow.state.token'); + $this->session + ->expects($this->once()) + ->method('getId') + ->willReturn('SessionId'); + $myToken = $this->createMock(IToken::class); + $myToken + ->expects($this->once()) + ->method('getLoginName') + ->willReturn('MyLoginName'); + $this->tokenProvider + ->expects($this->once()) + ->method('getToken') + ->with('SessionId') + ->willReturn($myToken); + $this->tokenProvider + ->expects($this->once()) + ->method('getPassword') + ->with($myToken, 'SessionId') + ->willReturn('MyPassword'); + $this->random + ->expects($this->once()) + ->method('generate') + ->with(60) + ->willReturn('MyGeneratedToken'); + $user = $this->createMock(IUser::class); + $user + ->expects($this->once()) + ->method('getUID') + ->willReturn('MyUid'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $this->tokenProvider + ->expects($this->once()) + ->method('generateToken') + ->with( + 'MyGeneratedToken', + 'MyUid', + 'MyLoginName', + 'MyPassword', + 'unknown', + IToken::PERMANENT_TOKEN, + IToken::DO_NOT_REMEMBER + ); + $this->request + ->expects($this->once()) + ->method('getServerHost') + ->willReturn('example.com'); + + $expected = new Http\RedirectResponse('nc://MyLoginName:MyGeneratedToken@example.com'); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); + } + + public function testGeneratePasswordWithoutPassword() { + $this->session + ->expects($this->once()) + ->method('get') + ->with('client.flow.state.token') + ->willReturn('MyStateToken'); + $this->session + ->expects($this->once()) + ->method('remove') + ->with('client.flow.state.token'); + $this->session + ->expects($this->once()) + ->method('getId') + ->willReturn('SessionId'); + $myToken = $this->createMock(IToken::class); + $myToken + ->expects($this->once()) + ->method('getLoginName') + ->willReturn('MyLoginName'); + $this->tokenProvider + ->expects($this->once()) + ->method('getToken') + ->with('SessionId') + ->willReturn($myToken); + $this->tokenProvider + ->expects($this->once()) + ->method('getPassword') + ->with($myToken, 'SessionId') + ->willThrowException(new PasswordlessTokenException()); + $this->random + ->expects($this->once()) + ->method('generate') + ->with(60) + ->willReturn('MyGeneratedToken'); + $user = $this->createMock(IUser::class); + $user + ->expects($this->once()) + ->method('getUID') + ->willReturn('MyUid'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $this->tokenProvider + ->expects($this->once()) + ->method('generateToken') + ->with( + 'MyGeneratedToken', + 'MyUid', + 'MyLoginName', + null, + 'unknown', + IToken::PERMANENT_TOKEN, + IToken::DO_NOT_REMEMBER + ); + $this->request + ->expects($this->once()) + ->method('getServerHost') + ->willReturn('example.com'); + + $expected = new Http\RedirectResponse('nc://MyLoginName:MyGeneratedToken@example.com'); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); + } +} From 05e1092c44196d840d02657f54c15e91bf3b0622 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 25 Apr 2017 09:50:07 +0200 Subject: [PATCH 5/9] Correctly case the stateToken Signed-off-by: Roeland Jago Douma --- core/templates/loginflow/redirect.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/templates/loginflow/redirect.php b/core/templates/loginflow/redirect.php index 544dcab831..9e51d1fcb2 100644 --- a/core/templates/loginflow/redirect.php +++ b/core/templates/loginflow/redirect.php @@ -29,6 +29,6 @@ $urlGenerator = $_['urlGenerator'];
- +
From bb5e5efa6d76d577d6657326f60daab7544054f4 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 25 Apr 2017 09:51:00 +0200 Subject: [PATCH 6/9] Do not remove the state token to early we should check the stateToken before we remove it. Else the check will always fail. Signed-off-by: Roeland Jago Douma --- core/Controller/ClientFlowLoginController.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 891910b8d0..f18af83a9c 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -192,11 +192,13 @@ class ClientFlowLoginController extends Controller { * @return Http\RedirectResponse|Response */ public function generateAppPassword($stateToken) { - $this->session->remove(self::stateName); if(!$this->isValidToken($stateToken)) { + $this->session->remove(self::stateName); return $this->stateTokenForbiddenResponse(); } + $this->session->remove(self::stateName); + try { $sessionId = $this->session->getId(); } catch (SessionNotAvailableException $ex) { From aae079aa292f70ec7025286260e8b9248daf94bc Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 25 Apr 2017 13:12:24 +0200 Subject: [PATCH 7/9] AppToken to 72 chars Signed-off-by: Roeland Jago Douma --- core/Controller/ClientFlowLoginController.php | 2 +- tests/Core/Controller/ClientFlowLoginControllerTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index f18af83a9c..ca9c092321 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -221,7 +221,7 @@ class ClientFlowLoginController extends Controller { return $response; } - $token = $this->random->generate(60); + $token = $this->random->generate(72); $this->tokenProvider->generateToken( $token, $this->userSession->getUser()->getUID(), diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index afaca3daca..7c525b5321 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -310,7 +310,7 @@ class ClientFlowLoginControllerTest extends TestCase { $this->random ->expects($this->once()) ->method('generate') - ->with(60) + ->with(72) ->willReturn('MyGeneratedToken'); $user = $this->createMock(IUser::class); $user @@ -374,7 +374,7 @@ class ClientFlowLoginControllerTest extends TestCase { $this->random ->expects($this->once()) ->method('generate') - ->with(60) + ->with(72) ->willReturn('MyGeneratedToken'); $user = $this->createMock(IUser::class); $user From 61af3f41f028103be677e0363cebf13bd6b582ea Mon Sep 17 00:00:00 2001 From: Jan-Christoph Borchardt Date: Tue, 25 Apr 2017 15:47:08 +0200 Subject: [PATCH 8/9] Fix auth flow background color and redirect view layout Signed-off-by: Jan-Christoph Borchardt --- core/css/login/authpicker.css | 10 +++++++--- core/templates/loginflow/redirect.php | 5 ++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/core/css/login/authpicker.css b/core/css/login/authpicker.css index d0f32173d2..85016ee6a0 100644 --- a/core/css/login/authpicker.css +++ b/core/css/login/authpicker.css @@ -1,5 +1,9 @@ .picker-window { - background: rgba(255,255,255,0.3); + display: block; + padding: 10px; + margin-bottom: 20px; + background-color: rgba(0,0,0,.3); + color: #fff; border-radius: 3px; - margin-bottom:20px; -} \ No newline at end of file + cursor: default; +} diff --git a/core/templates/loginflow/redirect.php b/core/templates/loginflow/redirect.php index 9e51d1fcb2..7ef0184f61 100644 --- a/core/templates/loginflow/redirect.php +++ b/core/templates/loginflow/redirect.php @@ -19,13 +19,16 @@ * */ script('core', 'login/redirect'); +style('core', 'login/authpicker'); /** @var array $_ */ /** @var \OCP\IURLGenerator $urlGenerator */ $urlGenerator = $_['urlGenerator']; ?> -t('Redirecting…')) ?> +
+

t('Redirecting …')) ?>

+
From fd74ad452af95539c26c9ac59a0a0dd17c22d964 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 25 Apr 2017 21:42:38 +0200 Subject: [PATCH 9/9] Remove js debug logging Signed-off-by: Christoph Wurst --- core/js/js.js | 1 - 1 file changed, 1 deletion(-) diff --git a/core/js/js.js b/core/js/js.js index 95c00dd644..8fa459d78d 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -1502,7 +1502,6 @@ function initCore() { var appList = $('#appmenu li'); var availableWidth = $('#header-left').width() - $('#nextcloud').width() - 44; var appCount = Math.floor((availableWidth)/44); - console.log(appCount); // show a maximum of 8 apps if(appCount >= maxApps) { appCount = maxApps;