Handling of unlock requests during user logout

Development
1

Hi everyone,

while trying to explain and fix flickering integration tests, I found that there is a true race condition in XWiki when logging out while an editor is open.

Basically, there are two requests that race:

  • The actual logout request.
  • The request for unlocking, triggered when the editor page is unloaded, which continues in the background.

There are two possible risks here:

  • The unlocking of the page might not succeed, as the user’s session has already been invalidated.
  • The unlocking request might mess with the session or persistent login cookies that are set/invalidated by the logout.

Solution 1: Unauthenticated unlock

One idea I had to clean up this mess is to make the unlock request unauthenticated, using credentials: 'omit'. Instead, the request should be authenticated by a random token that is unique to each lock and stored together with the lock, created when the lock is created. That way, unlock requests always succeed and don’t interfere with a logout.

There are several issues with this approach, though:

  • Some authenticators might be placed on a layer in front of XWiki such that XWiki in fact isn’t accessible when using credentials: 'omit'. As a way to mitigate that, we could detect that situation client-side once and store in a cookie if XWiki can be reached unauthenticated.
  • Unlocking currently also deletes the temporary uploads for the current session, which wouldn’t be available in an unauthenticated request.
  • Quite some added complexity for the tokens.

Solution 2: Unlock early on logout

We could add a JavaScript event handler on the logout link that performs the unlock request early, waiting for the request to finish before redirecting to the logout action. Not my favorite in terms of coupling between logout link and unlocking, and a bit ugly in the sense that logout might seemingly stop working - but we could minimize the impact when not in an editor, and display some notification that locks are released, so the user knows what’s going on.

In integration tests, we normally don’t use the logout link so we would need to decide what to do there, if we start using the actual logout link or if we do something else (like trigger the same mechanism as the logout link uses if we can make it re-usable).

Solution 3: Don’t care about unlock

We could decide to not care about unlock during logout and just ensure that the unlock request doesn’t interfere with the logout request by being careful with server-side steps/adding a little bit of server-side synchronization.

Do you have any opinions? I think solution 2 would be best, solution 3 being a good fallback as I would consider the use case unlikely. So +1 for 2 and 3, and -0 for 1 as it feels too complex and fragile.

2

-1 for option 1 as well.

Couldn’t the unlocking be part of the logout request?
For instance, an event listener that would clear all the locks of the current user during the logout.
That way it does not have to be handled by a separate request.

3

I think the idea is good, this in combination with solution 3 sounds like the best idea so far. The only problem that I see with this is that this means that logging out in one browser would remove locks for possibly still active edit sessions on other devices. Like if a user has a phone and a desktop, and logs out on the phone, I don’t think the locks on the desktop session should be removed.

What could potentially help is if we introduced a proper user session concept and stored them in the locks. @surli might be planning to implement something like this for unrelated reasons so maybe we could actually get this solution. I guess we should then introduce something more generic like a BrowserSessionCleanupHandler role that can be implemented by various parts to remove all locks, temporary attachments etc. that are related to the browser session.

4

I was also wondering why you were not proposing to cleanup locks when the session is invalidated (which is more accurate than logout IMO), but indeed the locks are associated to users and not sessions (but we should probably fix that).

Note that this is already what we do for what is IMO a very similar use cases: getting rid of temporary attachments when invalidating the related session.

5

Indeed, we could also use the existing session, as locks are necessarily short-lived and should be shorter lived than the Servlet container session, that feels very fitting. What’s not so clear to me, though, is what should happen if the user starts editing the same document in two different sessions - would we treat the page as locked by a different session then, with an adjusted message?

6

I guess. I mean having the same document being modified in two different sessions will cause the same kind of conflicts if both session belong to the same user or not. But the same is true when editing the same document in two different tabs with the same session.