Small refactoring of PasswordTool

  * remove blank spaces
  * use urlencode to encode url parameters
  * password_request_dict is now private (renamed into _password_request_dict)
  * various optimisations
  * update test according code refactoring 


git-svn-id: https://svn.erp5.org/repos/public/erp5/trunk@33674 20353a03-c40f-0410-a6d1-a30d3c3de9de

product/ERP5/Tool/PasswordTool.py

+# -*- coding: utf-8 -*-
 ##############################################################################
 #
 # Copyright (c) 2008 Nexedi SARL and Contributors. All Rights Reserved.
@@ -39,6 +40,7 @@ from DateTime import DateTime
 from Products.ERP5Type.Message import translateString
 from Acquisition import aq_base
 from Products.ERP5Type.Globals import PersistentMapping
+from urllib import urlencode
 
 class PasswordTool(BaseTool):
   """
@@ -58,10 +60,10 @@ class PasswordTool(BaseTool):
 
 
   _expiration_day = 1
-  password_request_dict = {}
-  
+  _password_request_dict = {}
+
   def __init__(self):
-    self.password_request_dict = PersistentMapping()
+    self._password_request_dict = PersistentMapping()
 
   def mailPasswordResetRequest(self, user_login=None, REQUEST=None):
     """
@@ -71,13 +73,16 @@ class PasswordTool(BaseTool):
       user_login = REQUEST["user_login"]
 
     # check user exists
-    user_list = self.acl_users.erp5_users.getUserByLogin(user_login)              
+    user_list = self.getPortalObject().acl_users.\
+                      erp5_users.getUserByLogin(user_login)
     if len(user_list) == 0:
       msg = translateString("User ${user} does not exist.",
                             mapping={'user':user_login})
       if REQUEST is not None:
-        ret_url = '%s/login_form?portal_status_message=%s' % \
-                  (self.getPortalObject().absolute_url(),msg)
+        parameter = urlencode(dict(portal_status_message=msg))
+        ret_url = '%s/login_form?%s' % \
+                  (self.getPortalObject().absolute_url(),
+                  parameter)
         return REQUEST.RESPONSE.redirect( ret_url )
       else:
         return msg
@@ -85,20 +90,24 @@ class PasswordTool(BaseTool):
     user = user_list[0].getObject()
     # generate a random string
     random_url = self._generateUUID()
-    url = "%s/portal_password/resetPassword?reset_key=%s" %(self.getPortalObject().absolute_url() , random_url)
+    parameter = urlencode(dict(reset_key=random_url))
+    url = "%s/portal_password/%s?%s" % (
+                                self.getPortalObject().absolute_url(),
+                                'PasswordTool_viewResetPassword',
+                                parameter)
     # generate expiration date
     expiration_date = DateTime() + self._expiration_day
-    
-    # XXX before r26093, password_request_dict was initialized by an OOBTree and
+
+    # XXX before r26093, _password_request_dict was initialized by an OOBTree and
     # replaced by a dict on each request, so if it's data structure is not up
     # to date, we update it if needed
-    if not isinstance(self.password_request_dict, PersistentMapping):
+    if not isinstance(self._password_request_dict, PersistentMapping):
       LOG('ERP5.PasswordTool', INFO, 'Updating password_request_dict to'
                                      ' PersistentMapping')
-      self.password_request_dict = PersistentMapping()
-    
+      self._password_request_dict = PersistentMapping()
+
     # register request
-    self.password_request_dict[random_url] = (user_login, expiration_date)
+    self._password_request_dict[random_url] = (user_login, expiration_date)
 
     # send mail
     subject = "[%s] Reset of your password" %(self.getPortalObject().getTitle())
@@ -107,14 +116,14 @@ class PasswordTool(BaseTool):
               "Please note that this link will be valid only one time, until %s.\n" \
               "After this date, or after having used this link, you will have to make " \
               "a new request\n\n" \
-              "Thank you" %(self.getPortalObject().getTitle(), url, expiration_date)    
-    self.portal_notifications.sendMessage(sender=None, recipient=[user,], subject=subject, message=message)
+              "Thank you" %(self.getPortalObject().getTitle(), url, expiration_date)
+    self.getPortalObject().portal_notifications.sendMessage(sender=None, recipient=[user,], subject=subject, message=message)
     if REQUEST is not None:
       msg = translateString("An email has been sent to you.")
-      ret_url = '%s/login_form?portal_status_message=%s' % \
-                (self.getPortalObject().absolute_url(),msg)
+      parameter = urlencode(dict(portal_status_message=msg))
+      ret_url = '%s/login_form?%s' % (self.getPortalObject().absolute_url(),
+                                      parameter)
       return REQUEST.RESPONSE.redirect( ret_url )
