[Freeipa-devel] [PATCH 0048] Add nfs:NONE to default PAC types only when needed

[Petr Viktorin]

On 04/12/2013 04:10 PM, Tomas Babej wrote:
> Hi,
>
> We need to add nfs:NONE as a default PAC type only if there's no
> other default PAC type for nfs. Adds a update plugin which
> determines whether default PAC type for nfs is set and adds
> nfs:NONE PAC type accordingly.
>
> https://fedorahosted.org/freeipa/ticket/3555
>
> Tomas

Looks good, works well, see a few nitpicks below.

[...]
> diff --git a/ipaserver/install/plugins/update_pacs.py b/ipaserver/install/plugins/update_pacs.py
[...]
> +from ipapython.ipa_log_manager import *

Please don't use star imports in new code. Only import what you need 
(that is, remove this import entirely).

> +class update_pacs(PostUpdate):
> +    """
> +    Includes default nfs:None only if no nfs: entry present in ipakrbauthzdata.
> +    """
> +
> +    order = MIDDLE
> +
> +    def execute(self, **options):
> +        ldap = self.obj.backend
> +
> +        try:
> +            dn = DN('cn=ipaConfig', 'cn=etc', api.env.basedn)
> +            entry = ldap.get_entry(dn, ['ipakrbauthzdata'])
> +            pacs = entry.get('ipakrbauthzdata')
> +
> +            if pacs is None:

This shouldn't happen (ipaKrbAuthzData gets a default in previous 
updates). It's not necessary to complicate the code, `pacs = 
entry.get('ipakrbauthzdata', [])` would do.

> +                self.log.warning('No ipakrbauthzdata attribute found.')
> +                return (False, False, [])
>
> +        except errors.NotFound:
> +            self.log.warning('Error retrieving: %s' % str(dn))
> +            return (False, False, [])
> +
> +        nfs_pac_set = any(pac.startswith('nfs:') for pac in pacs)
> +
> +        if not nfs_pac_set:
> +            self.log.debug('Adding nfs:None to default PAC types')
> +            updated_pacs = pacs + [u'nfs:NONE']
> +            update = dict(ipakrbauthzdata=updated_pacs)
> +            ldap.update_entry(dn, update)

This will work, but the preferred form for updates is now:
     entry['ipakrbauthzdata'] = updated_pacs
     ldap.update_entry(entry)

> +        else:
> +            self.log.debug('PAC for nfs is already set, not adding nfs:NONE.')
> +
> +        return (False, False, [])
> +
> +api.register(update_pacs)
> -- 1.8.1.4

Please also add the new update file to Makefile.am.

-- 
Petr³