On 14/10/2026 14:14, Mark Thomas wrote:
On 14/10/2026 11:56, Rémy Maucherat wrote:
On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <[email protected]> wrote:
I am tempted to refactor this. Assuming that Principals are normally
serializable, the serializing and throwing away seems wasteful.
I am leaning towards trying to write the Principal and catching the
NotSerializableException. I'll see what that looks like.
Yes, so the idea is to not corrupt everything which seemed like a good
plan. As for the solution it is of course relatively wasteful since it
serializes to a nested byte stream. It didn't seem too bad to me so I
validated it.
It is a trade-off. This approach is more robust as it handles custom
serailization safely whereas catching NotSerializableException may fail
if the Principal uses custom serialization.
That said, attributes are currently using the catch
NotSerializableException approach and switching to this more robust
approach would be expensive for every attribute.
Principals might be more out of the application's control so I can see
an argument for more robust checks compared to attributes.
I think you have convinced me to keep this code. I'll extend the same
fix to Delta session.
I had a few ideas for alternative solutions overnight but unfortunately
none of them were workable.
I have also been thinking more about this trade-off and looking at where
else we test p instanceof Serializable and I have changed my mind on
this change.
I think the right approach is to catch and handle the
NotSerializableException.
My reasoning is as follows:
- Principals returned by Tomcat Realms already handle this correctly.
GenericPrinipcal has custom serialization that drops any
non-serializable nested Principal
- Therefore the problem only applies to Principals provided by 3rd-party
authentication mechanisms
- There is already a instanceof Serializable test
- If a 3rd-party Principal passes that test but fails to serialize that
is a bug in the 3rd-party Principal. To quote the Javadoc:
"It is an error to declare a class Serializable if this is not the
case."
- This defence mechanism adds costs to all serialization events
- The change effectively penalises everybody to mitigate the risk of a
bug in 3rd-party code the vast majority will not be using
Therefore, I intend to re-work this code to use the same approach as is
currently used for session attributes. I have most of this already in a
stash from when I looked at this earlier this week. I need to update the
comments to include some of the above and check the tests.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
