Bug 63982 - CombinedRealm makes assumptions about principal implementation
Summary: CombinedRealm makes assumptions about principal implementation
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.29
Hardware: All All
: P2 major (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-02 11:04 UTC by Michael Osipov
Modified: 2020-08-25 13:50 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
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
Comment 8 Ole Ostergaard 2020-08-25 13:32:53 UTC
I'm hit by the upcoming removal of Realm#getRoles(). I was using the Realm#getRoles() to get the Roles of a Principal. Is there an alternate way to perform this action, in regard of the upcoming removal of the function in Realm?
Comment 9 Michael Osipov 2020-08-25 13:50:36 UTC
(In reply to Ole Ostergaard from comment #8)
> I'm hit by the upcoming removal of Realm#getRoles(). I was using the
> Realm#getRoles() to get the Roles of a Principal. Is there an alternate way
> to perform this action, in regard of the upcoming removal of the function in
> Realm?

Do what I do cast Principal object to your principal implementation and invoke your #getRoles() there: http://tomcatspnegoad.sourceforge.net/xref/net/sf/michaelo/tomcat/realm/ActiveDirectoryPrincipal.html#L134