shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Les Hazlewood <lhazlew...@apache.org>
Subject Re: Inheritance - SecurityManager
Date Mon, 25 Oct 2010 18:23:47 GMT
>> But I'm not sure that inheritance is used too often.  Sure, it is used
>> in some places where it shouldn't be and that there are definitely
>
> Side comment, it's used all over the place.  Filters, etc.

Inheritance is not bad - it's just that composition should generally
be favored over it.  The Filter hierarchy, while deep, has rarely been
refactored in the last few years.  This particular example actually
demonstrates where it can make sense.  Let me restate: Composition
Should Be Favored Over Inheritance.  I know this.  But we also have a
6+ year-old code base that can't be changed overnight and some parts
of it are probably just fine the way they are.

Now if we want to change the remaining places where this exists for
2.0, then great - let's talk about that.

> Let's look at the DefaultSecurityManager.  DefaultSecurityManager extends SessionsSecurityManager,
 SessionsSecurityManager extends AuthorizingSecurityManager,  AuthorizingSecurityManager
extends AuthenticatingSecurityManager,  AuthenticatingSecurityManager extends RealmSecurityManager,
and, finally, RealmSecurityManager extends CachingSecurityManager.
>
> I'm guessing there's over 1200 lines of code there spread across six classes.
>
> All this for wrapper classes.
>
> Don't get me wrong.  I'm all for delegation.  What I am against is the long chain of
inheritance.  The above inheritance list reads like Matthew chapter 1.  ;)  Why such a
large totem pole?  I haven't groked the entire set of code but I suspect there's some intimate
collaboration there.  If there isn't then they really should be broken up.

You've just complained about it without actually looking at the source
code, which to me is a bit presumptuous.  While your point may be
justified (and probably is!), it's a little arrogant to make
conclusions from assumptions without doing some homework first.  I
respect your opinion Alan - this just felt a little off to me - sorry
if I took it too personally.

Anyway, there is no intimate collaboration between subclasses - not
until you get to DefaultSecurityManager and DefaultWebSecurityManager,
so theoretically, all of the parent classes can be moved to
DefaultSecurityManager and still be backwards-compatible.

The hierarchy that exists today exists for logical grouping of code to
make it a little more manageable:  when you're editing code in the
SessionsSecurityManager subclass, you're only worrying about dealing
with the nested SessionManager.   When you want to work with the
Authorizer, you only touch the AuthorizingSecurityManager subclass,
etc.

It is purely a logical grouping mechanism so we as developers don't
get swallowed under the 1500 or 2000 lines of code that would exist in
a single class otherwise.  Now if this is something we as a team think
we should change and move it all to DefaultSecurityManager, then I
think we can do that.  It would be a huge class though ;)

> They should be broken up anyway.
>
> Take this with a grain of salt; as I mentioned before I don't grok the entire set of
the above code.

I think you'll find that I and others on this list are very open and
helpful and we try to help people understand things whenever we can -
and I often do this in great detail (maybe even to the chagrin of most
readers).  All it takes is a quick question to learn something.
(Passing informed judgement and suggesting improvements is fine by the
way, and I personally welcome it because it will help this project be
better).

> The managers should be separate final entities.  Collaborative behavior could be specified
by wiring in various listeners between them.  The DefaultSecurityManager could do this.

The Managers are separate entities - the SecurityManager
implementation acts as an umbrella for all of them, delegates to them
as necessary, and additionally performs some logic related to Subject
management.  We could have a SubjectManager that did this logic and
then the SecurityManager implementation would be nothing more than a
container of the others.

We can refactor this stuff and I have no issues with that.  But the
pinnacle feature of the framework is usability and simplicity above
all else - even if it causes some extra work for the development team.
 When an end-user instantiates the SecurityManager implementation and
provides at least one Realm - that's it - they're good to go.
Everything works out of the box.

I personally would like to keep this usage scenario and definitely
want to avoid the user having to know how to wire up all the
individual instances themselves.  Now we could do this through a
Builder, which I've thought about more than a few times in the past,
but never got around to it because no-one was complaining that the
SecurityManager implementations were causing anyone problems in
practice.

Also understand that the SecurityManager wrapper exists as an ideal
handle for INI-based IoC configuration.  We'd have to think about how
changing anything would affect that as well.

Best,

Les

Mime
View raw message