From 83a8cfb9e4b6510eb81a7a9ddfcd98bc4a47e9ec Mon Sep 17 00:00:00 2001
From: Jake Dallimore <jake@moodle.com>
Date: Tue, 26 Apr 2022 16:52:20 +0800
Subject: [PATCH] MDL-73736 core_auth: Fix concurrency issue in
 login_attempt_failed()

This patch wraps the login_failed_count logic in a resource lock and
forces a user preferences cache reload. Each thread must wait for the
lock and must fetch the current count before incrementing it. This
ensures that login_failed_count is correct across threads and that the
lockout threshold is correctly honoured.

Co-Authored-By: Sujith Haridasan <sujith@moodle.com>
---
 lib/authlib.php | 62 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/lib/authlib.php b/lib/authlib.php
index 057bd81f02d..54ffa3c0443 100644
--- a/lib/authlib.php
+++ b/lib/authlib.php
@@ -877,6 +877,7 @@ function login_attempt_valid($user) {
 /**
  * To be called after failed user login.
  * @param stdClass $user
+ * @throws moodle_exception
  */
 function login_attempt_failed($user) {
     global $CFG;
@@ -888,30 +889,53 @@ function login_attempt_failed($user) {
         return;
     }
 
-    $count = get_user_preferences('login_failed_count', 0, $user);
-    $last = get_user_preferences('login_failed_last', 0, $user);
-    $sincescuccess = get_user_preferences('login_failed_count_since_success', $count, $user);
-    $sincescuccess = $sincescuccess + 1;
-    set_user_preference('login_failed_count_since_success', $sincescuccess, $user);
+    // Force user preferences cache reload to ensure the most up-to-date login_failed_count is fetched.
+    // This is perhaps overzealous but is the documented way of reloading the cache, as per the test method
+    // 'test_check_user_preferences_loaded'.
+    unset($user->preference);
 
-    if (empty($CFG->lockoutthreshold)) {
-        // No threshold means no lockout.
-        // Always unlock here, there might be some race conditions or leftovers when switching threshold.
-        login_unlock_account($user);
-        return;
-    }
+    $resource = 'user:' . $user->id;
+    $lockfactory = \core\lock\lock_config::get_lock_factory('core_failed_login_count_lock');
 
-    if (!empty($CFG->lockoutwindow) and time() - $last > $CFG->lockoutwindow) {
-        $count = 0;
-    }
+    // Get a new lock for the resource, waiting for it for a maximum of 10 seconds.
+    if ($lock = $lockfactory->get_lock($resource, 10)) {
+        try {
+            $count = get_user_preferences('login_failed_count', 0, $user);
+            $last = get_user_preferences('login_failed_last', 0, $user);
+            $sincescuccess = get_user_preferences('login_failed_count_since_success', $count, $user);
+            $sincescuccess = $sincescuccess + 1;
+            set_user_preference('login_failed_count_since_success', $sincescuccess, $user);
 
-    $count = $count+1;
+            if (empty($CFG->lockoutthreshold)) {
+                // No threshold means no lockout.
+                // Always unlock here, there might be some race conditions or leftovers when switching threshold.
+                login_unlock_account($user);
+                $lock->release();
+                return;
+            }
 
-    set_user_preference('login_failed_count', $count, $user);
-    set_user_preference('login_failed_last', time(), $user);
+            if (!empty($CFG->lockoutwindow) and time() - $last > $CFG->lockoutwindow) {
+                $count = 0;
+            }
 
-    if ($count >= $CFG->lockoutthreshold) {
-        login_lock_account($user);
+            $count = $count + 1;
+
+            set_user_preference('login_failed_count', $count, $user);
+            set_user_preference('login_failed_last', time(), $user);
+
+            if ($count >= $CFG->lockoutthreshold) {
+                login_lock_account($user);
+            }
+
+            // Release locks when we're done.
+            $lock->release();
+        } catch (Exception $e) {
+            // Always release the lock on a failure.
+            $lock->release();
+        }
+    } else {
+        // We did not get access to the resource in time, give up.
+        throw new moodle_exception('locktimeout');
     }
 }