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

[Martin Kosek]

On 04/15/2013 02:02 PM, Petr Viktorin wrote:
> On 04/15/2013 12:32 PM, Tomas Babej wrote:
>> 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
> 
> ACK
> 

Pushed to master.

Martin