core icon indicating copy to clipboard operation
core copied to clipboard

Include tab communication for the forced logout config

Open jvillafanez opened this issue 2 years ago • 9 comments

Description

Possible solution to the issue with multiple tabs in https://github.com/owncloud/core/pull/39916.

When the tab closes, a message is sent to the rest of the tabs so they sent a heartbeat request to update the cookie. This will prevent the tabs from logging out since the cookie won't expire soon. It's expected that only one tab sends the heartbeat request, although it isn't fully guaranteed.

Note that this solution WON'T work in IE11, and for Safari it will only work in 15.4 but not for previous versions. We should consider whether we want to use this solution as it is, ignoring IE11, or if we want to provide a configuration option in order to try to keep the same behavior in all the browsers.

Related Issue

Part of https://github.com/owncloud/core/pull/39916

Motivation and Context

Right now, the session_forced_logout_timeout works with only one tab. Using multiple tabs will cause a logout in all the tabs when any of them closes. This is an unwanted behavior.

How Has This Been Tested?

Checked with firefox:

  1. Setup session_forced_logout_timeout => 10 in the config.php file
  2. Open 3 tabs with ownCloud. Note that you have logged in with just one of them.
  3. Close one of them
  4. Wait 10 secs before performing any action

You can perform the action without any problem because you haven't been logged out. You can check that one of the ownCloud tab has sent a heartbeat request to refresh the cookie.

As said, IE11 and Safari < 15.4 fails due to the lack of support for broadcast channels. (https://caniuse.com/broadcastchannel)

Screenshots (if appropriate):

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] 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:

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

jvillafanez avatar Apr 27 '22 16:04 jvillafanez

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.

update-docs[bot] avatar Apr 27 '22 16:04 update-docs[bot]

Thanks! I think it is fine to ignore IE11 for this corner-corner-case solution.

pmaier1 avatar Apr 29 '22 08:04 pmaier1

Added clarification on the behavior in the config.php file. Since this is a follow up of https://github.com/owncloud/core/pull/39916 , no changelog entry is needed.

jvillafanez avatar Apr 29 '22 08:04 jvillafanez

Security hotspots are known:

  • There is a Math.random() used to generate an id. This is just for identification purposes. It's expected to be short-lived, so collisions shouldn't happen.
  • The other Math.random() call is to try not to send the heartbeat request at the same time from multiple tabs, as well as to give some time to cancel the request if needed. While this could still happen, the probability should be small. At the very least, the number of heartbeat requests should be reduced.

jvillafanez avatar Apr 29 '22 09:04 jvillafanez

I'm having issues with edge Version 101.0.1210.32 (which should behave like Chrome). ~I'm still not fully sure, but it seems the broadcast message is also received by the sender, which according to the specs, it shouldn't happen. If this is the case, I think we'll have to wait a bit to fix this and retest the new solution.~

jvillafanez avatar Apr 29 '22 12:04 jvillafanez

I'm still not fully sure, but it seems the broadcast message is also received by the sender, which according to the specs, it shouldn't happen.

~I'm quite sure that this is the issue with MS Edge. The fact that Edge and Chrome share the engine makes me doubt about the behavior in Chrome.... Anyway, the PR assumes that the sender won't receive the broadcast message, so if this isn't true, the code won't work properly. I'm confident we can patch the PR somehow so don't rely on the sender not receiving the broadcast message, but it will require some changes in the code, and we'll have to retest all the browsers again.~

jvillafanez avatar Apr 29 '22 13:04 jvillafanez

It seems I did a wrong troubleshooting. Edge works within the specs, but the problem is that when the page is closed, the tab doesn't send the "postMessage" to the rest of the tabs. One tab closes, but the rest don't receive notification about it.

The new assumption is that the broadcast channel is somehow unusable after the heartbeat on close finishes. While the request can continue with the "keepalive" flag, the tab might have released all the resources, in particular, the connection with the broadcast channel. If this is the case, we might need a different approach.

jvillafanez avatar May 03 '22 09:05 jvillafanez

We'll skip this PR for now because it works only for firefox and chrome, the rest of the browsers don't work. We might need a different approach in order to support more browsers.

jvillafanez avatar May 03 '22 10:05 jvillafanez