Merge pull request #10322 from weeman1337/feature-9978-improve-disabled-user-login-message

Login: Implements the "user disabled" message like the "wrong password" message

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

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) { ?>

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

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
 	 */

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

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.