[Freeipa-devel] [PATCHES] 0086-0088 Generate Firefox extension on upgrades

[Martin Kosek]

On 10/10/2012 04:23 PM, Petr Viktorin wrote:
> On 10/10/2012 03:37 PM, Martin Kosek wrote:
>> On 10/10/2012 10:55 AM, Petr Viktorin wrote:
>>> On 10/09/2012 06:01 PM, Petr Vobornik wrote:
>>>> On 10/09/2012 05:26 PM, Petr Viktorin wrote:
>>>>> On 10/09/2012 05:16 PM, Petr Viktorin wrote:
>>>>>> https://fedorahosted.org/freeipa/ticket/3150
>>>>>>
>>>>>>
>>>>>> Patch 0086:
>>>>>> I found an old unused function while working on this, the patch
>>>>>> removes it.
>>>>>>
>>>>>> Patch 0087:
>>>>>> Replica files generated on older masters don't contain the Firefox
>>>>>> extension files. Skip installing them in this case.
>>>>>>
>>>>>> Patch 0088:
>>>>>> Servers upgraded from IPA 2.2 need the Firefox extension installed. This
>>>>>> is done in ipa-upgradeconfig if they're missing.
>>>>>> I made the setup_firefox_extension method independent on the
>>>>>> httpinstance state (which is mostly set in create_instance).
>>>>>> Similarly, the files are installed ipa-replica-install if they're
>>>>>> missing (i.e. skipped by the previous patch).
>>>>>> If the Signing-Cert is not on this master, create an unsigned extension
>>>>>> using the "zip" command. I needed to add Popen's `cwd` argument to
>>>>>> ipautil.run() to get the right filenames out of zip.
>>>>>>
>>>>>> The patches add "copy_template_file" and "copy_file_if_exists" utilities
>>>>>> I've written for some of my WIP patches, expect me to use them more when
>>>>>> I get time to work on the installer code.
>>>>>>
>>>>>
>>>>> In my previous mail I've attached an old version of patch 88. Please use
>>>>> this one. Sorry!
>>>>>
>>>>>
>>>>
>>>> nack
>>>>
>>>> 1) patch 83-01 doesn't apply.
>>>
>>> There were conflicts with recent CRL and audit cert renewal patches. Rebased.
>>>
>>>> 2) When pwd is supplied to setup_firefox_extension `db =
>>>> certs.CertDB(realm, subject_base=subject_base)` is skipped and therefore
>>>> `db.has_nickname` will fail.
>>>
>>> Thanks for the catch, fixed.
>>>
>>
>> I tried different installation and upgrade procedures and it seems to work
>> fine. I have found few minorish issues when inspecting the code:
>>
>> Patch 0086-01 looks OK.
>>
>> Patch 0087-01 looks OK.
>>
>> Patch 0088-02:
>>
>> 1) In http.setup_firefox_extension() - why do you require subject_base? AFAIK,
>> it is not needed for the signtool and you do not have it right anyway:
>>   a) You use 0=$REALM, i.e. *zero*=$REALM, which would not be a valid subject
>> base anyway
>>   b) Even when it would be used, a correct subject base is in IPA config (it
>> does not have to be O=$REALM.
>>
>> Thus, I would not require it at all, it would safe us some code and potential
>> confusion if subject base would be actually used.
>>
>>
>> 2) [nitpick] In http.setup_firefox_extension() I would not format the string
>> before logging:
>>
>> +            root_logger.info(
>> +                '%s exists, skipping install of Firefox extension' %
>> +                    target_fname)
>>
>> A desired pattern would be to pass formatting parameters as standard function
>> parameters, it may save us few cycles in some situations.
>>
>>
>> 3) In httpinstance.py, I would like to see an absolute path to zip executable.
>> It is a common pattern in IPA and more secure:
>>
>> +            ipautil.run(['zip', '-r', target_fname] + filenames, cwd=extdir)
>>
>>
>> Martin
>>
> 
> Thanks, fixed in attached patch.
> 

ACK. Pushed all three patches to master, ipa-3-0.

Martin