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

[Tomas Babej]

On 04/12/2013 04:52 PM, Petr Viktorin wrote:
> 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.
Yes, this was probably overcautious.
>
>> +                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.
>

Thank you for the review. Updated patch attached.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0048-2-Add-nfs-NONE-to-default-PAC-types-only-when-needed.patch
Type: text/x-patch
Size: 4262 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130415/a48465e5/attachment.bin>