[Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

[Jan Cholasta]

On 23.9.2013 19:41, Ana Krivokapic wrote:
> On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
>> Hello,
>>
>> This patch set adds automember rebuild membership functionality to IPA CLI.
>>
>> Design:http://www.freeipa.org/page/V3/Automember_rebuild_membership
>> Ticket:https://fedorahosted.org/freeipa/ticket/3752
>>
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> This updated patch set introduces only one automember-rebuild command,
> instead of three. The changes are covered in the design document:
> http://www.freeipa.org/page/V3/Automember_rebuild_membership#CLI.
>
> Also, while working on these patches, I hit this bug again:
> https://fedorahosted.org/freeipa/ticket/2708. Since the fix is pretty
> straightforward, and the bug is related to automember feature, I
> included a fix for it in this patch set (patch 0071).
>

Patch 68:

-            (gdn, entry_attrs) = ldap.get_entry(dn, [])
+            (odn, entry_attrs) = ldap.get_entry(dn, [])

Please use the new LDAP API in new code.


+                reason=_(u'%(otype)s: %(oname)s not found!') %

The colon looks weird here and looked weird in the original code too. 
Can you please remove it? And while you are at it, you could also add 
quotes around "%(oname)s".


+class automember_rebuild(LDAPQuery):

You don't use any LDAPQuery, PKQuery, BaseLDAPCommand or Method 
specifics here, so you can inherit straight from Command. Just make sure 
to use self.api.Backend.ldap2 instead of self.obj.backend in the code.


The takes_options attribute should look like this:

     takes_options = (
         group_type[0].clone(required=False),
         Str(
             'users*',
             label=_('Users'),
             doc=_('Users for which the rebuild task will be run'),
         ),
         Str(
             'hosts*',
             label=_('Hosts'),
             doc=_('Hosts for which the rebuild task will be run'),
         ),
     )

Or you can keep 'type' required and use default_from to create a default 
value for it based on --users and --hosts if it's not specified:

def _rebuild_type_default(users, hosts):
     if users and not hosts:
         return 'group'
     elif hosts and not users:
         return 'hostgroup'

...
     takes_options = (
         group_type[0].clone(default_from=_rebuild_type_default),
...

This should also simplify the rest of the code, as you will always have 
a value for 'type' if there were no errors.


In the validate method, you raise ValidationError with name='options'. 
This is wrong, as name should refer to a parameter name. You can use 
MutuallyExclusiveError for the errors:

     def validate(self, **kw):
         if kw.get('users') and kw.get('hosts'):
             raise errors.MutuallyExclusiveError(reason=_("users and 
hosts cannot both be set"))
         if kw.get('type') == 'group' and kw.get('hosts'):
             raise errors.MutuallyExclusiveError(reason=_("hosts cannot 
be set when type is 'group'"))
         if kw.get('type') == 'hostgroup' and kw.get('users'):
             raise errors.MutuallyExclusiveError(reason=_("users cannot 
be set when type is 'hostgroup'"))
         super(automember_rebuild, self).validate(**kw)

Note that if you use default_from for the 'type' parameter, the validate 
method above will fail with RequirementError if 'type' is not specified 
(both explicitly and implicitly).


+        basedn = realm_to_suffix(api.env.realm)

This should be api.env.basedn.


+        if users or hosts:
+            filters = []
+            for u in users:
+                self.obj.dn_exists('user', u)
+                filters.append(ldap.make_filter_from_attr('uid', u))
+            for h in hosts:
+                self.obj.dn_exists('host', h)
+                filters.append(ldap.make_filter_from_attr('fqdn', h))
+            search_filter = ldap.combine_filters(filters, ldap.MATCH_ANY)
+        else:
+            search_filter = '(uid=*)' if group_type == 'group' else 
'(fqdn=*)'

You can look all the attribute names in the right object plugin, no need 
to specify them explicitly. I think you can use something like this instead:

     types = {
         'group': ('user', 'users'),
         'hostgroup': ('host', 'hosts'),
     }

     obj_name, opt_name = types[kw['type']]
     obj = self.api.Object[obj_name]

     names = kw.get(opt_name)
     if names:
         for name in names:
             obj.get_dn_if_exists(name)
         search_filter = 
ldap.make_filter_from_attr(obj.primary_key.name, names, 
rules=ldap.MATCH_ANY)
     else:
         search_filter = '(%s=*)' % obj.primary_key.name


Patch 69:

I think the changes in the update file should be also done in the right 
LDIF files in install/share, though I don't know what is the recent 
consensus on this.


Honza

-- 
Jan Cholasta