[Enhancement] Mutability of PrincipalCollection and AuthenticationInfo

janitza-mage commented 2 weeks ago

Search before asking

[X] I had searched in the issues and found no similar issues.

Enhancement Request

First of all, this is neither a feature request, nor is it strictly a bug report -- but the latter cannot be decided, as I will explain below. I am currently evaluating Shiro for a project and have found an architectural decision in its code that I believe is dangerous.

The culprit is PrincipalCollection. This interface does not itself specify whether it allows mutable implementations, but the existence of MutablePrincipalCollection does. Code that receives a PrincipalCollection from an unknown source must therefore treat it as "potentially mutable". PCs are passed around a lot, so "unknown source" is a common case.

Unfortunately, receiving code doesn't do that. Received PCs are passed and stored as if they cannot be modified anymore. AbstractAuthenticator even stores them in a cache. All these places are affected by later mutations to the PC.

The code that does mutate a PC, AbstractAuthenticationStrategy (from the PAM package), will either accept an MPC from its caller and mutate that, or create its own copy in case the one from the caller is not mutable. This further obscures which PC gets mutated.

The current code from Shiro does not show faulty behavior from this problem because AbstractAuthenticationStrategy is the only place where PCs get mutated, and all subclasses of that class except for FirstSuccessfulStrategy only mutate an MPC they created themselves (the "aggregate"). FirstSuccessfulStrategy uses a PC returned by a Realm as the aggregate, but then doesn't actually mutate it.

Why is this a problem?

I think this is a problem because it is bug-free by chance, not by design. It depends on the implementations of the interfaces used to make stronger guarantees than the interfaces do -- and these implementations don't even do that, only the combined behavior of the implementations of multiple interfaces do.

While projects using Shiro could easily trip over this, my main concern is my own project being affected after upgrading to future Shiro versions, with its internals changing in such a way that it breaks one of those guarantees that the interfaces don't make but on which correctness of the code relies, and has to rely, on.

Describe the solution you'd like

Possible solutions:

My preferred solution: Make PrincipalCollection immutable by design. AFAIK, the PAM package doesn't really need it to be mutable and could instead make mutated copies. The biggest problem is code using Shiro that might make use of MPCs -- it's a breaking change.
Make defensive copies of PCs everywhere. This is ugly, it's easy to forget a place, and it might start to affect performance.
Establish rules about PC mutability. This is the non-invasive approach and might be a first step towards making PCs immutable.

An example for a "rule about mutability" could be: PCs should be treated as immutable, unless you created the MPC yourself and keep it to yourself -- or make a clear contract with your callers that the PC is treated as mutable. AbstractAuthenticationStrategy would have to be changed to adhere to that rule.

I am happy to contribute to the solution, but I first want to make sure that this is seen as a problem by others too, and discuss which solution approach is preferred.

Are you willing to submit PR?

[X] Yes I am willing to submit a PR!

lprimak commented 2 weeks ago

Started a discussion on ASF slack about this

lprimak commented 2 weeks ago

Looks like everyone's in agreement that this is a good idea. We were thinking of deprecating the MutablePrincipalCollection and making static copyOf() / merge() methods that combine PCs We would welcome a PR. Thank you!

janitza-mage commented 2 weeks ago

Thank you for the quick reply.

It might be useful to leave SimplePrincipalCollection mutable for now because I'd expect applications to use it as a "builder" for PCs. The question then is whether copyOf() / merge() should return a new, immutable implementation or just re-use SimplePC -- because if they return SimplePC, then copyOf() is the same as the existing copy-constructor and merge() is copyOf() followed by addAll(), so these methods would not really add a lot of value.

The alternative would be to add a new, immutable implementation, either with a builder or using SimplePC as a builder, and have copyOf() and merge() return that.

lprimak commented 2 weeks ago

I would say the only viable outcome here that's "worth the effort" is to convert SimplePrincipalCollection into immutable class with a builder utility. Copy/Merge would return another immutable copy

apache / shiro