Skip to content

Commit

Permalink
Adjust code and tests
Browse files Browse the repository at this point in the history 
ValidateSession will explicitly check if the user is disabled.
Previously, the check was done in the validateToken method, but
general check for the disabled user has been moved to the
loginInOwncloud method, which requires login that won't happen in the
validateSession method.

Token will be invalidated if a LoginException happens.

New info log added to notify the disabled user in the logs
  • Loading branch information
@jvillafanez
jvillafanez committed Dec 1, 2022
1 parent 989690d commit b9733ee
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 94 deletions.
74 changes: 33 additions & 41 deletions lib/private/User/Session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ public function getUser() {
* - For browsers, the session token validity is checked
*/
public function validateSession() {
if (!$this->getUser()) {
$sessionUser = $this->getUser();
if (!$sessionUser) {
return;
}

Expand All @@ -260,6 +261,11 @@ public function validateSession() {
$token = $appPassword;
}

if (!$sessionUser->isEnabled()) {
$this->tokenProvider->invalidateToken($token);
$this->logout();
}

if (!$this->validateToken($token)) {
// Session was invalidated
$this->logout();
Expand Down Expand Up @@ -567,11 +573,19 @@ private function loginWithToken($token) {
return false;
}

$loginOk = $this->loginInOwnCloud('token', $user, $password);
try {
$loginOk = $this->loginInOwnCloud('token', $user, $password);

// set the app password
if ($loginOk) {
$this->session->set('app_password', $token);
// set the app password
if ($loginOk) {
$this->session->set('app_password', $token);
} else {
$this->tokenProvider->invalidateToken($token);
}
} catch (LoginException $e) {
// need to invalidate the token
$this->tokenProvider->invalidateToken($token);
throw $e;
}

return $loginOk;
Expand Down Expand Up @@ -755,20 +769,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
} catch (PasswordlessTokenException $ex) {
// Token has no password

if ($this->activeUser !== null && !$this->activeUser->isEnabled()) {
$this->logger->debug(
'user {uid}, {email}, {displayName} was disabled',
[
'app' => __METHOD__,
'uid' => $this->activeUser->getUID(),
'email' => $this->activeUser->getEMailAddress(),
'displayName' => $this->activeUser->getDisplayName(),
]
);
$this->tokenProvider->invalidateToken($token);
return false;
}

$dbToken->setLastCheck($now);
$this->tokenProvider->updateToken($dbToken);
return true;
Expand All @@ -781,28 +781,14 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
return true;
}

if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false
|| ($this->activeUser !== null && !$this->activeUser->isEnabled())) {
// FIXME: protect debug statement this way to avoid regressions
if ($this->activeUser !== null) {
$this->logger->debug(
'user uid {uid}, email {email}, displayName {displayName} was disabled or password changed',
[
'app' => __METHOD__,
'uid' => $this->activeUser->getUID(),
'email' => $this->activeUser->getEMailAddress(),
'displayName' => $this->activeUser->getDisplayName(),
]
);
} else {
$this->logger->debug(
'user with login name {loginName} was disabled or password changed (no activeUser)',
[
'app' => __METHOD__,
'loginName' => $dbToken->getLoginName(),
]
);
}
if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false) {
$this->logger->debug(
'password changed for user with login name {loginName}',
[
'app' => __METHOD__,
'loginName' => $dbToken->getLoginName(),
]
);

$this->tokenProvider->invalidateToken($token);
// Password has changed or user was disabled -> log user out
Expand Down Expand Up @@ -958,6 +944,10 @@ public function tryAuthModuleLogin(IRequest $request) {
* @throws LoginException if an app canceled the login process or the user is not enabled
*/
public function loginUser(IUser $user = null, $password = null, $authModuleClass = null) {
if ($user === null) {
$this->emitFailedLogin(null);
return false;
}
// openidconnect calls the loginUser method. It might not have an $authModuleClass
return $this->loginInOwnCloud($authModuleClass, $user, $password);
}
Expand Down Expand Up @@ -1047,6 +1037,8 @@ private function loginInOwnCloud($loginType, $user, $password, $options = []) {
// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
$message = \OC::$server->getL10N('lib')->t('Login canceled by app');
throw new LoginException($message);
} else {
$this->logger->info("login {$user->getUID()} cancelled. User disabled", ['app' => __METHOD__]);
}

// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
Expand Down
86 changes: 33 additions & 53 deletions tests/lib/User/SessionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,16 +631,12 @@ public function testLogClientInNoTokenPasswordNo2fa() {

public function testRememberLoginValidToken() {
$session = $this->createMock(Memory::class);
$session->expects($this->exactly(1))
$session->expects($this->exactly(2))
->method('set')
->with($this->callback(function ($key) {
switch ($key) {
case 'user_id':
return true;
default:
return false;
}
}));
->withConsecutive(
['user_id', 'foo'],
['loginname', 'foo'],
);
$session->expects($this->once())
->method('regenerateId');

Expand Down Expand Up @@ -1083,14 +1079,21 @@ public function testTryTokenLoginWithDisabledUser() {
->method('getHeader')
->with('Authorization')
->will($this->returnValue('token xxxxx'));
$this->tokenProvider->expects($this->once())
$this->tokenProvider->expects($this->exactly(2)) // one for validation and other for login
->method('getToken')
->with('xxxxx')
->will($this->returnValue($token));
$manager->expects($this->once())
$this->tokenProvider->expects($this->exactly(2)) // one for validation and other for login
->method('getPassword')
->will($this->returnValue('myPassword'));
$manager->expects($this->exactly(2)) // one for validation and other for login
->method('get')
->with('fritz0')
->will($this->returnValue($user));
$manager->expects($this->once())
->method('checkPassword')
->with('fritz', 'myPassword')
->willReturn($user);
$user->expects($this->once())
->method('isEnabled')
->will($this->returnValue(false));
Expand Down Expand Up @@ -1165,9 +1168,6 @@ public function testValidateSessionNoPassword() {

/** @var IUser | \PHPUnit\Framework\MockObject\MockObject $user */
$user = $this->createMock(IUser::class);
$user->expects($this->once())
->method('isEnabled')
->willReturn(true);
$token = new DefaultToken();
$token->setLastCheck(20);

Expand Down Expand Up @@ -1575,12 +1575,9 @@ public function testFailedLoginWithApache() {

$failedEvent = new GenericEvent(null, ['user' => 'foo']);
$beforeLoginEvent = new GenericEvent(null, ['login' => 'foo', 'uid' => 'foo', 'password' => '', 'loginType' => 'apache']);
$eventDispatcher->expects($this->exactly(2))
$eventDispatcher->expects($this->once())
->method('dispatch')
->withConsecutive(
[$this->equalTo($beforeLoginEvent), $this->equalTo('user.beforelogin')],
[$this->equalTo($failedEvent), $this->equalTo('user.loginfailed')]
);
->with($this->equalTo($failedEvent), $this->equalTo('user.loginfailed'));

$userSession->loginWithApache($apacheBackend);
}
Expand Down Expand Up @@ -1617,16 +1614,16 @@ public function testLoginWithApache() {
$iUser->expects($this->exactly(2))
->method('isEnabled')
->willReturn(true);
$iUser->expects($this->exactly(4))
$iUser->expects($this->exactly(5))
->method('getUID')
->willReturn('foo');

$userManager->expects($this->exactly(2))
->method('get')
->willReturn($iUser);

$beforeLoginEvent = new GenericEvent(null, ['login' => 'foo', 'uid' => 'foo', 'password' => '', 'loginType' => 'apache']);
$afterLoginEvent = new GenericEvent(null, ['user' => $iUser, 'login' => 'foo', 'uid' => 'foo', 'password' => '', 'loginType' => 'apache']);
$beforeLoginEvent = new GenericEvent(null, ['login' => 'foo', 'uid' => 'foo', 'password' => '', 'loginType' => 'apache', '_uid' => 'deprecated: please use \'login\', the real uid is not yet known',]);
$afterLoginEvent = new GenericEvent(null, ['user' => $iUser, 'uid' => 'foo', 'password' => '', 'loginType' => 'apache']);
$eventDispatcher->expects($this->exactly(2))
->method('dispatch')
->withConsecutive(
Expand Down Expand Up @@ -1661,19 +1658,10 @@ public function testFailedLoginWithPassword() {
$eventDispatcher
);

$beforeEvent = new GenericEvent(
null,
['loginType' => 'password', 'login' => 'foo', 'uid' => 'foo',
'_uid' => 'deprecated: please use \'login\', the real uid is not yet known',
'password' => 'foo']
);
$failedLogin = new GenericEvent(null, ['user' => 'foo']);
$eventDispatcher->expects($this->exactly(2))
$eventDispatcher->expects($this->once())
->method('dispatch')
->withConsecutive(
[$this->equalTo($beforeEvent), $this->equalTo('user.beforelogin')],
[$this->equalTo($failedLogin), $this->equalTo('user.loginfailed')]
);
->with($this->equalTo($failedLogin), $this->equalTo('user.loginfailed'));

$this->invokePrivate($userSession, 'loginWithPassword', ['foo', 'foo']);
}
Expand Down Expand Up @@ -1768,14 +1756,10 @@ public function testLoginFailedWithToken() {
->method('get')
->willReturn(null);

$event = new GenericEvent(null, ['login' => 'foo', 'uid' => 'foo', 'password' => 'foobar', 'loginType' => 'token']);
$failedEvent = new GenericEvent(null, ['user' => 'foo']);
$eventDispatcher->expects($this->exactly(2))
$eventDispatcher->expects($this->once())
->method('dispatch')
->withConsecutive(
[$this->equalTo($event), $this->equalTo('user.beforelogin')],
[$this->equalTo($failedEvent), $this->equalTo('user.loginfailed')]
);
->with($this->equalTo($failedEvent), $this->equalTo('user.loginfailed'));

$this->invokePrivate($userSession, 'loginWithToken', ['token']);
}
Expand Down Expand Up @@ -1828,13 +1812,13 @@ public function testLoginWithToken() {
$beforeEvent = new GenericEvent(
null,
[
'login' => 'foo', 'uid' => 'foo', 'password' => 'foobar', 'loginType' => 'token'
'login' => 'foo', 'uid' => 'foo', 'password' => 'foobar', 'loginType' => 'token', '_uid' => 'deprecated: please use \'login\', the real uid is not yet known',
]
);
$afterEvent = new GenericEvent(
null,
[
'user' => $iUser, 'login' => 'foo', 'uid' => 'foo', 'password' => 'foobar', 'loginType' => 'token'
'user' => $iUser, 'uid' => 'foo', 'password' => 'foobar', 'loginType' => 'token'
]
);

Expand Down Expand Up @@ -2052,13 +2036,6 @@ public function testFailedLoginUserDisabled() {
$eventDispatcher
);

$failedEvent = new GenericEvent(null, ['user' => 'foo']);
$eventDispatcher->expects($this->once())
->method('dispatch')
->withConsecutive(
[$this->equalTo($failedEvent), $this->equalTo('user.loginfailed')]
);

$iUser = $this->createMock(IUser::class);
$iUser->expects($this->once())
->method('isEnabled')
Expand All @@ -2068,13 +2045,16 @@ public function testFailedLoginUserDisabled() {
$iUser->expects($this->never())
->method('updateLastLoginTimestamp');

$failedEvent = new GenericEvent(null, ['user' => 'foo']);
$beforeEvent = new GenericEvent(
null,
['loginType' => null, 'login' => 'foo', 'uid' => 'foo', // loginType == null because no authModule specified
'_uid' => 'deprecated: please use \'login\', the real uid is not yet known',
'password' => 'bar']
);
$eventDispatcher->expects($this->once())
->method('dispatch')
->withConsecutive(
[$this->equalTo($failedEvent), $this->equalTo('user.loginfailed')]
);
->with($this->equalTo($beforeEvent), $this->equalTo('user.beforelogin'));

$this->invokePrivate($userSession, 'loginUser', [$iUser, 'foo']);
$this->invokePrivate($userSession, 'loginUser', [$iUser, 'bar']);
}
}

0 comments on commit b9733ee

Please sign in to comment.