Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rullzer merged 3 commits into from
Jul 24, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c92d742
Implements handling for deactivated users
weeman1337 Jul 21, 2018
de7606d
Adds disabled user unit tests
weeman1337 Jul 21, 2018
801bf81
Adds disabled user acceptance tests
weeman1337 Jul 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 84 additions & 25 deletions core/Controller/LoginController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
use OCP\Util;

class LoginController extends Controller {

const LOGIN_MSG_INVALIDPASSWORD = 'invalidpassword';
const LOGIN_MSG_USERDISABLED = 'userdisabled';

/** @var IUserManager */
private $userManager;
/** @var IConfig */
Expand Down Expand Up @@ -171,19 +175,7 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
$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 !== '') {
Expand All @@ -209,6 +201,40 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
);
}

/**
* 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
Expand Down Expand Up @@ -256,6 +282,17 @@ public function tryLogin($user, $password, $redirect_url, $remember_login = true
}

$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);
Expand All @@ -270,20 +307,15 @@ public function tryLogin($user, $password, $redirect_url, $remember_login = true
}
}
}

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]);
Expand Down Expand Up @@ -330,6 +362,33 @@ public function tryLogin($user, $password, $redirect_url, $remember_login = true
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
Expand Down
12 changes: 9 additions & 3 deletions core/templates/login.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]-->
Expand Down Expand Up @@ -34,7 +36,7 @@
<!-- 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')); ?>"
Expand All @@ -44,7 +46,7 @@
<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')); ?>"
Expand All @@ -58,10 +60,14 @@
<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) { ?>
Expand Down
50 changes: 47 additions & 3 deletions tests/Core/Controller/LoginControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,52 @@ public function testShowLoginFormWithPasswordResetOption($canChangePassword,
$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')
Expand All @@ -297,8 +342,7 @@ public function testShowLoginFormForUserNamedNull() {
->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
Expand Down
16 changes: 16 additions & 0 deletions tests/acceptance/features/bootstrap/LoginPageContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ public static function wrongPasswordMessage() {
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
*/
Expand All @@ -96,6 +104,14 @@ public function iSeeThatAWrongPasswordMessageIsShown() {
$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
*/
Expand Down
6 changes: 6 additions & 0 deletions tests/acceptance/features/login.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions tests/acceptance/installAndConfigureServer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down