6:29 p.m.
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet upgrade,
which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format operation.
3. More progress for the format operation up until we attempt to upgrade the applet.
4. Added TPSException that holds a message and end op return code. Can be used to throw
from anywhere and the return code makes it back to the client.
---
9:18 p.m.
New subject: <pki-devel> [PATCH] 0005- Further work on TPS Processor, format operation.patch
Revised patch to address irc comments:
Mostly concerning error handling and a new way to declare the TPSEngine class.
----- Original Message -----
From: "John Magne" <jmagne(a)redhat.com>
To: "pki-devel" <pki-devel(a)redhat.com>
Sent: Thursday, March 13, 2014 4:29:17 PM
Subject: [Pki-devel] <pki-devel> [PATCH] 0004- Further work on TPS Processor,
format operation.patch
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet
upgrade, which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format
operation.
3. More progress for the format operation up until we attempt to upgrade the
applet.
4. Added TPSException that holds a message and end op return code. Can be
used to throw from anywhere and the return code makes it back to the client.
---
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
12:10 p.m.
New subject: <pki-devel> [PATCH] 0005- Further work on TPS Processor, format operation.patch
On 3/17/2014 9:18 PM, John Magne wrote:
Revised patch to address irc comments:
Mostly concerning error handling and a new way to declare the TPSEngine class.
Some comments/questions:
1. Is the TPSFormatProcessor still needed since the TPSSession can use
TPSProcessor directly? Or maybe should TPSProcessor.format() be moved
into TPSFormatProcessor?
2. As discussed over IRC, the TPSProcessor.format() right now is using
two mechanisms to return the status: via exception and via return value.
Will it be changed to use one mechanism in the future? Also already
discussed, the code that checks the return value of the check*() method
can actually be moved into the check*() method itself, then throw an
TPSException with the status if it fails the validation.
3. The cuid_x variable in getTokenType() doesn't seem to be necessary.
It looks like it can use cuid directly.
4. In the following code, the getString() will assign a default value if
the property is missing, so the exception will not happen for missing
property.
try {
mappingOrder = configStore.getString(configName, null);
} catch (EBaseException e1) {
//The whole feature won't work if this is wrong.
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Mising mapping order!",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
The exception could still happen for other errors, but in that case the
exception message above will be incorrect.
5. The loop can be simplified like this:
for (String mappingId : mappingOrder.split(",")) {
...
}
6. Similar to #4, the following code will catch an incorrect type of error:
try {
targetTokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Invalid or no targetTokenType.",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
7. Similar to #4, the following code will assign a default value if the
property is missing. If there's an exception, it must have been caused
by something else, not because of the missing property. In that case
it's better to break and report the error.
try {
tokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenType = null;
}
try {
tokenATR = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenATR = null;
}
try {
tokenCUIDStart = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenCUIDStart = null;
}
try {
tokenCUIDEnd = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
targetTokenType = null;
}
try {
majorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
majorVersion = null;
}
try {
minorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
minorVersion = null;
}
The code should either wrap EBaseException with TPSException and throw
it, or alternatively declare EBaseException in the getTokenType() and
remove the try-catch block.
8. The following lines can be combined:
int major = 0;
major = Integer.parseInt(majorVersion);
Same thing for the minor version.
9. There are checks in TPSProcessor to make sure the session is not
null. This is no longer necessary since the session is supplied when
creating the instance. If validation is necessary, it can be done just
one time in the constructor itself:
public TPSProcessor(TPSSession session) {
if (session == null) throw NullPointerException("TPS session is
null");
this.session = session;
}
--
Endi S. Dewata
----- Original Message -----
> From: "John Magne" <jmagne(a)redhat.com>
> To: "pki-devel" <pki-devel(a)redhat.com>
> Sent: Thursday, March 13, 2014 4:29:17 PM
> Subject: [Pki-devel] <pki-devel> [PATCH] 0004- Further work on TPS Processor,
format operation.patch
>
> Ticket #895 https://fedorahosted.org/pki/ticket/895
>
> Further work on TPS Processor, format operation.
>
> This patch gets a bit farther on the TPS format operation, just before applet
> upgrade, which will also need secure channel functionality.
>
> Also, patch provides some misc clean up and functionality.
>
> 1. Method to calculate the token type.
> 2. Some added convenience methods to get various config params for the Format
> operation.
> 3. More progress for the format operation up until we attempt to upgrade the
> applet.
> 4. Added TPSException that holds a message and end op return code. Can be
> used to throw from anywhere and the return code makes it back to the client.
9:03 p.m.
New subject: <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
Have addressed cfu's and edewatas comments in attached patch as per below:
Prefix feature for some reason isn't working on the email. Comments encclosed in:
<jack> comment </jack>
Edewata's comments:
Some comments/questions:
1. Is the TPSFormatProcessor still needed since the TPSSession can use
TPSProcessor directly? Or maybe should TPSProcessor.format() be moved
into TPSFormatProcessor?
<jack>
As per you and cfu, this one is history. We can always bring
it back if we need to. The other processors to come will have significant content.
</jack>
2. As discussed over IRC, the TPSProcessor.format() right now is using
two mechanisms to return the status: via exception and via return value.
Will it be changed to use one mechanism in the future? Also already
discussed, the code that checks the return value of the check*() method
can actually be moved into the check*() method itself, then throw an
TPSException with the status if it fails the validation.
<jack>
Done, now we only return an exception. Also validations are
inside check* methods when appropriate, based on intent of method.
</jack>
3. The cuid_x variable in getTokenType() doesn't seem to be necessary.
It looks like it can use cuid directly.
<jack>
Done
</jack>
4. In the following code, the getString() will assign a default value if
the property is missing, so the exception will not happen for missing
property.
try {
mappingOrder = configStore.getString(configName, null);
} catch (EBaseException e1) {
//The whole feature won't work if this is wrong.
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Mising mapping order!",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done.
</jack>
The exception could still happen for other errors, but in that case the
exception message above will be incorrect.
5. The loop can be simplified like this:
for (String mappingId : mappingOrder.split(",")) {
...
}
<jack>
Done
</jack>
6. Similar to #4, the following code will catch an incorrect type of error:
try {
targetTokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Invalid or no targetTokenType.",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done
</jack>
7. Similar to #4, the following code will assign a default value if the
property is missing. If there's an exception, it must have been caused
by something else, not because of the missing property. In that case
it's better to break and report the error.
try {
tokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenType = null;
}
try {
tokenATR = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenATR = null;
}
try {
tokenCUIDStart = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenCUIDStart = null;
}
try {
tokenCUIDEnd = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
targetTokenType = null;
}
try {
majorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
majorVersion = null;
}
try {
minorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
minorVersion = null;
}
The code should either wrap EBaseException with TPSException and throw
it, or alternatively declare EBaseException in the getTokenType() and
remove the try-catch block.
<jack>
Done. Each exception in routine is handled based on whether we can
tolerate a default.
</jack>
8. The following lines can be combined:
int major = 0;
major = Integer.parseInt(majorVersion);
<jack>
Done
</jack>
Same thing for the minor version.
9. There are checks in TPSProcessor to make sure the session is not
null. This is no longer necessary since the session is supplied when
creating the instance. If validation is necessary, it can be done just
one time in the constructor itself:
public TPSProcessor(TPSSession session) {
if (session == null) throw NullPointerException("TPS session is
null");
this.session = session;
}
<jack>
Done
</jack>
CFU's comments:
I find it easier to just apply the patches to the tree to have a better context. Some of
the things I've found may be from the past.
* TPSEngine -
- you have defined a number TKS_RESPONSE_xxx constants which I have defined in
IRemoteRequest.java. I think IRemoteRequest.java is a better place since I have TKS
server side using those constants as well. If agreed, how about removing those 6
constants from TPSEngine? I checked and i see in your code you are not using them yet
anyway, so it should not be too much trouble.
<jack>
Done.
</jack>
* TPSFormatProcessor
- In our old tps, I have always questioned the existance of the RA_Format_Processor as
well as the other xxx_Processor's other than the RA_Processor.cpp and
RA_Enroll_Processor.cpp where the real code were in.
My question is, is there going to be an attempt of putting real format code into the
TPSFormatProcessor, etc, or we are merely going to do a direct conversion of C++ to Java?
The reason why I ask is because the TPSFormatProcessor and all probably serve not much
purpose unless we put something useful in them. We can probably leave them as is for now
and think about it later. I just want to bring it up so we can keep it in mind to ponder
on.
<jack>
Done for both cfu and edewata.
</jack>
* Would it make sense for APDUResponse to carry a boolean status so that
checkTokenPDUResponse() only needs to be processed once and status set at the time?
Anyway, if there is concern of veering off to far from the old tps course then we can keep
the return rc style.
<jack>
Done. Here I moved the method to get a boolean result into the APDUResponse class. Also
added convenience class to get the two byte result for future use.
</jack>
* I don' t see in the format() where you pass in the skipAuth value.
<jack>
The original code didnt seem to be using this. If we determine otherwise, it can be fixed
later.
I suspect this is now controlled strictly by config.
</jack>
* I may have missed it, but I don't see you checking if cplc_data < 47 (a magic
number in old tps).
<jack>
Done. I was checking this value in the subsequent methods that fish strings out of the
cplc_data.
Now have the check in cplc_data specific routine.
</jack>
* why do you change the variable name token_status to status? I think token_status, or if
you wish, tokenStatus, is more descriptive.
<jack>
Done
</jack>
* why do you check if (session == null) in the middle of assigning major and minor
versions? Could you not have checked this earlier, like near the beginning of format()?
Done. Must have been some mystery fantom paste going on there.
* the old tps might be wrong, but I just want to know why is it that it would assign 0 to
all those versions if token_status == NULL, while you bumped it out?
I believe this is the way it works. If there is no applet installed, there is no status.
* perhaps we want to consider keeping the tokenType somewhere we can retrieve (not
re-calculated and re-retrieved) easily during a session, instead of passing it around as a
parameter. Where is a good place to do that?
<jack>
Good idea, done.
</jack>
that's it for now.
Christina
On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet upgrade,
which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format
operation.
3. More progress for the format operation up until we attempt to upgrade the applet.
4. Added TPSException that holds a message and end op return code. Can be used to
throw from anywhere and the return code makes it back to the client.
1:04 p.m.
New subject: <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
I checked areas addressing my comments.
A couple things:
* those logging/audit related log parameter name defines will eventually
have to follow the other subsystems' param name style in CS.cfg (nothing
to be done.)
* I think at some point, we (both of us) have to define those exception
messages in those properties file shared by other subsystems.
Also, I'm not certain I like the introduction of TPSException. If
EBaseException is not enough to serve your purpose, you should make it a
more generic exception and put it at a place and make it available for
all subsystems.
(I'll leave it up to you, maybe consult Endi)
* I might have missed it. I see you removed the session== null check but
I don't see you putting it back near the top of format().
Consider it an ACK if you have addressed 3rd comment, and possible 2nd.
Christina
On 03/18/2014 07:03 PM, John Magne wrote:
Have addressed cfu's and edewatas comments in attached patch as
per below:
Prefix feature for some reason isn't working on the email. Comments encclosed in:
<jack> comment </jack>
Edewata's comments:
Some comments/questions:
1. Is the TPSFormatProcessor still needed since the TPSSession can use
TPSProcessor directly? Or maybe should TPSProcessor.format() be moved
into TPSFormatProcessor?
<jack>
As per you and cfu, this one is history. We can always bring
it back if we need to. The other processors to come will have significant content.
</jack>
2. As discussed over IRC, the TPSProcessor.format() right now is using
two mechanisms to return the status: via exception and via return value.
Will it be changed to use one mechanism in the future? Also already
discussed, the code that checks the return value of the check*() method
can actually be moved into the check*() method itself, then throw an
TPSException with the status if it fails the validation.
<jack>
Done, now we only return an exception. Also validations are
inside check* methods when appropriate, based on intent of method.
</jack>
3. The cuid_x variable in getTokenType() doesn't seem to be necessary.
It looks like it can use cuid directly.
<jack>
Done
</jack>
4. In the following code, the getString() will assign a default value if
the property is missing, so the exception will not happen for missing
property.
try {
mappingOrder = configStore.getString(configName, null);
} catch (EBaseException e1) {
//The whole feature won't work if this is wrong.
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Mising mapping order!",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done.
</jack>
The exception could still happen for other errors, but in that case the
exception message above will be incorrect.
5. The loop can be simplified like this:
for (String mappingId : mappingOrder.split(",")) {
...
}
<jack>
Done
</jack>
6. Similar to #4, the following code will catch an incorrect type of error:
try {
targetTokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Invalid or no targetTokenType.",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done
</jack>
7. Similar to #4, the following code will assign a default value if the
property is missing. If there's an exception, it must have been caused
by something else, not because of the missing property. In that case
it's better to break and report the error.
try {
tokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenType = null;
}
try {
tokenATR = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenATR = null;
}
try {
tokenCUIDStart = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenCUIDStart = null;
}
try {
tokenCUIDEnd = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
targetTokenType = null;
}
try {
majorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
majorVersion = null;
}
try {
minorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
minorVersion = null;
}
The code should either wrap EBaseException with TPSException and throw
it, or alternatively declare EBaseException in the getTokenType() and
remove the try-catch block.
<jack>
Done. Each exception in routine is handled based on whether we can
tolerate a default.
</jack>
8. The following lines can be combined:
int major = 0;
major = Integer.parseInt(majorVersion);
<jack>
Done
</jack>
Same thing for the minor version.
9. There are checks in TPSProcessor to make sure the session is not
null. This is no longer necessary since the session is supplied when
creating the instance. If validation is necessary, it can be done just
one time in the constructor itself:
public TPSProcessor(TPSSession session) {
if (session == null) throw NullPointerException("TPS session is
null");
this.session = session;
}
<jack>
Done
</jack>
CFU's comments:
I find it easier to just apply the patches to the tree to have a better context. Some of
the things I've found may be from the past.
* TPSEngine -
- you have defined a number TKS_RESPONSE_xxx constants which I have defined in
IRemoteRequest.java. I think IRemoteRequest.java is a better place since I have TKS
server side using those constants as well. If agreed, how about removing those 6
constants from TPSEngine? I checked and i see in your code you are not using them yet
anyway, so it should not be too much trouble.
<jack>
Done.
</jack>
* TPSFormatProcessor
- In our old tps, I have always questioned the existance of the RA_Format_Processor as
well as the other xxx_Processor's other than the RA_Processor.cpp and
RA_Enroll_Processor.cpp where the real code were in.
My question is, is there going to be an attempt of putting real format code into the
TPSFormatProcessor, etc, or we are merely going to do a direct conversion of C++ to Java?
The reason why I ask is because the TPSFormatProcessor and all probably serve not much
purpose unless we put something useful in them. We can probably leave them as is for now
and think about it later. I just want to bring it up so we can keep it in mind to ponder
on.
<jack>
Done for both cfu and edewata.
</jack>
* Would it make sense for APDUResponse to carry a boolean status so that
checkTokenPDUResponse() only needs to be processed once and status set at the time?
Anyway, if there is concern of veering off to far from the old tps course then we can keep
the return rc style.
<jack>
Done. Here I moved the method to get a boolean result into the APDUResponse class. Also
added convenience class to get the two byte result for future use.
</jack>
* I don' t see in the format() where you pass in the skipAuth value.
<jack>
The original code didnt seem to be using this. If we determine otherwise, it can be
fixed later.
I suspect this is now controlled strictly by config.
</jack>
* I may have missed it, but I don't see you checking if cplc_data < 47 (a magic
number in old tps).
<jack>
Done. I was checking this value in the subsequent methods that fish strings out of the
cplc_data.
Now have the check in cplc_data specific routine.
</jack>
* why do you change the variable name token_status to status? I think token_status, or
if you wish, tokenStatus, is more descriptive.
<jack>
Done
</jack>
* why do you check if (session == null) in the middle of assigning major and minor
versions? Could you not have checked this earlier, like near the beginning of format()?
Done. Must have been some mystery fantom paste going on there.
* the old tps might be wrong, but I just want to know why is it that it would assign 0 to
all those versions if token_status == NULL, while you bumped it out?
I believe this is the way it works. If there is no applet installed, there is no
status.
* perhaps we want to consider keeping the tokenType somewhere we can retrieve (not
re-calculated and re-retrieved) easily during a session, instead of passing it around as a
parameter. Where is a good place to do that?
<jack>
Good idea, done.
</jack>
that's it for now.
Christina
On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet
upgrade, which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format
operation.
3. More progress for the format operation up until we attempt to upgrade the
applet.
4. Added TPSException that holds a message and end op return code. Can be used to
throw from anywhere and the return code makes it back to the client.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
2:03 p.m.
New subject: <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
cfu, thanks for responses.
My comments below:
----- Original Message -----
From: "Christina Fu" <cfu(a)redhat.com>
To: pki-devel(a)redhat.com
Sent: Wednesday, March 19, 2014 11:04:19 AM
Subject: Re: [Pki-devel] <pki-devel> [PATCH] 0006- Further work on TPS Processor,
format operation.patch
I checked areas addressing my comments.
A couple things:
* those logging/audit related log parameter name defines will eventually have
to follow the other subsystems' param name style in CS.cfg (nothing to be
done.)
Yes, agreed.
* I think at some point, we (both of us) have to define those
exception
messages in those properties file shared by other subsystems.
Also, I'm not certain I like the introduction of TPSException. If
EBaseException is not enough to serve your purpose, you should make it a
more generic exception and put it at a place and make it available for all
subsystems.
cfu, TPSException is kind of specific to TPS since it carries a TPS specific
error code that goes back to the client. It is true that EBaseException has
a facility to add some generic object "parameters" of some kind, I though the
new class might add some convenience and clarity. We can argue it going forward though.
(I'll leave it up to you, maybe consult Endi)
* I might have missed it. I see you removed the session== null check but I
don't see you putting it back near the top of format().
cfu, On this one Endi had me to a null check on the session when creating the
processor object constructor.
Consider it an ACK if you have addressed 3rd comment, and possible 2nd.
Christina
On 03/18/2014 07:03 PM, John Magne wrote:
Have addressed cfu's and edewatas comments in attached patch as per below:
Prefix feature for some reason isn't working on the email. Comments encclosed
in: <jack> comment </jack>
Edewata's comments:
Some comments/questions:
1. Is the TPSFormatProcessor still needed since the TPSSession can use
TPSProcessor directly? Or maybe should TPSProcessor.format() be moved
into TPSFormatProcessor?
<jack>
As per you and cfu, this one is history. We can always bring
it back if we need to. The other processors to come will have significant
content.
</jack>
2. As discussed over IRC, the TPSProcessor.format() right now is using
two mechanisms to return the status: via exception and via return value.
Will it be changed to use one mechanism in the future? Also already
discussed, the code that checks the return value of the check*() method
can actually be moved into the check*() method itself, then throw an
TPSException with the status if it fails the validation.
<jack>
Done, now we only return an exception. Also validations are
inside check* methods when appropriate, based on intent of method.
</jack>
3. The cuid_x variable in getTokenType() doesn't seem to be necessary.
It looks like it can use cuid directly.
<jack>
Done
</jack>
4. In the following code, the getString() will assign a default value if
the property is missing, so the exception will not happen for missing
property.
try {
mappingOrder = configStore.getString(configName, null);
} catch (EBaseException e1) {
//The whole feature won't work if this is wrong.
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Mising mapping order!",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done.
</jack>
The exception could still happen for other errors, but in that case the
exception message above will be incorrect.
5. The loop can be simplified like this:
for (String mappingId : mappingOrder.split(",")) {
...
}
<jack>
Done
</jack>
6. Similar to #4, the following code will catch an incorrect type of error:
try {
targetTokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
throw new TPSException(
"TPSProcessor.getTokenType: Token Type configuration
incorrect! Invalid or no targetTokenType.",
TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND);
}
<jack>
Done
</jack>
7. Similar to #4, the following code will assign a default value if the
property is missing. If there's an exception, it must have been caused
by something else, not because of the missing property. In that case
it's better to break and report the error.
try {
tokenType = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenType = null;
}
try {
tokenATR = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenATR = null;
}
try {
tokenCUIDStart = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
tokenCUIDStart = null;
}
try {
tokenCUIDEnd = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
targetTokenType = null;
}
try {
majorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
majorVersion = null;
}
try {
minorVersion = configStore.getString(mappingConfigName, null);
} catch (EBaseException e) {
minorVersion = null;
}
The code should either wrap EBaseException with TPSException and throw
it, or alternatively declare EBaseException in the getTokenType() and
remove the try-catch block.
<jack>
Done. Each exception in routine is handled based on whether we can
tolerate a default.
</jack>
8. The following lines can be combined:
int major = 0;
major = Integer.parseInt(majorVersion);
<jack>
Done
</jack>
Same thing for the minor version.
9. There are checks in TPSProcessor to make sure the session is not
null. This is no longer necessary since the session is supplied when
creating the instance. If validation is necessary, it can be done just
one time in the constructor itself:
public TPSProcessor(TPSSession session) {
if (session == null) throw NullPointerException("TPS session is
null");
this.session = session;
}
<jack>
Done
</jack>
CFU's comments:
I find it easier to just apply the patches to the tree to have a better
context. Some of the things I've found may be from the past.
* TPSEngine -
- you have defined a number TKS_RESPONSE_xxx constants which I have defined
in IRemoteRequest.java. I think IRemoteRequest.java is a better place
since I have TKS server side using those constants as well. If agreed,
how about removing those 6 constants from TPSEngine? I checked and i see
in your code you are not using them yet anyway, so it should not be too
much trouble.
<jack>
Done.
</jack>
* TPSFormatProcessor
- In our old tps, I have always questioned the existance of the
RA_Format_Processor as well as the other xxx_Processor's other than the
RA_Processor.cpp and RA_Enroll_Processor.cpp where the real code were in.
My question is, is there going to be an attempt of putting real format code
into the TPSFormatProcessor, etc, or we are merely going to do a direct
conversion of C++ to Java? The reason why I ask is because the
TPSFormatProcessor and all probably serve not much purpose unless we put
something useful in them. We can probably leave them as is for now and
think about it later. I just want to bring it up so we can keep it in
mind to ponder on.
<jack>
Done for both cfu and edewata.
</jack>
* Would it make sense for APDUResponse to carry a boolean status so that
checkTokenPDUResponse() only needs to be processed once and status set at
the time? Anyway, if there is concern of veering off to far from the old
tps course then we can keep the return rc style.
<jack>
Done. Here I moved the method to get a boolean result into the APDUResponse
class. Also added convenience class to get the two byte result for future
use.
</jack>
* I don' t see in the format() where you pass in the skipAuth value.
<jack>
The original code didnt seem to be using this. If we determine otherwise, it
can be fixed later.
I suspect this is now controlled strictly by config.
</jack>
* I may have missed it, but I don't see you checking if cplc_data < 47 (a
magic number in old tps).
<jack>
Done. I was checking this value in the subsequent methods that fish strings
out of the cplc_data.
Now have the check in cplc_data specific routine.
</jack>
* why do you change the variable name token_status to status? I think
token_status, or if you wish, tokenStatus, is more descriptive.
<jack>
Done
</jack>
* why do you check if (session == null) in the middle of assigning major and
minor versions? Could you not have checked this earlier, like near the
beginning of format()?
Done. Must have been some mystery fantom paste going on there.
* the old tps might be wrong, but I just want to know why is it that it would
assign 0 to all those versions if token_status == NULL, while you bumped it
out?
I believe this is the way it works. If there is no applet installed, there
is no status.
* perhaps we want to consider keeping the tokenType somewhere we can retrieve
(not re-calculated and re-retrieved) easily during a session, instead of
passing it around as a parameter. Where is a good place to do that?
<jack>
Good idea, done.
</jack>
that's it for now.
Christina
On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895 Further work on TPS
Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before
applet upgrade, which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the
Format operation.
3. More progress for the format operation up until we attempt to upgrade
the applet.
4. Added TPSException that holds a message and end op return code. Can be
used to throw from anywhere and the return code makes it back to the
client.
_______________________________________________
Pki-devel mailing list Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
8:32 p.m.
New subject: <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
Just one more comment, in TPSSession read(), write(), process(), and
probably some methods in TPSProcessor, we can actually catch the
IOException, log the message, then throw the original exception object.
No need to create a new IOException wrapper because we aren't changing
the exception type.
try {
...
} catch (IOException e) {
CMS.debug(...);
throw e;
}
Everything else is good, and the code is a lot easier to read. ACK.
--
Endi S. Dewata
1:25 p.m.
New subject: <pki-devel> [PATCH] 0006- Further work on TPS Processor, format operation.patch
Ticket #895
Thanks to ACKs from edewata and cfu, pushed to master.
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: "John Magne" <jmagne(a)redhat.com>, "Christina Fu"
<cfu(a)redhat.com>
Cc: pki-devel(a)redhat.com
Sent: Wednesday, March 19, 2014 6:32:15 PM
Subject: Re: [Pki-devel] <pki-devel> [PATCH] 0006- Further work on TPS Processor,
format operation.patch
Just one more comment, in TPSSession read(), write(), process(), and
probably some methods in TPSProcessor, we can actually catch the
IOException, log the message, then throw the original exception object.
No need to create a new IOException wrapper because we aren't changing
the exception type.
try {
...
} catch (IOException e) {
CMS.debug(...);
throw e;
}
Everything else is good, and the code is a lot easier to read. ACK.
--
Endi S. Dewata
1:02 p.m.
I find it easier to just apply the patches to the tree to have a better
context. Some of the things I've found may be from the past.
* TPSEngine -
- you have defined a number TKS_RESPONSE_xxx constants which I have
defined in IRemoteRequest.java. I think IRemoteRequest.java is a better
place since I have TKS server side using those constants as well. If
agreed, how about removing those 6 constants from TPSEngine? I checked
and i see in your code you are not using them yet anyway, so it should
not be too much trouble.
* TPSFormatProcessor
- In our old tps, I have always questioned the existance of the
RA_Format_Processor as well as the other xxx_Processor's other than the
RA_Processor.cpp and RA_Enroll_Processor.cpp where the real code were in.
My question is, is there going to be an attempt of putting real
format code into the TPSFormatProcessor, etc, or we are merely going to
do a direct conversion of C++ to Java? The reason why I ask is because
the TPSFormatProcessor and all probably serve not much purpose unless we
put something useful in them. We can probably leave them as is for now
and think about it later. I just want to bring it up so we can keep it
in mind to ponder on.
* Would it make sense for APDUResponse to carry a boolean status so that
checkTokenPDUResponse() only needs to be processed once and status set
at the time? Anyway, if there is concern of veering off to far from the
old tps course then we can keep the return rc style.
* I don' t see in the format() where you pass in the skipAuth value.
* I may have missed it, but I don't see you checking if cplc_data < 47
(a magic number in old tps).
* why do you change the variable name token_status to status? I think
token_status, or if you wish, tokenStatus, is more descriptive.
* why do you check if (session == null) in the middle of assigning major
and minor versions? Could you not have checked this earlier, like near
the beginning of format()?
* the old tps might be wrong, but I just want to know why is it that it
would assign 0 to all those versions if token_status == NULL, while you
bumped it out?
* perhaps we want to consider keeping the tokenType somewhere we can
retrieve (not re-calculated and re-retrieved) easily during a session,
instead of passing it around as a parameter. Where is a good place to
do that?
that's it for now.
Christina
On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet upgrade,
which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format operation.
3. More progress for the format operation up until we attempt to upgrade the applet.
4. Added TPSException that holds a message and end op return code. Can be used to throw
from anywhere and the return code makes it back to the client.
---
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
1:02 p.m.
I find it easier to just apply the patches to the tree to have a better
context. Some of the things I've found may be from the past.
* TPSEngine -
- you have defined a number TKS_RESPONSE_xxx constants which I have
defined in IRemoteRequest.java. I think IRemoteRequest.java is a better
place since I have TKS server side using those constants as well. If
agreed, how about removing those 6 constants from TPSEngine? I checked
and i see in your code you are not using them yet anyway, so it should
not be too much trouble.
* TPSFormatProcessor
- In our old tps, I have always questioned the existance of the
RA_Format_Processor as well as the other xxx_Processor's other than the
RA_Processor.cpp and RA_Enroll_Processor.cpp where the real code were in.
My question is, is there going to be an attempt of putting real
format code into the TPSFormatProcessor, etc, or we are merely going to
do a direct conversion of C++ to Java? The reason why I ask is because
the TPSFormatProcessor and all probably serve not much purpose unless we
put something useful in them. We can probably leave them as is for now
and think about it later. I just want to bring it up so we can keep it
in mind to ponder on.
* Would it make sense for APDUResponse to carry a boolean status so that
checkTokenPDUResponse() only needs to be processed once and status set
at the time? Anyway, if there is concern of veering off to far from the
old tps course then we can keep the return rc style.
* I don' t see in the format() where you pass in the skipAuth value.
* I may have missed it, but I don't see you checking if cplc_data < 47
(a magic number in old tps).
* why do you change the variable name token_status to status? I think
token_status, or if you wish, tokenStatus, is more descriptive.
* why do you check if (session == null) in the middle of assigning major
and minor versions? Could you not have checked this earlier, like near
the beginning of format()?
* the old tps might be wrong, but I just want to know why is it that it
would assign 0 to all those versions if token_status == NULL, while you
bumped it out?
* perhaps we want to consider keeping the tokenType somewhere we can
retrieve (not re-calculated and re-retrieved) easily during a session,
instead of passing it around as a parameter. Where is a good place to
do that?
that's it for now.
Christina
On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895
Further work on TPS Processor, format operation.
This patch gets a bit farther on the TPS format operation, just before applet upgrade,
which will also need secure channel functionality.
Also, patch provides some misc clean up and functionality.
1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format operation.
3. More progress for the format operation up until we attempt to upgrade the applet.
4. Added TPSException that holds a message and end op return code. Can be used to throw
from anywhere and the return code makes it back to the client.
---
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
3699
days inactive
3706
days old
9 comments
3 participants
participants (3)
-
Christina Fu -
Endi Sukma Dewata -
John Magne