From e2b744c258ff62ece9d5ac7172c3b4644ff4c2fe Mon Sep 17 00:00:00 2001
From: blag <blag@users.noreply.github.com>
Date: Thu, 13 Jan 2022 02:54:08 -0800
Subject: [PATCH] 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 <danielvazgaspar@gmail.com>
---
 flask_appbuilder/security/manager.py          | 13 ++--
 .../security/test_base_security_manager.py    | 69 +++++++++++++++++++
 2 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 flask_appbuilder/tests/security/test_base_security_manager.py

diff --git a/flask_appbuilder/security/manager.py b/flask_appbuilder/security/manager.py
index ada7865ca..186215b74 100644
--- a/flask_appbuilder/security/manager.py
+++ b/flask_appbuilder/security/manager.py
@@ -820,12 +820,17 @@ 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
@@ -833,10 +838,10 @@ def update_user_auth_stat(self, user, success=True):
             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):
diff --git a/flask_appbuilder/tests/security/test_base_security_manager.py b/flask_appbuilder/tests/security/test_base_security_manager.py
new file mode 100644
index 000000000..736b0b88d
--- /dev/null
+++ b/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)