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

[Christina Fu]

Thanks to Endi and Jack for your review comments and ACK.

All comments addressed and checked in:
commit 7c1fc987bdd28b70eee1a5a0bf18c252bb31fa3f

Ticket #879 has now been completed and closed.

Christina

On 05/12/2014 11:20 AM, Endi Sukma Dewata wrote:
> 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.
>