Bug 63982

Summary: CombinedRealm makes assumptions about principal implementation
Product: Tomcat 9 Reporter: Michael Osipov <michaelo>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: major CC: michaelo
Priority: P2    
Version: 9.0.29   
Target Milestone: -----   
Hardware: All   
OS: All   

Description Michael Osipov 2019-12-02 11:04:44 UTC
Consider the following configuration:

>   <Realm className="org.apache.catalina.realm.CombinedRealm">
>     <Realm className="CustomRealm"
>       ... />
>     <Realm className="CustomRealm"
>       ... />
>   </Realm>

CustomRealm uses CustomPrincipal, not of type GenericPrincipal. Two issues arise:

1. When AuthenticatorBase now invokes CombinedRealm#hasRole() it will delegate to RealmBase#hasRole() which will call RealmBase#hasRoleInternal(): it will always return false bacause CustomPrincipal is not instance of GenericPrincipal.
2. CustomRealm#getRoles() will again delegate to RealmBase#getRoles() and will throw an exception.

Thus, this realm is tied to the GenericPrincipal and cannot be used generically. You have to write a CustomCombinedRealm.

It could be solved the following way:
1. Delegate all #hasRole() calls to the underlying realms and return first true
2. Delegate all #getRoles() calls to the underlying realms, catch exceptions, rethrow at and return the first array.

Unfortunately, RealmBase throws an IllegalStateException for #getRoles(), but this is nowhere documented. If would return a null array, one could loop until the first non-null array. In my opinion, if this is not documented, it could simply return null.
Comment 1 Remy Maucherat 2019-12-02 12:01:10 UTC
It is obvious reading the code in the realm package that it is assumed GenericPrincipal will have to be used, so that applies to this hypothetical CustomRealm as well. Of course, there are plenty of people out there who are actively looking for trouble :)
Comment 2 Mark Thomas 2019-12-02 22:30:04 UTC
I think getRoles() can be deprecated. It isn't used anywhere now. It was added to support the failed GSoC JASPIC work.

The proposed solution for hasRole() looks reasonable to me.
Comment 3 Michael Osipov 2019-12-03 08:01:30 UTC
(In reply to Mark Thomas from comment #2)
> I think getRoles() can be deprecated. It isn't used anywhere now. It was
> added to support the failed GSoC JASPIC work.

Even if, it has to be supported until Tomcat 10. Do you consider returning null is better here? That would make like in CombinedRealm easier.

> The proposed solution for hasRole() looks reasonable to me.

Fine.

I will work on this.
Comment 4 Mark Thomas 2019-12-03 08:13:05 UTC
(In reply to Michael Osipov from comment #3)
> Even if, it has to be supported until Tomcat 10. Do you consider returning
> null is better here? That would make like in CombinedRealm easier.

It has to be present until Tomcat 10. The code is unused (by Tomcat). I think it is better to leave the code as is - apart from adding a deprecation marker. I don't think now is the time to be changing the behaviour of that method.
Comment 5 Michael Osipov 2019-12-03 11:45:21 UTC
(In reply to Mark Thomas from comment #4)
> (In reply to Michael Osipov from comment #3)
> > Even if, it has to be supported until Tomcat 10. Do you consider returning
> > null is better here? That would make like in CombinedRealm easier.
> 
> It has to be present until Tomcat 10. The code is unused (by Tomcat). I
> think it is better to leave the code as is - apart from adding a deprecation
> marker. I don't think now is the time to be changing the behaviour of that
> method.

Even if the behavior is not documented and an implementation detail? How would you properly call #getRoles() from the CombinedRealm then?
Comment 6 Mark Thomas 2019-12-03 13:16:58 UTC
(In reply to Michael Osipov from comment #5)

> Even if the behavior is not documented and an implementation detail? How
> would you properly call #getRoles() from the CombinedRealm then?

I wouldn't. I'd leave it alone. It isn't used internally in Tomcat and in case someone is using it, I don't want to change the API/behaviour. I'd just mark it as deprecated so no-one new starts using it.
Comment 7 Michael Osipov 2019-12-06 13:34:03 UTC
Fixed in:
- master for 9.0.30 onwards
- 8.5.x for 8.5.50 onwards
- 7.0.x for 7.0.99 onwards