[Zope-dev] z3c.password and "failedAttempts" andSessionCredentialsPlugin

[Jan-Wijbrand Kolman]

Hi Adam,

Adam GROSZER <agroszer at gmail.com> wrote:
> We're using it too...
> Go for 0.8.0.

I noticed that requesting for, for example, a resource could still trigger
account locked errors, where in my opinion only the login attempts
themselves should do that.

I created a branch...

  http://svn.zope.org/z3c.password/branches/jw-noraise-for-irrelevant-requests/

..that will check for request relevancy as early as possible.

All tests pass without modification, but still, after an account has been locked 
requests for, for example resources, will still work (the diff against the trunk 
is pasted below). Additionally I think the code is tad more readable now.

I realize now that I should add a test to demonstrate this.

Anyway, could you tell me what you think of this approach?

regards, jw




--- z3c.password-trunk/src/z3c/password/principal.py
	2010-02-04 12:15:46.632785192 +0100
+++ z3c.password/src/z3c/password/principal.py	2010-02-04 12:08:56.351805021 
+0100
@@ -13,7 +13,7 @@
 ##############################################################################
 """Principal MixIn for Advanced Password Management
 
-$Id: principal.py 108644 2010-01-29 17:28:55Z adamg $
+$Id: principal.py 108749 2010-02-04 11:08:55Z janwijbrand $
 """
 __docformat__ = "reStructuredText"
 import datetime
@@ -81,6 +81,30 @@
         #hook to facilitate testing and easier override
         return datetime.datetime.now()
 
+    def _isIrrelevantRequest(self, RELEVANT=False, IRRELEVANT=True):
+        fac = self._failedAttemptCheck()
+        if fac is None:
+            return RELEVANT
+
+        if fac == interfaces.TML_CHECK_ALL:
+            return RELEVANT
+
+        interaction = getInteraction()
+        try:
+            request = interaction.participations[0]
+        except IndexError:
+            return RELEVANT # no request, we regard that as relevant.
+
+        if fac == interfaces.TML_CHECK_NONRESOURCE:
+            if '/@@/' in request.getURL():
+                return IRRELEVANT
+            return RELEVANT
+
+        if fac == interfaces.TML_CHECK_POSTONLY:
+            if request.method == 'POST':
+                return RELEVANT
+            return IRRELEVANT
+
     def checkPassword(self, pwd, ignoreExpiration=False, ignoreFailures=False):
         # keep this as fast as possible, because it will be called (usually)
         # for EACH request
@@ -88,6 +112,11 @@
         # Check the password
         same = super(PrincipalMixIn, self).checkPassword(pwd)
 
+        # Do not try to record failed attempts or raise account locked
+        # errors for requests that are irrelevant in this regard.
+        if self._isIrrelevantRequest():
+            return same
+
         if not ignoreFailures and self.lastFailedAttempt is not None:
             if self.tooManyLoginFailures():
                 locked = self.accountLocked()
@@ -118,7 +147,9 @@
             add = 0
         else:
             #failed attempt, record it, increase counter
-            add = self.checkFailedAttempt()
+            self.failedAttempts += 1
+            self.lastFailedAttempt = self.now()
+            add = 1
 
         # If the maximum amount of failures has been reached notify the
         # system by raising an error.
@@ -133,45 +164,6 @@
 
         return same
 
-    def _getRequest(self):
-        interaction = getInteraction()
-        try:
-            return interaction.participations[0]
-        except IndexError:
-            return None
-
-    def checkFailedAttempt(self):
-        #failed attempt, record it, increase counter (in case we have to)
-        validRequest = True
-        fac = self._failedAttemptCheck()
-        if fac == interfaces.TML_CHECK_ALL:
-            validRequest = True
-        else:
-            request = self._getRequest()
-            if request is None:
-                validRequest = True
-            else:
-                if fac == interfaces.TML_CHECK_NONRESOURCE:
-                    url = request.getURL()
-                    if '/@@/' in url:
-                        #this is a resource
-                        validRequest = False
-                    else:
-                        validRequest = True
-                elif fac == interfaces.TML_CHECK_POSTONLY:
-                    if request.method == 'POST':
-                        #this is a POST request
-                        validRequest = True
-                    else:
-                        validRequest = False
-
-        if validRequest:
-            self.failedAttempts += 1
-            self.lastFailedAttempt = self.now()
-            return 1
-        else:
-            return 0
-