[Bug 63982] New: CombinedRealm makes assumptions about principal implementation

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 63982] New: CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
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: [hidden email]
          Reporter: [hidden email]
  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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #1 from Remy Maucherat <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #2 from Mark Thomas <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #3 from Michael Osipov <[hidden email]> ---
(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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #4 from Mark Thomas <[hidden email]> ---
(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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #5 from Michael Osipov <[hidden email]> ---
(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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

--- Comment #6 from Mark Thomas <[hidden email]> ---
(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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63982] CombinedRealm makes assumptions about principal implementation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63982

Michael Osipov <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |[hidden email]

--- Comment #7 from Michael Osipov <[hidden email]> ---
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: [hidden email]
For additional commands, e-mail: [hidden email]