4:05 p.m.
Please review the patch with implementations for review and approval of
a cert request using CLI.
--Abhishek Koneru
12:11 p.m.
On 7/24/2012 4:05 PM, Abhishek Koneru wrote:
Please review the patch with implementations for review and approval
of
a cert request using CLI.
I have not been able to run it successfully since I'm still working on
auth, but I have some comments below. I'll test this once I have a
working environment.
1. The CertRequestApproveCLI.printHelp() generates a message like this:
usage: cert -U <uri> -d <Certificate database directory>
-w <password> -n <cert> request-approve <request id>
The command name is split into "cert ... request-approve". It's supposed
to show "cert-request-approve". Also it's not necessary to show the -U,
-d, -w, -n options because they are part of the MainCLI's options. You
can view those options by executing "pki" without any parameters.
2. Same as #1 for CertRequestReviewCLI.printHelp().
3. In CAEnrollProfile.java there are some "Check Point <n>" debug
messages. I'd say we should either replace them with messages that
explain what the code is doing or just remove them.
4. In CertRequestResource.java the MediaType.TEXT_XML is actually not
needed. We should use MediaType.APPLICATION_XML for XML content type.
5. The CertRequestResourceService.assignRequest() returns an empty
CertRequestInfos. It doesn't seem to be necessary, but I haven't tested it.
6. In CertRequestResourceService.java:163 the original exception should
be attached to the new exception as follows:
throw new CMSException("Problem approving request in CertRequestRes
urce.assignRequest!", e);
7. In CertRequestResourceService.java:165 the debug message
"RequestNotFound" below is not necessary because the toString() will
include the exception name in the output. It should be just:
CMS.debug(e);
--
Endi S. Dewata
3:33 p.m.
New subject: [PATCH] 26-2 Fixes for review comments on[PATCH] 26 CLI - Cert Request Review and Cert Request Approve Implementation
Please find attached the fixes for comments given for Patch26.
--Abhishek Koneru
On Thu, 2012-07-26 at 12:11 -0500, Endi Sukma Dewata wrote:
On 7/24/2012 4:05 PM, Abhishek Koneru wrote:
> Please review the patch with implementations for review and approval of
> a cert request using CLI.
I have not been able to run it successfully since I'm still working on
auth, but I have some comments below. I'll test this once I have a
working environment.
1. The CertRequestApproveCLI.printHelp() generates a message like this:
usage: cert -U <uri> -d <Certificate database directory>
-w <password> -n <cert> request-approve <request id>
The command name is split into "cert ... request-approve". It's supposed
to show "cert-request-approve". Also it's not necessary to show the -U,
-d, -w, -n options because they are part of the MainCLI's options. You
can view those options by executing "pki" without any parameters.
2. Same as #1 for CertRequestReviewCLI.printHelp().
3. In CAEnrollProfile.java there are some "Check Point <n>" debug
messages. I'd say we should either replace them with messages that
explain what the code is doing or just remove them.
4. In CertRequestResource.java the MediaType.TEXT_XML is actually not
needed. We should use MediaType.APPLICATION_XML for XML content type.
5. The CertRequestResourceService.assignRequest() returns an empty
CertRequestInfos. It doesn't seem to be necessary, but I haven't tested it.
6. In CertRequestResourceService.java:163 the original exception should
be attached to the new exception as follows:
throw new CMSException("Problem approving request in CertRequestRes
urce.assignRequest!", e);
7. In CertRequestResourceService.java:165 the debug message
"RequestNotFound" below is not necessary because the toString() will
include the exception name in the output. It should be just:
CMS.debug(e);
6:39 p.m.
New subject: [PATCH] 26-2 Fixes for review comments on[PATCH] 26 CLI - Cert Request Review and Cert Request Approve Implementation
On 7/26/2012 3:33 PM, Abhishek Koneru wrote:
Please find attached the fixes for comments given for Patch26.
I've tested the patch. It works, but there's one issue:
8. The review command should store the output into a file so that the
agent can review and make changes then send it back to the server to
approve it. So the command would look something like this:
# pki cert-request-review 10 --output <file>
---------------------------------
Downloaded certificate request 10
---------------------------------
... <agent reviews & makes changes> ...
# pki cert-request-approve <file>
-------------------------------
Approved certificate request 10
-------------------------------
There is another issue, if you try to approve a request that's already
been approved it will generate an exception (the message is not nicely
displayed). This is probably due to a bug in RESTEasy where it doesn't
handle exception very well on methods that returns void. I think it will
be fixed when we upgrade to the latest RESTEasy version. See this
ticket: https://fedorahosted.org/pki/ticket/194
--
Endi S. Dewata
3:53 p.m.
New subject: [PATCH] 26-3 Fix for review comment given for [PATCH] 26-2
Please review the fix for the comment given for patch 26-2
--Abhishek Koneru
On Thu, 2012-07-26 at 18:39 -0500, Endi Sukma Dewata wrote:
On 7/26/2012 3:33 PM, Abhishek Koneru wrote:
> Please find attached the fixes for comments given for Patch26.
I've tested the patch. It works, but there's one issue:
8. The review command should store the output into a file so that the
agent can review and make changes then send it back to the server to
approve it. So the command would look something like this:
# pki cert-request-review 10 --output <file>
---------------------------------
Downloaded certificate request 10
---------------------------------
... <agent reviews & makes changes> ...
# pki cert-request-approve <file>
-------------------------------
Approved certificate request 10
-------------------------------
There is another issue, if you try to approve a request that's already
been approved it will generate an exception (the message is not nicely
displayed). This is probably due to a bug in RESTEasy where it doesn't
handle exception very well on methods that returns void. I think it will
be fixed when we upgrade to the latest RESTEasy version. See this
ticket: https://fedorahosted.org/pki/ticket/194
10:07 a.m.
New subject: [PATCH] 26-3 Fix for review comment given for [PATCH] 26-2
On 7/27/2012 3:53 PM, Abhishek Koneru wrote:
Please review the fix for the comment given for patch 26-2
ACK. Pushed to master.
Some possible improvements:
9. The "output" option can be marked as required like this:
option.setRequired(true);
This way if the option is missing it will fail during parsing, so it's
not necessary to write a code to check it.
10. The marshalling/unmarshalling code for AgentEnrollmentRequestData
can be refactored into reusable methods. For example:
public class AgentEnrollmentRequestData {
public static AgentEnrollmentRequestData valueOf(Reader reader) { }
public String toString() { }
}
11. The use of option/argument to pass the filename is not really
consistent in the submit/review/approve commands. I don't have a strong
opinion on this but here are some possibilities. With arguments the
commands will be shorter:
pki cert-request-submit <filename>
pki cert-request-review <request id> <filename>
pki cert-request-approve <filename>
With options we could have more flexibility:
pki cert-request-submit --input <filename>
pki cert-request-review <request id> --ouput <filename>
pki cert-request-approve --input <filename>
cat <filename> | pki cert-request-submit
pki cert-request-review <request id> --json <filename>
pki cert-request-approve <request id> --nonce <nonce>
We should consider other commands too (reject, cancel, update, etc.).
--
Endi S. Dewata
4290
days inactive
4296
days old
5 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata