From 9123fcb376355bed90db1fe865891bb9fd4a54b3 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Thu, 8 Feb 2024 13:45:03 +0000 Subject: [PATCH 1/2] fix: don't load inactive users with sessions (#2192) * fix: don't load inactive users with sessions * add test * fix test --- flask_appbuilder/security/manager.py | 11 ++++++---- tests/base.py | 3 ++- tests/security/test_mvc_security.py | 31 ++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/flask_appbuilder/security/manager.py b/flask_appbuilder/security/manager.py index 8f86b7ee51..df750d3765 100644 --- a/flask_appbuilder/security/manager.py +++ b/flask_appbuilder/security/manager.py @@ -2088,14 +2088,17 @@ def import_roles(self, path: str) -> None: raise NotImplementedError def load_user(self, pk): - return self.get_user_by_id(int(pk)) + user = self.get_user_by_id(int(pk)) + if user.is_active: + return user def load_user_jwt(self, _jwt_header, jwt_data): identity = jwt_data["sub"] user = self.load_user(identity) - # Set flask g.user to JWT user, we can't do it on before request - g.user = user - return user + if user.is_active: + # Set flask g.user to JWT user, we can't do it on before request + g.user = user + return user @staticmethod def before_request(): diff --git a/tests/base.py b/tests/base.py index 186b3c205a..8fda65a9b2 100644 --- a/tests/base.py +++ b/tests/base.py @@ -14,6 +14,7 @@ API_SECURITY_USERNAME_KEY, API_SECURITY_VERSION, ) +from flask_appbuilder.security.sqla.models import User from hiro import Timeline import jinja2 from tests.const import ( @@ -130,7 +131,7 @@ def create_user( last_name="user", email="admin@fab.org", role_names=None, - ): + ) -> User: user = appbuilder.sm.find_user(username=username) if user: appbuilder.session.delete(user) diff --git a/tests/security/test_mvc_security.py b/tests/security/test_mvc_security.py index 0ef3261ee2..d7adea63fc 100644 --- a/tests/security/test_mvc_security.py +++ b/tests/security/test_mvc_security.py @@ -93,6 +93,37 @@ def test_sec_login(self): data = rv.data.decode("utf-8") self.assertIn(INVALID_LOGIN_STRING, data) + def test_login_invalid_user(self): + """ + Test Security Login, Logout, invalid login, invalid access + """ + test_username = "testuser" + test_password = "password" + test_user = self.create_user( + self.appbuilder, + test_username, + test_password, + "Admin", + "user", + "user", + "testuser@fab.org", + ) + # Login and list with admin + self.browser_login(self.client, test_username, test_password) + rv = self.client.get("/model1view/list/") + self.assertEqual(rv.status_code, 200) + + # Using the same session make sure the user is not allowed to access when + # the user is deactivated + test_user.active = False + self.db.session.merge(test_user) + self.db.session.commit() + rv = self.client.get("/model1view/list/") + self.assertEqual(rv.status_code, 302) + + self.db.session.delete(test_user) + self.db.session.commit() + def test_db_login_no_next_url(self): """ Test Security no next URL From 6f00efcce7d6d88a3e957910581c3ac1de19a301 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Thu, 8 Feb 2024 15:39:10 +0000 Subject: [PATCH 2/2] feat: AUTH_REMOTE_USER_ENV_VAR config key for auth REMOTE_USER type (#2193) --- docs/config.rst | 5 ++ flask_appbuilder/security/manager.py | 7 +++ flask_appbuilder/security/views.py | 2 +- tests/security/test_auth_oauth.py | 5 -- tests/security/test_auth_remote_user.py | 75 +++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 tests/security/test_auth_remote_user.py diff --git a/docs/config.rst b/docs/config.rst index faa060b3ab..fc531ff45b 100755 --- a/docs/config.rst +++ b/docs/config.rst @@ -46,6 +46,11 @@ Use config.py to configure the following parameters. By default it will use SQLL | | Requires ``jmespath`` to be installed. | | | | See :ref:`jmespath-examples` for examples | | +----------------------------------------+--------------------------------------------+-----------+ +| AUTH_REMOTE_USER_ENV_VAR | When using AUTH_TYPE = AUTH_REMOTE_USER | No | +| | Optionally set the wsgi environment var | | +| | that holds the current logged in user | | +| | Default: REMOTE_USER | | ++----------------------------------------+--------------------------------------------+-----------+ | AUTH_ROLES_SYNC_AT_LOGIN | Sets if user's roles are replaced each | No | | | login with those received from LDAP/OAUTH | | | | Default: False | | diff --git a/flask_appbuilder/security/manager.py b/flask_appbuilder/security/manager.py index df750d3765..4f22aaa5af 100644 --- a/flask_appbuilder/security/manager.py +++ b/flask_appbuilder/security/manager.py @@ -258,6 +258,9 @@ def __init__(self, appbuilder): app.config.setdefault("AUTH_LDAP_LASTNAME_FIELD", "sn") app.config.setdefault("AUTH_LDAP_EMAIL_FIELD", "mail") + if self.auth_type == AUTH_REMOTE_USER: + app.config.setdefault("AUTH_REMOTE_USER_ENV_VAR", "REMOTE_USER") + # Rate limiting app.config.setdefault("AUTH_RATE_LIMITED", False) app.config.setdefault("AUTH_RATE_LIMIT", "10 per 20 second") @@ -415,6 +418,10 @@ def auth_user_registration_role(self) -> str: def auth_user_registration_role_jmespath(self) -> str: return self.appbuilder.get_app.config["AUTH_USER_REGISTRATION_ROLE_JMESPATH"] + @property + def auth_remote_user_env_var(self) -> str: + return self.appbuilder.get_app.config["AUTH_REMOTE_USER_ENV_VAR"] + @property def auth_roles_mapping(self) -> Dict[str, List[str]]: return self.appbuilder.get_app.config["AUTH_ROLES_MAPPING"] diff --git a/flask_appbuilder/security/views.py b/flask_appbuilder/security/views.py index c38e23f395..1787040d61 100644 --- a/flask_appbuilder/security/views.py +++ b/flask_appbuilder/security/views.py @@ -716,7 +716,7 @@ class AuthRemoteUserView(AuthView): @expose("/login/") def login(self) -> WerkzeugResponse: - username = request.environ.get("REMOTE_USER") + username = request.environ.get(self.appbuilder.sm.auth_remote_user_env_var) if g.user is not None and g.user.is_authenticated: next_url = request.args.get("next", "") return redirect(get_safe_redirect(next_url)) diff --git a/tests/security/test_auth_oauth.py b/tests/security/test_auth_oauth.py index 799ce46a6d..714c567775 100644 --- a/tests/security/test_auth_oauth.py +++ b/tests/security/test_auth_oauth.py @@ -1,4 +1,3 @@ -import logging import os import unittest from unittest.mock import MagicMock @@ -12,10 +11,6 @@ from tests.const import USERNAME_ADMIN, USERNAME_READONLY from tests.fixtures.users import create_default_users -logging.basicConfig(format="%(asctime)s:%(levelname)s:%(name)s:%(message)s") -logging.getLogger().setLevel(logging.DEBUG) -log = logging.getLogger(__name__) - class OAuthRegistrationRoleTestCase(unittest.TestCase): def setUp(self): diff --git a/tests/security/test_auth_remote_user.py b/tests/security/test_auth_remote_user.py new file mode 100644 index 0000000000..ff5a851e60 --- /dev/null +++ b/tests/security/test_auth_remote_user.py @@ -0,0 +1,75 @@ +import unittest + +from flask import Flask +from flask_appbuilder import AppBuilder, SQLA +from flask_appbuilder.const import AUTH_REMOTE_USER + + +class AuthRemoteUserTestCase(unittest.TestCase): + def setUp(self): + # start Flask + self.app = Flask(__name__) + self.app.config["AUTH_TYPE"] = AUTH_REMOTE_USER + + # start Database + self.db = SQLA(self.app) + + def tearDown(self): + # Remove test user + user_alice = self.appbuilder.sm.find_user("alice") + if user_alice: + self.db.session.delete(user_alice) + self.db.session.commit() + + # stop Flask + self.app = None + + # stop Flask-AppBuilder + self.appbuilder = None + + # stop Database + self.db.session.remove() + self.db = None + + def test_unset_remote_user_env_var(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + sm = self.appbuilder.sm + + self.assertEqual(sm.auth_remote_user_env_var, "REMOTE_USER") + + def test_set_remote_user_env_var(self): + self.app.config["AUTH_REMOTE_USER_ENV_VAR"] = "HTTP_REMOTE_USER" + self.appbuilder = AppBuilder(self.app, self.db.session) + sm = self.appbuilder.sm + + self.assertEqual(sm.auth_remote_user_env_var, "HTTP_REMOTE_USER") + + def test_normal_login(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + sm = self.appbuilder.sm + + # register a user + _ = sm.add_user( + username="alice", + first_name="Alice", + last_name="Doe", + email="alice@example.com", + role=[], + ) + + self.assertTrue(sm.auth_user_remote_user("alice")) + + def test_inactive_user_login(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + sm = self.appbuilder.sm + + # register a user + alice_user = sm.add_user( + username="alice", + first_name="Alice", + last_name="Doe", + email="alice@example.com", + role=[], + ) + alice_user.active = False + self.assertFalse(sm.auth_user_remote_user("alice"))