Standard security calculation to check if Assignment is expired or not

security calculation checks the validation state of the Assignment, but it should also check if it is expired or not started yet.

This merge request applies fix, extending tests to re-produce and test the situation.
Also, a cleanup in erp5_core: extension.erp5.StandardSecurity.

/reviewed-on nexedi/erp5!548

product/ERP5/bootstrap/erp5_core/ExtensionTemplateItem/portal_components/extension.erp5.StandardSecurity.py

@@ -24,12 +24,16 @@
 # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
 #
 ##############################################################################
+from DateTime import DateTime
 
-from Products.ERP5Security.ERP5GroupManager import ConsistencyError
-from Products.ZSQLCatalog.SQLCatalog import SimpleQuery
-
-def getSecurityCategoryFromAssignment(self, base_category_list, user_name, object, portal_type,
-                                      child_category_list=[]):
+def getSecurityCategoryFromAssignment(
+  self,
+  base_category_list,
+  user_name,
+  object,  # pylint: disable=redefined-builtin
+  portal_type,
+  child_category_list=None
+):
   """
   This script returns a list of dictionaries which represent
   the security groups which a person is member of. It extracts
@@ -49,9 +53,9 @@ def getSecurityCategoryFromAssignment(self, base_category_list, user_name, objec
     object             -- object which we want to assign roles to
     portal_type        -- portal type of object
   """
-  context = self
-
   category_list = []
+  if child_category_list is None:
+    child_category_list = []
 
   user_path_set = {
     x['path'] for x in self.acl_users.searchUsers(
@@ -66,10 +70,15 @@ def getSecurityCategoryFromAssignment(self, base_category_list, user_name, objec
     return []
   user_path, = user_path_set
   person_object = self.getPortalObject().unrestrictedTraverse(user_path)
+  now = DateTime()
 
   # We look for every valid assignments of this user
   for assignment in person_object.contentValues(filter={'portal_type': 'Assignment'}):
-    if assignment.getValidationState() == 'open':
+    if assignment.getValidationState() == "open" and (
+      not assignment.hasStartDate() or assignment.getStartDate() <= now
+    ) and (
+      not assignment.hasStopDate() or assignment.getStopDate() >= now
+    ):
       category_dict = {}
       for base_category in base_category_list:
         category_value_list = assignment.getAcquiredValueList(base_category)
@@ -89,22 +98,31 @@ def getSecurityCategoryFromAssignment(self, base_category_list, user_name, objec
   return category_list
 
 
-def getSecurityCategoryFromAssignmentParent(self, base_category_list,
-                                       user_name, object, portal_type):
-  return getSecurityCategoryFromAssignment(self, base_category_list,
-                                       user_name, object, portal_type, child_category_list=base_category_list)
+def getSecurityCategoryFromAssignmentParent(self, base_category_list, user_name,
+                                            object, # pylint: disable=redefined-builtin
+                                            portal_type):
+  return getSecurityCategoryFromAssignment(self, base_category_list, user_name,
+                                           object, # pylint: disable=redefined-builtin
+                                           portal_type, child_category_list=base_category_list)
+
+def getSecurityCategoryFromAssignmentParentGroup(self, base_category_list, user_name,
+                                                 object, # pylint: disable=redefined-builtin
+                                                 portal_type):
+  return getSecurityCategoryFromAssignment(self, base_category_list, user_name,
+                                           object, # pylint: disable=redefined-builtin
+                                           portal_type, child_category_list=('group',))
 
-def getSecurityCategoryFromAssignmentParentGroup(self, base_category_list,
-                                       user_name, object, portal_type):
-  return getSecurityCategoryFromAssignment(self, base_category_list,
-                                       user_name, object, portal_type, child_category_list=('group',))
+def getSecurityCategoryFromAssignmentParentFunction(self, base_category_list, user_name,
+                                                    object, # pylint: disable=redefined-builtin
+                                                    portal_type):
+  return getSecurityCategoryFromAssignment(self, base_category_list, user_name,
+                                           object, # pylint: disable=redefined-builtin
+                                           portal_type, child_category_list=('function',))
 
-def getSecurityCategoryFromAssignmentParentFunction(self, base_category_list,
-                                       user_name, object, portal_type):
-  return getSecurityCategoryFromAssignment(self, base_category_list,
-                                       user_name, object, portal_type, child_category_list=('function',))
+def getSecurityCategoryFromAssignmentParentFunctionParentGroup(self, base_category_list, user_name,
+                                                               object, # pylint: disable=redefined-builtin
+                                                               portal_type):
+  return getSecurityCategoryFromAssignment(self, base_category_list, user_name,
+                                           object, # pylint: disable=redefined-builtin
+                                           portal_type, child_category_list=('function', 'group'))
 
-def getSecurityCategoryFromAssignmentParentFunctionParentGroup(self, base_category_list,
-                                       user_name, object, portal_type):
-  return getSecurityCategoryFromAssignment(self, base_category_list,
-                                       user_name, object, portal_type, child_category_list=('function', 'group'))

product/ERP5/bootstrap/erp5_core/ExtensionTemplateItem/portal_components/extension.erp5.StandardSecurity.xml

@@ -6,10 +6,22 @@
     </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>StandardSecurity</string> </value>
         </item>
+        <item>
+            <key> <string>description</string> </key>
+            <value>
+              <none/>
+            </value>
+        </item>
         <item>
             <key> <string>id</string> </key>
             <value> <string>extension.erp5.StandardSecurity</string> </value>
@@ -24,6 +36,18 @@
               <none/>
             </value>
         </item>
+        <item>
+            <key> <string>text_content_error_message</string> </key>
+            <value>
+              <tuple/>
+            </value>
+        </item>
+        <item>
+            <key> <string>text_content_warning_message</string> </key>
+            <value>
+              <tuple/>
+            </value>
+        </item>
         <item>
             <key> <string>version</string> </key>
             <value> <string>erp5</string> </value>
@@ -31,13 +55,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>
@@ -50,7 +89,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>
@@ -59,7 +98,7 @@
       </dictionary>
     </pickle>
   </record>
-  <record id="3" aka="AAAAAAAAAAM=">
+  <record id="4" aka="AAAAAAAAAAQ=">
     <pickle>
       <global name="WorkflowHistoryList" module="Products.ERP5Type.patches.WorkflowTool"/>
     </pickle>

product/ERP5Security/tests/testERP5Security.py

@@ -105,7 +105,7 @@ class TestUserManagement(ERP5TypeTestCase):
     newSecurityManager(None, user)
 
   def _makePerson(self, login=AUTO_LOGIN, open_assignment=1, assignment_start_date=None,
-                  assignment_stop_date=None, tic=True, password='secret', **kw):
+                  assignment_stop_date=None, tic=True, password='secret', group_value=None, **kw):
     """Creates a person in person module, and returns the object, after
     indexing is done. """
     person_module = self.getPersonModule()
@@ -113,7 +113,8 @@ class TestUserManagement(ERP5TypeTestCase):
                      portal_type='Person', **kw)
     assignment = new_person.newContent(portal_type = 'Assignment',
                                        start_date=assignment_start_date,
-                                       stop_date=assignment_stop_date,)
+                                       stop_date=assignment_stop_date,
+                                       group_value=group_value,)
     if open_assignment:
       assignment.open()
     if login is not None:
@@ -160,6 +161,48 @@ class TestUserManagement(ERP5TypeTestCase):
            "Plugin %s should not have authenticated '%s' with password '%s'" %
            (plugin_name, login, password))
 
