Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions changelog/unreleased/39916
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
Enhancement: Changes regarding cookie handling

The following changes have been implemented:
* The expiration set for the passphrase cookie will be refreshed each time
a page is loaded or when the "heartbeat" endpoint is hit
* If the "session_keepalive" config option is set to true, a periodic request
to the "heartbeat" endpoint will be made automatically regardless of any
activity going on. This will extend the session lifetime preventing its
expiration.
* If the "session_keepalive" config option is set to false, a "heartbeat" will
be sent based on activity in order to extend the session lifetime. If we
don't detect any activity, the session might expire, and the user will need to
login again.
* The new "session_forced_logout_timeout" option has been added to the
config.php. It's disabled by default, and setting a positive (non-zero) value
will enable the feature. If it's enabled, the passphrase cookie will expire
after those number of seconds pass, when the tab or the browser closes.
This will force the user to login again.

https://github.com/owncloud/core/pull/39916
25 changes: 23 additions & 2 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,17 +220,38 @@
'remember_login_cookie_lifetime' => 60*60*24*15,

/**
* Define the lifetime of a session after inactivity
* Define the lifetime of a session after inactivity.
* The web UI might send a "heartbeat" based on the activity happening
* in order to extend the session lifetime and keeping it from timing out
* prematurely. If there is no activity happening and the lifetime is
* reached, you'll have to login again.
* The default is 20 minutes, expressed in seconds.
*/
'session_lifetime' => 60 * 20,

/**
* Enable or disable session keep-alive when a user is logged in to the Web UI
* Enabling this sends a "heartbeat" to the server to keep it from timing out.
* Enabling this sends a "heartbeat" to the server to keep it from
* timing out regardless of any activity happening. This heartbeat will
* keep extending the session over again, so the user won't be logged out
* even if he isn't active in the web UI.
*/
'session_keepalive' => true,

/**
* Force the user to logout when he closes the tab or the browser, after
* the specified number of seconds. A negative or 0 value disables this
* feature.
* Note that the user can still access the page without re-authenticating
* (having valid access) if the timeout hasn't been reached.
* The recommended minimum value is 5 or 10 seconds. Using a lower value
* might cause unwanted logouts for the users.
* Also note that this feature works properly if the user uses only one
* tab. If a user uses multiple tabs, closing one of them will likely
* force the rest to re-authenticate.
*/
'session_forced_logout_timeout' => 0,

/**
* Enforce token only authentication for apps and clients connecting to ownCloud
* If enabled, all access requests using the user's password are blocked for enhanced security.
Expand Down
1 change: 1 addition & 0 deletions core/js/config.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@
"oc_config" => [
'session_lifetime' => \min(\OC::$server->getConfig()->getSystemValue('session_lifetime', OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')),
'session_keepalive' => \OC::$server->getConfig()->getSystemValue('session_keepalive', true),
'session_forced_logout_timeout' => \OC::$server->getConfig()->getSystemValue('session_forced_logout_timeout', 0),
'enable_avatars' => \OC::$server->getConfig()->getSystemValue('enable_avatars', true) === true,
'lost_password_link' => \OC::$server->getConfig()->getSystemValue('lost_password_link', null),
'modRewriteWorking' => (\getenv('front_controller_active') === 'true'),
Expand Down
105 changes: 103 additions & 2 deletions core/js/js.js
Original file line number Diff line number Diff line change
Expand Up @@ -1567,14 +1567,115 @@ function initCore() {
$.post(url);
}, interval * 1000);
};
$(document).ajaxComplete(heartBeat);
heartBeat();
}

/**
* Check mouse movement to send a heartbeat if the mouse moved
*/
function initMouseTrack() {
var interval = 120; // 2 minutes
if (oc_config.session_lifetime) {
interval = Math.floor(oc_config.session_lifetime / 2);
}
interval = Math.min(Math.max(interval, 60), 3600); // ensure interval is in [60, 3600]

var extendableHeartbeat = null;
var extendableMouseMoved = false;
var mouseMoved = false;
var heartbeatTimestamp = 0;

// If the mouse has moved within this interval, set a timeout to send
// a heartbeat request. If the mouse moves again in the next interval,
// the timeout will be extended. Note that the heartbeat request might
// be delayed indefinitely.
setInterval(function() {
if (extendableMouseMoved) {
clearTimeout(extendableHeartbeat);
extendableHeartbeat = setTimeout(function() {
if (!extendableMouseMoved) {
// if the mouse has moved, either this timeout has been cleared and
// a new one is being created, or a new timeout will be created soon
// (if this timeout is executed before the setInterval function)
var currentTimestamp = Date.now();
if (currentTimestamp > heartbeatTimestamp + (30 * 1000)) {
// if a heartbeat has been sent recently, don't send a new one
console.log('I1 sending heartbeat');
$.post(OC.generateUrl('/heartbeat'));
heartbeatTimestamp = currentTimestamp;
} else {
console.log('I1 skipping heartbeat');
}
} else {
console.log('I1 delaying hearbeat because mouse moved.');
}
}, 30 * 1000);
}
extendableMouseMoved = false;
}, 30 * 1000);

// If the mouse has been moved within this interval, send a heartbeat
// immediately. This will prevent the session to expire.
setInterval(function() {
if (mouseMoved) {
var currentTimestamp = Date.now();
if (currentTimestamp > heartbeatTimestamp + (interval * 1000)) {
// if a heartbeat has been sent recently, don't send a new one
console.log('I2 sending heartbeat');
$.post(OC.generateUrl('/heartbeat'));
heartbeatTimestamp = currentTimestamp;
} else {
console.log('I2 skipping heartbeat');
}
}
mouseMoved = false; // no need to wait for the response
}, interval * 1000);

// set a couple of variables to indicate that the mouse has moved
$(window).on('mousemove.sessiontrack', function() {
mouseMoved = true;
extendableMouseMoved = true;
});
}

