diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 7bf2555819..5bd06ac7e6 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -58,6 +58,10 @@ use OC\Hooks\PublicEmitter; use OCP\Util; class LoginController extends Controller { + + const LOGIN_MSG_INVALIDPASSWORD = 'invalidpassword'; + const LOGIN_MSG_USERDISABLED = 'userdisabled'; + /** @var IUserManager */ private $userManager; /** @var IConfig */ @@ -171,19 +175,7 @@ class LoginController extends Controller { $parameters['redirect_url'] = $redirect_url; } - $parameters['canResetPassword'] = true; - $parameters['resetPasswordLink'] = $this->config->getSystemValue('lost_password_link', ''); - if (!$parameters['resetPasswordLink']) { - if ($user !== null && $user !== '') { - $userObj = $this->userManager->get($user); - if ($userObj instanceof IUser) { - $parameters['canResetPassword'] = $userObj->canChangePassword(); - } - } - } elseif ($parameters['resetPasswordLink'] === 'disabled') { - $parameters['canResetPassword'] = false; - } - + $parameters = $this->setPasswordResetParameters($user, $parameters); $parameters['alt_login'] = OC_App::getAlternativeLogIns(); if ($user !== null && $user !== '') { @@ -209,6 +201,40 @@ class LoginController extends Controller { ); } + /** + * Sets the password reset params. + * + * Users may not change their passwords if: + * - The account is disabled + * - The backend doesn't support password resets + * - The password reset function is disabled + * + * @param string $user + * @param array $parameters + * @return array + */ + private function setPasswordResetParameters( + string $user = null, array $parameters): array { + if ($user !== null && $user !== '') { + $userObj = $this->userManager->get($user); + } else { + $userObj = null; + } + + $parameters['resetPasswordLink'] = $this->config + ->getSystemValue('lost_password_link', ''); + + if (!$parameters['resetPasswordLink'] && $userObj !== null) { + $parameters['canResetPassword'] = $userObj->canChangePassword(); + } else if ($userObj !== null && $userObj->isEnabled() === false) { + $parameters['canResetPassword'] = false; + } else { + $parameters['canResetPassword'] = true; + } + + return $parameters; + } + /** * @param string $redirectUrl * @return RedirectResponse @@ -256,6 +282,17 @@ class LoginController extends Controller { } $originalUser = $user; + + $userObj = $this->userManager->get($user); + + if ($userObj !== null && $userObj->isEnabled() === false) { + $this->logger->warning('Login failed: \''. $user . '\' disabled' . + ' (Remote IP: \''. $this->request->getRemoteAddress(). '\')', + ['app' => 'core']); + return $this->createLoginFailedResponse($user, $originalUser, + $redirect_url, self::LOGIN_MSG_USERDISABLED); + } + // TODO: Add all the insane error handling /* @var $loginResult IUser */ $loginResult = $this->userManager->checkPasswordNoLogging($user, $password); @@ -270,20 +307,15 @@ class LoginController extends Controller { } } } + if ($loginResult === false) { - $this->logger->warning('Login failed: \''. $user .'\' (Remote IP: \''. $this->request->getRemoteAddress(). '\')', ['app' => 'core']); - // Read current user and append if possible - we need to return the unmodified user otherwise we will leak the login name - $args = !is_null($user) ? ['user' => $originalUser] : []; - if (!is_null($redirect_url)) { - $args['redirect_url'] = $redirect_url; - } - $response = new RedirectResponse($this->urlGenerator->linkToRoute('core.login.showLoginForm', $args)); - $response->throttle(['user' => $user]); - $this->session->set('loginMessages', [ - ['invalidpassword'], [] - ]); - return $response; + $this->logger->warning('Login failed: \''. $user . + '\' (Remote IP: \''. $this->request->getRemoteAddress(). '\')', + ['app' => 'core']); + return $this->createLoginFailedResponse($user, $originalUser, + $redirect_url, self::LOGIN_MSG_INVALIDPASSWORD); } + // TODO: remove password checks from above and let the user session handle failures // requires https://github.com/owncloud/core/pull/24616 $this->userSession->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]); @@ -330,6 +362,33 @@ class LoginController extends Controller { return $this->generateRedirect($redirect_url); } + /** + * Creates a login failed response. + * + * @param string $user + * @param string $originalUser + * @param string $redirect_url + * @param string $loginMessage + * @return RedirectResponse + */ + private function createLoginFailedResponse( + $user, $originalUser, $redirect_url, string $loginMessage) { + // Read current user and append if possible we need to + // return the unmodified user otherwise we will leak the login name + $args = !is_null($user) ? ['user' => $originalUser] : []; + if (!is_null($redirect_url)) { + $args['redirect_url'] = $redirect_url; + } + $response = new RedirectResponse( + $this->urlGenerator->linkToRoute('core.login.showLoginForm', $args) + ); + $response->throttle(['user' => $user]); + $this->session->set('loginMessages', [ + [$loginMessage], [] + ]); + return $response; + } + /** * @NoAdminRequired * @UseSession diff --git a/core/templates/login.php b/core/templates/login.php index 8fc68edb92..989ea1eaad 100644 --- a/core/templates/login.php +++ b/core/templates/login.php @@ -2,6 +2,8 @@ @@ -34,7 +36,7 @@ script('core', 'merged-login');
-

+

t('Username or email')); ?>

