Skip to content

Commit

Permalink
fix: remove default login listener when setting form action (#6669)
Browse files Browse the repository at this point in the history 
Setting both login listeners and form action can cause unexpected behaviors because of concurrent processing of form submission processing and login event.
For example, if form submission ends in a session ID change and a redirect to a different page, the UIDL request may fail with a session expiration response, causing the Flow client to reload the page and potentially cancel the ongoing redirection.
In addition, after from submission, the login event would be sent to a dismissed UI.

This change removes the default login listener when setting a form action, and logs a warning when setting both an action and custom login listeners.
  • Loading branch information
@mcollovati
mcollovati authored Oct 29, 2024
1 parent b963ad6 commit 7e54c05
Show file tree
Hide file tree
Showing 3 changed files with 238 additions and 13 deletions.
78 changes: 65 additions & 13 deletions ...parent/vaadin-login-flow/src/main/java/com/vaadin/flow/component/login/AbstractLogin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package com.vaadin.flow.component.login;

import org.slf4j.LoggerFactory;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.ComponentEvent;
import com.vaadin.flow.component.ComponentEventListener;
Expand All @@ -24,6 +26,7 @@
import com.vaadin.flow.component.HasEnabled;
import com.vaadin.flow.component.Synchronize;
import com.vaadin.flow.dom.DisabledUpdateMode;
import com.vaadin.flow.dom.DomListenerRegistration;
import com.vaadin.flow.dom.PropertyChangeListener;
import com.vaadin.flow.internal.JsonSerializer;
import com.vaadin.flow.shared.Registration;
Expand All @@ -40,6 +43,17 @@
* {@link com.vaadin.flow.component.HasEnabled#setEnabled(boolean)} method.
* Setting error {@link #setError(boolean)} true makes component automatically
* enabled for the next login attempt.
* <p>
* </p>
* Server side login listener do not work in combination with HTML form
* submission configured by setting the {@code action} attribute. The reason is
* that form submission, depending on the authentication process result, will
* cause a redirection to a different page or to current login view. In both
* cases a new Flow UI will be created and the event will potentially be
* forwarded to a dismissed UI. In addition, if the HTTP session ID is changed
* as a consequence of the authentication process, the server may respond to the
* login event with a session expiration error, cause a client resynchronization
* that can in turn cancel a potential redirect issued by the form submission.
*
* @author Vaadin Ltd
*/
Expand All @@ -54,19 +68,15 @@ public abstract class AbstractLogin extends Component implements HasEnabled {

private static final PropertyChangeListener NO_OP = event -> {
};
private Registration registration;

/**
* Initializes a new AbstractLogin with a default localization.
*/
public AbstractLogin() {
this(LoginI18n.createDefault());
getElement().addPropertyChangeListener(PROP_DISABLED, LOGIN_EVENT,
NO_OP);
getElement().setProperty("_preventAutoEnable", true);
addLoginListener(e -> {
setEnabled(false);
setError(false);
});
registerDefaultLoginListener();
}

/**
Expand All @@ -79,15 +89,43 @@ public AbstractLogin(LoginI18n i18n) {
setI18n(i18n);
}

private void registerDefaultLoginListener() {
DomListenerRegistration disabledPropertyRegistration = getElement()
.addPropertyChangeListener(PROP_DISABLED, LOGIN_EVENT, NO_OP);
Registration loginListenerRegistration = addLoginListener(e -> {
setEnabled(false);
setError(false);
});
this.registration = Registration.combine(disabledPropertyRegistration,
loginListenerRegistration);
}

/**
* Sets the path where to send the form-data when a form is submitted. Once
* action is defined a {@link AbstractLogin.LoginEvent} is not fired
* anymore.
* <p>
* The {@code action} attribute should not be used together with login
* listeners added with {@link #addLoginListener(ComponentEventListener)}.
* See class Javadoc for more information.
*
* @see #getAction()
* @see #addLoginListener(ComponentEventListener)
*/
public void setAction(String action) {
getElement().setProperty(PROP_ACTION, action);
if (action == null) {
getElement().removeProperty(PROP_ACTION);
if (registration == null) {
registerDefaultLoginListener();
}
} else {
getElement().setProperty(PROP_ACTION, action);
if (registration != null) {
registration.remove();
registration = null;
}
warnIfActionAndLoginListenerUsedTogether();
}
}

/**
Expand All @@ -105,11 +143,10 @@ public String getAction() {
*
* Calling this method with {@code true} will also enable the component.
*
* @see #isError()
*
* @param error
* {@code true} to show the error message and enable component
* for next login attempt, {@code false} to hide an error
* @see #isError()
*/
public void setError(boolean error) {
if (error) {
Expand All @@ -132,10 +169,9 @@ public boolean isError() {
* Sets whether to show or hide the forgot password button. The button is
* visible by default
*
* @see #isForgotPasswordButtonVisible()
*
* @param forgotPasswordButtonVisible
* whether to display or hide the button
* @see #isForgotPasswordButtonVisible()
*/
public void setForgotPasswordButtonVisible(
boolean forgotPasswordButtonVisible) {
Expand Down Expand Up @@ -207,11 +243,19 @@ public void showErrorMessage(String title, String message) {
}

/**
* Adds `login` event listener
* Adds `login` event listener.
* <p>
* Login listeners should not be used together with the {@code action}
* attribute. See class Javadoc for more information.
*
* @see #setAction(String)
*/
public Registration addLoginListener(
ComponentEventListener<LoginEvent> listener) {
return ComponentUtil.addListener(this, LoginEvent.class, listener);
Registration registration = ComponentUtil.addListener(this,
LoginEvent.class, listener);
warnIfActionAndLoginListenerUsedTogether();
return registration;
}

/**
Expand Down Expand Up @@ -267,4 +311,12 @@ public ForgotPasswordEvent(AbstractLogin source, boolean fromClient) {
public void onEnabledStateChanged(boolean enabled) {
getElement().setProperty(PROP_DISABLED, !enabled);
}

private void warnIfActionAndLoginListenerUsedTogether() {
if (getElement().hasProperty(PROP_ACTION)
&& !getListeners(LoginEvent.class).isEmpty()) {
LoggerFactory.getLogger(getClass()).warn(
"Using the action attribute together with login listeners is discouraged. See the AbstractLogin JavaDoc for more information. This may throw an exception in the future.");
}
}
}
85 changes: 85 additions & 0 deletions ...parent/vaadin-login-flow/src/test/java/com/vaadin/flow/component/login/LoginFormTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,37 @@

import java.util.concurrent.atomic.AtomicInteger;

import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.vaadin.flow.component.ComponentUtil;
import com.vaadin.flow.component.HasStyle;
import com.vaadin.flow.shared.Registration;

public class LoginFormTest {

private MockedStatic<LoggerFactory> mockedLoggerFactory;
private Logger mockLogger;

@Before
public void setUp() {
mockedLoggerFactory = Mockito.mockStatic(LoggerFactory.class);
mockLogger = Mockito.mock(Logger.class);
mockedLoggerFactory.when(() -> LoggerFactory.getLogger(LoginForm.class))
.thenReturn(mockLogger);
}

@After
public void tearDown() {
mockedLoggerFactory.close();
}

@Test
public void onForgotPasswordEvent() {
LoginForm loginFormComponent = new LoginForm();
Expand Down Expand Up @@ -102,4 +125,66 @@ public void showErrorMessage_preservesExistingI18n() {
Assert.assertEquals("Custom username",
form.getI18n().getForm().getUsername());
}

@Test
public void addLoginListeners_setAction_logsWarning() {
final LoginForm form = new LoginForm();
Registration registration1 = form.addLoginListener(ev -> {
});
Registration registration2 = form.addLoginListener(ev -> {
});

form.setAction("login1");
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

registration1.remove();
form.setAction("login2");
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

registration2.remove();
form.setAction("login3");
Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.anyString());
}

@Test
public void setAction_addLoginListener_logsWarning() {
final LoginForm form = new LoginForm();
form.setAction("login");
form.addLoginListener(ev -> {
});
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

form.setAction(null);
form.addLoginListener(ev -> {
});
Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.anyString());
}

@Test
public void setAction_unregisterAndRegisterDefaultLoginListener() {
final LoginForm form = new LoginForm();
form.setAction("login");
form.setError(true);

ComponentUtil.fireEvent(form, new AbstractLogin.LoginEvent(form, true,
"username", "password"));
Assert.assertTrue(
"Expected form not being disabled by default listener",
form.isEnabled());
Assert.assertTrue(
"Expected error status not being reset by default listener",
form.isError());

form.setAction(null);
ComponentUtil.fireEvent(form, new AbstractLogin.LoginEvent(form, true,
"username", "password"));
Assert.assertFalse("Expected form being disabled by default listener",
form.isEnabled());
Assert.assertFalse(
"Expected error status being reset by default listener",
form.isError());
}
}
88 changes: 88 additions & 0 deletions ...ent/vaadin-login-flow/src/test/java/com/vaadin/flow/component/login/LoginOverlayTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,36 @@
*/
package com.vaadin.flow.component.login;

import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.vaadin.flow.component.ComponentUtil;
import com.vaadin.flow.shared.Registration;

public class LoginOverlayTest {
private MockedStatic<LoggerFactory> mockedLoggerFactory;
private Logger mockLogger;

@Before
public void setUp() {
mockedLoggerFactory = Mockito.mockStatic(LoggerFactory.class);
mockLogger = Mockito.mock(Logger.class);
mockedLoggerFactory
.when(() -> LoggerFactory.getLogger(LoginOverlay.class))
.thenReturn(mockLogger);
}

@After
public void tearDown() {
mockedLoggerFactory.close();
}

@Test
public void showErrorMessage_fromNullI18n() {
final LoginOverlay overlay = new LoginOverlay(null);
Expand Down Expand Up @@ -57,4 +83,66 @@ public void showErrorMessage_preservesExistingI18n() {
Assert.assertEquals("Custom username",
overlay.getI18n().getForm().getUsername());
}

@Test
public void addLoginListeners_setAction_logsWarning() {
final LoginOverlay overlay = new LoginOverlay();
Registration registration1 = overlay.addLoginListener(ev -> {
});
Registration registration2 = overlay.addLoginListener(ev -> {
});

overlay.setAction("login1");
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

registration1.remove();
overlay.setAction("login2");
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

registration2.remove();
overlay.setAction("login3");
Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.anyString());
}

@Test
public void setAction_addLoginListener_logsWarning() {
final LoginOverlay overlay = new LoginOverlay();
overlay.setAction("login");
overlay.addLoginListener(ev -> {
});
Mockito.verify(mockLogger, Mockito.times(1)).warn(Mockito.anyString());
Mockito.reset(mockLogger);

overlay.setAction(null);
overlay.addLoginListener(ev -> {
});
Mockito.verify(mockLogger, Mockito.never()).warn(Mockito.anyString());
}

@Test
public void setAction_unregisterAndRegisterDefaultLoginListener() {
final LoginOverlay overlay = new LoginOverlay();
overlay.setAction("login");
overlay.setError(true);

ComponentUtil.fireEvent(overlay, new AbstractLogin.LoginEvent(overlay,
true, "username", "password"));
Assert.assertTrue(
"Expected form not being disabled by default listener",
overlay.isEnabled());
Assert.assertTrue(
"Expected error status not being reset by default listener",
overlay.isError());

overlay.setAction(null);
ComponentUtil.fireEvent(overlay, new AbstractLogin.LoginEvent(overlay,
true, "username", "password"));
Assert.assertFalse("Expected form being disabled by default listener",
overlay.isEnabled());
Assert.assertFalse(
"Expected error status being reset by default listener",
overlay.isError());
}
}

0 comments on commit 7e54c05

Please sign in to comment.