// session heartbeat (defaults to enabled)
if (typeof(oc_config.session_keepalive) === 'undefined' || !!oc_config.session_keepalive) {

initSessionHeartBeat();
} else {
initMouseTrack();
}

if (oc_config.session_forced_logout_timeout !== undefined && oc_config.session_forced_logout_timeout > 0) {
$(window).on('beforeunload.sessiontrack', function() {
var requestData = {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-Requested-With': 'XMLHttpRequest'
},
body: JSON.stringify({t: oc_config.session_forced_logout_timeout}),
credentials: 'same-origin',
keepalive: true
};
var r = undefined;
if (typeof Request !== "undefined") {
// IE 11 doesn't have support "Request", so we'll fallback to ajax
r = new Request(OC.generateUrl('/heartbeat'), requestData);
}
if (r === undefined || r.keepalive === undefined) {
// firefox doesn't support keepalive (checked 11th Apr 2022)
// try a sync ajax call instead
$.ajax({
method: 'POST',
url: OC.generateUrl('/heartbeat'),
data: {t: oc_config.session_forced_logout_timeout},
async: false
});
} else {
fetch(r);
}
});
}

OC.registerMenu($('#expand'), $('#expanddiv'));
Expand Down Expand Up @@ -3128,4 +3229,4 @@ $.datepicker._attachments = function (input, inst) {
this.uiDialog.attr( {
"aria-labelledby": uiDialogTitle.attr( "id" )
} );
};
};
3 changes: 2 additions & 1 deletion core/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@

// used for heartbeat
$this->create('heartbeat', '/heartbeat')->action(function () {
// do nothing
$expire = \OC::$server->getRequest()->getParam('t');
\OC::$server->getSessionCryptoWrapper()->refreshCookie(\OC::$server->getConfig(), $expire);
});

/**
Expand Down
1 change: 1 addition & 0 deletions lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ public static function initSession() {
}

$cryptoWrapper = \OC::$server->getSessionCryptoWrapper();
$cryptoWrapper->sendCookie(self::$server->getConfig());
$session = $cryptoWrapper->wrapSession($session);
self::$server->setSession($session);

Expand Down
1 change: 0 additions & 1 deletion lib/private/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,6 @@ public function __construct($webRoot, \OC\Config $config) {
);

return new CryptoWrapper(
$c->getConfig(),
$c->getCrypto(),
$c->getSecureRandom(),
$request,
Expand Down
128 changes: 96 additions & 32 deletions lib/private/Session/CryptoWrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ class CryptoWrapper {
/** @var string */
private $passphrase;

/** @var IRequest */
private $request;

/** @var ITimeFactory */
private $timeFactory;

