From 81be47486c299298058fd3093124eff717418743 Mon Sep 17 00:00:00 2001
From: Joseph Schorr <joseph.schorr@coreos.com>
Date: Thu, 4 Jan 2018 15:20:25 -0500
Subject: [PATCH] Reduce lock contention on invalid user login calls

Fixes https://jira.coreos.com/browse/QS-110
---
 data/model/user.py | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/data/model/user.py b/data/model/user.py
index fdd17e445..b4fc6cf56 100644
--- a/data/model/user.py
+++ b/data/model/user.py
@@ -660,20 +660,28 @@ def get_matching_users(username_prefix, robot_namespace=None, organization=None,
 
 
 def verify_user(username_or_email, password):
+  """ Verifies that the given username/email + password pair is valid. If the username or e-mail
+      address is invalid, returns None. If the password specified does not match for the given user,
+      either returns None or raises TooManyLoginAttemptsException if there have been too many
+      invalid login attempts. Returns the user object if the login was valid.
+  """
+
   # Make sure we didn't get any unicode for the username.
   try:
     str(username_or_email)
   except ValueError:
     return None
 
+  # Fetch the user with the matching username or e-mail address.
   try:
-    fetched = User.get((User.username == username_or_email) |
-                       (User.email == username_or_email))
+    fetched = User.get((User.username == username_or_email) | (User.email == username_or_email))
   except User.DoesNotExist:
     return None
 
+  # If the user has any invalid login attempts, check to see if we are within the exponential
+  # backoff window for the user. If so, we raise an exception indicating that the user cannot
+  # login.
   now = datetime.utcnow()
-
   if fetched.invalid_login_attempts > 0:
     can_retry_at = exponential_backoff(fetched.invalid_login_attempts, EXPONENTIAL_BACKOFF_SCALE,
                                        fetched.last_invalid_login)
@@ -682,17 +690,25 @@ def verify_user(username_or_email, password):
       retry_after = can_retry_at - now
       raise TooManyLoginAttemptsException('Too many login attempts.', retry_after.total_seconds())
 
+  # Hash the given password and compare it to the specified password.
   if (fetched.password_hash and
       hash_password(password, fetched.password_hash) == fetched.password_hash):
-    if fetched.invalid_login_attempts > 0:
-      fetched.invalid_login_attempts = 0
-      fetched.save()
 
+    # If the user previously had any invalid login attempts, clear them out now.
+    if fetched.invalid_login_attempts > 0:
+      (User
+       .update(invalid_login_attempts=0)
+       .where(User.id == fetched.id)
+       .execute())
+
+    # Return the valid user.
     return fetched
 
-  fetched.invalid_login_attempts += 1
-  fetched.last_invalid_login = now
-  fetched.save()
+  # Otherwise, update the user's invalid login attempts.
+  (User
+   .update(invalid_login_attempts=User.invalid_login_attempts+1, last_invalid_login=now)
+   .where(User.id == fetched.id)
+   .execute())
 
   # We weren't able to authorize the user
   return None