improve fix of #236: only push has remove-group semantics

* Changed the fix of #236 so that the semantics of sync is unchanged from before (for better backward compatibility).  Only push does remove of groups, because it's operating in the blind.
* Updated the nosetests so they work in py2 and py3
* Updated the noestests to add testing the rules with push strategy

Author: adobeDan

tests/config_file_test.py

@@ -23,7 +23,7 @@
 import os
 import os.path
 import yaml
-import six
+import six.moves
 
 import mock
 import tests.helper
@@ -43,7 +43,7 @@ def assert_eq(self, result, expected, error_message):
         '''
         self.assertEqual(result, expected, error_message + '\nexpected: %s, got: %s' % (expected, result))
 
-    @mock.patch('builtins.open')
+    @mock.patch('six.moves.builtins.open')
     @mock.patch('yaml.load')
     @mock.patch('os.path.isfile')
     def test_load_root_config(self, mock_isfile, mock_yaml, mock_open):
@@ -93,7 +93,7 @@ def test_load_root_config(self, mock_isfile, mock_yaml, mock_open):
             self.assert_eq(yml['other']['test-list'][2]['test-string-3'], 'xyz',
                            '/other/test-list/[2] value should not change')
 
-    @mock.patch('builtins.open')
+    @mock.patch('six.moves.builtins.open')
     @mock.patch('yaml.load')
     @mock.patch('os.path.isfile')
     def test_load_root_default_config(self, mock_isfile, mock_yaml, mock_open):
@@ -123,7 +123,7 @@ def test_load_root_default_config(self, mock_isfile, mock_yaml, mock_open):
             self.assert_eq(yml['adobe_users']['connectors']['umapi'], os.path.abspath('config-test-2/test-123'),
                            'default primary umapi configuration path is incorrect')
 
-    @mock.patch('builtins.open')
+    @mock.patch('six.moves.builtins.open')
     @mock.patch('yaml.load')
     @mock.patch('os.path.isfile')
     def test_load_sub_config(self, mock_isfile, mock_yaml, mock_open):

tests/connector/directory_ldap_test.py

@@ -2,6 +2,7 @@
 import unittest
 
 import mock.mock
+import six
 
 import tests.helper
 import user_sync.connector.directory
@@ -46,11 +47,11 @@ def mock_result3(*args, **kwargs):
             rtype = ldap.RES_SEARCH_RESULT
             rmsgid = None
             serverctrls = []
-            rdata = [(user['firstname'], {
-                'givenName': [user['firstname']],
-                'sn': [user['lastname']],
-                'c': [user['country']],
-                'mail': [user['email']],
+            rdata = [(user['firstname'].encode('utf8', 'strict'), {
+                'givenName': [user['firstname'].encode('utf8', 'strict')],
+                'sn': [user['lastname'].encode('utf8', 'strict')],
+                'c': [user['country'].encode('utf8', 'strict')],
+                'mail': [user['email'].encode('utf8', 'strict')],
             }) for user in all_users]
             return rtype, rdata, rmsgid, serverctrls
 

tests/rules_test.py

@@ -27,75 +27,156 @@
 import tests.helper
 
 class RulesTest(unittest.TestCase):
-    
-    def test_normal(self):
+
+    def test_sync(self):
         primary_umapi_name = user_sync.rules.PRIMARY_UMAPI_NAME
         secondary_1_umapi_name = "secondary1"
-        directory_group_1 = 'acrobat1' 
-        directory_group_2 = 'acrobat2' 
+        directory_group_1 = 'acrobat1'
+        directory_group_2 = 'acrobat2'
         primary_group_11 = 'acrobat11'
-        primary_group_12 = 'acrobat12'
+        secondary_group_12 = 'acrobat12'
         primary_group_21 = 'acrobat21'
         directory_groups = {
-            directory_group_1: [user_sync.rules.AdobeGroup(primary_group_11, primary_umapi_name), user_sync.rules.AdobeGroup('acrobat12', secondary_1_umapi_name)],
+            directory_group_1: [user_sync.rules.AdobeGroup(primary_group_11, primary_umapi_name),
+                                user_sync.rules.AdobeGroup(secondary_group_12, secondary_1_umapi_name)],
             directory_group_2: [user_sync.rules.AdobeGroup(primary_group_21, primary_umapi_name)]
         }
         everybody = [tests.helper.create_test_user([directory_group_1]),
             tests.helper.create_test_user([directory_group_2]),
             tests.helper.create_test_user([])]
-        
+
         for user in everybody:
             user['username'] = user['email']
             user['domain'] = None
-            
+
         primary_users = []
         primary_user_1 = everybody[1].copy()
         primary_user_1['groups'] = [primary_group_11]
         primary_users.append(primary_user_1)
-        
+
         def mock_load_users_and_groups(groups=None, extended_attributes=None, all_users=True):
             return list(everybody)
         mock_directory_connector = mock.mock.create_autospec(user_sync.connector.directory.DirectoryConnector)
         mock_directory_connector.load_users_and_groups = mock_load_users_and_groups
-        
+
         primary_commands_list = []
         mock_primary_umapi_connector = self.create_mock_umapi_connector(primary_users, primary_commands_list)
 
         secondary_commands_list = []
         mock_secondary_umapi_connector = self.create_mock_umapi_connector([], secondary_commands_list)
-        
+
         umapi_connectors = user_sync.rules.UmapiConnectors(mock_primary_umapi_connector, {
             secondary_1_umapi_name: mock_secondary_umapi_connector
         })
-        
+
         rule_processor = user_sync.rules.RuleProcessor({'manage_groups': True})
         rule_processor.run(directory_groups, mock_directory_connector, umapi_connectors)
 
         rule_options = rule_processor.options
 
         expected_primary_commands_list = []
-        
+        # when syncing, the existing Adobe user is processsed first
         user = everybody[1]
         commands = tests.helper.create_umapi_commands(user)
         commands.add_groups(set([primary_group_21]))
         commands.remove_groups(set([primary_group_11]))
         expected_primary_commands_list.append(commands)
-        
         user = everybody[0]
         commands = tests.helper.create_umapi_commands(user)
         commands.add_user(self.create_user_attributes_for_commands(user, rule_options['update_user_info']))
         commands.add_groups(set([primary_group_11]))
         expected_primary_commands_list.append(commands)
-        
         user = everybody[2]
         commands = tests.helper.create_umapi_commands(user)
         commands.add_user(self.create_user_attributes_for_commands(user, rule_options['update_user_info']))
         expected_primary_commands_list.append(commands)
-                
+
+        expected_secondary_commands_list = []
+        user = everybody[0]
+        commands = tests.helper.create_umapi_commands(user)
+        commands.add_groups(set([secondary_group_12]))
+        expected_secondary_commands_list.append(commands)
+
+        tests.helper.assert_equal_umapi_commands_list(self, expected_primary_commands_list, primary_commands_list)
+        tests.helper.assert_equal_umapi_commands_list(self, expected_secondary_commands_list, secondary_commands_list)
+
+    def test_push(self):
+        primary_umapi_name = user_sync.rules.PRIMARY_UMAPI_NAME
+        secondary_1_umapi_name = "secondary1"
+        directory_group_1 = 'acrobat1'
+        directory_group_2 = 'acrobat2'
+        primary_group_11 = 'acrobat11'
+        secondary_group_12 = 'acrobat12'
+        primary_group_21 = 'acrobat21'
+        directory_groups = {
+            directory_group_1: [user_sync.rules.AdobeGroup(primary_group_11, primary_umapi_name),
+                                user_sync.rules.AdobeGroup(secondary_group_12, secondary_1_umapi_name)],
+            directory_group_2: [user_sync.rules.AdobeGroup(primary_group_21, primary_umapi_name)]
+        }
+        everybody = [tests.helper.create_test_user([directory_group_1]),
+                     tests.helper.create_test_user([directory_group_2]),
+                     tests.helper.create_test_user([])]
+
+        for user in everybody:
+            user['username'] = user['email']
+            user['domain'] = None
+
+        primary_users = []
+        primary_user_1 = everybody[1].copy()
+        primary_user_1['groups'] = [primary_group_11]
+        primary_users.append(primary_user_1)
+
+        def mock_load_users_and_groups(groups=None, extended_attributes=None, all_users=True):
+            return list(everybody)
+
+        mock_directory_connector = mock.mock.create_autospec(user_sync.connector.directory.DirectoryConnector)
+        mock_directory_connector.load_users_and_groups = mock_load_users_and_groups
+
+        primary_commands_list = []
+        mock_primary_umapi_connector = self.create_mock_umapi_connector(primary_users, primary_commands_list)
+
+        secondary_commands_list = []
+        mock_secondary_umapi_connector = self.create_mock_umapi_connector([], secondary_commands_list)
+
+        umapi_connectors = user_sync.rules.UmapiConnectors(mock_primary_umapi_connector, {
+            secondary_1_umapi_name: mock_secondary_umapi_connector
+        })
+
+        rule_processor = user_sync.rules.RuleProcessor({'manage_groups': True,
+                                                        'strategy': 'push',
+                                                        'update_user_info': True,
+                                                        })
+        rule_processor.run(directory_groups, mock_directory_connector, umapi_connectors)
+
+        rule_options = rule_processor.options
+
+        expected_primary_commands_list = []
+        # when pushing, they are in directory order
+        user = everybody[0]
+        commands = tests.helper.create_umapi_commands(user)
+        commands.add_user(self.create_user_attributes_for_commands(user, rule_options['update_user_info']))
+        commands.add_groups(set([primary_group_11]))
+        commands.remove_groups(set([primary_group_21]))
+        expected_primary_commands_list.append(commands)
+
+        user = everybody[1]
+        commands = tests.helper.create_umapi_commands(user)
+        commands.add_user(self.create_user_attributes_for_commands(user, rule_options['update_user_info']))
+        commands.add_groups(set([primary_group_21]))
+        commands.remove_groups(set([primary_group_11]))
+        expected_primary_commands_list.append(commands)
+
+        user = everybody[2]
+        commands = tests.helper.create_umapi_commands(user)
+        commands.add_user(self.create_user_attributes_for_commands(user, rule_options['update_user_info']))
+        commands.remove_groups(set([primary_group_11, primary_group_21]))
+        expected_primary_commands_list.append(commands)
+
         expected_secondary_commands_list = []
         user = everybody[0]
         commands = tests.helper.create_umapi_commands(user)
-        commands.add_groups(set([primary_group_12]))
+        commands.add_user(self.create_user_attributes_for_commands(user, False))
+        commands.add_groups(set([secondary_group_12]))
         expected_secondary_commands_list.append(commands)
 
         tests.helper.assert_equal_umapi_commands_list(self, expected_primary_commands_list, primary_commands_list)
@@ -105,7 +186,7 @@ def mock_load_users_and_groups(groups=None, extended_attributes=None, all_users=
     @mock.patch('logging.getLogger')
     def _do_country_code_test(self, mock_umapi_commands, mock_connectors, identity_type, default_country_code, user_country_code, expected_country_code, mock_logger):
         user_key = identity_type + ',[email protected],'
-        expected_result = {'lastname': 'User1', 'email': '[email protected]', 'firstname': '!Openldap CCE', 'option': 'updateIfAlreadyExists'}
+        expected_result = {'lastname': 'User1', 'email': '[email protected]', 'firstname': '!Openldap CCE', 'option': 'ignoreIfAlreadyExists'}
         if (expected_country_code):
             expected_result['country'] = expected_country_code
 
@@ -124,7 +205,7 @@ def _do_country_code_test(self, mock_umapi_commands, mock_connectors, identity_t
         mock_rules.add_umapi_user(user_key, set(), mock_connectors)
 
         if (identity_type == 'federatedID' and default_country_code == None and user_country_code == None):
-            mock_rules.logger.error.assert_called_with('User %s cannot be added as it has a blank country code and no default has been specified.', user_key)
+            mock_rules.logger.error.assert_called_with('Federated user cannot be added without a specified country code: %s', user_key)
         else:
             mock_umapi_commands.return_value.add_user.assert_called_with(expected_result)
 

user_sync/connector/directory_ldap.py

@@ -366,6 +366,7 @@ def generate_value(self, record):
     @classmethod
     def get_attribute_value(cls, attributes, attribute_name):
         """
+        The attribute value type must be decodable (str in py2, bytes in py3)
         :type attributes: dict
         :type attribute_name: unicode
         """

user_sync/rules.py

@@ -407,7 +407,7 @@ def sync_umapi_users(self, umapi_connectors):
         # Handle creates for new users.  This also drives adding the new user to the secondaries,
         # but the secondary adobe groups will be managed below in the usual way.
         for user_key, groups_to_add in six.iteritems(primary_adds_by_user_key):
-            self.add_umapi_user(user_key, groups_to_add, umapi_connectors, manage_secondary_groups=False)
+            self.add_umapi_user(user_key, groups_to_add, umapi_connectors)
         # we just did a bunch of adds, we need to flush the connections before we can sync groups
         umapi_connectors.execute_actions()
 
@@ -434,7 +434,7 @@ def push_umapi_users(self, umapi_connectors):
         primary_umapi_info = self.get_umapi_info(PRIMARY_UMAPI_NAME)
         # Create all the users, putting them in their groups
         for user_key, groups_to_add in six.iteritems(primary_umapi_info.get_desired_groups_by_user_key()):
-            self.add_umapi_user(user_key, groups_to_add, umapi_connectors, manage_secondary_groups=True)
+            self.add_umapi_user(user_key, groups_to_add, umapi_connectors)
 
     def is_selected_user_key(self, user_key):
         """
@@ -604,11 +604,13 @@ def create_commands_from_directory_user(self, directory_user, identity_type=None
                                                       directory_user['username'], directory_user['domain'])
         return commands
 
-    def add_umapi_user(self, user_key, groups_to_add, umapi_connectors, manage_secondary_groups=True):
+    def add_umapi_user(self, user_key, groups_to_add, umapi_connectors):
         """
         Add the user to the primary umapi with the given groups, and create the user in any secondaries
-        in which he should be in a group.  If directed, also add the user to those groups in the secondary.
-        If we are managing groups, we also remove the user from any mapped groups he shouldn't be in.
+        in which he should be in a group.  If we are syncing, that's all we do.  If we are pushing rather
+        than syncing, we also add the user to the correct group in the secondaries, and we remove
+        the user from any mapped groups he shouldn't be in both in the primary and in the secondaries.
+        (This way, when we push blindly, we manage his entire set of mapped groups rather than just some.)
         :type user_key: str
         :type groups_to_add: list
         :type umapi_connectors: UmapiConnectors
@@ -617,6 +619,7 @@ def add_umapi_user(self, user_key, groups_to_add, umapi_connectors, manage_secon
         options = self.options
         update_user_info = options['update_user_info']
         manage_groups = self.will_manage_groups()
+        doing_push = not self.sync_umapi
 
         # put together the user's attributes
         directory_user = self.directory_user_by_user_key[user_key]
@@ -646,7 +649,9 @@ def add_umapi_user(self, user_key, groups_to_add, umapi_connectors, manage_secon
         primary_commands.add_user(attributes)
         if manage_groups:
             primary_commands.add_groups(groups_to_add)
-            primary_commands.remove_groups(self.get_umapi_info(PRIMARY_UMAPI_NAME).get_mapped_groups() - groups_to_add)
+            if doing_push:
+                groups_to_remove = self.get_umapi_info(PRIMARY_UMAPI_NAME).get_mapped_groups() - groups_to_add
+                primary_commands.remove_groups(groups_to_remove)
         umapi_connectors.get_primary_connector().send_commands(primary_commands)
         # add the user to secondaries, maybe with groups
         attributes['option'] = 'ignoreIfAlreadyExists'  # can only update in the owning org
@@ -658,9 +663,10 @@ def add_umapi_user(self, user_key, groups_to_add, umapi_connectors, manage_secon
                 self.logger.info('Adding directory user to %s with user key: %s', umapi_name, user_key)
                 secondary_commands = self.create_commands_from_directory_user(directory_user, identity_type)
                 secondary_commands.add_user(attributes)
-                if manage_secondary_groups and manage_groups:
+                if manage_groups and doing_push:
                     secondary_commands.add_groups(groups_to_add)
-                    secondary_commands.remove_groups(secondary_umapi_info.get_mapped_groups() - groups_to_add)
+                    groups_to_remove = secondary_umapi_info.get_mapped_groups() - groups_to_add
+                    secondary_commands.remove_groups(groups_to_remove)
                 umapi_connector.send_commands(secondary_commands)
 
     def update_umapi_user(self, umapi_info, user_key, umapi_connector,