Search before asking
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?
Search before asking
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:
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?