[Pki-devel] [PATCH] 92-2 Fixes for comments on PATCH 92- Implemented the methods for Cert resource in the CertClient on the python side(Dogtag 10)

[Fraser Tweedale]

On Thu, May 08, 2014 at 10:10:34AM -0400, Abhishek Koneru wrote:
> Please review the attached patch with fixes for Ade's comments on IRC.
> 
> - Fix all the errors/warnings and some typos shown by PyCharm.
> - Add Link class which stores information of the Link object.
> - Add an internal method _submit_revoke_request and for the common code
> in revoke_cert and revoke_ca_cert.
> - Mention the need of agent authentication for the methods where it is
> required.
> - Default revocation_reason should be REASON_UNSPECIFIED
> - Add the decorator pki.handle_exceptions() to handle the PKIExceptions
> sent by the server.
> 
> --Abhishek
> On Mon, 2014-05-05 at 06:46 -0400, Abhishek Koneru wrote:
> > Please review the attached patch which implements the Cert resource
> > methods in CertClient on the python side. 
> > 
> > --Abhishek 
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
> 

This change (along with patch 93) works fine for the examples in the
``main`` method.  I'm going to test it some more, though.

Some stylistic comments inline below.

Cheers,
 Fraser

8< SNIP >8

>  class CertSearchRequest(object):
> +    """
> +        An object of this class is used to store the search parameters
> +        and send them to server.
> +    """
>  
> -    def __init__(self):
> -        self.serialNumberRangeInUse = False
> -        self.serialTo = None
> -        self.serialFrom = None
> -        self.subjectInUse = False
> -        self.eMail = None
> -        self.commonName = None
> -        self.userID = None
> -        self.orgUnit = None
> -        self.org = None
> -        self.locality = None
> -        self.state = None
> -        self.country = None
> -        self.matchExactly = None
> -        self.status = None
> -        self.revokedBy = None
> -        self.revokedOnFrom = None
> -        self.revokedOnTo = None
> -        self.revocationReason = None
> -        self.issuedBy = None
> -        self.issuedOnFrom = None
> -        self.issuedOnTo = None
> -        self.validNotBeforeFrom = None
> -        self.validNotBeforeTo = None
> -        self.validNotAfterFrom = None
> -        self.validNotAfterTo = None
> -        self.validityOperation = None
> -        self.validityCount = None
> -        self.validityUnit = None
> -        self.certTypeSubEmailCA = None
> -        self.certTypeSubSSLCA = None
> -        self.certTypeSecureEmail = None
> -        self.certTypeSSLClient = None
> -        self.certTypeSSLServer = None
> -        self.revokedByInUse = False
> -        self.revokedOnInUse = False
> -        self.revocationReasonInUse = None
> -        self.issuedByInUse = False
> -        self.issuedOnInUse = False
> -        self.validNotBeforeInUse = False
> -        self.validNotAfterInUse = False
> -        self.validityLengthInUse = False
> -        self.certTypeInUse = False
> +    search_params = {'serial_to': 'serialTo', 'serial_from': 'serialFrom',
> +                     'email': 'eMail', 'common_name': 'commonName', 'user_id': 'userID',
> +                     'org_unit': 'orgUnit', 'org': 'org', 'locality': 'locality',
> +                     'state': 'state', 'country': 'country', 'match_exactly': 'matchExactly',
> +                     'status': 'status', 'revoked_by': 'revokedBy', 'revoked_on_from': 'revokedOnFrom',
> +                     'revoked_on_to': 'revokedOnTo', 'revocation_reason': 'revocationReason',
> +                     'issued_by': 'issuedBy', 'issued_on_from': 'issuedOnFrom', 'issued_on_to': 'issuedOnTo',
> +                     'valid_not_before_from': 'validNotBeforeFrom', 'valid_not_before_to': 'validNotBeforeTo',
> +                     'valid_not_after_from': 'validNotAfterFrom', 'valid_not_after_to': 'validNotAfterTo',
> +                     'validity_operation': 'validityOperation', 'validity_count': 'validityCount',
> +                     'validity_unit': 'validityUnit', 'cert_type_sub_email_ca': 'certTypeSubEmailCA',
> +                     'cert_type_sub_ssl_ca': 'certTypeSubSSLCA', 'cert_type_secure_email': 'certTypeSecureEmail',
> +                     'cert_type_ssl_client': 'certTypeSSLClient', 'cert_type_ssl_server': 'certTypeSSLServer'}
> +
> +    def __init__(self, **cert_search_params):
> +        """ Constructor """
> +
> +        if len(cert_search_params) == 0:
> +            setattr(self, 'serialNumberRangeInUse', True)
> +
> +        for param in cert_search_params:

Here you could iterate the (key, value) pairs to save keystrokes and
a bit of CPU doing ``cert_search_params[param]`` in all those places
below.  e.g.:

    for param, value in cert_search_params.viewitems():
        ...

``dict.viewitems()`` returns a *dictview* iterator-like object that
avoids creating an intermediate list as ``dict.items()`` would.

> +            if not param in CertSearchRequest.search_params:
> +                raise ValueError('Invalid search parameter: ' + param)
> +
> +            if param == 'serial_to' or param == 'serial_from':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'serialNumberRangeInUse', True)
> +
> +            if param == 'email' or param == 'common_name' or param == 'user_id' or param == 'org_unit' \
> +                    or param == 'org' or param == 'locality' or param == 'state' or param == 'country' \
> +                    or param == 'match_exactly':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'subjectInUse', True)

Here and in a few other places below it might improve readability to
test for set membership, e.g.:

    if param in {
        'email', 'common_name', 'user'_id', 'org_unit',
        'org', ...
    }:
        setattr...

> +
> +            if param == 'status':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +
> +            if param == 'revoked_by':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'revokedByInUse', True)
> +
> +            if param == 'revoked_on_from' or param == 'revoked_on_to':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'revokedOnInUse', True)
> +
> +            if param == 'revocation_reason':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'revocationReasonInUse', True)
> +
> +            if param == 'issued_by':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'issuedByInUse', True)
> +
> +            if param == 'issued_on_from' or param == 'issued_on_to':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'issuedOnInUse', True)
> +
> +            if param == 'valid_not_before_from' or param == 'valid_not_before_to':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'validNotBeforeInUse', True)
> +
> +            if param == 'valid_not_after_from' or param == 'valid_not_after_to':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'validNotAfterInUse', True)
> +
> +            if param == 'validity_operation' or param == 'validity_count' or param == 'validity_unit':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'validityLengthInUse', True)
> +
> +            if param == 'cert_type_sub_email_ca' or param == 'cert_type_sub_ssl_ca' \
> +                    or param == 'cert_type_secure_email' or param == 'cert_type_ssl_client' \
> +                    or param == 'cert_type_ssl_server':
> +                setattr(self, CertSearchRequest.search_params[param], cert_search_params[param])
> +                setattr(self, 'certTypeInUse', True)

8< SNIP >8