Expand All @@ -73,51 +76,112 @@ class CryptoWrapper {
* @param ITimeFactory $timeFactory
*/
public function __construct(
IConfig $config,
ICrypto $crypto,
ISecureRandom $random,
IRequest $request,
ITimeFactory $timeFactory
) {
$this->crypto = $crypto;
$this->random = $random;
$this->request = $request;
$this->timeFactory = $timeFactory;
}

if ($request->getCookie(self::COOKIE_NAME) !== null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just wonder why this part was done in constructor in the past

$this->passphrase = $request->getCookie(self::COOKIE_NAME);
} else {
$this->passphrase = $this->random->generate(128);
$secureCookie = $request->getServerProtocol() === 'https';
// FIXME: Required for CI
if (!\defined('PHPUNIT_RUN')) {
$webRoot = \OC::$WEBROOT;
if ($webRoot === '') {
$webRoot = '/';
/**
* This `sendCookie` method is mandatory before wrapping the session (`wrapSession` method).
* A passphrase will be prepared in order to wrap the session.
* If the target cookie hasn't been received from the request, a cookie containing the passphrase
* will also be sent to the browser for the future.
*/
public function sendCookie(IConfig $config) {
if ($this->request->getCookie(self::COOKIE_NAME) !== null) {
// just set the passphrase, don't send it to the browser.
$this->passphrase = $this->request->getCookie(self::COOKIE_NAME);
if ($this->request->getHeader('X-Requested-With') !== 'XMLHttpRequest') {
// refresh cookie expiration
if (!\defined('PHPUNIT_RUN')) {
$options = $this->prepareOptions($config);
$this->sendCookieToBrowser($this->passphrase, $options);
}
}
return;
}

$sessionLifetime = $config->getSystemValue('session_lifetime', 0);
if ($sessionLifetime > 0) {
$sessionLifetime += $this->timeFactory->getTime();
} else {
$sessionLifetime = 0;
}
$this->passphrase = $this->random->generate(128);
// FIXME: Required for CI
if (!\defined('PHPUNIT_RUN')) {
$options = $this->prepareOptions($config);
$this->sendCookieToBrowser($this->passphrase, $options);
}
}

if (\version_compare(PHP_VERSION, '7.3.0') === -1) {
\setcookie(self::COOKIE_NAME, $this->passphrase, $sessionLifetime, $webRoot, '', $secureCookie, true);
} else {
$samesite = $config->getSystemValue('http.cookie.samesite', 'Strict');
$options = [
"expires" => $sessionLifetime,
"path" => $webRoot,
"domain" => '',
"secure" => $secureCookie,
"httponly" => true,
"samesite" => $samesite
];

\setcookie(self::COOKIE_NAME, $this->passphrase, $options);
}
/**
* Refresh the cookie expiration time
*/
public function refreshCookie(IConfig $config, $expire = null) {
if ($this->request->getCookie(self::COOKIE_NAME) === null) {
return;
}

$this->passphrase = $this->request->getCookie(self::COOKIE_NAME);
// FIXME: Required for CI
if (!\defined('PHPUNIT_RUN')) {
$options = $this->prepareOptions($config);
if ($expire !== null) {
$options['expires'] = $this->timeFactory->getTime() + $expire;
}
$this->sendCookieToBrowser($this->passphrase, $options);
}
}

/**
* Try to delete the cookie
*/
public function deleteCookie() {
// FIXME: Required for CI
if (!\defined('PHPUNIT_RUN')) {
$options = [
'expires' => \time() - 3600,
'path' => '',
'domain' => '',
'secure' => false,
'httponly' => false,
'samesite' => 'None',
];
$this->sendCookieToBrowser('', $options);
}
}

private function prepareOptions(IConfig $config) {
$secureCookie = $this->request->getServerProtocol() === 'https';
$webRoot = \OC::$WEBROOT;
if ($webRoot === '') {
$webRoot = '/';
}

$sessionLifetime = $config->getSystemValue('session_lifetime', 60 * 20);
if ($sessionLifetime > 0) {
$sessionLifetime += $this->timeFactory->getTime();
} else {
$sessionLifetime = 0;
}

$samesite = $config->getSystemValue('http.cookie.samesite', 'Strict');
return [
'expires' => $sessionLifetime,
'path' => $webRoot,
'domain' => '',
'secure' => $secureCookie,
'httponly' => true,
'samesite' => $samesite,
];
}

private function sendCookieToBrowser($value, $options) {
if (\version_compare(PHP_VERSION, '7.3.0') === -1) {
\setcookie(self::COOKIE_NAME, $value, $options['expires'], $options['path'], $options['domain'], $options['secure'], $options['httpOnly']);
} else {
\setcookie(self::COOKIE_NAME, $value, $options);
}
}

Expand Down
1 change: 1 addition & 0 deletions lib/private/User/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ public function logout() {
$this->setUser(null);
$this->setLoginName(null);
$this->unsetMagicInCookie();
OC::$server->getSessionCryptoWrapper()->deleteCookie();
Copy link
Copy Markdown
Contributor

@mrow4a mrow4a Apr 26, 2022

Choose a reason for hiding this comment

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

do not want to be picky, but might need to go to constructor.. I know this would cause massive changes chain.. your call

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll leave it there for now. I agree the service should be injected, but it isn't the only one. In addition, the class has a bunch of dependencies already, so we might need to consider a UserSessionHelper to act as a facade of some services to offload some of those dependencies. So it seems we might need a refactor here at some point.

$this->session->clear();
$this->manager->emit('\OC\User', 'postLogout');
return true;
Expand Down