shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Les Hazlewood <lhazlew...@apache.org>
Subject Re: Proposed changes to Shiro annotations
Date Tue, 03 Aug 2010 17:30:15 GMT
On Mon, Aug 2, 2010 at 10:46 PM, Kalle Korhonen
<kalle.o.korhonen@gmail.com> wrote:
> I just closed SHIRO-175, but I have to make several remarks:
> 1) The javadoc for DefaultAnnotationResolver says "Unfortunately
> Java's default reflection API for Annotations is not very robust...
> More complex class hierarchy traversal logic is required" which seems
> unnecessarily harsh. The JCP took a design decision that annotations
> don't inherit to make processing them as fast as possible. It sounded
> to me the Javadoc is written as a response to Spring's proxy and
> annotation processing mechanisms. Personally, I think the Spring folks
> just shot themselves in the foot by creating the proxies as a default
> but then leaving the implementation half-way and not copying the
> annotations to the auto-created proxies (Tapestry does this, for
> example). After all, creation is a one-time cost but traversing the
> inheritance hierarchy up and down incurs a cost every time the
> operation's invoked. I'd like to rephrase the Javadoc if there are no
> objections.

That the JDK does not support a way to do this out of the box (a
simple method would do,  or a method argument to turn it on or off),
and requires multiple frameworks to repeat related code (Spring's
AnnotationUtils, Tapestry's implementation, Commons Lang, etc),
inherently means the JDK support is not robust.  Why doesn't the JDK
provide a method to copy the annotations to the proxy as Tapestry does
if that is a better approach?  Maybe it's because they can't know the
reasons behind either approach so they went with the Lowest Common
Denominator solution (as they often do).  They could have easily added
a few extra methods to support these alternative approaches and let
the caller decide what's best for them. That they didn't means the JDK
support is not robust.

There are _many_ parts of the JDK that aren't robust (why does every
project create their own StringUtils/ClassUtils/*Utils classes that
are nearly identical?)  I don't believe there is anything wrong
stating that in the documentation, especially as it tells the reader
exactly why the component(s) exist.  It's not being harsh - it's not
like I said "The JDK annotation support sucks and I can't believe I
have to jump through hoops", etc - I was just stating that the JDK is
not flexible enough to do what we need to do to support other
frameworks.

In the end, I don't think it's harsh at all ('not robust' does not
feel derogatory to me), so I would vote -0 for the change to indicate
that it's not that big a deal if we do change it, I just don't think
it necessary.

> 2) I didn't implement Logical.NOT but it'd be trivial to implement it
> now for RequiresRoles and RequiresPermissions. Do you think it'd be
> useful or would it be too odd? (It may read a little funny and
> semantics may not always be obvious - e.g. RequiresRoles(value =
> "user", logical = Logical.NOT) )

My personal opinion is that it feels odd.  I honestly think the best
solution for all of these annotations is to come up with an single
@SecurityCheck annotation that reads an expression backed by an antlr
grammar to handle all of the logical/boolean conditions.  It wouldn't
be hard, but would take a little bit of time to implement it.  It
would be _very_ cool though :).

> 3) I think we should revisit the authorizing API in order to clean up
> and deprecate some of the operations. I'm not suggesting we should
> remove anything for a long time, possible ever, but I just don't like
> the checkXXX operations. They needlessly enlarge the surface area of
> the whole API and spread out the authorization logic. For example, in
> most cases the Authorizer throws the UnauthenticatedExceptions but
> GuestAnnotationHandler makes that decision by itself. In a typical
> case, the authorization logic is routed through Subject ->
> SecurityManager -> Authorizer -> Realm. For convenience, Subject
> interface likely needs to remain extensive, but I'd be happier if
> either the Authorizer contained only the hasXX and isPermitted
> operations and not their checkXXX counterparts, or, there was a marker
> interface a AuthorizerProvider marker interface

My gut feeling is to remove the checkXXX methods from the Authorizer
interface rather than add another interface.  Yes, it is not backwards
compatible, but I think very few end-users actually call that method
directly.  The large majority would be calling the Subject.check*
methods (if they even call them at all).

The reasoning behind these methods - way back in the framework's
history (0.1) - was to provide identical check* methods that are
common in JAAS (e.g. java.security.AccessControlContext), assuming
that's what people wanted.  In retrospect, I believe most end-users
would prefer the isPermitted/hasRole methods and then throw their own
more meaningful exception to indicate exactly why access failed.

So, if we removed the methods from the Authorizer interface, but kept
them in the Subject interface, we'd have backwards compatibility in
the areas that are most likely to be used.  It's not a perfect
solution, but it should meet 95% of all use cases.

Les

Mime
View raw message