Re: Proposed changes to Shiro annotations

[Kalle Korhonen <ka...@gmail.com>]

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.

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) )

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 so DelegatingSubject
could only do authorizerProvider.getAuthorizer().checkXXX() rather
than going through the securitymanager every time. I think the actual
issue originates from the fact that Realm and SecurityManager
interfaces both implement the Authorizer interface. An
AuthorizerProvider interface with a single getAuthorizer() operation
might be relatively painless to implement and wouldn't exclude doing
the other suggested things a later point.

Kalle


On Thu, Jul 29, 2010 at 1:04 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> On Thu, Jul 29, 2010 at 1:01 PM, Les Hazlewood <lh...@apache.org> wrote:
>> My initial feeling is to add the checkRoles(String... roleIdentifiers)
>> method and not remove the others, until say, a 2.0 release.  They're
>> fairly harmless as is and my personal desire is to refrain from making
>> backward-incompatible changes in as much as is possible.
>
> Right, agree 100%.
>
> Kalle
>

Re: Proposed changes to Shiro annotations

[Mike K <mk...@semanticresearch.com>]

I realize I am quite late to this discussion, but I wanted to put my 2 cents
in.
I am currently using Shrio with AOP, however, I am not using Shiro
annotations, instead, using my own AspectJ scheme with point cuts based on
jax-rs annotation. One annotation that I have created for myself was
@RequiresPermission("domain:action") that can annotate both methods and
parameters.
In the case of method annotation, the permission is checked. In the case a
parameter is annotated the the parameter is converted to string (or a comma
separated list if its a collection) and is used as the target in the
permission.

Mike. 
-- 
View this message in context: http://shiro-developer.582600.n2.nabble.com/Proposed-changes-to-Shiro-annotations-tp5223639p5652673.html
Sent from the Shiro Developer mailing list archive at Nabble.com.

Re: Proposed changes to Shiro annotations

[Kalle Korhonen <ka...@gmail.com>]

On Tue, Aug 3, 2010 at 10:30 AM, Les Hazlewood <lh...@apache.org> wrote:
> On Mon, Aug 2, 2010 at 10:46 PM, Kalle Korhonen
> <ka...@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
> 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.

Thanks, I do appreciate your viewpoint. Yeap, let's leave it as is,
there's always more than one side to a story.

>> 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 :).

Ok, no to Logical.NOT. I dropped @SecurityCheck that was suggested as
part of the issue since it's not clear how to best express the
permissions. I agree that a more generalized syntax would be desired
but I'm not convinced it needs to be exactly one annotation. The
current annotations should get us closer to covering 80%-90% of use
cases and they probably don't need to be deprecated any time soon. The
issue with a single, uniform grammar is that its very easy to create
conflicting rules by combining different types of permissions.

> 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.

Thanks for the background info. We could deprecate them early just to
indicate the future direction of the api for our users but we are in
no hurry to remove them completely. Keeping the Subject interface
intact makes sense as well. We can probably go to 1.1.0 without
further changes in the api and then formulate a roadmap for refining
the Authorizer interface and deprecating the checkXXX methods.

Kalle

Re: Proposed changes to Shiro annotations

[Les Hazlewood <lh...@apache.org>]

On Mon, Aug 2, 2010 at 10:46 PM, Kalle Korhonen
<ka...@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