[Freeipa-devel] [PATCHES 187-201] Improvements and coverage for sudorule plugin

Petr Viktorin pviktori at redhat.com
Mon May 26 16:20:49 UTC 2014


On 05/20/2014 06:15 PM, Tomas Babej wrote:
> Hi,
>
> the following set of patches fixes:
>
> https://fedorahosted.org/freeipa/ticket/4274
> https://fedorahosted.org/freeipa/ticket/4263
> https://fedorahosted.org/freeipa/ticket/4324
> https://fedorahosted.org/freeipa/ticket/4340
> https://fedorahosted.org/freeipa/ticket/4341
>
> and additional minor issues.
>
> The improvemed CI test coverage for the sudorule plugin is added as a bonus.

Thanks!
I haven't tested yet; here are my comments on the code.


0187 - sudorule: PEP8 fixes in sudorule.py

This change:

> @@ -527,11 +551,15 @@ def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
>              _entry_attrs = ldap.get_entry(dn, self.obj.default_attributes)
>          except errors.NotFound:
>              self.obj.handle_not_found(*keys)
> -        if is_all(_entry_attrs, 'hostcategory'):
> -            raise errors.MutuallyExclusiveError(reason=_("hosts cannot be added when host category='all'"))
> +
> +        if is_all(self._entry_attrs, 'hostcategory'):
> +            raise errors.MutuallyExclusiveError(
> +                reason=_("hosts cannot be added when host category='all'"))
> +
>          return add_external_pre_callback('host', ldap, dn, keys, options)

adds `self` to `self._entry_attrs`. That should be a part of the next patch.
Git tip: `git log --word-diff` helps a lot when you play with whitespace

(Speaking of PEP8, if you could remove the baseldap star import from 
sudorule.py, it would be great...)



General thoughts:

Would it be possible to merge schema_compat.uldif and 
install/updates/10-schema_compat.update into one file? Is the uldif 
special somehow? I guess this is a question for Rob.
It would be nice to add a link to some schema-compat-entry-attribute 
documentation to these files.



0188 - sudorule: Allow using hostmasks for setting allowed hosts

Here:

> --- a/install/updates/40-delegation.update
> +++ b/install/updates/40-delegation.update
> @@ -174,7 +174,7 @@ default:member: cn=Sudo Administrator,cn=privileges,cn=pbac,$SUFFIX
>  dn: $SUFFIX
>  add:aci: '(target = "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Add Sudo rule";allow (add) groupdn = "ldap:///cn=Add Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
>  add:aci: '(target = "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Delete Sudo rule";allow (delete) groupdn = "ldap:///cn=Delete Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
> -add:aci: '(targetattr = "description || ipaenabledflag || usercategory || hostcategory || cmdcategory || ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser || ipasudorunasextuser || ipasudorunasextgroup || memberdenycmd || memberallowcmd || memberuser")(target = "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Modify Sudo rule";allow (write) groupdn = "ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
> +add:aci: '(targetattr = "description || ipaenabledflag || usercategory || hostcategory || cmdcategory || ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser || ipasudorunasextuser  || ipasudorunasextgroup || memberdenycmd || memberallowcmd || memberuser || hostmask")(target = "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Modify Sudo rule";allow (write) groupdn = "ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
>
>  remove:aci: '(targetattr = "description")(target = "ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Modify Sudo command";allow (write) groupdn = "ldap:///cn=Modify Sudo command,cn=permissions,cn=pbac,$SUFFIX";)'
>  remove:aci: '(target = "ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX")(version 3.0;acl "permission:Delete Sudo command";allow (delete) groupdn = "ldap:///cn=Delete Sudo command,cn=permissions,cn=pbac,$SUFFIX";)'

you'll want to remove the old ACI before adding the new version.

In get_options of sudorule_add_host and sudorule_remove_host, it would 
be DRY-er to put the hostmask in a shared variable.

You write:
> -            _entry_attrs = ldap.get_entry(dn, self.obj.default_attributes)
> +            self._entry_attrs = ldap.get_entry(dn, self.obj.default_attributes)

Never store anything on the Command object. They're currently 
singletons. Another thread could change _entry_attrs to a different value.
This affects all the other uses of self._entry_attrs, of course.

In:
> +            num_added = len(new_masks.difference(old_masks))
and:
> +                entry_attrs['hostmask'] = list(old_masks.union(new_masks))
and:
> +            num_added = len(removed_masks.intersection(old_masks))
etc.: with two sets, you can use the `-`, `|`, and `&` operators.


Since you're touching this:
> +        return add_external_post_callback('memberhost', 'host', 'externalhost',
> +                                          ldap, completed, failed, dn,
> +                                          entry_attrs, keys, options)
it would be nice to pass the arguments by name, for clarity and to avoid 
mistakes. I guess this should be in the PEP8 patch, too.




0189 sudorule: Allow using external groups as groups of runAsUsers

In sudorule_add_runasuser and sudorule_remove_runasuser, could you again 
pass the arguments by name?
I'd welcome comment explaining why completed=0. If you could throw in a 
comment for add_external_post_callback explaining the inputs and 
outputs, it would be great -- I'm getting quite lost in this code.
(I'd even go as far as changing the signature of 
*_external_post_callback -- AFAICS, in all the places we call it, we do 
`*keys, **options` wrong, and the `completed` argument is not useful at 
all. Most of the calls are from sudorule so it would make sense to do it 
in this patchset.)

And a heads-up: the change to "permission:Modify Sudo rule" conflicts 
with my ACI work. Let me know if there are problems.




0190 sudorule: Make sure sudoRunAsGroup is dereferencing the correct 
attribute

Looks good




0191 sudorule: Include externalhost and ipasudorunasextgroup in  the 
list of default attributes

Looks good




0192 sudorule: Allow adding deny commands when command category set to ALL

Looks good




0193 sudorule: Make sure all the relevant attributes are checked when 
setting category to ALL

Wow, haven't seen a copyright year change in some time. We are sloppy 
with those. Portions of the file are still from 2010 so you should 
definitely leave that year in, though. Actually I think the proper year 
field would read "2010,2011,2012,2013,2014".
I am not a lawyer.

You're missing the `_` for the hostcategory error message.
Did you think about using something like _("%s cannot be set to 'all' 
while there are %s")?




0194 sudorule: Fix the order of the parameters to have less chaotic output

Looks fine




0195-sudorule-Enforce-category-ALL-checks-on-dirsrv-level.patch

Looks fine




0196-0198 (Add tests)

Looks fine. Just remind me, why is this all in integration tests? 
Couldn't it at least some of it be in the main test suite? It looks to 
me like most of it doesn't need multiple machines.




0199-0201 (test fixes)

OK


-- 
Petr³




More information about the Freeipa-devel mailing list