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.