From c5e453ec228663381158b4ba7e8850a63624a1ac Mon Sep 17 00:00:00 2001 From: Dosenpfand Date: Wed, 22 Feb 2023 17:19:40 +0100 Subject: [PATCH] fix: Save next URL on failed login attempt (#1936) * Save next URL on failed login attempt * Save next URL also for LDAP * Add test for DB auth * Add test for LDAP auth * Fix formatting * Fix formatting --------- Co-authored-by: Daniel Vaz Gaspar --- flask_appbuilder/base.py | 5 ++++ flask_appbuilder/security/views.py | 12 ++++---- .../tests/security/test_auth_ldap.py | 30 ++++++++++++++++++- .../tests/security/test_mvc_security.py | 22 +++++++++++++- 4 files changed, 61 insertions(+), 8 deletions(-) diff --git a/flask_appbuilder/base.py b/flask_appbuilder/base.py index 8776719f4c..5974c49e62 100644 --- a/flask_appbuilder/base.py +++ b/flask_appbuilder/base.py @@ -610,6 +610,11 @@ def security_converge(self, dry: bool = False) -> Dict[str, Any]: return {} return self.sm.security_converge(self.baseviews, self.menu.menu, dry) + def get_url_for_login_with(self, next_url: str = None) -> str: + if self.sm.auth_view is None: + return "" + return url_for("%s.%s" % (self.sm.auth_view.endpoint, "login"), next=next_url) + @property def get_url_for_login(self) -> str: if self.sm.auth_view is None: diff --git a/flask_appbuilder/security/views.py b/flask_appbuilder/security/views.py index 3fe0b0b32f..a4905f218e 100644 --- a/flask_appbuilder/security/views.py +++ b/flask_appbuilder/security/views.py @@ -514,15 +514,15 @@ def login(self): return redirect(self.appbuilder.get_url_for_index) form = LoginForm_db() if form.validate_on_submit(): + next_url = get_safe_redirect(request.args.get("next", "")) user = self.appbuilder.sm.auth_user_db( form.username.data, form.password.data ) if not user: flash(as_unicode(self.invalid_login_message), "warning") - return redirect(self.appbuilder.get_url_for_login) + return redirect(self.appbuilder.get_url_for_login_with(next_url)) login_user(user, remember=False) - next_url = request.args.get("next", "") - return redirect(get_safe_redirect(next_url)) + return redirect(next_url) return self.render_template( self.login_template, title=self.title, form=form, appbuilder=self.appbuilder ) @@ -537,15 +537,15 @@ def login(self): return redirect(self.appbuilder.get_url_for_index) form = LoginForm_db() if form.validate_on_submit(): + next_url = get_safe_redirect(request.args.get("next", "")) user = self.appbuilder.sm.auth_user_ldap( form.username.data, form.password.data ) if not user: flash(as_unicode(self.invalid_login_message), "warning") - return redirect(self.appbuilder.get_url_for_login) + return redirect(self.appbuilder.get_url_for_login_with(next_url)) login_user(user, remember=False) - next_url = request.args.get("next", "") - return redirect(get_safe_redirect(next_url)) + return redirect(next_url) return self.render_template( self.login_template, title=self.title, form=form, appbuilder=self.appbuilder ) diff --git a/flask_appbuilder/tests/security/test_auth_ldap.py b/flask_appbuilder/tests/security/test_auth_ldap.py index b2b8fdb8e9..28f934ffc1 100644 --- a/flask_appbuilder/tests/security/test_auth_ldap.py +++ b/flask_appbuilder/tests/security/test_auth_ldap.py @@ -10,7 +10,6 @@ import ldap from mockldap import MockLdap - from ..const import USERNAME_ADMIN, USERNAME_READONLY logging.basicConfig(format="%(asctime)s:%(levelname)s:%(name)s:%(message)s") @@ -1187,3 +1186,32 @@ def test__indirect_bind__registered__multi_role__with_role_sync(self): self.call_bind_alice, ], ) + + def test_login_failed_keep_next_url(self): + """ + LDAP: Keeping next url after failed login attempt + """ + + self.app.config["AUTH_LDAP_SEARCH"] = "ou=users,o=test" + self.app.config["AUTH_LDAP_USERNAME_FORMAT"] = "uid=%s,ou=users,o=test" + self.app.config["AUTH_USER_REGISTRATION"] = True + self.app.config["AUTH_USER_REGISTRATION_ROLE"] = "Public" + self.app.config["WTF_CSRF_ENABLED"] = False + self.app.config["SECRET_KEY"] = "thisismyscretkey" + + self.appbuilder = AppBuilder(self.app, self.db.session) + client = self.app.test_client() + client.get("/logout/") + + response = client.post( + "/login/?next=/users/userinfo/", + data=dict(username="natalie", password="wrong_natalie_password"), + follow_redirects=False, + ) + response = client.post( + response.location, + data=dict(username="natalie", password="natalie_password"), + follow_redirects=False, + ) + + assert response.location == "http://localhost/users/userinfo/" diff --git a/flask_appbuilder/tests/security/test_mvc_security.py b/flask_appbuilder/tests/security/test_mvc_security.py index a71be5ffa4..8e8fc0b5f3 100644 --- a/flask_appbuilder/tests/security/test_mvc_security.py +++ b/flask_appbuilder/tests/security/test_mvc_security.py @@ -203,11 +203,31 @@ def test_db_login_invalid_control_characters_next_url(self): self.client, USERNAME_ADMIN, PASSWORD_ADMIN, - next_url=u"\u0001" + "sample.com", + next_url="\u0001" + "sample.com", follow_redirects=False, ) assert response.location == "http://localhost/" + def test_db_login_failed_keep_next_url(self): + """ + Test Security Keeping next url after failed login attempt + """ + self.browser_logout(self.client) + response = self.browser_login( + self.client, + USERNAME_ADMIN, + f"wrong_{PASSWORD_ADMIN}", + next_url="/users/list/", + follow_redirects=False, + ) + response = self.client.post( + response.location, + data=dict(username=USERNAME_ADMIN, password=PASSWORD_ADMIN), + follow_redirects=False, + ) + + assert response.location == "http://localhost/users/list/" + def test_auth_builtin_roles(self): """ Test Security builtin roles readonly