erp5_core: Password Tool should not leak info on users

for security reasons, info on users, or existence of usernames shouldn't be leaked from the system.

bt5/erp5_core_test/TestTemplateItem/portal_components/test.erp5.testPasswordTool.py

@@ -428,8 +428,11 @@ class TestPasswordTool(ERP5TypeTestCase):
     self.logout()
     ret = self.portal.portal_password.mailPasswordResetRequest(
                   user_login='user-login', REQUEST=self.portal.REQUEST)
-    self.assertTrue("portal_status_message=User+user-login+does+not+have+an+email+"\
-        "address%2C+please+contact+site+administrator+directly" in str(ret))
+
+    # For security reasons, the message should always be the same
+    self.assertTrue("portal_status_message=An+email+has+been+sent+to+you." in str(ret))
+    # But no mail has been sent
+    self.stepCheckNoMailSent()
 
   def test_acquired_email_on_person(self):
     organisation = self.portal.organisation_module.newContent(
@@ -452,8 +455,11 @@ class TestPasswordTool(ERP5TypeTestCase):
     self.logout()
     ret = self.portal.portal_password.mailPasswordResetRequest(
                   user_login='user-login', REQUEST=self.portal.REQUEST)
-    self.assertTrue("portal_status_message=User+user-login+does+not+have+an+email+"\
-        "address%2C+please+contact+site+administrator+directly" in str(ret))
+
+    # For security reasons, the message should always be the same
+    self.assertTrue("portal_status_message=An+email+has+been+sent+to+you." in str(ret))
+    # But no mail has been sent
+    self.stepCheckNoMailSent()
 
 def test_suite():
   suite = unittest.TestSuite()

bt5/erp5_core_test/TestTemplateItem/portal_components/test.erp5.testPasswordTool.xml

@@ -6,6 +6,12 @@
     </pickle>
     <pickle>
       <dictionary>
+        <item>
+            <key> <string>_recorded_property_dict</string> </key>
+            <value>
+              <persistent> <string encoding="base64">AAAAAAAAAAI=</string> </persistent>
+            </value>
+        </item>
         <item>
             <key> <string>default_reference</string> </key>
             <value> <string>testPasswordTool</string> </value>
@@ -53,13 +59,28 @@
         <item>
             <key> <string>workflow_history</string> </key>
             <value>
-              <persistent> <string encoding="base64">AAAAAAAAAAI=</string> </persistent>
+              <persistent> <string encoding="base64">AAAAAAAAAAM=</string> </persistent>
             </value>
         </item>
       </dictionary>
     </pickle>
   </record>
   <record id="2" aka="AAAAAAAAAAI=">
+    <pickle>
+      <global name="PersistentMapping" module="Persistence.mapping"/>
+    </pickle>
+    <pickle>
+      <dictionary>
+        <item>
+            <key> <string>data</string> </key>
+            <value>
+              <dictionary/>
+            </value>
+        </item>
+      </dictionary>
+    </pickle>
+  </record>
+  <record id="3" aka="AAAAAAAAAAM=">
     <pickle>
       <global name="PersistentMapping" module="Persistence.mapping"/>
     </pickle>
@@ -72,7 +93,7 @@
                 <item>
                     <key> <string>component_validation_workflow</string> </key>
                     <value>
-                      <persistent> <string encoding="base64">AAAAAAAAAAM=</string> </persistent>
+                      <persistent> <string encoding="base64">AAAAAAAAAAQ=</string> </persistent>
                     </value>
                 </item>
               </dictionary>
@@ -81,7 +102,7 @@
       </dictionary>
     </pickle>
   </record>
-  <record id="3" aka="AAAAAAAAAAM=">
+  <record id="4" aka="AAAAAAAAAAQ=">
     <pickle>
       <global name="WorkflowHistoryList" module="Products.ERP5Type.Workflow"/>
     </pickle>

bt5/erp5_credential/TestTemplateItem/portal_components/test.erp5.testERP5Credential.py

@@ -1404,7 +1404,7 @@ class TestERP5Credential(ERP5TypeTestCase):
     # Execute alarm, it will fail because this person has no email
     with self.assertRaisesRegexp(
         RuntimeError,
-        "User .* does not have an email address, please contact site administrator directly"):
+        "An email has been sent to you"):
       self.tic()
 
     # run alarm again, this does not cause another activity failure.

product/ERP5/bootstrap/erp5_core/ToolComponentTemplateItem/portal_components/tool.erp5.PasswordTool.py

@@ -126,6 +126,8 @@ class PasswordTool(BaseTool):
     substitution_method_parameter_dict -- additional substitution dict for
                                           creating an email.
     """
+    error_encountered = False
+    msg = translateString("An email has been sent to you.")
     if REQUEST is None:
       REQUEST = get_request()
 
@@ -136,15 +138,18 @@ class PasswordTool(BaseTool):
     if REQUEST and 'came_from' in REQUEST:
       site_url = REQUEST.came_from
 
-    msg = None
+    error_encountered = False
     # check user exists, and have an email
     user_path_set = {x['path'] for x in self.getPortalObject().acl_users.searchUsers(
       login=user_login,
       exact_match=True,
     ) if 'path' in x}
     if len(user_path_set) == 0:
-      msg = translateString("User ${user} does not exist.",
-                            mapping={'user':user_login})
+      error_encountered = True
+      LOG(
+        'ERP5.PasswordTool', INFO,
+        "User {user} does not exist.".format(user=user_login)
+      )
     else:
       # We use checked_permission to prevent errors when trying to acquire
       # email from organisation
@@ -154,10 +159,12 @@ class PasswordTool(BaseTool):
       email_value = user_value.getDefaultEmailValue(
         checked_permission='Access content information')
       if email_value is None or not email_value.asText():
-        msg = translateString(
-            "User ${user} does not have an email address, please contact site "
-            "administrator directly", mapping={'user':user_login})
-    if msg:
+        error_encountered = True
+        LOG(
+          'ERP5.PasswordTool', INFO,
+          "User {user} does not have an email address".format(user=user_login)
+        )
+    if error_encountered:
       if batch:
         raise RuntimeError(msg)
       else:

[Jérome Perrin]

@Nicolas BTW, what about CredentialRequest_checkLoginAvailability ? ( and callers such as ERP5Site_newCredentialRequest )

[Nicolas Wavrant]

Hi,

IMO the case for Credential Requests is different, as we let customers choose their own login. So at some point we need to inform them that the login they choose is not available. Maybe we could have a generic message instead (ie, "Selected login is not valid", in https://lab.nexedi.com/nexedi/erp5/blob/master/bt5/erp5_credential/SkinTemplateItem/portal_skins/erp5_credential/ERP5Site_newCredentialRequest.py#L14), but attackers wouldn't be duped.

I would argue that this use of Credential Requests is a bad practice, and should be avoided in projects :)

What do you think ?

[Jérome Perrin]

Thanks, maybe we could just change CredentialRequest_checkLoginAvailability to not allow calling it directly.

[Jérome Perrin]

I looked a bit more and found https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#account-creation . ( this is in context of !1795 )