[Freeipa-devel] [PATCH] 042 Password policy commands do not include cospriority

[Rob Crittenden]

Martin Kosek wrote:
> On Fri, 2011-04-01 at 13:51 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> Target branches: master, ipa-2-0
>>> ---
>>>
>>> Most of the pwpolicy_* commands do include cospriority in the result
>>> and potentially in the attribute rights (--all --rights). Especially
>>> when --raw output is requested. This patch fixes it for all
>>> pwpolicy commands.
>>>
>>> https://fedorahosted.org/freeipa/ticket/1103
>>>
>>
>> nack. I see a couple of problems.
>>
>> You should test for rights before doing the cosentry_show(). If rights
>> is False then we won't add the data whatever it is so it is more
>> efficient to exit earlier.
>
> We have to call cosentry_show every time (except for the case when we
> pull data for the global policy) because we read cospriority attribute.
> But the function was indeed not efficient (it called cosentry_show
> twice), I rewrote it.
>
>>
>> Same with pwpolicy_name == global_policy_name. I think you should drop
>> the try/except and make it:
>>
>> if not rights or pwpolicy_name == global_policy_name:
>>       return
>>
>> ...
>>
>> It should never be the case that the cosentry is not found so I'd let it
>> fail if that does occur.
>
> Fixed.
>
>>
>> I think that keys[-1] can be None so be aware.
>
> Fixed.
>
>>
>> You hardcode rights == False in pwpolicy_find(), a good thing. I think
>> you should add make it explict rights=False and add a comment explaining
>> that you can't get accessrights with a find.
>
> Fixed.
>
> Fixed patch attached.
>
> Martin

Looks great, ack.

rob