Bug 61795

Summary: Make JASPIC callback handler class configurable via a property of the authenticator
Product: Tomcat 8 Reporter: Lazar Kirchev <lazar.kirchev>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement    
Priority: P2    
Version: 8.5.23   
Target Milestone: ----   
Hardware: PC   
OS: All   

Description Lazar Kirchev 2017-11-21 13:17:06 UTC
As discussed in the mailing list, it could be useful to provide a property of the authenticator with which to specify the name of a custom JASPIC callback handler implementation.

This pull request https://github.com/apache/tomcat/pull/93 illustrates the idea.

If we agree that this is OK and this will be the name of the property, I will also add it to the documentation.

I have not included a test because I had some concerns. I want to test only the instantiation of the Callback Handler instance, but this is done in a private method. So I see several approaches:

- in such cases I sometimes make the method package private so that it can be accessed by the test. However, I see that this is not an accepted practice in the Tomcat code, so I do not want to use it

- call the method with reflection - I see that in some Tomcat tests reflection is actually used

- use a lot of mocking and test via the authenticate method of a child class of the AuthenticatorBase

- go for a test with a Tomcat instance and and a sample application and test the whole scenario of JAPSIC authentication with custom callback handler 

Which of these approaches do you prefer?
Comment 1 Mark Thomas 2017-11-21 21:25:51 UTC
The approach looks good to me.

a) The package private approach has been used in a few places. It is correct that it isn't often the preferred approach.

b) Reflection might be the simplest option here.

c) Mocking is also an option although not one that is used very often.

d) A test with a Tomcat instance is often used - especially when the amount of re-use possible means minimal additional test code needs to be written.

Ultimately it is your call. My own preference would be for b) or d) followed by c) then a) but I'm not the one implementing this. If you have a strong preference, go with that.
Comment 2 Lazar Kirchev 2017-11-22 08:30:59 UTC
Thanks Mark! As I want to test only the creation logic, I also prefer b). 

I updated the pull request with tests and added the property to the documentation.
Comment 3 Mark Thomas 2017-11-22 20:49:42 UTC
Yet again, many thanks. Patch applied.

Fixed in:
- trunk for 9.0.2 onwards
- 8.5.x for 8.5.24 onwards