Re: [Pki-devel] [PATCH] 0005 TPS User Authenticator Framework
On 5/8/2014 10:52 PM, Christina Fu wrote:
This patch implements ticket https://fedorahosted.org/pki/ticket/879
TPS
Rewrite: Implement User Authentication
This patch provides the framework that allows people to
1. write their own authentication plugins using the authentication
plugin framework
2. map the authenticaiton credential from client side (e.g. ESC or
alike)
in both display language characters and numbers of credential
parameters
to the specified authentication plugin required parameters.
I will create a separate ticket to provide plugin instruction much like
http://pki.fedoraproject.org/wiki/PKI_Authentication_Plug-ins to RHCS
Please review.
Thanks!
Christina
Some comments:
1. As discussed over IRC, there are legacy properties that should be
updated so that the UI can manage the authenticator configuration.
target.Authentication_Sources.displayname=Authentication Source
target.Authentication_Sources.list=0,1
target.Authentication_Sources.pattern=auth\.instance\.$name\..*
2. Also discussed over IRC, in AuthenticationManager initAuthInstances()
the code ignores createAuthentication() failures and continues with the
loop. I'd suggest we don't catch the exception so the problem can be
detected early.
3. In general we should avoid using Hashtable because it's synchronized
(i.e. slower). Unless synchronization is required, a HashMap or
LinkedHashMap would be a better alternative. See:
http://stackoverflow.com/questions/40471/differences-between-hashmap-and-...
4. Checking both for null and empty string can be simplified with
StringUtils.isEmpty():
http://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache...
if (title == null || title.equals("")) {
if (description == null || description.equals("")) {
if (name == null || name.equals("")) {
if (desc == null || desc.equals("")) {
if (prefix == null || prefix.equals("")
|| tokenType == null || tokenType.equals("")) {
if (op == null || op.equals("")
|| userAuth == null || userCred == null) {
if (op == null || op.equals("") ||
cuid == null || cuid.equals("") || auth == null || extensions ==
null) {
if (title==null || title.equals(""))
if (description==null || description.equals(""))
if (parameters == null || title == null || title.equals("") ||
description == null || description.equals("") || auth == null)
5. In TPSProcessor.authenticateUser() if aToken is null it will throw an
exception, then the exception will be caught and thrown again.
try {
IAuthToken aToken = auth.authenticate(...);
if (aToken != null)
...
else {
throw new TPSException(...);
}
} catch (Exception e) {
throw new TPSException(...);
}
To avoid this problem the code can be changed to only catch EBaseException.
Everything else is good. ACK.
--
Endi S. Dewata