More feedback. 1. Revocation does not seem right. There are two kinds of revocations: 1) revoking a cert as an agent 2) revoking a cert as a user We have implemented type 1 only so far. If you look at CertRevokeCLI, you can see that it basically does the following: cert_data = review_cert(cert_id) cert_revocation_request = certRevocationRequest(....) request_cert_revocation(cert_id, cert_revocation_request) Note that the first step (review_cert) requires you to be an agent and is used to obtain a nonce. The second step must be done within the same session. As a simplification, you dont need to provide these details to the end-user of the API. Instead, you can have the following call: revoke_cert(cert_id, revocation_reason=None, invalidity_date=None, comments = None, is_ca_cert = False) This method will automatically call review_cert and get the nonce, generate the revocation request and submit the revocation. No need to expose revocation request to the end user. You should note that this method is to be used by agents ie. not for self revocation. We'll have to create a self revocation method later. 2. You should also provide : hold_cert(cert_id, comments = None) this will call revoke_cert(cert_id, revocation_reason = CERTIFICATE_HOLD, comments=comments) 3. I really don't understand what CertReleaseHold is doing. We need to figure this out and fix it. Its not clear to me that you need an interface to unrevoke a cert. It seems to be a special case of revoking a cert with revocation reason REMOVE_FROM_CRL On Fri, 2014-04-25 at 09:02 -0400, Ade Lee wrote: > First feedback right off is that the code that will be in > create_and_submit_request() should be placed in a code snippet under > that method. You can say something like - > > The implementation of this method will likely look something like this. > {code snippet} > > Then in the code samples, you use the create_and_submit_request() call > instead. The point of the code samples is partly to show how easy it is > to use the API. You do that in the second sample - do it with the first > one too. > > In the code samples, the PKIConnection constructor call should include > the host/port of the CA (ca_host, ca_port) > > Also, the code samples should show the actual cert as well. > > You should also show an example of listing certs and getting a specific > cert, and another example of listing cert requests and getting cert > requests. > > Ade > > > On Fri, 2014-04-25 at 01:38 -0400, Abhishek Koneru wrote: > > I think i addressed all the comments and added the code snippets for user and server cert enrollment. > > Please review the document and share your views - i will post it to pki-devel and ipa-samba-team-list > > after you take a look at it. > > > > http://pki.fedoraproject.org/wiki/Python-cert-client-design > > > > --Abhishek > > > > ----- Original Message ----- > > From: "Endi Sukma Dewata" <edewata(a)redhat.com&gt; > > To: "Abhishek Koneru" <akoneru(a)redhat.com&gt;, "alee" <alee(a)redhat.com&gt; > > Sent: Wednesday, April 23, 2014 11:36:57 PM > > Subject: Re: Fwd: [Pki-devel] Python cert client design document > > > > Some comments about the formatting: > > > > 1. Let's not use preformatted text for the class/method descriptions. > > Some lines are too short/long it's hard to read. Preformatted text is > > good for code, not for paragraphs. > > > > 2. Let's use different level of headers for the class/methods so they > > can be edited individually and will appear in the ToC. > > > > 3. Please use bullet list for the attributes. It's easier to read and we > > can add more info to each attribute. > > > > 4. Please put the sample code into a separate section and keep the > > preformatted text style. > > > > Refer to this page for formatting: > > http://www.mediawiki.org/wiki/Help:Formatting > > > > More comments below. > > > > On 4/23/2014 2:46 PM, Abhishek Koneru wrote: > > >>> Please review the wiki page that documents all the classes and methods required > > >>> to implement the CertClient for the python client. > > >>> > > >>> Page:http://pki.fedoraproject.org/wiki/Python-cert-client-design > > >>> > > >>> Thanks, > > >>> Abhishek > > >> > > >> Hello, > > >> Did someone ask for API criticism? > > >> To me, this looks somewhat complicated to use. > > >> > > >> CertId: is this a class containing a single int/string? Why not just use > > >> a int/string instead? > > > > > > The method parameters can be string. We can create the CertId object > > > internally. > > > > Agreed. The Java API probably can take a string ID too. > > > > >> CertDataInfos/CertRequestInfos: Why not just use a list? > > >> > > > Need help on this. > > > > This is needed for pagination of the search results. With pagination we > > don't get the whole results at once. We only get one page at a time. See > > the base class of the equivalent Java classes. The class contains the > > total number of search results, the entries in the requested page, and > > the REST URLs of those entries. > > > > >> CertSearchRequest, Cert[Un]RevokeRequest: Could you use keyword > > >> arguments for the find_certs/[un]revoke_cert functions instead? (It's > > >> not a sin to have many keyword arguments.) > > > > > > We can add all the attributes of the requests as parameters along with a > > > parameter for the request object. This would make the methods easier to > > > use. > > > > Right. Please add more sample code showing how the parameters are supplied. > > > > >> {approve_,cancel_,...}request functions: Do these need to take > > >> request_id? Could CertReviewResponse contain it already? Even better, > > >> could they be methods on CertReviewResponse? Any function that takes an > > >> object of a custom class to operate on is probably better as a method. > > >> > > > I believe request id is available in the CertReviewResponse object. We > > > can remove that parameter and fetch the request id from the > > > CertReviewReponse object on the server side in the DAO method. > > > > Yes. The request ID probably can be removed from the Java API as well. > > See comments below about adding methods in the response object. > > > > >> The create_and_submit_request pseudocode could probably be simplified to > > >> something like: > > >> > > >> request = create_request(profile_id, inputs) > > >> request.validate() > > >> certs = [] > > >> for request_info in request.enroll(): > > >> review_response = request_info.review() > > >> review_response.approve() > > >> certs.append(review_response.get_cert()) > > >> return certs > > > > Interesting suggestion, but I'm not sure if we should do this. For > > approve() to work the review_response needs to have an active connection > > to the server. If the object is obtained from a server operation like > > above it may work. But if the object is constructed manually or loaded > > from a file it will not have a connection, so some methods could fail. > > Also, it will be difficult to distinguish between methods that require a > > connection and the ones that don't. > > > > >> Note: The dictionary specs are better written {profile_attribute_name: > > >> value}; I read [a, b] as a two-element list. > > > > Right. We should use a Python dictionary syntax like above or a > > mathematical syntax like (a, b) in the docs. > > > > Additional comments: > > > > 5. We probably want to merge list_certs() and find_certs() because they > > are kind of redundant. This also applies to the Java API. > > > > 6. The sample code should be a real working code instead of a pseudo > > code because people will copy this code to write real applications. It's > > hard to follow the example if the code doesn't match the actual API > > (e.g. there's no client object, there's no validate_request()). > > > > 7. Please declare the variable and give sample values in the sample > > code. For example, profile_id = "caUserCert". > > > > 8. I don't think we should mention the REST URLs in this document. It's > > irrelevant to the API and some of the URLs listed in this doc is incomplete. > > > > That's it for now. Let's have another pass at this document later. Thanks. > > >