trjhtr

I'm building an app and I want to check if a user should be locked out after too many failed login attempts.

Here's 2 approaches to the problem, and my gut says that the second version is "cleaner", or adheres more to software engineering principles, (ex. SRP, modularity, etc.). Is this in fact true or am I mistaken?

Am I not seeing something in the first example, that would make it "cleaner" code?

Both versions are functionally similar.

This first version trades off modularity and the violation of the Law of Demeter in favor of less obfuscation (less helper functions) and less "spread out" logic.

 @property
 def is_locked_out(self):
 """ Check to see if user has been locked out due to too many failed login attempts. """
 if not self.user.is_staff:
 return (self.user_lockout_datetime is not None and timezone.now() <=
 self.user_lockout_datetime + timedelta(minutes=self.super_user.user_lockout_duration_mins))
 else:
 return False


 def reset_lockout_counters(self):
 """ Clears user's login_failure_count and user_lockout_datetime. """
 if (self.login_failure_count > 0):
 self.login_failure_count = 0

 if (self.user_lockout_datetime):
 self.user_lockout_datetime = None

 self.save()

 def increment_lockout_counters(self):
 """ Increment and then check to see if user is locked out, and raises an exception if so. Also manages user's
 lockout times. """
 self.login_failure_count += 1
 self.save(update_fields=['login_failure_count'])

 if super_user and not self.user.is_staff: # prevent breaking tests that may have User without linking super_user
 if (super_user.failed_login_count_threshold == 0 and super_user.user_lockout_duration_mins == 0):
 return # if super_user threshold are not set don't bother managing lockout times

 if self.is_locked_out:
 lockout_time_remaining = ((super_user.user_lockout_duration_mins) -
 (timezone.now().minute - self.user_lockout_datetime.minute))
 lockout_message = ('This account has been locked after too many failed login attempts. '
 'Please wait minute(s) and try again.'.format(lockout_time_remaining))

 if (self.super_user.user_lockout_duration_mins == 0 and
 self.super_user.failed_login_count_threshold > 0):
 lockout_message = ('This account has been locked after too many failed login attempts.'
 'Please contact Radial Analytics to reset the password')
 raise AccountValidationException(lockout_message, level='WARNING')

 # we check that self.login_failure_count > 0 because 0 % any number is always 0
 user_exceeded_count_threshold = (self.login_failure_count % super_user.failed_login_count_threshold == 0 and
 self.login_failure_count > 0)

 if (user_exceeded_count_threshold):
 self.user_lockout_datetime = timezone.now()
 self.save(update_fields=['user_lockout_datetime'])
 else:
 if (self.user_lockout_datetime and (self.user_lockout_datetime +
 timedelta(minutes=self.super_user.user_lockout_duration_mins) <
 timezone.now())):
 self.user_lockout_datetime = None
 self.save(update_fields=['user_lockout_datetime'])

And then this version makes everything more modular, but may obfuscate logic flow more.

 @property
 def is_locked_out(self):
 """ Check to see if user has been locked out due to too many failed login attempts. """

 if super_user: # Defensive `if` to avoid breaking legacy tests where `super_user` doesn't exist

 no_failure_count_threshold_set = super_user.failed_login_count_threshold == 0
 no_lockout_duration_set = super_user.user_lockout_duration_mins == 0
 if (no_failure_count_threshold_set and no_lockout_duration_set):
 return False # use case: super_user does not have failed login thresholds set

 is_within_lockout_time = False
 user_has_existing_lockout_time = (self.user_lockout_datetime and
 super_user.user_lockout_duration_mins > 0)
 if (user_has_existing_lockout_time):
 is_within_lockout_time = (self.user_lockout_datetime +
 timedelta(minutes=super_user.user_lockout_duration_mins) >
 timezone.now())

 # we only initiate a new lockout on every nth failed attempt
 is_fresh_lockout = (self.login_failure_count % super_user.failed_login_count_threshold) == 0
 login_fail_count_exceeded = is_fresh_lockout and (self.login_failure_count > 0)

 if (is_within_lockout_time): # use case: user is already within a lockout period
 return True
 if (login_fail_count_exceeded and self.user_lockout_datetime is None):
 return True # use case: user has passed count threshold and is not currently within a lockout period
 return False
 else:
 return False

 def increment_login_fail_count(self):
 """ Increases the failure count for a specific user by 1. """
 self.login_failure_count += 1
 self.save(update_fields=['login_failure_count'])

 def clear_login_fail_count(self):
 """ Clear the login failure count for specific user. Resets to 0. """
 if (self.login_failure_count > 0):
 self.login_failure_count = 0
 self.save(update_fields=['login_failure_count'])

 def initiate_fresh_lockout(self):
 user_lockout_datetime_not_set = self.user_lockout_datetime is None

 if user_lockout_datetime_not_set:
 self.user_lockout_datetime = timezone.now()
 self.save(update_fields=['user_lockout_datetime'])

 def clear_last_lockout_datetime(self):
 """ Clear the user_lockout_datetime field, which indicates the last time the user was locked out. """
 if (self.user_lockout_datetime):
 self.user_lockout_datetime = None
 self.save(update_fields=['user_lockout_datetime'])

 def reset_lockout_time_if_expired(self):
 """ Resets the lockout time for a user if it's expired. """
 lockout_time_expired = False

 if (self.user_lockout_datetime):
 lockout_time_expired = (self.user_lockout_datetime +
 timedelta(minutes=self.super_user.user_lockout_duration_mins) <
 timezone.now())
 if (lockout_time_expired):
 self.clear_last_lockout_datetime()