Merge pull request #55823 from twangboy/fix_lgpo_long_names

Fix issue with overly long names in the LGPO module

salt/modules/win_lgpo.py

@@ -4918,7 +4918,7 @@ def _parse_xml(adm_file):
         for file_path in file_list:
             os.remove(file_path)
 
-        # Load the file
+        # Lowercase all the keys
         with salt.utils.files.fopen(adm_file, 'rb') as rfh:
 
             encoding = 'utf-8'
@@ -4938,6 +4938,13 @@ def _parse_xml(adm_file):
                     found_key = True
                 modified_xml += line + '\r\n'
 
+        # Convert smart quotes to regular quotes
+        modified_xml = modified_xml.replace('\u201c', '"').replace('\u201d', '"')
+        modified_xml = modified_xml.replace('\u2018', '\'').replace('\u2019', '\'')
+
+        # Convert em dash and en dash to dash
+        modified_xml = modified_xml.replace('\u2013', '-').replace('\u2014', '-')
+
         with salt.utils.files.fopen(out_file, 'wb') as wfh:
             wfh.write(modified_xml.encode(encoding))
 
@@ -5655,23 +5662,6 @@ def _getAdmlPresentationRefId(adml_data, ref_id):
     if search_results:
         for result in search_results:
             the_localname = etree.QName(result.tag).localname
-            presentation_element = PRESENTATION_ANCESTOR_XPATH(result)
-            if presentation_element:
-                presentation_element = presentation_element[0]
-                if TEXT_ELEMENT_XPATH(presentation_element):
-                    for p_item in presentation_element.getchildren():
-                        if p_item == result:
-                            break
-                        else:
-                            if etree.QName(p_item.tag).localname == 'text':
-                                if prepended_text:
-                                    prepended_text = ' '.join((text for text in (prepended_text, getattr(p_item, 'text', '').rstrip()) if text))
-                                else:
-                                    prepended_text = getattr(p_item, 'text', '').rstrip() if getattr(p_item, 'text', '') else ''
-                            else:
-                                prepended_text = ''
-                    if prepended_text.endswith('.'):
-                        prepended_text = ''
             if the_localname == 'textBox' \
                     or the_localname == 'comboBox':
                 label_items = result.xpath('.//*[local-name() = "label"]')
