Improve session cookie handling

[jvillafanez]

The oc_sessionPassphrase cookie will always be sent for all the non-ajax
requests in order to update the expiration time accordingly. The
expiration time will match the one set in the 'session_lifetime'
configuration in config.php. If the 'session_lifetime' isn't set, the
expiration for the cookie will be set to "session" time.

If the 'session_keepalive' isn't set as false, a periodic "heartbeat"
request will be performed to prevent the browser session from being
expired. The "/heartbeat" will always update the expiration time for the
oc_sessionPassphrase cookie.

Login out will cause the cookie to be deleted.

Description

Usually, an auth cookie is sent without explicit expiration, so it will last for the whole browsing session.
If session_lifetime is explicitly set, the cookie will have the corresponding expiration time. Once the cookie expires, the browser will stop sending the cookie, which will cause the user to be kicked out and he'll have to login again.
(Minimum expiration time for the cookie is 60 secs. Setting the session_lifetime to an equal or lower value will likely cause problems)

Loading pages will cause the cookie expiration to be refreshed. This will allow extending the period where the user can perform actions in ownCloud. Note that this doesn't apply to ajax requests. In particular, browsing through the files page won't prevent the user from being kicked out.

Unless the session_keepalive is explicitly set to false, a periodic heartbeat request will be sent to the server. The "/heartbeat" endpoint will always refresh the cookie's expiration time. The "heartbeat" request is expected to happen every session_lifetime / 2 secs. This will help preventing the cookie from being expired.

Login out will remove the cookie. A new one will be generated.

Note that this have been only tested with basic auth. Other authentication mechanisms haven't been tested yet, so there could be problems with those.
Also note that the behavior is mostly related to the browser. In order to refresh the expiration time for the cookie, we have to send a cookie to overwrite the old one, otherwise the browser won't send the cookie if it's expired.

NOTE: Additional features described in #39916 (comment) have been added

Related Issue

https://github.com/owncloud/enterprise/issues/5067

Motivation and Context

How Has This Been Tested?

Manually tested with basic auth

1. Setup session_lifetime = 120 and session_keepalive = false
2. Login with user
3. Browse through ownCloud from more than 5 minutes
4. Check you haven't logged out automatically
5. Don't do anything for more than 2 minutes (*)
6. Check you get log out.

(*) After those 2 minutes, you have to perform any request to the server. If you have the notifications app enabled, a request is performed automatically each 30 secs, so worst case, you are logged out after 2 minutes and a half. If you don't have the notifications app enabled, you'll have to perform any request by yourself.

1. Setup session_lifetime = 120 (session_keepalive is unset)
2. Login with user
3. Wait for more than 3 minutes without doing anything
4. Check that you're still logged in

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[jvillafanez]

⚠️ it seems the heartbeat changes in this PR are causing issues with shibboleth. Requires further investigation.

It was a setup issue. The "heartbeat" endpoint must be out of the shibboleth auth scope, same as with it was for the "status.php", public sharing, and other endpoints.
No code changes are needed.

[IljaN]

Remarks:

• Now heartbeat and notification is sent
• Session starts on login-window but gets extended after login

[IljaN]

• OIDC seems to work

[jvillafanez]

For oauth2, at least for the web UI it doesn't seem to be relevant. The web UI will still use the cookie, so it falls into the usual use case.

For the case of the new web frontend, it works although the requests are a bit odd. The web frontend is using the bearer token, and it seems that even if the cookie expires, the bearer token kicks in, so the requests go through.
From the user perspective, there is no issue using the new web frontend. Note that the session_lifetime and session_keepalive won't work since it's a different mechanism.

[jvillafanez]

Session starts on login-window but gets extended after login

Just to clarify things: we're dealing with 2 different cookies. The cookie containing the session id, which doesn't expire (or expires with the session), and the cookie containing the passphrase to decrypt the contents of the session, which now expires.
The session id changes when the user logs in (likely for security), but the passphrase not. The expiration of the passphrase cookie is refreshed when a new page is loaded or with the heartbeat. When that cookie expires, the server isn't able to access to the session (it can't decrypt the contents) so it logs the user out because it can't perform any verification.

From the user's perspective, yes, the session is extended when the user logs in.

[jvillafanez]

rebased

[jvillafanez]

I don't think we can improve the quality for sonarcloud in this case:

• The test cases should test whether a cookie is sent, or at least if the method is called, but the call is explicitly removed from the tests (conditional "if-is-phpunit"), which means we can't properly test the results.
• https://github.com/owncloud/core/pull/39916/files#diff-e31f93f9d84244e0fc80c00a22fa6fc0463bfdbd1d90a0312af854ec7fb0ad73R102-R105 seems more explicit if we manage to improve the code in order to remove the phpunit condition. It should be easier to spot and remove if needed.

[voroyam]

https://cloud.owncloud.com/index.php/s/U5MnLdl4VB82V4D

Installed a 10.9.1 and the PR and I did not get logged out.

