Fix the pwpolicy_find post_callback

Always call convert_time_for_output so time gets reported correctly.
That method has its own checks for whether the attributes are present;
an additional check is unnecessary.

Use a key function for sorting; cmp is deprecated, slower and
more complicated.

Add a test

https://fedorahosted.org/freeipa/ticket/2726

ipalib/plugins/pwpolicy.py

@@ -459,34 +459,36 @@ class pwpolicy_find(LDAPSearch):
     # this command does custom sorting in post_callback
     sort_result_entries = False
 
-    def sort_priority(self,x,y):
+    def priority_sort_key(self, entry):
+        """Key for sorting password policies
+
+        returns a pair: (is_global, priority)
+        """
         # global policy will be always last in the output
-        if x[1]['cn'][0] == global_policy_name:
+        if entry[1]['cn'][0] == global_policy_name:
-            return 1
+            return True, 0
-        elif y[1]['cn'][0] == global_policy_name:
-            return -1
         else:
-            # policies with higher priority will be at the beginning of the list
+            # policies with higher priority (lower number) will be at the
+            # beginning of the list
             try:
-                x =  cmp(int(x[1]['cospriority'][0]), int(y[1]['cospriority'][0]))
+                cospriority = entry[1]['cospriority'][0]
             except KeyError:
                 # if cospriority is not present in the entry, rather return 0
                 # than crash
-                x = 0
+                cospriority = 0
-            return x
+            return False, cospriority
 
     def post_callback(self, ldap, entries, truncated, *args, **options):
         for e in entries:
-            # attribute rights are not allowed for pwpolicy_find
+            # When pkey_only flag is on, entries should contain only a cn.
+            # Add a cospriority attribute that will be used for sorting.
+            # Attribute rights are not allowed for pwpolicy_find.
             self.obj.add_cospriority(e[1], e[1]['cn'][0], rights=False)
-            if options.get('pkey_only', False):
+
-                # when pkey_only flag is on, entries should contain only a cn
+            self.obj.convert_time_for_output(e[1], **options)
-                # and a cospriority attribute that will be used for sorting
-                # When the entries are sorted, cosentry is removed
-                self.obj.convert_time_for_output(e[1], **options)
 
         # do custom entry sorting by its cospriority
-        entries.sort(self.sort_priority)
+        entries.sort(key=self.priority_sort_key)
 
         if options.get('pkey_only', False):
             # remove cospriority that was used for sorting
@@ -497,4 +499,3 @@ class pwpolicy_find(LDAPSearch):
                     pass
 
 api.register(pwpolicy_find)
-

tests/test_xmlrpc/test_pwpolicy_plugin.py

@@ -180,6 +180,21 @@ class test_pwpolicy(XMLRPC_test):
         assert_attr_equal(entry, 'cospriority', '3')
 
     def test_c_pwpolicy_find(self):
+        """Test that password policies are sorted and reported properly"""
+        result = api.Command['pwpolicy_find']()['result']
+        assert len(result) == 4
+        assert result[0]['cn'] == (self.group,)
+        assert result[1]['cn'] == (self.group2,)
+        assert result[2]['cn'] == (self.group3,)
+        assert result[3]['cn'] == ('global_policy',)
+
+        # Test that returned values match the arguments
+        # Only test the second and third results; the first one was modified
+        for entry, expected in (result[1], self.kw2), (result[2], self.kw3):
+            for name, value in expected.iteritems():
+                assert_attr_equal(entry, name, str(value))
+
+    def test_c_pwpolicy_find_pkey_only(self):
         """Test that password policies are sorted properly with --pkey-only"""
         result = api.Command['pwpolicy_find'](pkey_only=True)['result']
         assert len(result) == 4
@@ -225,4 +240,3 @@ class test_pwpolicy(XMLRPC_test):
 
         # Remove the user we created
         api.Command['user_del'](self.user)
-