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

[Martin Kosek]

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