2:24 p.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel