Bug 61784 - NPE if AuthConfigFactoryImpl.registerConfigProvider() is called with null provider name parameter
Summary: NPE if AuthConfigFactoryImpl.registerConfigProvider() is called with null pro...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.23
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-18 14:11 UTC by Lazar Kirchev
Modified: 2017-11-22 20:19 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lazar Kirchev 2017-11-18 14:11:35 UTC
When AuthConfigFactoryImpl.doRegisterConfigProvider() tries to load the class of the provider it does not check for null. However, according to the javadoc in the jaspic specification null could be passed as provider name.

Therefore a check for null is necessary in this place. And if it is null, according to the spec the registration ID should be returned, but subsequent calls to getConfigProvider() should return null.
 
The only thing which shoud be added to the current implementation in order to support this behavior is in case of null provider class name, only to return the registration ID without modifying the provider registration structures 
or the persistent storage. 

In this way it will actually return a non-existing registration ID, but if getConfigProvider() is called with non-existing registration ID it returns null, so it behaves according to the spec.
Comment 1 Lazar Kirchev 2017-11-18 14:12:46 UTC
This pull request contains a test case which illustrates the problem and a fix for it: https://github.com/apache/tomcat/pull/92
Comment 2 Mark Thomas 2017-11-20 20:08:01 UTC
the suggested patch addresses the NPE but doesn't handle the case of an existing persistent registration for the given registration ID. My reading of the spec that such a registration should be removed.
Comment 3 Lazar Kirchev 2017-11-21 07:58:31 UTC
Thanks Mark!  I overlooked this case. The persistent registration definitely should be removed, because otherwise the subsequent call to getConfigProvider() will return it instead of null.

I will update the patch with this.
Comment 4 Lazar Kirchev 2017-11-21 12:52:00 UTC
Now here comes the question what should getRegistrationIds(null) return if there is an auth config provider with null class name registered. 

According to the spec, if a provider with a null class name is registered, a subsequent call to getCofigProvider() with this layer and app context should return null, but it does not specify if its registration id should be returned in the list of all registration ids. 

It seems reasonable to be included. However, in this case it should be possible to persist providers with null class names and also it should be stored in the in-memory registration structures. 

Or, we may assume that this is no actual registration as functionally it does not bring any value and not store it in-memory or persist it.

Marc, what do you think about this? How do you interpret the spec?
Comment 5 Mark Thomas 2017-11-21 20:06:04 UTC
My reading of the spec leans towards the ID being returned in getRegistrationIds() and all that that implies.
Comment 6 Lazar Kirchev 2017-11-22 15:13:10 UTC
Thanks Mark! Agree.

Here is a pull request with patch + tests: https://github.com/apache/tomcat/pull/94
Comment 7 Mark Thomas 2017-11-22 20:19:07 UTC
Thanks for the updated patch and test cases.

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