On 5/11/2015 1:39 PM, Christina Fu wrote:
This updated patch address the issue that Endi found which would cause startup to fail for anonymous access.
thanks, Christina
More comments: 1. Let's not use Hungarian notation in new code: protected ILogger mSignedAuditLogger = CMS.getSignedAuditLogger(); 2. The PKIRealm and the REST method may be executed by different threads (see ticket #1054), so the following code may not work as intended: SessionContext ctx = SessionContext.getContext(); ctx.put(SessionContext.AUTH_MANAGER_ID, ...); The code should be moved into SessionContextInterceptor and the authManager should be obtained/inferred from the authToken: PKIPrincipal pkiPrincipal = (PKIPrincipal) principal; IAuthToken authToken = pkiPrincipal.getAuthToken(); String authManager = ... SessionContext ctx = SessionContext.getContext(); ctx.put(SessionContext.AUTH_MANAGER_ID, authManager); 3. If the authentication fails for any reason, the subject ID and attempted audit UID are not consistently logged. In PKIRealm.authenticate(username, password) the variables are initialized as follows: auditSubjectID = ILogger.UNIDENTIFIED; attemptedAuditUID = username; In PKIRealm.authenticate(certs) the variables are initialized as follows: auditSubjectID = getAuditUserfromCert(certs[0]); attemptedAuditUID = ILogger.UNIDENTIFIED; Probably the second one should have been like this: auditSubjectID = ILogger.UNIDENTIFIED; attemptedAuditUID = getAuditUserfromCert(certs[0]); 4. The PKIRealm.authenticate(certs) will not be called if there's no client certificate, so the following check is unnecessary: if (certs.length == 0) { logDebug("missing client cert"); } 5. The getAuditUserfromCert() can be simplified as follows: String certUID = clientCert.getSubjectDN().getName(); return StringUtils.stripToNull(certUID); See http://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/co...) 6. Not sure how important this is, but the simple class name and method name may not be enough to uniquely identify a method: auditInfo = clazz.getSimpleName() + "." + method.getName() To properly identify a method we need to use the full class name, the method name, and the method signature. Unfortunately there's no built-in Java method to generate the signature. Possible solution: http://www.java2s.com/Code/Java/Reflection/Methodsignature.htm Another solution using objectweb library: http://asm.ow2.org/asm33/javadoc/user/org/objectweb/asm/Type.html#getMethodD...) Or just leave it as is for now. 7. Let's not use negative variable name: boolean noAuthzRequired = false; since it will be more difficult to follow the logic (double negatives !noAuthzRequired). This would be better: boolean authzRequired = true; // equivalent to aclMapping != null 8. It's not necessary to check if securityContext is null since it's a context variable provided by Tomcat: if (securityContext != null) principal = securityContext.getUserPrincipal(); 9. The catch clause for IOException can be moved up just to surround the operation that might generate it: try { loadProperties(); } catch (IOException e) { ... } -- Endi S. Dewata