XWIKI-22430: Logging out does not unlock pages that were being edited #3380

surli · 2024-08-29T16:36:21Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-22430

Changes

Description

Provide a new UserUnauthenticatedEvent to detect whenever a user logged out, the same way that we have UserAuthenticatedEvent
Trigger that new event when processing a logout in MyFormAuthenticator
Refactor the code in XWikiHibernateStore to listen for UserUnauthenticatedEvent for cleaning the locks of a user, instead of listening to an action event
Rename UserAuthenticatedEventNotifier and use it for both authenticated and unauthenticated events
Fix test

Clarifications

Screenshots & Video

N/A

Executed Tests

mvn clean install -Pquality on:

xwiki-platform-security-authentication-api
xwiki-platform-oldcore
xwiki-platform-legacy-oldcore

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- None: the changes might impact perf and provides new API, better fix it only in stable

tmortagne · 2024-08-29T17:04:14Z

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

+                for (String wikiId : this.wikiDescriptorManager.getAllIds()) {
+                    if (!currentWiki.equals(wikiId)) {
+                        context.setWikiReference(new WikiReference(wikiId));
+                        this.releaseAllLocksForUser(userDoc, context);


If this fail, the context wiki will be broken. Would be better to restore the context wiki only once in a try/finally around the whole for (there is no need to restore it after each releaseAllLocksForUser anyway).

tmortagne · 2024-08-29T17:06:27Z

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

-                this.releaseAllLocksForCurrentUser(ctx);
-            } finally {
-                ctx.setWikiId(cdb);
+                String currentWiki = context.getWikiId();


Would probably make more sense to use getWikiReference here, it avoids creating a new WikiReference just to restore it.

tmortagne · 2024-08-29T17:06:40Z

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

+                String currentWiki = context.getWikiId();
+                for (String wikiId : this.wikiDescriptorManager.getAllIds()) {
+                    if (!currentWiki.equals(wikiId)) {
+                        context.setWikiReference(new WikiReference(wikiId));


Feels simpler to use setWikiId here.

I thought setWikiId was deprecated. Will check.

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

tmortagne · 2024-08-29T17:13:08Z

.../xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/user/impl/xwiki/MyFormAuthenticator.java

@@ -292,7 +292,8 @@ public boolean processLogout(SecurityRequestWrapper securityRequestWrapper,
        HttpServletResponse httpServletResponse, URLPatternMatcher urlPatternMatcher) throws Exception
    {
        boolean result = super.processLogout(securityRequestWrapper, httpServletResponse, urlPatternMatcher);
-        if (result == true) {
+        if (result) {
+            this.getUserAuthenticatedEventNotifier().notifyUserUnauthenticated(securityRequestWrapper.getRemoteUser());


Hmm, this should probably be called after the forgetLogin.

I'll add a comment about that one: I thought the same at first but then what I'm doing doesn't work anymore because forgetLogin reset the wrapped request user to null.

forgetLogin reset the wrapped request user to null.

Yes, that's why the event should be sent after. You just need to remember the user reference before it's reseted.

tmortagne · 2024-08-29T17:15:15Z

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

-            } finally {
-                ctx.setWikiId(cdb);
+                String currentWiki = context.getWikiId();
+                for (String wikiId : this.wikiDescriptorManager.getAllIds()) {


This feels too big a change for a LTS (I'm afraid logout on myxwiki.org is going to be quite long now, for example).

Honestly I wasn't really sure about this loop: it feels a bit overkill but on the other side that's the only way to guarantee that a user properly has release all locks to my knowledge.

It's not overkill, the user could have lock anywhere in the farm (there are many use cases where you have only main wiki users and various subwikis, xwiki.org for example). It's just very expensive in a farm like myxwiki.org (I guess we might want to move the locks to Solr eventually, or just stop with the locks since that's another thing we talked about).

Ok, so it will impact perf, you think it's enough to prevent backporting?

I guess we might want to move the locks to Solr eventually

Interesting idea.

Ok, so it will impact perf, you think it's enough to prevent backporting?

I updated the description to not backport.

tmortagne · 2024-08-29T17:17:51Z

...entication-api/src/main/java/org/xwiki/security/authentication/UserUnauthenticatedEvent.java

+ * @since 15.10.13
+ */
+@Unstable
+public class UserUnauthenticatedEvent implements Event


Maybe UserUnauthenticatedEvent and UserAuthenticatedEvent should share a common abstract class since their content is mostly the same.

Yeah, I hesitated about doing that but was lazy :)

tmortagne · 2024-08-29T17:19:42Z

...entication-api/src/main/java/org/xwiki/security/authentication/UserUnauthenticatedEvent.java

+    @Override
+    public boolean matches(Object other)
+    {
+        return other instanceof UserUnauthenticatedEvent;


It would worth improving this to take into account this.userReference as otherwise it's quite missleading to have two different constructors from the point of view of a listener.

Honestly I'm surprised that the code for UserAuthenticatedEvent doesn't use it, since the javadoc says otherwise, see: https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-security/xwiki-platform-security-authentication/xwiki-platform-security-authentication-api/src/main/java/org/xwiki/security/authentication/UserAuthenticatedEvent.java#L39

I'd preferably have same behaviour for both events, but changing this implem in UserAuthenticatedEvent is a breaking change behaviour...

I'd preferably have same behaviour for both events, but changing this implem in UserAuthenticatedEvent is a breaking change behaviour...

IMO it's fixing a bug and that's typically something that should go in the abstract.

IMO it's fixing a bug

Would need to check usage first but I tend to agree that we should use it.

that's typically something that should go in the abstract.

+1

Can't find any usage in xwiki-contrib, except to trigger the event: https://github.com/search?q=org%3Axwiki-contrib%20UserAuthenticatedEvent&type=code

michitux · 2024-08-30T07:27:03Z

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java

+                if (userUnauthenticatedEvent.getUserReference() != null) {
+                    DocumentReference userDoc = XWikiHibernateStore.this.userReferenceSerializer.serialize(
+                        userUnauthenticatedEvent.getUserReference());
+                    releaseAllLocksForUser(userDoc, (XWikiContext) data);


Why does this release all locks? Shouldn't this only release locks of the current session of the user? I mean when I log out on my phone, this shouldn't release locks that I'm holding on my desktop browser, should it?

We don't really have the concept of session right now in the locks, but yes ideally they should.

That would be an improvment to add in the future I think, the expectation of current mechanism is that it does release all locks

Could be done at the same time as moving the locks to solr for better perf as Thomas suggested.

Storing a reference to all active locks of the user in the session would solve both the session and the performance problem.

Storing a reference to all active locks of the user in the session would solve both the session and the performance problem.

It would probably be enough for the performance problem, yes (there is very little chance that the user has locks on many wikis at the same time in practice).

I'd do that as part of another improvment ticket: IMO it's out of the scope of current PR.

It's kind of the same scope as the for you added, the session references was another way to fix this problem (but yes, it's more complex than adding a for :)). It would also make the new event useless for this need, as the trigger would be more the session invalidation (which is also triggered by the logout) and not the logout (as a bonus this would work with all authenticators right away).

* Provide a new UserUnauthenticatedEvent to detect whenever a user logged out, the same way that we have UserAuthenticatedEvent * Trigger that new event when processing a logout in MyFormAuthenticator * Refactor the code in XWikiHibernateStore to listen for UserUnauthenticatedEvent for cleaning the locks of a user, instead of listening to an action event * Rename UserAuthenticatedEventNotifier and use it for both authenticated and unauthenticated events * Fix test

* Provide an abstract event for authentication

surli requested a review from tmortagne August 29, 2024 16:36

tmortagne reviewed Aug 29, 2024

View reviewed changes

...tform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java Show resolved Hide resolved

tmortagne reviewed Aug 29, 2024

View reviewed changes

michitux reviewed Aug 30, 2024

View reviewed changes

surli marked this pull request as draft August 30, 2024 08:43

surli added 2 commits September 12, 2024 15:57

XWIKI-22430: Logging out does not unlock pages that were being edited

c6571cf

* Provide an abstract event for authentication

surli force-pushed the XWIKI-22430 branch from 5994612 to c6571cf Compare September 13, 2024 07:24

tmortagne assigned surli Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-22430: Logging out does not unlock pages that were being edited #3380

XWIKI-22430: Logging out does not unlock pages that were being edited #3380

surli commented Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024

surli Aug 30, 2024

tmortagne Aug 29, 2024

surli Aug 30, 2024

tmortagne Aug 30, 2024

surli Aug 30, 2024

tmortagne Aug 29, 2024 •

edited

Loading

surli Aug 30, 2024

tmortagne Aug 30, 2024 •

edited

Loading

surli Aug 30, 2024

surli Aug 30, 2024

tmortagne Aug 29, 2024

surli Aug 30, 2024

tmortagne Aug 29, 2024

surli Aug 30, 2024

tmortagne Aug 30, 2024

surli Aug 30, 2024

surli Aug 30, 2024

michitux Aug 30, 2024

tmortagne Aug 30, 2024

surli Aug 30, 2024

surli Aug 30, 2024

michitux Aug 30, 2024

tmortagne Aug 30, 2024

surli Aug 30, 2024

tmortagne Aug 30, 2024 •

edited

Loading

XWIKI-22430: Logging out does not unlock pages that were being edited #3380

Are you sure you want to change the base?

XWIKI-22430: Logging out does not unlock pages that were being edited #3380

Conversation

surli commented Aug 29, 2024 • edited Loading

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

tmortagne Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

tmortagne Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

surli commented Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024 •

edited

Loading

tmortagne Aug 29, 2024 •

edited

Loading

tmortagne Aug 30, 2024 •

edited

Loading

tmortagne Aug 30, 2024 •

edited

Loading