Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
084ef7f
XWIKI-22430: Logging out does not unlock pages that were being edited
surli Aug 29, 2024
c6571cf
XWIKI-22430: Logging out does not unlock pages that were being edited
surli Sep 13, 2024
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
47 changes: 37 additions & 10 deletions .../user/UserAuthenticatedEventNotifier.java → ...user/UserAuthenticationEventNotifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,22 @@
package com.xpn.xwiki.internal.user;

import javax.inject.Inject;
import javax.inject.Provider;
import javax.inject.Singleton;

import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;

import org.xwiki.component.annotation.Component;
import org.xwiki.observation.ObservationManager;
import org.xwiki.observation.event.Event;
import org.xwiki.security.authentication.UserAuthenticatedEvent;
import org.xwiki.security.authentication.UserUnauthenticatedEvent;
import org.xwiki.user.UserReference;
import org.xwiki.user.UserReferenceResolver;

import com.xpn.xwiki.XWikiContext;

/**
* This notifier helps dealing with events triggered when a user is authenticated through XWiki Oldcore's
* authenticators. It wraps an {@code ObservationManager} and a {@code UserReferenceResolver} to
Expand All @@ -40,9 +46,9 @@
* @version $Id$
* @since 13.3RC1
*/
@Component(roles = UserAuthenticatedEventNotifier.class)
@Component(roles = UserAuthenticationEventNotifier.class)
@Singleton
public class UserAuthenticatedEventNotifier
public class UserAuthenticationEventNotifier
{

@Inject
Expand All @@ -54,29 +60,50 @@ public class UserAuthenticatedEventNotifier
@Inject
private UserReferenceResolver<String> userReferenceResolver;

@Inject
private Provider<XWikiContext> contextProvider;

/**
* Resolve a string as a {@code UserReference} and notify a {@code UserAuthenticatedEvent} created with that user
* reference.
*
* @param stringUserReference string form of the reference of user that will be resolved as a {@code
* UserReference} and passed to the {@code UserAuthenticatedEvent} instance creation
*/
public void notify(String stringUserReference)
public void notifyUserAuthenticated(String stringUserReference)
{
if (!StringUtils.isBlank(stringUserReference)) {
UserReference userReference = this.userReferenceResolver.resolve(stringUserReference);
if (this.logger.isDebugEnabled()) {
this.logger.debug("User [{}] authenticated", userReference);
}
this.notify(new UserAuthenticatedEvent(userReference));
}
}

/**
* Notify that the given user is now logged out.
*
* @param stringUserReference the user for whom to fire a {@link UserUnauthenticatedEvent}.
*/
public void notifyUserUnauthenticated(String stringUserReference)
{
UserReference userReference = this.userReferenceResolver.resolve(stringUserReference);
this.notify(new UserAuthenticatedEvent(userReference));
if (!StringUtils.isBlank(stringUserReference)) {
UserReference userReference = this.userReferenceResolver.resolve(stringUserReference);
if (this.logger.isDebugEnabled()) {
this.logger.debug("User [{}] unauthenticated", userReference);
}
this.notify(new UserUnauthenticatedEvent(userReference));
}
}

/**
* Notify a {@link UserAuthenticatedEvent} that has already been created.
*
* @param event {@code UserAuthenticatedEvent}
*/
private void notify(UserAuthenticatedEvent event)
private void notify(Event event)
{
if (this.logger.isDebugEnabled()) {
this.logger.debug("User authenticated for [{}]", event.getUserReference());
}
this.observationManager.notify(event, null);
this.observationManager.notify(event, null, this.contextProvider.get());
}
}
50 changes: 28 additions & 22 deletions ...rm-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/XWikiHibernateStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import org.hibernate.query.Query;
import org.slf4j.Logger;
import org.suigeneris.jrcs.rcs.Version;
import org.xwiki.bridge.event.ActionExecutingEvent;
import org.xwiki.component.annotation.Component;
import org.xwiki.component.manager.ComponentLookupException;
import org.xwiki.component.manager.ComponentManager;
Expand All @@ -86,7 +85,9 @@
import org.xwiki.observation.event.Event;
import org.xwiki.query.QueryException;
import org.xwiki.query.QueryManager;
import org.xwiki.security.authentication.UserUnauthenticatedEvent;
import org.xwiki.store.UnexpectedException;
import org.xwiki.user.UserReferenceSerializer;
import org.xwiki.wiki.descriptor.WikiDescriptorManager;
import org.xwiki.wiki.manager.WikiManagerException;