Only installed OAuth2 and 2FA as apps, and run the updates on the marketplace.

No logout after the defined time

<?php
$CONFIG = array (
'session_lifetime' => 60 * 1,
'session_keepalive' => true,

Enabled Apps:

Enabled:
  - activity 2.7.0
  - activity 2.7.0
  - comments 0.3.0
  - configreport 0.2.0
  - dav 0.7.0
  - federatedfilesharing 0.5.0
  - federation 0.1.0
  - files 1.5.2
  - files_external 0.8.0
  - files_mediaviewer 1.0.5
  - files_pdfviewer 1.0.1
  - files_sharing 0.14.0
  - files_texteditor 2.4.1
  - files_trashbin 0.9.1
  - files_versions 1.3.0
  - firstrunwizard 1.2.0
  - market 0.6.3
  - market 0.6.3
  - notifications 0.5.4
  - oauth2 0.5.2
  - oauth2 0.5.2
  - provisioning_api 0.5.0
  - systemtags 0.3.0
  - templateeditor 0.4.0
  - twofactor_totp 0.7.4
  - updatenotification 0.2.1
Disabled:

for some reason OAuth2 is listed twice.

[voroyam]

opened a ticket for the duplicate app #39930

[IljaN]

@voroyam Could you test with higher timeouts? i.e 120 or 180 secs? Very low timeouts might not be possible due to variouse things (notification etc.)

[voroyam]

Sure

[voroyam]

@voroyam Could you test with higher timeouts? i.e 120 or 180 secs? Very low timeouts might not be possible due to various things (notification etc.)

tested with 180s

same result. no logout.

https://cloud.owncloud.com/index.php/s/AsgZ4EQ3N1yyiLt

[IljaN]

@jvillafanez Not sure if this works as expected as the heartbeat always extends the session. Are we doing something wrong?

[jvillafanez]

@jvillafanez Not sure if this works as expected as the heartbeat always extends the session. Are we doing something wrong?

That's intended. The heartbeat should always extend the session. You can set the session_keepalive => false in the config.php file to disable the heartbeat. In this case, the session is only extended by reloading or changing the web page.
If the session_keepalive isn't set, it will be considered as true.

Note that this will only affect the cookie. For the case of oAuth2, it's expected that the client will always send a bearer token in the authentication header. The handling for the bearer token isn't modified by this PR, so ownCloud will likely authorize access due to the bearer token, assuming the token is still valid.
Anyway, I don't think the web UI uses the oAuth2 token but the cookie, so the web UI should behave the same way as without oAuth2.

[IljaN]

But the session is not extended on upload/download/propfind? I guess this would be a big architectural change to implement it like this.

I fear this won't fix the use-case as in normal use the page is never refreshed.

Edit: But at least this fixes the issue where you are always logged out if timeout is set.

[jvillafanez]

In general, I don't think it will give problems. If a download happens, once it's started it doesn't matter if the cookie expires. Unless the client has to perform a second request, which could have the cookie expired, the download should end without any issue.

For the downloads in the web UI, the problem could be that the page refreshes "randomly" at some point, because it isn't clear what happens with the download that is in progress. If such random refresh doesn't happen (it shouldn't happen with the session_keepalive active), there shouldn't be problems.

From my point of view:

• propfind shouldn't give problems. If the cookie expires, the user will have to re-authenticate before the next propfind request.
• downloads shouldn't neither. If the download goes through, there shouldn't be any secondary request needed, and the download is likely handled by the browser, so it's unlikely it will be stopped if the page refreshes (I think it's handled separately from the web page, although it might be browser-dependent).
• chunked uploads are tricky. If the second request isn't properly authenticated (cookie isn't sent), what happens with the flow?. As said, this shouldn't happen by default since the heartbeat should keep the cookie with the updated expiration, but we might need to check what happens with the flow if this doesn't happen. I'd rather not touch anything for now unless it breaks too badly.

[jvillafanez]

Changes added to the PR:

• Added mouse tracking if session_keepalive is false. If the mouse stops moving, a heartbeat request will be sent after a while
• Added new config option session_forced_logout_timeout to expire the session when the page is unloaded.
  • The logout will happen not only when the browser or the tab is closed, but also when the user navigates to a different site (this could happen while setting up google drive as external storage, for example).
  • The config option must be a positive number indicating the number of seconds allowed for the user to access the page again without re-authenticating.
    • A value of 0 or a negative value will disable this option.
    • Recommended value should be between 5 and 10 seconds. Lower values might be problematic because you might be logged out while navigating through ownCloud normally.

Note that the new config option still needs to be added and documented in the config.php file.

[jvillafanez]

Main changes are already in the PR. There are a couple of non-code things left, to be done after validation: include information in the config.sample.php file about the new config option added, and add the changelog item.

No plans to change anything due to sonarCloud: code smells are known, and the coverage doesn't seem to be easy to rise

[DeepDiver1975]

I have nothing to comment from a technical perspective.

