[Freeipa-devel] [PATCH] 0102..0105 Better handling for cert-request to disabled CA

[Martin Babinsky]

On 09/06/2016 04:51 PM, Fraser Tweedale wrote:
> On Tue, Aug 30, 2016 at 10:54:32AM +0200, Martin Babinsky wrote:
>> On 08/26/2016 04:19 AM, Fraser Tweedale wrote:
>>> The attached patches add better handling of cert-request failure due
>>> to target CA being disabled (#6260).  To do this, rather than go and
>>> do extra work in Dogtag that we would depend on, instead I bite the
>>> bullet and refactor ra.request_certificate to use the Dogtag REST
>>> API, which correctly responds with status 409 in this case.
>>>
>>> Switching RA to Dogtag REST API is an old ticket (#3437) so these
>>> patches address it in part, and show the way forward for the rest of
>>> it.
>>>
>>> These patches don't technically depend on patch 0101 which adds the
>>> ca-enable and ca-disable commands, but 0101 may help for testing :)
>>>
>>> Thanks,
>>> Fraser
>>>
>>>
>>>
>>
>> Hi Fraser,
>>
>> PATCH 102:
>>
>> LGTM, but please use the standard ":param " annotations in the docstring for
>> `_ssldo` method. It will make out life easier if we decide to use Sphinx or
>> similar tool to auto-generate documentation from sources.
>>
>> You can also add ":raises:" section describing that RemoteRetrieveError is
>> raised when use_session is True but the session cookie wasn't acquired. It
>> is kind of obvious but it may trip the uninitiated.
>>
>> PATCH 103:
>>
>> Due to magical behavior of our public errors, the exception body should look
>> like this:
>>
>> --- a/ipalib/errors.py
>> +++ b/ipalib/errors.py
>> @@ -1413,10 +1413,7 @@ class HTTPRequestError(RemoteRetrieveError):
>>      """
>>
>>      errno = 4035
>> -
>> -    def __init__(self, status=None, **kw):
>> -        assert status is not None
>> -        super(HTTPRequestError, self).__init__(status=status, **kw)
>> +    format = _('Request failed with status %(status)s: %(reason)')
>>
>> The format string will be then automatically be supplied with status and
>> reason if you pass them to the constructor ass you already do. The errors
>> will be also handled magically (such as status which is None etc.)
>>
>> PATCH 104:
>>
>> 1.) please don't use bare except here:
>>
>> """
>> +        try:
>> +            resp_obj = json.loads(http_body)
>> +        except:
>> +            raise errors.RemoteRetrieveError(reason=_("Response from CA was
>> not valid JSON"))
>> """
>>
>> use 'except Exception' at least.
>>
>> PATCH 105:
>>
>> +            if e.status == 409:  # pylint: disable=E1101
>> +                raise errors.CertificateOperationError(
>> +                    error=_("CA '%s' is disabled") % ca)
>> +            else:
>> +                raise e
>> +
>>
>> please use named errors instead of error codes in pylint annotations:
>> # pylint: disable=no-member
>>
> Thanks for your review, Martin.  Updated patches attached; they
> address all mentioned issues.
>
> Cheers,
> Fraser
>

Thanks, ACK.

Pushed to:
ipa-4-4: b8491490c2dbb3b2db3ce64cd154b499142bc250
master: 520ad7d865ff147d3ff8819d3e384d7cbd69bfb7

-- 
Martin^3 Babinsky