@@ -6652,13 +6642,14 @@ def _checkAllAdmxPolicies(policy_class,
                         unpathed_dict[policy_namespace] = {}
                     unpathed_dict[policy_namespace][full_names[policy_namespace][policy_item]] = policy_item
             # go back and remove any "unpathed" policies that need a full path
-            for path_needed in unpathed_dict[policy_namespace]:
-                # remove the item with the same full name and re-add it w/a path'd version
-                full_path_list = hierarchy[policy_namespace][unpathed_dict[policy_namespace][path_needed]]
-                full_path_list.reverse()
-                full_path_list.append(path_needed)
-                log.trace('full_path_list == %s', full_path_list)
-                policy_vals['\\'.join(full_path_list)] = policy_vals[policy_namespace].pop(path_needed)
+            if policy_namespace in unpathed_dict:
+                for path_needed in unpathed_dict[policy_namespace]:
+                    # remove the item with the same full name and re-add it w/a path'd version
+                    full_path_list = hierarchy[policy_namespace][unpathed_dict[policy_namespace][path_needed]]
+                    full_path_list.reverse()
+                    full_path_list.append(path_needed)
+                    log.trace('full_path_list == %s', full_path_list)
+                    policy_vals['\\'.join(full_path_list)] = policy_vals[policy_namespace].pop(path_needed)
         log.trace('Compilation complete: %s seconds', time.time() - start_time)
     for policy_namespace in list(policy_vals):
         if policy_vals[policy_namespace] == {}:
@@ -8226,10 +8217,8 @@ def _get_policy_adm_setting(admx_policy,
                                 configured_elements[this_element_name] = True
                                 log.trace('element %s is configured true',
                                           child_item.attrib['id'])
-                    elif etree.QName(child_item).localname == 'decimal' \
-                        or etree.QName(child_item).localname == 'text' \
-                        or etree.QName(child_item).localname == 'longDecimal' \
-                        or etree.QName(child_item).localname == 'multiText':
+                    elif etree.QName(child_item).localname in [
+                            'decimal', 'text', 'longDecimal', 'multiText']:
                         # https://msdn.microsoft.com/en-us/library/dn605987(v=vs.85).aspx
                         if _regexSearchRegPolData(
                                 re.escape(_processValueItem(
@@ -8301,10 +8290,12 @@ def _get_policy_adm_setting(admx_policy,
                                             configured_elements[this_element_name] = _getAdmlDisplayName(
                                                 adml_xml_data=adml_policy_resources,
                                                 display_name=enum_item.attrib['displayName'])
+                                            break
                                     else:
                                         configured_elements[this_element_name] = _getAdmlDisplayName(
                                             adml_xml_data=adml_policy_resources,
                                             display_name=enum_item.attrib['displayName'])
+                                        break
                     elif etree.QName(child_item).localname == 'list':
                         return_value_name = False
                         if 'explicitValue' in child_item.attrib \

salt/states/win_lgpo.py

@@ -112,6 +112,7 @@
 import salt.utils.data
 import salt.utils.dictdiffer
 import salt.utils.json
+import salt.utils.versions
 import salt.utils.win_functions
 
 # Import 3rd party libs
@@ -266,6 +267,7 @@ def set_(name,
             'policy_lookup': {}}}
 
     current_policy = {}
+    deprecation_comments = []
     for p_class, p_data in six.iteritems(pol_data):
         if p_data['requested_policy']:
             for p_name, _ in six.iteritems(p_data['requested_policy']):
@@ -283,10 +285,37 @@ def set_(name,
                         policy_class=p_class,
                         adml_language=adml_language,
                         return_value_only=True)
+                    # Validate element names
+                    if isinstance(p_data['requested_policy'][p_name], dict):
+                        valid_names = []
+                        for element in lookup['policy_elements']:
+                            valid_names.extend(element['element_aliases'])
+                        for e_name in p_data['requested_policy'][p_name]:
+                            if e_name not in valid_names:
+                                new_e_name = e_name.split(':')[-1].strip()
+                                # If we find an invalid name, test the new
+                                # format. If found, replace the old with the
+                                # new
+                                if new_e_name in valid_names:
+                                    msg = 'The LGPO module changed the way ' \
+                                          'it gets policy element names.\n'\
+                                          '"{0}" is no longer valid.\n'\
+                                          'Please use "{1}" instead.' \
+                                          ''.format(e_name, new_e_name)
+                                    salt.utils.versions.warn_until('Phosphorus', msg)
+                                    pol_data[p_class]['requested_policy'][p_name][new_e_name] = \
+                                        pol_data[p_class]['requested_policy'][p_name].pop(e_name)
+                                    deprecation_comments.append(msg)
+                                else:
+                                    msg = 'Invalid element name: {0}'.format(e_name)
+                                    ret['comment'] = '\n'.join([ret['comment'], msg]).strip()
+                                    ret['result'] = False
                 else:
-                    ret['comment'] = ' '.join([ret['comment'], lookup['message']])
+                    ret['comment'] = '\n'.join([ret['comment'], lookup['message']]).strip()
                     ret['result'] = False
     if not ret['result']:
+        deprecation_comments.append(ret['comment'])
+        ret['comment'] = '\n'.join(deprecation_comments).strip()
         return ret
 
     log.debug('pol_data == %s', pol_data)
@@ -299,8 +328,6 @@ def set_(name,
         if requested_policy:
             for p_name, p_setting in six.iteritems(requested_policy):
                 if p_name in current_policy[class_map[p_class]]:
-                    currently_set = True
-                if currently_set:
                     # compare
                     log.debug('need to compare %s from current/requested '
                               'policy', p_name)
@@ -320,61 +347,46 @@ def set_(name,
                         requested_policy_check, current_policy_check)
 
                     if not policies_are_equal:
-                        additional_policy_comments = []
                         if p_data['policy_lookup'][p_name]['rights_assignment'] and cumulative_rights_assignments:
                             for user in p_data['requested_policy'][p_name]:
                                 if user not in current_policy[class_map[p_class]][p_name]:
                                     user = salt.utils.win_functions.get_sam_name(user)
                                     if user not in current_policy[class_map[p_class]][p_name]:
                                         changes = True
-                                    else:
-                                        additional_policy_comments.append('"{0}" is already granted the right'.format(user))
-                                else:
-                                    additional_policy_comments.append('"{0}" is already granted the right'.format(user))
                         else:
                             changes = True
                         if changes:
-                            log.debug('%s current policy != requested policy',
-                                      p_name)
-                            log.debug(
-                                'we compared %s to %s',
-                                requested_policy_json, current_policy_json
-                            )
+                            log.debug('%s current policy != requested policy', p_name)
+                            log.debug('We compared %s to %s', requested_policy_json, current_policy_json)
                             policy_changes.append(p_name)
-                        else:
-                            if additional_policy_comments:
-                                ret['comment'] = '"{0}" is already set ({1})\n'.format(p_name, ', '.join(additional_policy_comments))
-                            else:
-                                ret['comment'] = '"{0}" is already set\n'.format(p_name) + ret['comment']
                     else:
-                        log.debug('%s current setting matches '
-                                  'the requested setting', p_name)
-                        ret['comment'] = '"{0}" is already set\n'.format(p_name) + ret['comment']
+                        msg = '"{0}" is already set'.format(p_name)
+                        log.debug(msg)
                 else:
                     policy_changes.append(p_name)
-                    log.debug('policy %s is not set, we will configure it',
-                              p_name)
+                    log.debug('policy %s is not set, we will configure it', p_name)
     if __opts__['test']:
         if policy_changes:
+            msg = 'The following policies are set to change:\n{0}' \
+                  ''.format('\n'.join(policy_changes))
             ret['result'] = None
-            ret['comment'] = 'The following policies are set to change:\n{0}'.format(
-                    '\n'.join(policy_changes))
         else:
-            ret['comment'] = 'All specified policies are properly configured'
+            msg = 'All specified policies are properly configured'
+        deprecation_comments.append(msg)
+        ret['comment'] = '\n'.join(deprecation_comments).strip()
     else:
         if policy_changes:
             _ret = __salt__['lgpo.set'](
-                computer_policy=computer_policy,
-                user_policy=user_policy,
+                computer_policy=pol_data['machine']['requested_policy'],
+                user_policy=pol_data['user']['requested_policy'],
                 cumulative_rights_assignments=cumulative_rights_assignments,
                 adml_language=adml_language)
             if _ret:
                 ret['result'] = _ret
                 new_policy = {}
                 for p_class, p_data in six.iteritems(pol_data):
                     if p_data['requested_policy']:
-                        for p_name, p_setting in six.iteritems(
-                            p_data['requested_policy']):
+                        for p_name, p_setting in six.iteritems(p_data['requested_policy']):
                             new_policy.setdefault(class_map[p_class], {})
                             new_policy[class_map[p_class]][p_name] = __salt__['lgpo.get_policy'](
                                 policy_name=p_name,
@@ -384,13 +396,20 @@ def set_(name,
                 ret['changes'] = salt.utils.dictdiffer.deep_diff(
                     old=current_policy, new=new_policy)
                 if ret['changes']:
-                    ret['comment'] = 'The following policies changed:\n{0}' \
-                                     ''.format('\n'.join(policy_changes))
+                    msg = 'The following policies changed:\n{0}' \
+                          ''.format('\n'.join(policy_changes))
                 else:
-                    ret['comment'] = 'The following policies are in the correct state:\n{0}' \
-                                     ''.format('\n'.join(policy_changes))
+                    msg = 'The following policies are in the correct ' \
+                          'state:\n{0}'.format('\n'.join(policy_changes))
             else:
+                msg = 'Errors occurred while attempting to configure ' \
+                      'policies: {0}'.format(_ret)
                 ret['result'] = False
-                ret['comment'] = 'Errors occurred while attempting to configure policies: {0}'.format(_ret)
+            deprecation_comments.append(msg)
+            ret['comment'] = '\n'.join(deprecation_comments).strip()
+        else:
+            msg = 'All specified policies are properly configured'
+            deprecation_comments.append(msg)
+            ret['comment'] = '\n'.join(deprecation_comments).strip()
 
     return ret

salt/version.py

@@ -109,9 +109,9 @@ class SaltStackVersion(object):
         'Sodium'        : (MAX_SIZE - 98, 0),
         'Magnesium'     : (MAX_SIZE - 97, 0),
         'Aluminium'     : (MAX_SIZE - 96, 0),
+        'Silicon'      : (MAX_SIZE - 95, 0),
+        'Phosphorus'   : (MAX_SIZE - 94, 0),
         # pylint: disable=E8265
-        #'Silicon'      : (MAX_SIZE - 95, 0),
-        #'Phosphorus'   : (MAX_SIZE - 94, 0),
         #'Sulfur'       : (MAX_SIZE - 93, 0),
         #'Chlorine'     : (MAX_SIZE - 92, 0),
         #'Argon'        : (MAX_SIZE - 91, 0),

tests/integration/modules/test_win_lgpo.py

@@ -215,7 +215,7 @@ def test_set_user_policy_point_and_print_restrictions(self):
                 'Users can only point and print to these servers': True,
                 'Enter fully qualified server names separated by semicolons': 'fakeserver1;fakeserver2',
                 'Users can only point and print to machines in their forest': True,
-                'Security Prompts: When installing drivers for a new connection': 'Show warning and elevation prompt',
+                'When installing drivers for a new connection': 'Show warning and elevation prompt',
                 'When updating drivers for an existing connection': 'Show warning only',
             },
             [

tests/unit/modules/test_win_lgpo.py

@@ -585,8 +585,7 @@ def setUp(self):
                     'Users can only point and print to machines in their '
                     'forest':
                         True,
-                    'Security Prompts: When installing drivers for a new '
-                    'connection':
+                    'When installing drivers for a new connection':
                         'Show warning and elevation prompt',
                     'When updating drivers for an existing connection':
                         'Show warning only',
@@ -671,8 +670,7 @@ def test_point_and_print_enabled_full_names(self):
             'Printers\\Point and Print Restrictions': {
                 'Enter fully qualified server names separated by semicolons':
                     'fakeserver1;fakeserver2',
-                'Security Prompts: When installing drivers for a new '
-                'connection':
+                'When installing drivers for a new connection':
                     'Show warning and elevation prompt',
                 'Users can only point and print to machines in their forest':
                     True,
@@ -696,8 +694,7 @@ def test_point_and_print_enabled_full_names_hierarchical(self):
                             'Enter fully qualified server names separated by '
                             'semicolons':
                                 'fakeserver1;fakeserver2',
-                            'Security Prompts: When installing drivers for a '
-                            'new connection':
+                            'When installing drivers for a new connection':
                                 'Show warning and elevation prompt',
                             'Users can only point and print to machines in '
                             'their forest':