diff options
| -rw-r--r-- | core/Controller/LoginController.php | 109 | ||||
| -rw-r--r-- | core/templates/login.php | 12 | ||||
| -rw-r--r-- | tests/Core/Controller/LoginControllerTest.php | 50 | ||||
| -rw-r--r-- | tests/acceptance/features/bootstrap/LoginPageContext.php | 16 | ||||
| -rw-r--r-- | tests/acceptance/features/login.feature | 6 | ||||
| -rwxr-xr-x | tests/acceptance/installAndConfigureServer.sh | 2 |
6 files changed, 164 insertions, 31 deletions
diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 7bf2555819d..5bd06ac7e66 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 !== '') { @@ -210,6 +202,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]); @@ -331,6 +363,33 @@ class LoginController extends Controller { } /** + * 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 * @BruteForceProtection(action=sudo) diff --git a/core/templates/login.php b/core/templates/login.php index 8fc68edb92d..989ea1eaad5 100644 --- a/core/templates/login.php +++ b/core/templates/login.php @@ -2,6 +2,8 @@ <?php vendor_script('jsTimezoneDetect/jstz'); script('core', 'merged-login'); + +use OC\Core\Controller\LoginController; ?> <!--[if IE 8]><style>input[type="checkbox"]{padding:0;}</style><![endif]--> @@ -34,7 +36,7 @@ script('core', 'merged-login'); <!-- the following div ensures that the spinner is always inside the #message div --> <div style="clear: both;"></div> </div> - <p class="grouptop<?php if (!empty($_['invalidpassword'])) { ?> shake<?php } ?>"> + <p class="grouptop<?php if (!empty($_[LoginController::LOGIN_MSG_INVALIDPASSWORD])) { ?> shake<?php } ?>"> <input type="text" name="user" id="user" placeholder="<?php p($l->t('Username or email')); ?>" aria-label="<?php p($l->t('Username or email')); ?>" @@ -44,7 +46,7 @@ script('core', 'merged-login'); <label for="user" class="infield"><?php p($l->t('Username or email')); ?></label> </p> - <p class="groupbottom<?php if (!empty($_['invalidpassword'])) { ?> shake<?php } ?>"> + <p class="groupbottom<?php if (!empty($_[LoginController::LOGIN_MSG_INVALIDPASSWORD])) { ?> shake<?php } ?>"> <input type="password" name="password" id="password" value="" placeholder="<?php p($l->t('Password')); ?>" aria-label="<?php p($l->t('Password')); ?>" @@ -58,10 +60,14 @@ script('core', 'merged-login'); <div class="submit-icon icon-confirm-white"></div> </div> - <?php if (!empty($_['invalidpassword'])) { ?> + <?php if (!empty($_[LoginController::LOGIN_MSG_INVALIDPASSWORD])) { ?> <p class="warning wrongPasswordMsg"> <?php p($l->t('Wrong password.')); ?> </p> + <?php } else if (!empty($_[LoginController::LOGIN_MSG_USERDISABLED])) { ?> + <p class="warning userDisabledMsg"> + <?php p(\OC::$server->getL10N('lib')->t('User disabled')); ?> + </p> <?php } ?> <?php if ($_['throttle_delay'] > 5000) { ?> diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 1e26d86a039..7ebd6ee8340 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 1496e3030c2..df7944aa912 100644 --- a/tests/acceptance/features/bootstrap/LoginPageContext.php +++ b/tests/acceptance/features/bootstrap/LoginPageContext.php @@ -71,6 +71,14 @@ class LoginPageContext implements Context, ActorAwareInterface { } /** + * @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 */ public function iLogInWithUserAndPassword($user, $password) { @@ -97,6 +105,14 @@ class LoginPageContext implements Context, ActorAwareInterface { } /** + * @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 */ public function getOtherRequiredSiblingContexts(BeforeScenarioScope $scope) { diff --git a/tests/acceptance/features/login.feature b/tests/acceptance/features/login.feature index 3a31d3f88bd..44353d37c65 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 c61faeda238..98de72bf45e 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. |