On Fri, Oct 16, 2026 at 9:29 AM Mark Thomas <[email protected]> wrote: > > 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.
+1 Ultimately it's indeed an unlikely situation, so this is very reasonable. The most important is then to add a comment on the decision so that the explanation is remembered. Rémy --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
