This is an automated email from the ASF dual-hosted git repository. ephraimanierobi pushed a commit to branch v2-8-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 2a008cfb57cbb8831d3784fc8c0ae8ba13791c7a Author: Peter Debelak <pdebe...@gmail.com> AuthorDate: Mon Dec 4 17:16:45 2023 -0600 Update reset_user_sessions to work from either CLI or web (#36056) This updates the `reset_user_sessions` method to not class `flash` if it is not being run within the web context but to log the warning message instead. Without this change, `flask fab reset-password` would fail if a warning message needed to be printed. (cherry picked from commit 7ececfdb2183516d9a30195ffcd76632167119c5) --- .../auth/managers/fab/security_manager/override.py | 46 ++++++++++++---------- tests/www/views/test_views_custom_user_views.py | 36 ++++++++++++++++- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/airflow/auth/managers/fab/security_manager/override.py b/airflow/auth/managers/fab/security_manager/override.py index 3814036bb5..60de860091 100644 --- a/airflow/auth/managers/fab/security_manager/override.py +++ b/airflow/auth/managers/fab/security_manager/override.py @@ -28,7 +28,7 @@ from typing import TYPE_CHECKING, Any, Callable, Collection, Container, Iterable import jwt import re2 -from flask import flash, g, session +from flask import flash, g, has_request_context, session from flask_appbuilder import const from flask_appbuilder.const import ( AUTH_DB, @@ -476,17 +476,15 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): user_session_model = interface.sql_session_model num_sessions = session.query(user_session_model).count() if num_sessions > MAX_NUM_DATABASE_USER_SESSIONS: - flash( - Markup( - f"The old sessions for user {user.username} have <b>NOT</b> been deleted!<br>" - f"You have a lot ({num_sessions}) of user sessions in the 'SESSIONS' table in " - f"your database.<br> " - "This indicates that this deployment might have an automated API calls that create " - "and not reuse sessions.<br>You should consider reusing sessions or cleaning them " - "periodically using db clean.<br>" - "Make sure to reset password for the user again after cleaning the session table " - "to remove old sessions of the user." - ), + _cli_safe_flash( + f"The old sessions for user {user.username} have <b>NOT</b> been deleted!<br>" + f"You have a lot ({num_sessions}) of user sessions in the 'SESSIONS' table in " + f"your database.<br> " + "This indicates that this deployment might have an automated API calls that create " + "and not reuse sessions.<br>You should consider reusing sessions or cleaning them " + "periodically using db clean.<br>" + "Make sure to reset password for the user again after cleaning the session table " + "to remove old sessions of the user.", "warning", ) else: @@ -495,15 +493,13 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): if session_details.get("_user_id") == user.id: session.delete(s) else: - flash( - Markup( - "Since you are using `securecookie` session backend mechanism, we cannot prevent " - f"some old sessions for user {user.username} to be reused.<br> If you want to make sure " - "that the user is logged out from all sessions, you should consider using " - "`database` session backend mechanism.<br> You can also change the 'secret_key` " - "webserver configuration for all your webserver instances and restart the webserver. " - "This however will logout all users from all sessions." - ), + _cli_safe_flash( + "Since you are using `securecookie` session backend mechanism, we cannot prevent " + f"some old sessions for user {user.username} to be reused.<br> If you want to make sure " + "that the user is logged out from all sessions, you should consider using " + "`database` session backend mechanism.<br> You can also change the 'secret_key` " + "webserver configuration for all your webserver instances and restart the webserver. " + "This however will logout all users from all sessions.", "warning", ) @@ -2666,3 +2662,11 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): ).one() return dm.root_dag_id or dm.dag_id return dag_id + + +def _cli_safe_flash(text: str, level: str) -> None: + """Shows a flash in a web context or prints a message if not.""" + if has_request_context(): + flash(Markup(text), level) + else: + getattr(log, level)(text.replace("<br>", "\n").replace("<b>", "*").replace("</b>", "*")) diff --git a/tests/www/views/test_views_custom_user_views.py b/tests/www/views/test_views_custom_user_views.py index b4b2166a8f..1e2e1ab995 100644 --- a/tests/www/views/test_views_custom_user_views.py +++ b/tests/www/views/test_views_custom_user_views.py @@ -248,8 +248,9 @@ class TestResetUserSessions: return self.db.session.query(self.model).filter(self.model.session_id == session_id).scalar() @mock.patch("airflow.auth.managers.fab.security_manager.override.flash") + @mock.patch("airflow.auth.managers.fab.security_manager.override.has_request_context", return_value=True) @mock.patch("airflow.auth.managers.fab.security_manager.override.MAX_NUM_DATABASE_USER_SESSIONS", 1) - def test_refuse_delete(self, flash_mock): + def test_refuse_delete(self, _mock_has_context, flash_mock): self.create_user_db_session("session_id_1", timedelta(days=1), self.user_1.id) self.create_user_db_session("session_id_2", timedelta(days=1), self.user_2.id) self.db.session.commit() @@ -268,7 +269,8 @@ class TestResetUserSessions: assert self.get_session_by_id("session_id_2") is not None @mock.patch("airflow.auth.managers.fab.security_manager.override.flash") - def test_warn_securecookie(self, flash_mock): + @mock.patch("airflow.auth.managers.fab.security_manager.override.has_request_context", return_value=True) + def test_warn_securecookie(self, _mock_has_context, flash_mock): self.app.session_interface = SecureCookieSessionInterface() self.security_manager.reset_password(self.user_1.id, "new_password") assert flash_mock.called @@ -276,3 +278,33 @@ class TestResetUserSessions: "Since you are using `securecookie` session backend mechanism, we cannot" in flash_mock.call_args[0][0] ) + + @mock.patch("airflow.auth.managers.fab.security_manager.override.log") + @mock.patch("airflow.auth.managers.fab.security_manager.override.MAX_NUM_DATABASE_USER_SESSIONS", 1) + def test_refuse_delete_cli(self, log_mock): + self.create_user_db_session("session_id_1", timedelta(days=1), self.user_1.id) + self.create_user_db_session("session_id_2", timedelta(days=1), self.user_2.id) + self.db.session.commit() + self.db.session.flush() + assert self.db.session.query(self.model).count() == 2 + assert self.get_session_by_id("session_id_1") is not None + assert self.get_session_by_id("session_id_2") is not None + self.security_manager.reset_password(self.user_1.id, "new_password") + assert log_mock.warning.called + assert ( + "The old sessions for user user_to_delete_1 have *NOT* been deleted!\n" + in log_mock.warning.call_args[0][0] + ) + assert self.db.session.query(self.model).count() == 2 + assert self.get_session_by_id("session_id_1") is not None + assert self.get_session_by_id("session_id_2") is not None + + @mock.patch("airflow.auth.managers.fab.security_manager.override.log") + def test_warn_securecookie_cli(self, log_mock): + self.app.session_interface = SecureCookieSessionInterface() + self.security_manager.reset_password(self.user_1.id, "new_password") + assert log_mock.warning.called + assert ( + "Since you are using `securecookie` session backend mechanism, we cannot" + in log_mock.warning.call_args[0][0] + )