-

+

- +

t('Wrong password.')); ?>

+ +

+ getL10N('lib')->t('User disabled')); ?> +

5000) { ?> diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 1e26d86a03..7ebd6ee834 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -286,7 +286,52 @@ class LoginControllerTest extends TestCase { $this->assertEquals($expectedResponse, $this->loginController->showLoginForm('LdapUser', '', '')); } - public function testShowLoginFormForUserNamedNull() { + /** + * Asserts that a disabled user can't login and gets the expected response. + */ + public function testLoginForDisabledUser() { + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); + $user->method('getUID') + ->willReturn('uid'); + $user->method('isEnabled') + ->willReturn(false); + + $this->request + ->expects($this->once()) + ->method('passesCSRFCheck') + ->willReturn(true); + + $this->userSession + ->method('isLoggedIn') + ->willReturn(false); + + $loginName = 'iMDisabled'; + $password = 'secret'; + + $this->session + ->expects($this->once()) + ->method('set') + ->with('loginMessages', [ + [LoginController::LOGIN_MSG_USERDISABLED], [] + ]); + + $this->userManager + ->expects($this->once()) + ->method('get') + ->with($loginName) + ->willReturn($user); + + $expected = new RedirectResponse(''); + $expected->throttle(['user' => $loginName]); + + $response = $this->loginController->tryLogin( + $loginName, $password, null, false, 'Europe/Berlin', '1' + ); + $this->assertEquals($expected, $response); + } + + public function testShowLoginFormForUserNamed0() { $this->userSession ->expects($this->once()) ->method('isLoggedIn') @@ -297,8 +342,7 @@ class LoginControllerTest extends TestCase { ->with('lost_password_link') ->willReturn(false); $user = $this->createMock(IUser::class); - $user - ->expects($this->once()) + $user->expects($this->once()) ->method('canChangePassword') ->willReturn(false); $this->userManager diff --git a/tests/acceptance/features/bootstrap/LoginPageContext.php b/tests/acceptance/features/bootstrap/LoginPageContext.php index 1496e3030c..df7944aa91 100644 --- a/tests/acceptance/features/bootstrap/LoginPageContext.php +++ b/tests/acceptance/features/bootstrap/LoginPageContext.php @@ -70,6 +70,14 @@ class LoginPageContext implements Context, ActorAwareInterface { describedAs("Wrong password message in Login page"); } + /** + * @return Locator + */ + public static function userDisabledMessage() { + return Locator::forThe()->xpath("//*[@class = 'warning userDisabledMsg' and normalize-space() = 'User disabled']")-> + describedAs('User disabled message on login page'); + } + /** * @When I log in with user :user and password :password */ @@ -96,6 +104,14 @@ class LoginPageContext implements Context, ActorAwareInterface { $this->actor->find(self::wrongPasswordMessage(), 10)->isVisible()); } + /** + * @Then I see that the disabled user message is shown + */ + public function iSeeThatTheDisabledUserMessageIsShown() { + PHPUnit_Framework_Assert::assertTrue( + $this->actor->find(self::userDisabledMessage(), 10)->isVisible()); + } + /** * @BeforeScenario */ diff --git a/tests/acceptance/features/login.feature b/tests/acceptance/features/login.feature index 3a31d3f88b..44353d37c6 100644 --- a/tests/acceptance/features/login.feature +++ b/tests/acceptance/features/login.feature @@ -28,6 +28,12 @@ Feature: login Then I see that the current page is the Login page And I see that a wrong password message is shown + Scenario: try to log in as disabled user + Given I visit the Home page + When I log in with user disabledUser and password 123456acb + Then I see that the current page is the Login page + And I see that the disabled user message is shown + Scenario: log in with invalid user once fixed by admin Given I act as John And I can not log in with user unknownUser and password 123456acb diff --git a/tests/acceptance/installAndConfigureServer.sh b/tests/acceptance/installAndConfigureServer.sh index c61faeda23..98de72bf45 100755 --- a/tests/acceptance/installAndConfigureServer.sh +++ b/tests/acceptance/installAndConfigureServer.sh @@ -35,6 +35,8 @@ fi php occ maintenance:install --admin-pass=admin OC_PASS=123456acb php occ user:add --password-from-env user0 +OC_PASS=123456acb php occ user:add --password-from-env disabledUser +php occ user:disable disabledUser if [ "$NEXTCLOUD_SERVER_DOMAIN" != "" ]; then # Default first trusted domain is "localhost"; replace it with given domain.