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@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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel