[Freeipa-devel] [PATCH 0161] Fix dyndb-ldap working dir permission

[Jan Cholasta]

Hi,

Dne 13.11.2014 v 14:50 Martin Basti napsal(a):
> On 13/11/14 13:59, Jan Cholasta wrote:
>> Dne 12.11.2014 v 13:33 Martin Basti napsal(a):
>>> On 11/11/14 16:58, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> Dne 11.11.2014 v 16:22 Martin Basti napsal(a):
>>>>> Using specfile to create file doesn't work if named user is not on
>>>>> system.
>>>>> Appropriate permission have to be set during ipa-dns installation.
>>>>>
>>>>> Patch attached
>>>>>
>>>>
>>>> Why is the directory set up in dnskeysyncinstance instead of
>>>> bindinstance?
>>> Because, dnskeysyncinstance is the daemon which requires permission
>>> change.
>>> (dir is created by dyndb-ldap plugin)
>>
>> OK. But please rename the method to something more suitable
>> (fix_dyndb_ldap_workdir_permissions?) and add a docstring/comment.
>>
>> Also please change the ticket link to
>> <https://fedorahosted.org/freeipa/ticket/4716> (cloned from BZ).
>>
>>>
>>>>
>>>> The original patch was released with 4.1.1, shouldn't there be update
>>>> in ipa-upgradeconfig?
>>> Cases:
>>> 1) fresh RPM install, no named user during RPM install -> named doesn't
>>> start, user had to fix it immediately, can't wait until next release.
>>>
>>> 2) fresh RPM install,  named user -> no impact
>>>
>>> 3) upgrade IPA with DNS -> no impact
>>>
>>> 4) upgrade IPA without DNS -> after DNS installation, same as 1)
>>>
>>> 5) IPA 4.1.0 with installed DNS, upgrade to 4.1.2 ->  DNSSEC will not
>>> work (If user doesnt use DNSSEC)
>>>
>>> Only 5) looks serious for me, so here is updated patch.
>>
>> Could you do the update without the code duplication? In similar code
>> an appropriate *instance method is usually called.

The uid/gid resolution in ipa-upgradeconfig still looks like duplicated 
code to me. I would suggest doing something along these lines in 
ipa-upgradeconfig:

     dnskeysync = dnskeysyncinstance.DNSKeySyncInstance()
     dnskeysync.set_dyndb_ldap_workdir_permissions()

and have DNSKeySyncInstance.set_dyndb_ldap_workdir_permissions() do all 
the real work.

>>
>>>
>>> Martin^2
>>>>
>>>> Honza
>>>>
>>>
>>>
>>
>> Honza
>>
> Thanks.
> updated patch attached.
> Martin^2
>

Honza

-- 
Jan Cholasta