-  
 
   def _generateUUID(self, args=""):
     """
@@ -131,7 +140,7 @@ class PasswordTool(BaseTool):
     except:
       # if we can't get a network address, just imagine one
       a = random.random()*100000000000000000L
-    data = str(t)+' '+str(r)+' '+str(a)+' '+str(args)
+    data = ' '.join((str(t), str(r), str(a), str(args)))
     data = md5.md5(data).hexdigest()
     return data
 
@@ -141,7 +150,7 @@ class PasswordTool(BaseTool):
     """
     if REQUEST is None:
       REQUEST = get_request()
-    user_login, expiration_date = self.password_request_dict.get(reset_key, (None, None))
+    user_login, expiration_date = self._password_request_dict.get(reset_key, (None, None))
     if reset_key is None or user_login is None:
       ret_url = '%s/login_form' % self.getPortalObject().absolute_url()
       return REQUEST.RESPONSE.redirect( ret_url )
@@ -150,10 +159,11 @@ class PasswordTool(BaseTool):
     current_date = DateTime()
     if current_date > expiration_date:
       msg = translateString("Date has expire.")
-      ret_url = '%s/login_form?portal_status_message=%s' % \
-                (self.getPortalObject().absolute_url(), msg)
+      parameter = urlencode(dict(portal_status_message=msg))
+      ret_url = '%s/login_form?%s' % (self.getPortalObject().absolute_url(),
+                                      parameter)
       return REQUEST.RESPONSE.redirect( ret_url )
-      
+
     # redirect to form as all is ok
     REQUEST.set("password_key", reset_key)
     return self.reset_password_form(REQUEST=REQUEST)
@@ -164,22 +174,24 @@ class PasswordTool(BaseTool):
     Browse dict and remove expired request
     """
     current_date = DateTime()
-    for key, (login, date) in self.password_request_dict.items():
+    for key, (login, date) in self._password_request_dict.items():
       if date < current_date:
-        self.password_request_dict.pop(key)
-        
-         
-  def changeUserPassword(self, user_login, password, password_confirmation, password_key, REQUEST=None):
+        self._password_request_dict.pop(key)
+
+
+  def changeUserPassword(self, user_login, password, password_confirmation,
+                         password_key, REQUEST=None):
     """
     Reset the password for a given login    
     """
     # check the key
-    register_user_login, expiration_date = self.password_request_dict.get(password_key, (None, None))
+    register_user_login, expiration_date = self._password_request_dict.get(
+                                                    password_key, (None, None))
 
     current_date = DateTime()
     msg = None
     if register_user_login is None:
-      msg = ""
+      msg = "Key not known. Please ask reset password."
     elif register_user_login != user_login:
       msg = translateString("Bad login provided.")
     elif current_date > expiration_date:
@@ -190,15 +202,16 @@ class PasswordTool(BaseTool):
       msg = translateString("Passwords do not match.")
     if msg is not None:
       if REQUEST is not None:
-        ret_url = '%s/login_form?portal_status_message=%s' % \
-                  (self.getPortalObject().absolute_url(), msg)
+        parameter = urlencode(dict(portal_status_message=msg))
+        ret_url = '%s/login_form?%s' % (self.getPortalObject().absolute_url(),
+                                        parameter)
         return REQUEST.RESPONSE.redirect( ret_url )
       else:
         return msg
 
     # all is OK, change password and remove it from request dict
-    self.password_request_dict.pop(password_key)
-    persons = self.acl_users.erp5_users.getUserByLogin(user_login)              
+    self._password_request_dict.pop(password_key)
+    persons = self.getPortalObject().acl_users.erp5_users.getUserByLogin(user_login)
     person = persons[0]
     # Calling private method starts with __ from outside is normally BAD,
     # but if we leave the method as a normal method starts with _ and follow
@@ -208,8 +221,9 @@ class PasswordTool(BaseTool):
     person.reindexObject()
     if REQUEST is not None:
       msg = translateString("Password changed.")
-      ret_url = '%s/login_form?portal_status_message=%s' % \
-                (self.getPortalObject().absolute_url(), msg)
+      parameter = urlencode(dict(portal_status_message=msg))
+      ret_url = '%s/login_form?%s' % (self.getPortalObject().absolute_url(),
+                                      parameter)
       return REQUEST.RESPONSE.redirect( ret_url )
-    
+
 InitializeClass(PasswordTool)

product/ERP5/tests/testPasswordTool.py

+# -*- coding: utf-8 -*-
 ##############################################################################
 #
 # Copyright (c) 2004 Nexedi SARL and Contributors. All Rights Reserved.
@@ -63,7 +64,7 @@ class TestPasswordTool(ERP5TypeTestCase):
     # clear modules if necessary
     self.portal.person_module.manage_delObjects(list(self.portal.person_module.objectIds()))
     # reset password tool internal structure
-    self.portal.portal_password.password_request_dict.clear()
+    self.portal.portal_password._password_request_dict.clear()
     transaction.commit()
     self.tic()
 
@@ -180,7 +181,7 @@ class TestPasswordTool(ERP5TypeTestCase):
     We don't check use of random url in mail here as we have on request
     But random is also check by changeUserPassword, so it's the same
     """
