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³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
