CAS 6.6.8 - performance issue in AbstractPrincipalAttributesRepository and support of custom error messages in delegated authentication

[Jean-noel RIBETTE]

Hi,

I've been doing some work with CAS 6.6.8 lately and come up with two problems.

First one is about the synchronized block in AbstractPrincipalAttributesRepository.retrievePersonAttributesFromAttributeRepository. In our performance test, we got dozen of threads waiting for this lock. Do you know why is what added ? Does the lock have something to do with getting the IPersonAttributeDao from Spring, or the default implementation of IPersonAttributeDao not being thread safe ? We are using a custom implementation of IPersonAttributeDao so maybe we can just remove the lock in a customized version of AbstractPrincipalAttributesRepository ? Or is it not supposed to be used in production ?

We also got a issue with the lock being null sometime, probably after the objet got deserialized. I didn't investigate this issue yet, which may be related to our use case. We just override the method with a non transient lock for now.

Second one is about the handling of exceptions coming from delegated authentication. We have a custom AuthenticationHandler handling the pac4j profile which eventually throws an exception if it doesn't accept the pac4j profile. We'd like to display proper messages to the user in those cases. I thought I could use the handling of the exception in casDelegatedAuthnStopWebflow.html (see the code rootCauseException). However the rootCauseException is set if the code fails in DelegatedClientAuthenticationAction.populateContextWithClientCredential but not in DelegatedClientAuthenticationAction.finalizeDelegatedClientAuthentication as DefaultAuthenticationManager catch the exception and convert it into a failure Event. We finally customized DelegatedClientAuthenticationFailureAction to extract the exception from the Event, but I wonder if this is the right way to go for our requirements. I'm willing to provide a PR to use some CasWebflowExceptionHandler in DelegatedClientAuthenticationFailureAction as in AuthenticationExceptionHandlerAction to make it easier to configure custom error message if you're interested.

Best regards and thanks to all for the great job done in CAS.

Jean-Noël RIBETTE

[Misagh]

> First one is about the synchronized block in AbstractPrincipalAttributesRepository.retrievePersonAttributesFromAttributeRepository. In our performance test, we got dozen of threads waiting for this lock. Do you know why is what added ?

This is very old code that probably can be taken out now. If you run
enough tests to show/prove that removing this does not cause any
adverse effects, we can certainly remove this with a PR.

> However the rootCauseException is set if the code fails in DelegatedClientAuthenticationAction.populateContextWithClientCredential but not in DelegatedClientAuthenticationAction.finalizeDelegatedClientAuthentication as DefaultAuthenticationManager catch the exception and convert it into a failure Event.

I'd have to see the proposed changeset in the PR, but generally
speaking losing a cause is a bug.