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

[Martin Kosek]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-042-2-password-policy-commands-do-not-include-cospriority.patch
Type: text/x-patch
Size: 5272 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110404/82080418/attachment.bin>