fix: prevent setting both login listeners and form action

vaadin-login-flow-parent/vaadin-login-flow/src/main/java/com/vaadin/flow/component/login/AbstractLogin.java

@@ -24,6 +24,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;
@@ -40,6 +41,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
  */
@@ -54,19 +66,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();
     }
 
     /**
@@ -79,15 +87,47 @@ 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>
+     * </p>
+     * The {@code action} attribute cannot be set if login listeners have been
+     * previously added. See class Javadoc for more information.
      *
+     * @throws IllegalStateException
+     *             if any login listeners have been previously added.
      * @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 if (getListeners(LoginEvent.class).size() > 1) {
+            throw new IllegalStateException(
+                    "Action attribute cannot be set along with login listeners");
+        } else {
+            getElement().setProperty(PROP_ACTION, action);
+            if (registration != null) {
+                registration.remove();
+                registration = null;
+            }
+        }
     }
 
     /**
@@ -105,11 +145,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) {
@@ -132,10 +171,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) {
@@ -207,10 +245,23 @@ public void showErrorMessage(String title, String message) {
     }
 
     /**
-     * Adds `login` event listener
+     * Adds `login` event listener.
+     * <p>
+     * </p>
+     * Listeners cannot be added if {@code action} attribute has been previously
+     * set. See class Javadoc for more information.
+     *
+     * @throws IllegalStateException
+     *             if {@literal action} attribute has previously been set to a
+     *             not {@literal null} value.
+     * @see #setAction(String)
      */
     public Registration addLoginListener(
             ComponentEventListener<LoginEvent> listener) {
+        if (getElement().hasProperty(PROP_ACTION)) {
+            throw new IllegalStateException(
+                    "Login listener cannot work in combination with 'action' attribute");
+        }
         return ComponentUtil.addListener(this, LoginEvent.class, listener);
     }
 

vaadin-login-flow-parent/vaadin-login-flow/src/test/java/com/vaadin/flow/component/login/LoginFormTest.java

@@ -22,6 +22,7 @@
 
 import com.vaadin.flow.component.ComponentUtil;
 import com.vaadin.flow.component.HasStyle;
+import com.vaadin.flow.shared.Registration;
 
 public class LoginFormTest {
 
@@ -102,4 +103,73 @@ public void showErrorMessage_preservesExistingI18n() {
         Assert.assertEquals("Custom username",
                 form.getI18n().getForm().getUsername());
     }
+
+    @Test
+    public void setAction_customLoginListeners_throws() {
+        final LoginForm form = new LoginForm();
+        Registration registration1 = form.addLoginListener(ev -> {
+        });
+        Registration registration2 = form.addLoginListener(ev -> {
+        });
+        Assert.assertThrows(IllegalStateException.class,
+                () -> form.setAction("login"));
+
+        registration1.remove();
+        Assert.assertThrows(IllegalStateException.class,
+                () -> form.setAction("login"));
+
+        registration2.remove();
+        form.setAction("login");
+    }
+
+    @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());
+    }
+
+    @Test
+    public void setAction_nullValue_restoreDefaultListener() {
+        final LoginForm form = new LoginForm();
+        form.setAction("login");
+        form.setError(true);
+
+        ComponentUtil.fireEvent(form, new AbstractLogin.LoginEvent(form, true,
+                "username", "password"));
+        Assert.assertTrue(form.isEnabled());
+        Assert.assertTrue(form.isError());
+    }
+
+    @Test
+    public void addLoginListener_actionSet_throws() {
+        final LoginForm form = new LoginForm();
+        form.setAction("login");
+        Assert.assertThrows(IllegalStateException.class,
+                () -> form.addLoginListener(ev -> {
+                }));
+
+        form.setAction(null);
+        form.addLoginListener(ev -> {
+        });
+    }
+
 }

vaadin-login-flow-parent/vaadin-login-flow/src/test/java/com/vaadin/flow/component/login/LoginOverlayTest.java

@@ -18,6 +18,9 @@
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.vaadin.flow.component.ComponentUtil;
+import com.vaadin.flow.shared.Registration;
+
 public class LoginOverlayTest {
     @Test
     public void showErrorMessage_fromNullI18n() {
@@ -57,4 +60,73 @@ public void showErrorMessage_preservesExistingI18n() {
         Assert.assertEquals("Custom username",
                 overlay.getI18n().getForm().getUsername());
     }
+
+    @Test
+    public void setAction_customLoginListeners_throws() {
+        final LoginOverlay overlay = new LoginOverlay();
+        Registration registration1 = overlay.addLoginListener(ev -> {
+        });
+        Registration registration2 = overlay.addLoginListener(ev -> {
+        });
+        Assert.assertThrows(IllegalStateException.class,
+                () -> overlay.setAction("login"));
+
+        registration1.remove();
+        Assert.assertThrows(IllegalStateException.class,
+                () -> overlay.setAction("login"));
+
+        registration2.remove();
+        overlay.setAction("login");
+    }
+
+    @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());
+    }
+
+    @Test
+    public void setAction_nullValue_restoreDefaultListener() {
+        final LoginOverlay overlay = new LoginOverlay();
+        overlay.setAction("login");
+        overlay.setError(true);
+
+        ComponentUtil.fireEvent(overlay, new AbstractLogin.LoginEvent(overlay,
+                true, "username", "password"));
+        Assert.assertTrue(overlay.isEnabled());
+        Assert.assertTrue(overlay.isError());
+    }
+
+    @Test
+    public void addLoginListener_actionSet_throws() {
+        final LoginOverlay overlay = new LoginOverlay();
+        overlay.setAction("login");
+        Assert.assertThrows(IllegalStateException.class,
+                () -> overlay.addLoginListener(ev -> {
+                }));
+
+        overlay.setAction(null);
+        overlay.addLoginListener(ev -> {
+        });
+    }
+
 }