You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2019/12/02 11:04:44 UTC
[Bug 63982] New: CombinedRealm makes assumptions about principal
implementation
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
Bug ID: 63982
Summary: CombinedRealm makes assumptions about principal
implementation
Product: Tomcat 9
Version: 9.0.29
Hardware: All
OS: All
Status: NEW
Severity: major
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: michaelo@apache.org
Target Milestone: -----
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #3 from Michael Osipov <mi...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #9 from Michael Osipov <mi...@apache.org> ---
(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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #4 from Mark Thomas <ma...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #6 from Mark Thomas <ma...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #8 from Ole Ostergaard <ol...@knime.com> ---
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?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
Michael Osipov <mi...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
CC| |michaelo@apache.org
--- Comment #7 from Michael Osipov <mi...@apache.org> ---
Fixed in:
- master for 9.0.30 onwards
- 8.5.x for 8.5.50 onwards
- 7.0.x for 7.0.99 onwards
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #1 from Remy Maucherat <re...@apache.org> ---
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 :)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #5 from Michael Osipov <mi...@apache.org> ---
(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?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 63982] CombinedRealm makes assumptions about principal
implementation
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982
--- Comment #2 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org