Expand Down Expand Up @@ -176,6 +177,10 @@ public class XWikiHibernateStore extends XWikiHibernateBaseStore implements XWik
@Named("local")
private EntityReferenceSerializer<String> localEntityReferenceSerializer;

@Inject
@Named("document")
private UserReferenceSerializer<DocumentReference> userReferenceSerializer;

@Inject
private ComponentManager componentManager;

Expand Down Expand Up @@ -2055,7 +2060,7 @@ private void registerLogoutListener()
{
this.observationManager.addListener(new EventListener()
{
private final Event ev = new ActionExecutingEvent();
private static final List<Event> EVENT_LIST = List.of(new UserUnauthenticatedEvent());

@Override
public String getName()
Expand All @@ -2066,59 +2071,60 @@ public String getName()
@Override
public List<Event> getEvents()
{
return Collections.<Event>singletonList(this.ev);
return EVENT_LIST;
}

@Override
public void onEvent(Event event, Object source, Object data)
{
if ("logout".equals(((ActionExecutingEvent) event).getActionName())) {
final XWikiContext ctx = (XWikiContext) data;
if (ctx.getUserReference() != null) {
releaseAllLocksForCurrentUser(ctx);
}
UserUnauthenticatedEvent userUnauthenticatedEvent = (UserUnauthenticatedEvent) event;
if (userUnauthenticatedEvent.getUserReference() != null) {
DocumentReference userDoc = XWikiHibernateStore.this.userReferenceSerializer.serialize(
userUnauthenticatedEvent.getUserReference());
releaseAllLocksForUser(userDoc, (XWikiContext) data);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@tmortagne tmortagne Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

}
}
});
}

/**
* Release all of the locks held by the currently logged in user.
*
* @param ctx the XWikiContext, used to start the connection and get the user name.
*/
private void releaseAllLocksForCurrentUser(final XWikiContext ctx)
private void releaseAllLocksForUser(final DocumentReference userDoc, final XWikiContext context)
{
try {
executeWrite(ctx, session -> {
executeWrite(context, session -> {
final Query query = session.createQuery("delete from XWikiLock as lock where lock.userName=:userName");
// Using deprecated getUser() because this is how locks are created.
// It would be a maintainibility disaster to use different code paths
// for calculating names when creating and removing.
query.setParameter("userName", ctx.getUser());
query.setParameter("userName", this.compactWikiEntityReferenceSerializer.serialize(userDoc));
query.executeUpdate();

return null;
});
} catch (Exception e) {
String msg = "Error while deleting active locks held by user.";
try {
this.endTransaction(ctx, false);
this.endTransaction(context, false);
} catch (Exception utoh) {
msg += " Failed to commit OR rollback [" + utoh.getMessage() + "]";
}
throw new UnexpectedException(msg, e);
}

// If we're in a non-main wiki & the user is global,
// switch to the global wiki and delete locks held there.
if (!ctx.isMainWiki() && ctx.isMainWiki(ctx.getUserReference().getWikiReference().getName())) {
final String cdb = ctx.getWikiId();
if (this.wikiDescriptorManager.isMainWiki(userDoc.getWikiReference().getName())) {
try {
ctx.setWikiId(ctx.getMainXWiki());
this.releaseAllLocksForCurrentUser(ctx);
} finally {
ctx.setWikiId(cdb);
String currentWiki = context.getWikiId();
Copy link
Member

@tmortagne tmortagne Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

for (String wikiId : this.wikiDescriptorManager.getAllIds()) {
Copy link
Member

@tmortagne tmortagne Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tmortagne tmortagne Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

I updated the description to not backport.

if (!currentWiki.equals(wikiId)) {
context.setWikiReference(new WikiReference(wikiId));
Copy link
Member

Choose a reason for hiding this comment

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

Feels simpler to use setWikiId here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought setWikiId was deprecated. Will check.

this.releaseAllLocksForUser(userDoc, context);
Copy link
Member

@tmortagne tmortagne Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

context.setWikiReference(new WikiReference(currentWiki));
}
}
} catch (WikiManagerException e) {
this.logger.error("Error for getting list of wikis to release locks for user [{}]", userDoc, e);
tmortagne marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
18 changes: 9 additions & 9 deletions ...ki-platform-oldcore/src/main/java/com/xpn/xwiki/user/impl/xwiki/MyBasicAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,20 @@

import com.xpn.xwiki.XWikiContext;
import com.xpn.xwiki.XWikiException;
import com.xpn.xwiki.internal.user.UserAuthenticatedEventNotifier;
import com.xpn.xwiki.internal.user.UserAuthenticationEventNotifier;
import com.xpn.xwiki.web.Utils;

public class MyBasicAuthenticator extends BasicAuthenticator implements XWikiAuthenticator
{

private UserAuthenticatedEventNotifier userAuthenticatedEventNotifier;
private UserAuthenticationEventNotifier userAuthenticationEventNotifier;

private UserAuthenticatedEventNotifier getUserAuthenticatedEventNotifier()
private UserAuthenticationEventNotifier getUserAuthenticatedEventNotifier()
{
if ( this.userAuthenticatedEventNotifier == null ) {
this.userAuthenticatedEventNotifier = Utils.getComponent(UserAuthenticatedEventNotifier.class);
if ( this.userAuthenticationEventNotifier == null ) {
this.userAuthenticationEventNotifier = Utils.getComponent(UserAuthenticationEventNotifier.class);
}
return this.userAuthenticatedEventNotifier;
return this.userAuthenticationEventNotifier;
}

@Override
Expand Down Expand Up @@ -89,7 +89,7 @@ public boolean processLogin(String username, String password, String rememberme,

request.setUserPrincipal(principal);

this.getUserAuthenticatedEventNotifier().notify(principal.getName());
this.getUserAuthenticatedEventNotifier().notifyUserAuthenticated(principal.getName());

return false;
} else {
Expand Down Expand Up @@ -135,8 +135,8 @@ public static Principal checkLogin(SecurityRequestWrapper request, HttpServletRe

// Since this scope is static, no UserAuthenticatedEventNotifier is available
// So we create one here
UserAuthenticatedEventNotifier notifier = Utils.getComponent(UserAuthenticatedEventNotifier.class);
notifier.notify(principal.getName());
UserAuthenticationEventNotifier notifier = Utils.getComponent(UserAuthenticationEventNotifier.class);
notifier.notifyUserAuthenticated(principal.getName());

return principal;
} else {
Expand Down
19 changes: 10 additions & 9 deletions ...iki-platform-oldcore/src/main/java/com/xpn/xwiki/user/impl/xwiki/MyFormAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,21 @@

import com.xpn.xwiki.XWikiContext;
import com.xpn.xwiki.XWikiException;
import com.xpn.xwiki.internal.user.UserAuthenticatedEventNotifier;
import com.xpn.xwiki.internal.user.UserAuthenticationEventNotifier;
import com.xpn.xwiki.web.Utils;

public class MyFormAuthenticator extends FormAuthenticator implements XWikiAuthenticator
{
private static final Logger LOGGER = LoggerFactory.getLogger(MyFormAuthenticator.class);

private UserAuthenticatedEventNotifier userAuthenticatedEventNotifier;
private UserAuthenticationEventNotifier userAuthenticationEventNotifier;

private UserAuthenticatedEventNotifier getUserAuthenticatedEventNotifier()
private UserAuthenticationEventNotifier getUserAuthenticatedEventNotifier()
{
if ( this.userAuthenticatedEventNotifier == null ) {
this.userAuthenticatedEventNotifier = Utils.getComponent(UserAuthenticatedEventNotifier.class);
if ( this.userAuthenticationEventNotifier == null ) {
this.userAuthenticationEventNotifier = Utils.getComponent(UserAuthenticationEventNotifier.class);
}
return this.userAuthenticatedEventNotifier;
return this.userAuthenticationEventNotifier;
}

/**
Expand Down Expand Up @@ -168,7 +168,7 @@ public boolean processLogin(SecurityRequestWrapper request, HttpServletResponse

request.setUserPrincipal(principal);

this.getUserAuthenticatedEventNotifier().notify(principal.getName());
this.getUserAuthenticatedEventNotifier().notifyUserAuthenticated(principal.getName());

} else {
// Failed to authenticate, better cleanup the user stored in the session
Expand Down Expand Up @@ -240,7 +240,7 @@ public boolean processLogin(String username, String password, String rememberme,

request.setUserPrincipal(principal);

this.getUserAuthenticatedEventNotifier().notify(principal.getName());
this.getUserAuthenticatedEventNotifier().notifyUserAuthenticated(principal.getName());

Boolean bAjax = (Boolean) context.get("ajax");
if ((bAjax == null) || (!bAjax.booleanValue())) {
Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this should probably be called after the forgetLogin.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

if (this.persistentLoginManager != null) {
this.persistentLoginManager.forgetLogin(securityRequestWrapper, httpServletResponse);
}
Expand Down
2 changes: 1 addition & 1 deletion xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/META-INF/components.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ com.xpn.xwiki.internal.render.DefaultOldRendering
com.xpn.xwiki.internal.render.OldRenderingProvider
com.xpn.xwiki.internal.render.groovy.ParseGroovyFromString
com.xpn.xwiki.internal.user.MyPersistentLoginManagerProvider
com.xpn.xwiki.internal.user.UserAuthenticatedEventNotifier
com.xpn.xwiki.internal.user.UserAuthenticationEventNotifier
com.xpn.xwiki.internal.user.UserCreatedEventListener
com.xpn.xwiki.internal.velocity.DefaultVelocityEvaluator
org.xwiki.internal.web.EffectiveAuthorSetterListener
Expand Down
19 changes: 14 additions & 5 deletions ...ore/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/store/XWikiHibernateStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import org.junit.jupiter.api.extension.RegisterExtension;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.xwiki.bridge.event.ActionExecutingEvent;
import org.xwiki.context.Execution;
import org.xwiki.context.ExecutionContext;
import org.xwiki.model.reference.DocumentReference;
Expand All @@ -51,12 +50,15 @@
import org.xwiki.observation.EventListener;
import org.xwiki.observation.ObservationManager;
import org.xwiki.query.QueryManager;
import org.xwiki.security.authentication.UserUnauthenticatedEvent;
import org.xwiki.test.LogLevel;
import org.xwiki.test.junit5.LogCaptureExtension;
import org.xwiki.test.junit5.mockito.ComponentTest;
import org.xwiki.test.junit5.mockito.InjectMockComponents;
import org.xwiki.test.junit5.mockito.MockComponent;
import org.xwiki.test.mockito.MockitoComponentManager;
import org.xwiki.user.UserReference;
import org.xwiki.user.UserReferenceSerializer;
import org.xwiki.wiki.descriptor.WikiDescriptorManager;
import org.xwiki.wiki.manager.WikiManagerException;

Expand Down Expand Up @@ -143,6 +145,10 @@ public class XWikiHibernateStoreTest
@Named("compactwiki")
private EntityReferenceSerializer<String> compactWikiEntityReferenceSerializer;

@MockComponent
@Named("document")
private UserReferenceSerializer<DocumentReference> userReferenceSerializer;

@MockComponent
private WikiDescriptorManager wikiDescriptorManager;

Expand Down Expand Up @@ -240,14 +246,17 @@ void locksAreReleasedOnLogout() throws Exception

Query query = mock(Query.class);
when(session.createQuery("delete from XWikiLock as lock where lock.userName=:userName")).thenReturn(query);
when(xcontext.getUserReference()).thenReturn(new DocumentReference("xwiki", "XWiki", "LoggerOutter"));
when(xcontext.getUser()).thenReturn("XWiki.LoggerOutter");
UserReference userReference = mock(UserReference.class);
String username = "XWiki.LoggerOutter";
DocumentReference userDoc = new DocumentReference("xwiki", "XWiki", "LoggerOutter");
when(this.userReferenceSerializer.serialize(userReference)).thenReturn(userDoc);
when(this.compactWikiEntityReferenceSerializer.serialize(userDoc)).thenReturn(username);
when(this.hibernateStore.beginTransaction()).thenReturn(true);

// Fire the logout event.
eventListenerCaptor.getValue().onEvent(new ActionExecutingEvent("logout"), null, xcontext);
eventListenerCaptor.getValue().onEvent(new UserUnauthenticatedEvent(userReference), null, xcontext);

verify(query).setParameter("userName", "XWiki.LoggerOutter");
verify(query).setParameter("userName", username);
verify(query).executeUpdate();
verify(this.hibernateStore).beginTransaction();
verify(this.hibernateStore).endTransaction(true);
Expand Down
Loading