Skip to content

fix remember me login #1347

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
MorrisJobke merged 6 commits into from
Nov 2, 2016
Merged

fix remember me login #1347

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d907666
bring back remember-me
ChristophWurst Sep 6, 2016
6f86e46
inject ISecureRandom into user session and use injected config too
ChristophWurst Oct 31, 2016
b269ed5
Fix invalid PHPDocs
LukasReschke Nov 2, 2016
271f2a4
Fix typ in constant name
LukasReschke Nov 2, 2016
9d6e01e
Add missing tests and fix PHPDoc
LukasReschke Nov 2, 2016
4da6b20
document what the method does
ChristophWurst Nov 2, 2016
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
Next Next commit
bring back remember-me 
* try to reuse the old session token for remember me login
* decrypt/encrypt token password and set the session id accordingly
* create remember-me cookies only if checkbox is checked and 2fa solved
* adjust db token cleanup to store remembered tokens longer
* adjust unit tests

Signed-off-by: Christoph Wurst <[email protected]>
  • Loading branch information
@ChristophWurst @LukasReschke
ChristophWurst authored and LukasReschke committed Nov 2, 2016
commit d907666232468503ab6ed2bdac44b6500be2beb6
11 changes: 8 additions & 3 deletions core/Controller/LoginController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,10 @@ private function generateRedirect($redirectUrl) {
* @param string $user
* @param string $password
* @param string $redirect_url
* @param boolean $remember_login
* @return RedirectResponse
*/
public function tryLogin($user, $password, $redirect_url) {
public function tryLogin($user, $password, $redirect_url, $remember_login = false) {
$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress());
$this->throttler->sleepDelay($this->request->getRemoteAddress());

Expand Down Expand Up @@ -236,13 +237,13 @@ public function tryLogin($user, $password, $redirect_url) {
// TODO: remove password checks from above and let the user session handle failures
// requires https://github.com/owncloud/core/pull/24616
$this->userSession->login($user, $password);
$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password);
$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, $remember_login);

// User has successfully logged in, now remove the password reset link, when it is available
$this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword');

if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) {
$this->twoFactorManager->prepareTwoFactorLogin($loginResult);
$this->twoFactorManager->prepareTwoFactorLogin($loginResult, $remember_login);

$providers = $this->twoFactorManager->getProviders($loginResult);
if (count($providers) === 1) {
Expand All @@ -265,6 +266,10 @@ public function tryLogin($user, $password, $redirect_url) {
return new RedirectResponse($this->urlGenerator->linkToRoute($url, $urlParams));
}

if ($remember_login) {
$this->userSession->createRememberMeToken($loginResult);
}

return $this->generateRedirect($redirect_url);
}

Expand Down
9 changes: 9 additions & 0 deletions db_structure.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,15 @@
<length>2</length>
</field>

<field>
<name>remember</name>
<type>integer</type>
<default>0</default>
<notnull>true</notnull>
<unsigned>true</unsigned>
<length>1</length>
</field>

<field>
<name>last_activity</name>
<type>integer</type>
Expand Down
6 changes: 6 additions & 0 deletions lib/base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,12 @@ static function handleLogin(OCP\IRequest $request) {
if ($userSession->tryTokenLogin($request)) {
return true;
}
if (isset($_COOKIE['nc_username'])
&& isset($_COOKIE['nc_token'])
&& isset($_COOKIE['nc_session_id'])
&& $userSession->loginWithCookie($_COOKIE['nc_username'], $_COOKIE['nc_token'], $_COOKIE['nc_session_id'])) {
return true;
}
if ($userSession->tryBasicAuthLogin($request, \OC::$server->getBruteForceThrottler())) {
return true;
}
Expand Down
1 change: 1 addition & 0 deletions lib/private/AppFramework/DependencyInjection/DIContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ public function __construct($appName, $urlParams = array()){
$this->registerService('OCP\\IUserSession', function($c) {
return $this->getServer()->getUserSession();
});
$this->registerAlias(\OC\User\Session::class, \OCP\IUserSession::class);

$this->registerService('OCP\\ISession', function($c) {
return $this->getServer()->getSession();
Expand Down
7 changes: 7 additions & 0 deletions lib/private/Authentication/Token/DefaultToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
* @method string getToken()
* @method void setType(string $type)
* @method int getType()
* @method void setRemember(int $remember)
* @method int getRemember()
* @method void setLastActivity(int $lastActivity)
* @method int getLastActivity()
*/
Expand Down Expand Up @@ -70,6 +72,11 @@ class DefaultToken extends Entity implements IToken {
*/
protected $type;

/**
* @var int
*/
protected $remember;

/**
* @var int
*/
Expand Down
17 changes: 9 additions & 8 deletions lib/private/Authentication/Token/DefaultTokenMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,25 @@ public function __construct(IDBConnection $db) {
* @param string $token
*/
public function invalidate($token) {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$qb->delete('authtoken')
->andWhere($qb->expr()->eq('token', $qb->createParameter('token')))
->where($qb->expr()->eq('token', $qb->createParameter('token')))
->setParameter('token', $token)
->execute();
}

/**
* @param int $olderThan
* @param int $remember
*/
public function invalidateOld($olderThan) {
public function invalidateOld($olderThan, $remember = IToken::DO_NOT_REMEMBER) {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$qb->delete('authtoken')
->where($qb->expr()->lt('last_activity', $qb->createParameter('last_activity')))
->andWhere($qb->expr()->eq('type', $qb->createParameter('type')))
->setParameter('last_activity', $olderThan, IQueryBuilder::PARAM_INT)
->setParameter('type', IToken::TEMPORARY_TOKEN, IQueryBuilder::PARAM_INT)
->where($qb->expr()->lt('last_activity', $qb->createNamedParameter($olderThan, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('type', $qb->createNamedParameter(IToken::TEMPORARY_TOKEN, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('remember', $qb->createNamedParameter($remember, IQueryBuilder::PARAM_INT)))
->execute();
}

Expand All @@ -71,7 +72,7 @@ public function invalidateOld($olderThan) {
public function getToken($token) {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check')
$result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'remember', 'token', 'last_activity', 'last_check')
->from('authtoken')
->where($qb->expr()->eq('token', $qb->createParameter('token')))
->setParameter('token', $token)
Expand All @@ -97,7 +98,7 @@ public function getToken($token) {
public function getTokenByUser(IUser $user) {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check')
$qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'remember', 'token', 'last_activity', 'last_check')
->from('authtoken')
->where($qb->expr()->eq('uid', $qb->createNamedParameter($user->getUID())))
->setMaxResults(1000);
Expand Down
32 changes: 29 additions & 3 deletions lib/private/Authentication/Token/DefaultTokenProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ public function __construct(DefaultTokenMapper $mapper, ICrypto $crypto, IConfig
* @param string|null $password
* @param string $name
* @param int $type token type
* @param int $remember whether the session token should be used for remember-me
* @return IToken
*/
public function generateToken($token, $uid, $loginName, $password, $name, $type = IToken::TEMPORARY_TOKEN) {
public function generateToken($token, $uid, $loginName, $password, $name, $type = IToken::TEMPORARY_TOKEN, $remember = IToken::DO_NOT_REMEMBER) {
$dbToken = new DefaultToken();
$dbToken->setUid($uid);
$dbToken->setLoginName($loginName);
Expand All @@ -85,6 +86,7 @@ public function generateToken($token, $uid, $loginName, $password, $name, $type
$dbToken->setName($name);
$dbToken->setToken($this->hashToken($token));
$dbToken->setType($type);
$dbToken->setRemember($remember);
$dbToken->setLastActivity($this->time->getTime());

$this->mapper->insert($dbToken);
Expand Down Expand Up @@ -151,6 +153,27 @@ public function getToken($tokenId) {
}
}

/**
* @param string $oldSessionId
* @param string $sessionId
*/
public function renewSessionToken($oldSessionId, $sessionId) {
Copy link
Member

@LukasReschke LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

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

Function needs tests. Will write myself later.

$token = $this->getToken($oldSessionId);

$newToken = new DefaultToken();
$newToken->setUid($token->getUID());
$newToken->setLoginName($token->getLoginName());
if (!is_null($token->getPassword())) {
$password = $this->decryptPassword($token->getPassword(), $oldSessionId);
$newToken->setPassword($this->encryptPassword($password, $sessionId));
}
$newToken->setName($token->getName());
$newToken->setToken($this->hashToken($sessionId));
$newToken->setType(IToken::TEMPORARY_TOKEN);
$newToken->setLastActivity($this->time->getTime());
$this->mapper->insert($newToken);
}

/**
* @param IToken $savedToken
* @param string $tokenId session token
Expand Down Expand Up @@ -207,8 +230,11 @@ public function invalidateTokenById(IUser $user, $id) {
*/
public function invalidateOldTokens() {
$olderThan = $this->time->getTime() - (int) $this->config->getSystemValue('session_lifetime', 60 * 60 * 24);
$this->logger->info('Invalidating tokens older than ' . date('c', $olderThan));
$this->mapper->invalidateOld($olderThan);
$this->logger->info('Invalidating session tokens older than ' . date('c', $olderThan));
$this->mapper->invalidateOld($olderThan, IToken::DO_NOT_REMEMBER);
$rememberThreshold = $this->time->getTime() - (int) $this->config->getSystemValue('remember_login_cookie_lifetime', 60 * 60 * 24 * 15);
$this->logger->info('Invalidating remembered session tokens older than ' . date('c', $rememberThreshold));
$this->mapper->invalidateOld($rememberThreshold, IToken::REMEMBER);
}

/**
Expand Down
10 changes: 9 additions & 1 deletion lib/private/Authentication/Token/IProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

interface IProvider {


/**
* Create and persist a new token
*
Expand All @@ -37,9 +38,10 @@ interface IProvider {
* @param string|null $password
* @param string $name
* @param int $type token type
* @param int $remember whether the session token should be used for remember-me
* @return IToken
*/
public function generateToken($token, $uid, $loginName, $password, $name, $type = IToken::TEMPORARY_TOKEN);
public function generateToken($token, $uid, $loginName, $password, $name, $type = IToken::TEMPORARY_TOKEN, $remember = IToken::DO_NOT_REMEMBER);

/**
* Get a token by token id
Expand All @@ -50,6 +52,12 @@ public function generateToken($token, $uid, $loginName, $password, $name, $type
*/
public function getToken($tokenId) ;

/**
* @param string $oldSessionId
Copy link
Member

@LukasReschke LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

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

Add at least a single line comment what the function is supposed to do? :)

Copy link
Member Author

@ChristophWurst ChristophWurst Nov 2, 2016

Choose a reason for hiding this comment

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

done

* @param string $sessionId
*/
public function renewSessionToken($oldSessionId, $sessionId);

/**
* Invalidate (delete) the given session token
*
Expand Down
2 changes: 2 additions & 0 deletions lib/private/Authentication/Token/IToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ interface IToken extends JsonSerializable {

const TEMPORARY_TOKEN = 0;
const PERMANENT_TOKEN = 1;
const DO_NOT_REMEMBER = 0;
const REMEMBER = 1;

/**
* Get the token ID
Expand Down
19 changes: 14 additions & 5 deletions lib/private/Authentication/TwoFactorAuth/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class Manager {
const SESSION_UID_KEY = 'two_factor_auth_uid';
const BACKUP_CODES_APP_ID = 'twofactor_backupcodes';
const BACKUP_CODES_PROVIDER_ID = 'backup_codes';
const REMEBER_LOGIN = 'two_factor_remember_login';
Copy link
Member

@LukasReschke LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

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

Typo in the constant name


/** @var AppManager */
private $appManager;
Expand All @@ -51,6 +52,7 @@ class Manager {
* @param AppManager $appManager
* @param ISession $session
* @param IConfig $config
* @param Session $userSession
Copy link
Member

@LukasReschke LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

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

We're passing a ISession and not a Session

Copy link
Member

@LukasReschke LukasReschke Nov 2, 2016

Choose a reason for hiding this comment

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

Actually it seems like that is even not ever used :)

*/
public function __construct(AppManager $appManager, ISession $session, IConfig $config) {
$this->appManager = $appManager;
Expand Down Expand Up @@ -171,11 +173,16 @@ public function verifyChallenge($providerId, IUser $user, $challenge) {
return false;
}

$result = $provider->verifyChallenge($user, $challenge);
if ($result) {
$passed = $provider->verifyChallenge($user, $challenge);
if ($passed) {
if ($this->session->get(self::REMEBER_LOGIN) === true) {
// TODO: resolve cyclic dependency and use DI
\OC::$server->getUserSession()->createRememberMeToken($user);
}
$this->session->remove(self::SESSION_UID_KEY);
$this->session->remove(self::REMEBER_LOGIN);
}
return $result;
return $passed;
}

/**
Expand All @@ -202,12 +209,14 @@ public function needsSecondFactor(IUser $user = null) {
}

/**
* Prepare the 2FA login (set session value)
* Prepare the 2FA login
*
* @param IUser $user
* @param boolean $rememberMe
*/
public function prepareTwoFactorLogin(IUser $user) {
public function prepareTwoFactorLogin(IUser $user, $rememberMe) {
$this->session->set(self::SESSION_UID_KEY, $user->getUID());
$this->session->set(self::REMEBER_LOGIN, $rememberMe);
}

}
2 changes: 1 addition & 1 deletion lib/private/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ public function __construct($webRoot, \OC\Config $config) {
return $userSession;
});

$this->registerService('\OC\Authentication\TwoFactorAuth\Manager', function (Server $c) {
$this->registerService(\OC\Authentication\TwoFactorAuth\Manager::class, function (Server $c) {
return new \OC\Authentication\TwoFactorAuth\Manager($c->getAppManager(), $c->getSession(), $c->getConfig());
});

Expand Down
Loading