From a dont-break-everything-perspective it would be great to isolate this in an app on it's own ..... 99.99% of the installations don't need this feature ....

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

25.5% Coverage
0.0% Duplication

[mrow4a]

@jvillafanez so if I have an app e.g. video browser etc , and to be sure it does not get logout while browsing, it just needs to implement heartbeat with $.post(OC.generateUrl('/heartbeat')); in some endless loop if opened or so ?
Could you summarize in keypoints what are the main changes (I checked a code but wanted to understand if this is what is changed) ?

[mrow4a]

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

[jvillafanez]

The basic setup is with the session_keepalive = true. This is the default. In this case, a periodic heartbeat will be sent automatically. You shouldn't need to touch anything since the "core/js/js.js" file sets it up for you (and the file should be already included in all the pages).

If session_keepalive = false, the heartbeats will be sent based on the mouse activity. This is also set up for you automatically in the "core/js/js.js" file. If there is no mouse activity, the cookie will eventually expire and you'll have to re-login.

The session_lifetime can be adjusted if needed. This will affect the intervals at which the heartbeat is sent in order to keep the session alive. Anyway, the overall behavior won't change.

The new session_forced_logout_timeout ensure that, when the tab is closed and that timeout is reached, you'll have to re-login again. The trick here is that it happens anytime the page is unloaded, which includes navigating throw the web, but if you load a new ownCloud page, a fresh cookie will replace the "expire now" one (assuming the page loads in time).
The session_forced_logout_timeout will trigger in the following scenarios:

• When the browser closes
• When the tab closes
• When you navigate to a different site (in the same tab)

There is a known issue with the session_forced_logout_timeout when is use with multiple tabs. If the forced logout happens in one tab, you'll be kicked out in all the tabs. This happens because the cookie expires, but it's shared across all the tabs, so the next request that happens won't have the cookie.

---

In general, everything is managed for you with those 3 config options, so you don't need to touch anything else.
Technically, you can send your own heartbeat request to extend the session, but I don't think you need it. If you don't want the session to expire, just set the session_keepalive = true and you're done.

@jvillafanez so if I have an app e.g. video browser etc , and to be sure it does not get logout while browsing, it just needs to implement heartbeat with $.post(OC.generateUrl('/heartbeat')); in some endless loop if opened or so ?

If you set the session_keepalive = true, that's done already for you, so you don't need to do anything.
If session_keepalive = false, the current options are:

• Set a session_lifetime long enough to see the whole video
• Move the mouse from time to time.

[mrow4a]

There is a known issue with the session_forced_logout_timeout when is use with multiple tabs. If the forced logout happens in one tab, you'll be kicked out in all the tabs. This happens because the cookie expires, but it's shared across all the tabs, so the next request that happens won't have the cookie.

Issue or design? :> btw, my bank website works the same way

If you set the session_keepalive = true, that's done already for you, so you don't need to do anything.
If session_keepalive = false, the current options are..

what if I want to disable session_keepalive, but for certain app it should not logout? can apps somehow use heartbeat functionality in the app code to prevent logout when using the app ?

[jvillafanez]

There is a known issue with the session_forced_logout_timeout when is use with multiple tabs. If the forced logout happens in one tab, you'll be kicked out in all the tabs. This happens because the cookie expires, but it's shared across all the tabs, so the next request that happens won't have the cookie.

Issue or design? :> btw, my bank website works the same way

Cookies work like that.
I might have an idea using broadcast channels, but it won't work on IE because that feature isn't available. Anyway, I think it's better to leave it like this for now.

what if I want to disable session_keepalive, but for certain app it should not logout? can apps somehow use heartbeat functionality in the app code to prevent logout when using the app ?

They can but I don't think they should, at least for now.
The feature is more or less isolated so the behavior is under control. If an app starts extending the session lifetime by itself, then it will be more difficult to predict when the session will expire. The overall behavior might become more erratic if some apps hit the heartbeat and other not in the same circumstances.

[mrow4a]

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

[jvillafanez]

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.

[mrow4a]

small comments but in general concept looks good. do not forget documentation.

[jvillafanez]

@mmattel added owncloud/docs-server#283 in order to document this.
As said, this is expected to be in 10.9.2

[jvillafanez]

One thing that might be worthy to notice is that the session_forced_logout_timeout relies on the well-behavior of the browser. If the browser doesn't trigger the "unload" event, or doesn't send the heartbeat request somehow, we simply won't know when the browser has been closed. We also rely on the browser to expire and not send the cookie.

While all of this isn't expected to happen on any common browser, there could be clients that misbehave. In this case, the session will still be active in the server, so they could still access ownCloud without re-authenticating, assuming the session is still alive in the server (which is controlled by the session_lifetime option).

I think it's important to note that the behavior isn't enforced by the server, but a hint for the clients. As explained, I don't think we can enforce the behavior server-side because the client might not trigger the "unload" event and the server might not know when the client has closed.

[jnweiger]

Confirmed fixed in 10.10.0 RC2