[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)

Ade Lee alee at redhat.com
Fri May 23 19:24:44 UTC 2014


In general it looks pretty good.  Some minor points:

1. pycharm appears to be set to 120 columns width by default.  We need
to set to 80 and reformat accordingly.  Please check in a pycharm
settings file.  All new code should follow PEP8.

2. In CertRevokeRequest, a number of constants are defined for possible
reason settings.  You should group those, and test for invalid reasons.

3.  Do we use CertID anywhere?

Other than that, looks pretty good.  ACK once you have addressed these
issues.

Ade
On Thu, 2014-05-22 at 18:52 +1000, Fraser Tweedale wrote:
> 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
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel





More information about the Pki-devel mailing list