fix: Only update user.last_login on successful authentication (#1775)

* Only update user.last_login if they successfully authenticated

* Update docstrings for update_user_auth_stat

* Add tests for BaseSecurityManager.update_user_auth_stat

Co-authored-by: Daniel Vaz Gaspar <[email protected]>

Author: blag

flask_appbuilder/security/manager.py

@@ -820,23 +820,28 @@ def reset_password(self, userid, password):
 
     def update_user_auth_stat(self, user, success=True):
         """
-        Update authentication successful to user.
+        Update user authentication stats upon successful/unsuccessful
+        authentication attempts.
 
         :param user:
-            The authenticated user model
+            The identified (but possibly not successfully authenticated) user
+            model
         :param success:
-            Default to true, if false increments fail_login_count on user model
+        :type success: bool or None
+            Defaults to true, if true increments login_count, updates
+            last_login, and resets fail_login_count to 0, if false increments
+            fail_login_count on user model.
         """
         if not user.login_count:
             user.login_count = 0
         if not user.fail_login_count:
             user.fail_login_count = 0
         if success:
             user.login_count += 1
+            user.last_login = datetime.datetime.now()
             user.fail_login_count = 0
         else:
             user.fail_login_count += 1
-        user.last_login = datetime.datetime.now()
         self.update_user(user)
 
     def auth_user_db(self, username, password):

flask_appbuilder/tests/security/test_base_security_manager.py

@@ -0,0 +1,69 @@
+import datetime
+import unittest
+from unittest.mock import MagicMock, patch
+
+from flask_appbuilder.security.manager import BaseSecurityManager
+
+
+@patch.object(BaseSecurityManager, "update_user")
+@patch.object(BaseSecurityManager, "__init__", return_value=None)
+class BaseSecurityManagerUpdateUserAuthStatTestCase(unittest.TestCase):
+    def test_first_successful_auth(self, mock1, mock2):
+        bsm = BaseSecurityManager()
+
+        user_mock = MagicMock()
+        user_mock.login_count = None
+        user_mock.fail_login_count = None
+        user_mock.last_login = None
+
+        bsm.update_user_auth_stat(user_mock, success=True)
+
+        self.assertEqual(user_mock.login_count, 1)
+        self.assertEqual(user_mock.fail_login_count, 0)
+        self.assertEqual(type(user_mock.last_login), datetime.datetime)
+        self.assertTrue(bsm.update_user.called_once)
+
+    def test_first_unsuccessful_auth(self, mock1, mock2):
+        bsm = BaseSecurityManager()
+
+        user_mock = MagicMock()
+        user_mock.login_count = None
+        user_mock.fail_login_count = None
+        user_mock.last_login = None
+
+        bsm.update_user_auth_stat(user_mock, success=False)
+
+        self.assertEqual(user_mock.login_count, 0)
+        self.assertEqual(user_mock.fail_login_count, 1)
+        self.assertEqual(user_mock.last_login, None)
+        self.assertTrue(bsm.update_user.called_once)
+
+    def test_subsequent_successful_auth(self, mock1, mock2):
+        bsm = BaseSecurityManager()
+
+        user_mock = MagicMock()
+        user_mock.login_count = 5
+        user_mock.fail_login_count = 9
+        user_mock.last_login = None
+
+        bsm.update_user_auth_stat(user_mock, success=True)
+
+        self.assertEqual(user_mock.login_count, 6)
+        self.assertEqual(user_mock.fail_login_count, 0)
+        self.assertEqual(type(user_mock.last_login), datetime.datetime)
+        self.assertTrue(bsm.update_user.called_once)
+
+    def test_subsequent_unsuccessful_auth(self, mock1, mock2):
+        bsm = BaseSecurityManager()
+
+        user_mock = MagicMock()
+        user_mock.login_count = 5
+        user_mock.fail_login_count = 9
+        user_mock.last_login = None
+
+        bsm.update_user_auth_stat(user_mock, success=False)
+
+        self.assertEqual(user_mock.login_count, 5)
+        self.assertEqual(user_mock.fail_login_count, 10)
+        self.assertEqual(user_mock.last_login, None)
+        self.assertTrue(bsm.update_user.called_once)