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

[Martin Babinsky]

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

-- 
Martin^3 Babinsky