[Pki-devel] [PATCH] 0005 TPS User Authenticator Framework

[Endi Sukma Dewata]

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-hashtable

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/commons/lang/StringUtils.html#isEmpty(java.lang.String)

   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