Skip to content

Commit

Permalink
do not remember session tokens by default
Browse files Browse the repository at this point in the history
We have to respect the value of the remember-me checkbox. Due to an error
in the source code the default value for the session token was to remember
it.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
  • Loading branch information
ChristophWurst committed Nov 27, 2016
1 parent 7e6f829 commit 9b808c4
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 4 deletions.
2 changes: 1 addition & 1 deletion lib/private/User/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ public function createSessionToken(IRequest $request, $uid, $loginName, $passwor
try {
$sessionId = $this->session->getId();
$pwd = $this->getPassword($password);
$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, IToken::REMEMBER);
$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);
return true;
} catch (SessionNotAvailableException $ex) {
// This can happen with OCC, where a memory session is used
Expand Down
46 changes: 43 additions & 3 deletions tests/lib/User/SessionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,6 @@ public function testActiveUserAfterSetSession() {
public function testCreateSessionToken() {
$manager = $this->createMock(Manager::class);
$session = $this->createMock(ISession::class);
$token = $this->createMock(IToken::class);
$user = $this->createMock(IUser::class);
$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random);

Expand Down Expand Up @@ -801,11 +800,52 @@ public function testCreateSessionToken() {

$this->tokenProvider->expects($this->once())
->method('generateToken')
->with($sessionId, $uid, $loginName, $password, 'Firefox');
->with($sessionId, $uid, $loginName, $password, 'Firefox', IToken::DO_NOT_REMEMBER, IToken::TEMPORARY_TOKEN);

$this->assertTrue($userSession->createSessionToken($request, $uid, $loginName, $password));
}

public function testCreateRememberedSessionToken() {
$manager = $this->createMock(Manager::class);
$session = $this->createMock(ISession::class);
$user = $this->createMock(IUser::class);
$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random);

$random = $this->createMock(ISecureRandom::class);
$config = $this->createMock(IConfig::class);
$csrf = $this->getMockBuilder('\OC\Security\CSRF\CsrfTokenManager')
->disableOriginalConstructor()
->getMock();
$request = new \OC\AppFramework\Http\Request([
'server' => [
'HTTP_USER_AGENT' => 'Firefox',
]
], $random, $config, $csrf);

$uid = 'user123';
$loginName = 'User123';
$password = 'passme';
$sessionId = 'abcxyz';

$manager->expects($this->once())
->method('get')
->with($uid)
->will($this->returnValue($user));
$session->expects($this->once())
->method('getId')
->will($this->returnValue($sessionId));
$this->tokenProvider->expects($this->once())
->method('getToken')
->with($password)
->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException()));

$this->tokenProvider->expects($this->once())
->method('generateToken')
->with($sessionId, $uid, $loginName, $password, 'Firefox', IToken::TEMPORARY_TOKEN, IToken::REMEMBER);

$this->assertTrue($userSession->createSessionToken($request, $uid, $loginName, $password, true));
}

public function testCreateSessionTokenWithTokenPassword() {
$manager = $this->getMockBuilder('\OC\User\Manager')
->disableOriginalConstructor()
Expand Down Expand Up @@ -850,7 +890,7 @@ public function testCreateSessionTokenWithTokenPassword() {

$this->tokenProvider->expects($this->once())
->method('generateToken')
->with($sessionId, $uid, $loginName, $realPassword, 'Firefox');
->with($sessionId, $uid, $loginName, $realPassword, 'Firefox', IToken::TEMPORARY_TOKEN, IToken::DO_NOT_REMEMBER);

$this->assertTrue($userSession->createSessionToken($request, $uid, $loginName, $password));
}
Expand Down

5 comments on commit 9b808c4

@justin-sleep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks insertion of auth tokens into my PostgreSQL database (the oc_authtokens table). Instead of the remember value being an integer (0 or 1) as it was before this change, it tries to insert the actual word "true" or "false" into the "remember" column, resulting in no device without an existing auth token being able to log in. I've confirmed that reverting the change made to Session.php fixes the issue.

Could this be changed to return a 0 or 1, please?

@ChristophWurst
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your report. Could you please file an issue so we can fix this for NC11? Thanks

@justin-sleep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll go ahead and do that now.

@ChristophWurst
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @MorrisJobke something we chatted about 🙈

@MorrisJobke
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @MorrisJobke something we chatted about 🙈

Fix it 🙈 (Shouldn't be that hard - either cast it to int or just change all the usages)

Please sign in to comment.