[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
ftweedal at redhat.com
Thu May 22 08:52:40 UTC 2014
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
More information about the Pki-devel
mailing list