+  def _getOrCreateGroupValue(self):
+    group_id = 'dummy_group'
+    group = self.portal.portal_categories.group
+    if group_id in group.objectIds():
+      return group[group_id]
+    else:
+      return self.portal.portal_categories.group.newContent(
+        id=group_id
+      )
+
+  def _createDummyDocument(self):
+    types_tool = self.portal.portal_types
+    # Create Portal Types if needed
+    if 'Dummy Object' not in types_tool.objectIds():
+      dummy_type = types_tool.newContent(
+        'Dummy Object',
+        'Base Type',
+        type_class='XMLObject'
+      )
+      dummy_type.newContent(
+        portal_type='Role Information',
+        role_category_list=self.portal.portal_categories.\
+          group.dummy_group.getRelativeUrl(),
+        role_name_list=('Assignee', )
+      )
+    if 'Dummy Module' not in types_tool.objectIds():
+      types_tool.newContent(
+        'Dummy Module',
+        'Base Type',
+        type_class='Folder',
+        type_filter_content_type=1,
+        type_allowed_content_type_list=('Dummy Object', ),
+      )
+    # clean-up dummy_module in any way
+    if 'dummy_module' in self.portal.objectIds():
+      self.portal.manage_delObjects(['dummy_module'])
+    self.tic()
+    self.portal.newContent(portal_type='Dummy Module', id='dummy_module')
+    dummy_document = self.portal.dummy_module.newContent(portal_type='Dummy Object')
+    self.tic()
+    return dummy_document
+
   def test_PersonWithLoginPasswordAreUsers(self):
     """Tests a person with a login & password is a valid user."""
     _, login, password = self._makePerson()
@@ -546,6 +589,57 @@ class TestUserManagement(ERP5TypeTestCase):
     )
     self._assertUserDoesNotExists(login, password)
 
+  def test_securityGroupAssignmentCorrectDate(self):
+    """
+      Tests a person with an assignment with correct date
+      gets correctly assigned to security groups.
+    """
+    date = DateTime()
+    user_id, login, password = self._makePerson(
+      assignment_start_date=date - 5,
+      assignment_stop_date=date + 5,
+      group_value=self._getOrCreateGroupValue()
+    )
+    self.assertIn(
+      'Assignee',
+      self.portal.acl_users.getUserById(user_id).\
+        getRolesInContext(self._createDummyDocument())
+    )
+
+  def test_securityGroupAssignmentBadStartDate(self):
+    """
+      Tests a person with an assignment with bad (future) start date
+      does not get assigned to security groups.
+    """
+    date = DateTime()
+    user_id, login, password = self._makePerson(
+      assignment_start_date=date + 1,
+      assignment_stop_date=date + 5,
+      group_value=self._getOrCreateGroupValue()
+    )
+    self.assertNotIn(
+      'Assignee',
+      self.portal.acl_users.getUserById(user_id).\
+        getRolesInContext(self._createDummyDocument())
+    )
+
+  def test_securityGroupAssignmentBadStopDate(self):
+    """
+      Tests a person with an assignment with bad (past) stop date
+      does not get assigned to security groups.
+    """
+    date = DateTime()
+    user_id, login, password = self._makePerson(
+      assignment_start_date=date - 5,
+      assignment_stop_date=date - 1,
+      group_value=self._getOrCreateGroupValue()
+    )
+    self.assertNotIn(
+      'Assignee',
+      self.portal.acl_users.getUserById(user_id).\
+        getRolesInContext(self._createDummyDocument())
+    )
+
   def test_DeletedPersonIsNotUser(self):
     user_id, login, password = self._makePerson()
     self._assertUserExists(login, password)