-    key = self.portal.portal_password.password_request_dict.keys()[0]
+    key = self.portal.portal_password._password_request_dict.keys()[0]
     self.portal.portal_password.changeUserPassword(user_login="userA",
                                                    password="secret",
                                                    password_confirmation="secret",
@@ -194,7 +195,7 @@ class TestPasswordTool(ERP5TypeTestCase):
     Call method that change the password with a bad user name
     This must not work
     """
-    key = self.portal.portal_password.password_request_dict.keys()[0]
+    key = self.portal.portal_password._password_request_dict.keys()[0]
     sequence.edit(key=key)
     self.portal.portal_password.changeUserPassword(user_login="userZ",
                                                    password="secret",
@@ -235,13 +236,13 @@ class TestPasswordTool(ERP5TypeTestCase):
     Change expiration date so that reset of password is not available
     """
     # save key for url
-    key = self.portal.portal_password.password_request_dict.keys()[0]
+    key = self.portal.portal_password._password_request_dict.keys()[0]
     sequence.edit(key=key)
     # modify date
-    for k, v in self.portal.portal_password.password_request_dict.items():
+    for k, v in self.portal.portal_password._password_request_dict.items():
       login, date = v
       date = DateTime() - 1
-      self.portal.portal_password.password_request_dict[k] = (login, date)
+      self.portal.portal_password._password_request_dict[k] = (login, date)
 
   def stepSimulateExpirationAlarm(self, sequence=None, sequence_list=None, **kw):
     """
@@ -253,7 +254,7 @@ class TestPasswordTool(ERP5TypeTestCase):
     """
     after alarm all expired request must have been removed
     """
-    self.assertEqual(len(self.portal.portal_password.password_request_dict), 0)
+    self.assertEqual(len(self.portal.portal_password._password_request_dict), 0)
 
 
   def stepLogout(self, sequence=None, sequence_list=None, **kw):
@@ -353,16 +354,16 @@ class TestPasswordTool(ERP5TypeTestCase):
     self._assertUserExists('userA', 'passwordA')
     self._assertUserExists('userB', 'passwordB')
     
-    self.assertEquals(0, len(self.portal.portal_password.password_request_dict))
+    self.assertEquals(0, len(self.portal.portal_password._password_request_dict))
     self.portal.portal_password.mailPasswordResetRequest(user_login="userA")
-    self.assertEquals(1, len(self.portal.portal_password.password_request_dict))
-    key_a = self.portal.portal_password.password_request_dict.keys()[0]
+    self.assertEquals(1, len(self.portal.portal_password._password_request_dict))
+    key_a = self.portal.portal_password._password_request_dict.keys()[0]
     transaction.commit()
     self.tic()
 
     self.portal.portal_password.mailPasswordResetRequest(user_login="userB")
     possible_key_list =\
-        self.portal.portal_password.password_request_dict.keys()
+        self.portal.portal_password._password_request_dict.keys()
     self.assertEquals(2, len(possible_key_list))
     key_b = [k for k in possible_key_list if k != key_a][0]
     transaction.commit()
@@ -405,14 +406,14 @@ class TestPasswordTool(ERP5TypeTestCase):
 
     self._assertUserExists('userZ ', 'passwordZ')
     
-    self.assertEquals(0, len(self.portal.portal_password.password_request_dict))
+    self.assertEquals(0, len(self.portal.portal_password._password_request_dict))
     # No reset should be send if trailing space is not entered
     self.portal.portal_password.mailPasswordResetRequest(user_login="userZ")
-    self.assertEquals(0, len(self.portal.portal_password.password_request_dict))
+    self.assertEquals(0, len(self.portal.portal_password._password_request_dict))
     self.portal.portal_password.mailPasswordResetRequest(user_login="userZ ")
-    self.assertEquals(1, len(self.portal.portal_password.password_request_dict))
+    self.assertEquals(1, len(self.portal.portal_password._password_request_dict))
 
-    key_a = self.portal.portal_password.password_request_dict.keys()[0]
+    key_a = self.portal.portal_password._password_request_dict.keys()[0]
     transaction.commit()
     self.tic()