Proposed changes to Shiro annotations

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

Related to https://issues.apache.org/jira/browse/SHIRO-175 and some of
the Shiro extensions we (Tynamo.org) have made, I'd like to change
@Target(ElementType.METHOD) of all annotations to
@Target({ElementType.TYPE, ElementType.METHOD}). The obvious semantics
of attaching a security annotation to a class is that the same
permission would apply to all method invocations in that class.

SHIRO-175 suggests that we'd deprecate the existing annotations in
favor of RequiresAnyXXX and RequiresAllXXX annotations, forcing to
target 1.1 release for the issue resolution. We could also keep the
existing annotations and pick either ANY or ALL semantics in order to
avoid deprecating anything - not sure which one is better. The current
semantics are unclear, the Javadoc implies ANY for RequiresRoles but
ALL for RequiresPermissions.

Here's what I'd suggest:
- 1.0.1 change @Target to apply to TYPEs as well, fix ambiguity of
current annotations (especially if we decide to keep them)
- 1.1.0 resolve SHIRO-175 and add RequiresAnyXXX and RequiresAllXXX
annotations (and deprecate existing annotations if we choose to do so)

I can handle implementing the changes. Any comments?

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

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

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

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

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

On Mon, Jul 26, 2010 at 11:14 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> For completeness, I think we should add:
> void checkRoles(String... roleIdentifiers) throws AuthorizationException;

I agree that this should be there - it is more convenient than
creating a wrapper collection.

> I'd rather deprecate both checkXXX(String) and checkXXXs(Collection)
> and leave only checkXXXs(String...) - I don't see value for the
> additional methods but not sure if cleaning that up is worth the
> effort.

I'm assuming you mean for the checkRoles methods only?  We can't have
string-only checkPermissions methods, since people need the ability to
check type-safe Permission instances as well.

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.

Just my .02,

Les

Re: Proposed changes to Shiro annotations

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

Everything else besides the proposed Logical parameter is committed
for review. Also, I noticed there's some inconsistency in the Subject
interface, specifically there's these two operations:
void checkPermissions(Collection<Permission> permissions) throws
AuthorizationException;
void checkPermissions(String... permissions) throws AuthorizationException;

but only:
void checkRoles(Collection<String> roleIdentifiers) throws
AuthorizationException;

For completeness, I think we should add:
void checkRoles(String... roleIdentifiers) throws AuthorizationException;

I'd rather deprecate both checkXXX(String) and checkXXXs(Collection)
and leave only checkXXXs(String...) - I don't see value for the
additional methods but not sure if cleaning that up is worth the
effort.

Kalle


On Mon, Jul 26, 2010 at 4:32 PM, Kalle Korhonen
<ka...@gmail.com> wrote:
> I'm rethinking my position on implementing @RequiresAnyRoles and
> @RequiresAnyPermissions for SHIRO-175 since there's a fair bit of
> logic that would need to be duplicated. The AnnotationHandler
> hierarchy pretty much assumes that a specific type of handler deals
> with only one type of annotations. The simplest way to include OR
> logic would be to add a parameter to the annotations to indicate the
> logical type that should be used (e.g. Logical.AND or Logical.OR), AND
> probably being the default since that's how it's currently interpreted
> (though I think OR as a default might be more useful but then again,
> AND is the more secure default assumption). The drawback is that if
> you want OR logic, you'd always have to explicitly name the value
> parameter as well (e.g @RequiresRoles(value={"admin", "user"},
> logic=Logical.OR). Is that a reasonable price to pay?
>
> One other thing is that currently the value parameter is defined as
> plain String rather a string array, forcing us to parse the String
> ourselves. I think that's just a mistake and I'll fix that as part of
> the issue.
>
> I already implemented extending all annotations to allow
> @Target(ElementType.TYPE) (but haven't committed) with accompanied
> tests.
>
> Kalle
>
>
> On Mon, Jun 28, 2010 at 2:37 PM, Les Hazlewood <lh...@apache.org> wrote:
>> Sounds good.  Also, if necessary, I'm good with getting 1.1 release out as
>> quickly as would be prudent.  No need to delay unless we have a complex
>> feature we want to add.
>>
>> Les
>>
>> On Mon, Jun 28, 2010 at 12:55 PM, Kalle Korhonen <kalle.o.korhonen@gmail.com
>>> wrote:
>>
>>> On Mon, Jun 28, 2010 at 11:21 AM, Les Hazlewood <lh...@apache.org>
>>> wrote:
>>> >> > then downgrade to 1.0.0, then their software could break because that
>>> >> > behavior won't be enabled, right?
>>> >> After downgrading, it'd show up as a compiler error - which makes all
>>> >> the sense to me.
>>> > This means it's not backwards compatibile according to the APR guidelines
>>> > (that guarantee both binary and source compatibility across patch
>>> versions):
>>>
>>> Sure, let's put it all in for 1.1.0 only.
>>>
>>> Kalle
>>>
>>
>

Re: Proposed changes to Shiro annotations

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

>> Wouldn't this break backwards compatibility?  But I do agree that
>> having the String array is a better approach.
>
> It will, but it's very very minor.
>
> I think it's just something that needs to be documented properly.

Gotcha.  Sounds good to me.  I think we should create a wiki document
in the documentation section for 1.1 that explicitly lists any
backwards-incompatible changes.  This way it will be very clear when
we perform the release.

Les

Re: Proposed changes to Shiro annotations

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

On Thu, Jul 29, 2010 at 11:06 AM, Les Hazlewood <lh...@apache.org> wrote:
>> @RequiresAnyPermissions for SHIRO-175 since there's a fair bit of
>> logic that would need to be duplicated. The AnnotationHandler
>> hierarchy pretty much assumes that a specific type of handler deals
>> with only one type of annotations.
>
> Should this be rethought then?  The single-annotation-per-handler was
> just an implementation strategy - we could probably add new support if
> there should be an additional solution.
> For example, the Spring support module has a single method interceptor
> that uses one or more AnnotationHandlers to support more than one
> annotation per method.

Right, it's certainly possible to implement an annotation handler that
supports multiple annotations, it's just that the super classes don't
support it.

>> AND is the more secure default assumption). The drawback is that if
>> you want OR logic, you'd always have to explicitly name the value
>> parameter as well (e.g @RequiresRoles(value={"admin", "user"},
>> logic=Logical.OR). Is that a reasonable price to pay?
> I think to retain the current semantics (AND behavior) we would have
> to do this.  I think the logical type addition is a good idea - it
> doesn't require us to add new annotations and the existing handlers
> can be easily updated.

Yes, I think there are enough reasons for the Logical parameter to go
that route.

>> One other thing is that currently the value parameter is defined as
>> plain String rather a string array, forcing us to parse the String
>> ourselves. I think that's just a mistake and I'll fix that as part of
>> the issue.
> Wouldn't this break backwards compatibility?  But I do agree that
> having the String array is a better approach.

It will, but it's very very minor. If you only have one value (which I
suspect is far more common), it's still legal to write it as:
@MyAnnotation("value") rather than forced to do @MyAnnotation({"value"}).
It'll break backwards compatibility only if you have:
@MyAnnotation("value1,value2") that now needs to be written as
@MyAnnotation({"value1","value2")

I think it's just something that needs to be documented properly. The
good thing is that they are security annotations so you should find
out pretty quick if they don't work right. The other good thing about
it is that we didn't trim the values before, so somebody could have
easily written @MyAnnotation("value1, value2") (note the space) and
wondering why it didn't work.

Kalle

Re: Proposed changes to Shiro annotations

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

> @RequiresAnyPermissions for SHIRO-175 since there's a fair bit of
> logic that would need to be duplicated. The AnnotationHandler
> hierarchy pretty much assumes that a specific type of handler deals
> with only one type of annotations.

Should this be rethought then?  The single-annotation-per-handler was
just an implementation strategy - we could probably add new support if
there should be an additional solution.

For example, the Spring support module has a single method interceptor
that uses one or more AnnotationHandlers to support more than one
annotation per method.

> The simplest way to include OR
> logic would be to add a parameter to the annotations to indicate the
> logical type that should be used (e.g. Logical.AND or Logical.OR), AND
> probably being the default since that's how it's currently interpreted
> (though I think OR as a default might be more useful but then again,
> AND is the more secure default assumption). The drawback is that if
> you want OR logic, you'd always have to explicitly name the value
> parameter as well (e.g @RequiresRoles(value={"admin", "user"},
> logic=Logical.OR). Is that a reasonable price to pay?

I think to retain the current semantics (AND behavior) we would have
to do this.  I think the logical type addition is a good idea - it
doesn't require us to add new annotations and the existing handlers
can be easily updated.

> One other thing is that currently the value parameter is defined as
> plain String rather a string array, forcing us to parse the String
> ourselves. I think that's just a mistake and I'll fix that as part of
> the issue.

Wouldn't this break backwards compatibility?  But I do agree that
having the String array is a better approach.

>
> I already implemented extending all annotations to allow
> @Target(ElementType.TYPE) (but haven't committed) with accompanied
> tests.

+1

Looks good!

Les

Re: Proposed changes to Shiro annotations

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

I'm rethinking my position on implementing @RequiresAnyRoles and
@RequiresAnyPermissions for SHIRO-175 since there's a fair bit of
logic that would need to be duplicated. The AnnotationHandler
hierarchy pretty much assumes that a specific type of handler deals
with only one type of annotations. The simplest way to include OR
logic would be to add a parameter to the annotations to indicate the
logical type that should be used (e.g. Logical.AND or Logical.OR), AND
probably being the default since that's how it's currently interpreted
(though I think OR as a default might be more useful but then again,
AND is the more secure default assumption). The drawback is that if
you want OR logic, you'd always have to explicitly name the value
parameter as well (e.g @RequiresRoles(value={"admin", "user"},
logic=Logical.OR). Is that a reasonable price to pay?

One other thing is that currently the value parameter is defined as
plain String rather a string array, forcing us to parse the String
ourselves. I think that's just a mistake and I'll fix that as part of
the issue.

I already implemented extending all annotations to allow
@Target(ElementType.TYPE) (but haven't committed) with accompanied
tests.

Kalle


On Mon, Jun 28, 2010 at 2:37 PM, Les Hazlewood <lh...@apache.org> wrote:
> Sounds good.  Also, if necessary, I'm good with getting 1.1 release out as
> quickly as would be prudent.  No need to delay unless we have a complex
> feature we want to add.
>
> Les
>
> On Mon, Jun 28, 2010 at 12:55 PM, Kalle Korhonen <kalle.o.korhonen@gmail.com
>> wrote:
>
>> On Mon, Jun 28, 2010 at 11:21 AM, Les Hazlewood <lh...@apache.org>
>> wrote:
>> >> > then downgrade to 1.0.0, then their software could break because that
>> >> > behavior won't be enabled, right?
>> >> After downgrading, it'd show up as a compiler error - which makes all
>> >> the sense to me.
>> > This means it's not backwards compatibile according to the APR guidelines
>> > (that guarantee both binary and source compatibility across patch
>> versions):
>>
>> Sure, let's put it all in for 1.1.0 only.
>>
>> Kalle
>>
>

Re: Proposed changes to Shiro annotations

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

Sounds good.  Also, if necessary, I'm good with getting 1.1 release out as
quickly as would be prudent.  No need to delay unless we have a complex
feature we want to add.

Les

On Mon, Jun 28, 2010 at 12:55 PM, Kalle Korhonen <kalle.o.korhonen@gmail.com
> wrote:

> On Mon, Jun 28, 2010 at 11:21 AM, Les Hazlewood <lh...@apache.org>
> wrote:
> >> > then downgrade to 1.0.0, then their software could break because that
> >> > behavior won't be enabled, right?
> >> After downgrading, it'd show up as a compiler error - which makes all
> >> the sense to me.
> > This means it's not backwards compatibile according to the APR guidelines
> > (that guarantee both binary and source compatibility across patch
> versions):
>
> Sure, let's put it all in for 1.1.0 only.
>
> Kalle
>

Re: Proposed changes to Shiro annotations

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

On Mon, Jun 28, 2010 at 11:21 AM, Les Hazlewood <lh...@apache.org> wrote:
>> > then downgrade to 1.0.0, then their software could break because that
>> > behavior won't be enabled, right?
>> After downgrading, it'd show up as a compiler error - which makes all
>> the sense to me.
> This means it's not backwards compatibile according to the APR guidelines
> (that guarantee both binary and source compatibility across patch versions):

Sure, let's put it all in for 1.1.0 only.

Kalle

Re: Proposed changes to Shiro annotations

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

>
> >> Yes, for 1.0.1 I only suggested adding ElementType.Type for @Target,
> >> that should be perfectly backwards compatible right?
> > If we add that in 1.0.1, and they rely on the behavior that it triggers
> and
> > then downgrade to 1.0.0, then their software could break because that
> > behavior won't be enabled, right?
>
> After downgrading, it'd show up as a compiler error - which makes all
> the sense to me.
>

This means it's not backwards compatibile according to the APR guidelines
(that guarantee both binary and source compatibility across patch versions):

"We define "source compatible" to mean that an application will continue to
build without error, and that the semantics will remain unchanged."

Don't we want to retain those guidelines?

Les

Re: Proposed changes to Shiro annotations

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

On Sat, Jun 26, 2010 at 9:41 AM, Les Hazlewood <lh...@apache.org> wrote:
>> Yes, for 1.0.1 I only suggested adding ElementType.Type for @Target,
>> that should be perfectly backwards compatible right?
> If we add that in 1.0.1, and they rely on the behavior that it triggers and
> then downgrade to 1.0.0, then their software could break because that
> behavior won't be enabled, right?

After downgrading, it'd show up as a compiler error - which makes all
the sense to me.

Kalle


>> > about adding expression support to annotations using a proper grammar.
>> > Something like:
>> > @SecurityCheck("perm(user:read:{0}.id) && (role[admin] ||
>> role[developer])")
>> > public void someSecureMethod(User user) { ... }
>> > where {0} represents the first method argument, {1} the second, etc.
>>
>> I have to say the syntax looks super ugly :P.
>
>
> Haha, I definitely don't have any preferences on syntax - anything that
> makes sense/looks good would be nice :)
>
>
>> It's easy to extend Shiro and it might be better
>> to see first what users come up with.
>>
>
> Sure - this isn't a high priority at the moment.  We'll see if anyone wants
> to take a crack at this.
>
>>
>> > As for the existing annotations, I believe they're both supposed to be
>> > aggregating - i.e. 'ALL' for both of them.  My personal preference is
>> that
>> > we keep the existing annotations and have retain 'ALL' semantics.
>> >  @RequiresRoles(new String[] { "dev", "teamLead"}) to me definitely reads
>> as
>> > I should have both the 'dev' and 'teamLead' roles in order for the method
>> to
>> > execute.  @RequiresAnyRoles (again, to me) sounds like a good alternative
>> > for 'OR' semantics.
>>
>> Agree. Javadoc for @RequiresRoles says "Requires the currently
>> executing Subject to have one or more specified roles" so at least I
>> could read it as "any of these roles". Maybe best to rewrite it as
>> "Requires the currently executing Subject to have all of the specified
>> roles" and be done with it. We can then keep the existing annotations
>> for ALL semantics and introduce @RequiresAnyXXX in 1.1.
>>
>
> +1 Sounds good to me!
>
> Les
>

Re: Proposed changes to Shiro annotations

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

>
> Yes, for 1.0.1 I only suggested adding ElementType.Type for @Target,
> that should be perfectly backwards compatible right?


If we add that in 1.0.1, and they rely on the behavior that it triggers and
then downgrade to 1.0.0, then their software could break because that
behavior won't be enabled, right?


> > about adding expression support to annotations using a proper grammar.
> > Something like:
> > @SecurityCheck("perm(user:read:{0}.id) && (role[admin] ||
> role[developer])")
> > public void someSecureMethod(User user) { ... }
> > where {0} represents the first method argument, {1} the second, etc.
>
> I have to say the syntax looks super ugly :P.


Haha, I definitely don't have any preferences on syntax - anything that
makes sense/looks good would be nice :)


> It's easy to extend Shiro and it might be better
> to see first what users come up with.
>

Sure - this isn't a high priority at the moment.  We'll see if anyone wants
to take a crack at this.

>
> > As for the existing annotations, I believe they're both supposed to be
> > aggregating - i.e. 'ALL' for both of them.  My personal preference is
> that
> > we keep the existing annotations and have retain 'ALL' semantics.
> >  @RequiresRoles(new String[] { "dev", "teamLead"}) to me definitely reads
> as
> > I should have both the 'dev' and 'teamLead' roles in order for the method
> to
> > execute.  @RequiresAnyRoles (again, to me) sounds like a good alternative
> > for 'OR' semantics.
>
> Agree. Javadoc for @RequiresRoles says "Requires the currently
> executing Subject to have one or more specified roles" so at least I
> could read it as "any of these roles". Maybe best to rewrite it as
> "Requires the currently executing Subject to have all of the specified
> roles" and be done with it. We can then keep the existing annotations
> for ALL semantics and introduce @RequiresAnyXXX in 1.1.
>

+1 Sounds good to me!

Les

Re: Proposed changes to Shiro annotations

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

On Fri, Jun 25, 2010 at 2:18 PM, Les Hazlewood <lh...@apache.org> wrote:
> My initial concern is that 1.0.1 should be backwards and forwards compatible
> w/ 1.0.0 and 1.0.3 respectively per traditional APR compatibility

Yes, for 1.0.1 I only suggested adding ElementType.Type for @Target,
that should be perfectly backwards compatible right? 1.0.0 only allows
ElementType.Method and so this is just an addition to an existing
feature, not a change.

> As to the Annotations, Jeremy and I have talked a number of times (long ago)
> about adding expression support to annotations using a proper grammar.
> Something like:
> @SecurityCheck("perm(user:read:{0}.id) && (role[admin] || role[developer])")
> public void someSecureMethod(User user) { ... }
> where {0} represents the first method argument, {1} the second, etc.

I have to say the syntax looks super ugly :P. Yes, I recall reading
some of these discussions and I'm keen to have it but at the same
time, I'd add this very conservatively to the Shiro core through
existing use cases. It's easy to extend Shiro and it might be better
to see first what users come up with.

> As for the existing annotations, I believe they're both supposed to be
> aggregating - i.e. 'ALL' for both of them.  My personal preference is that
> we keep the existing annotations and have retain 'ALL' semantics.
>  @RequiresRoles(new String[] { "dev", "teamLead"}) to me definitely reads as
> I should have both the 'dev' and 'teamLead' roles in order for the method to
> execute.  @RequiresAnyRoles (again, to me) sounds like a good alternative
> for 'OR' semantics.

Agree. Javadoc for @RequiresRoles says "Requires the currently
executing Subject to have one or more specified roles" so at least I
could read it as "any of these roles". Maybe best to rewrite it as
"Requires the currently executing Subject to have all of the specified
roles" and be done with it. We can then keep the existing annotations
for ALL semantics and introduce @RequiresAnyXXX in 1.1.

Kalle



> On Fri, Jun 25, 2010 at 1:24 PM, Kalle Korhonen
> <ka...@gmail.com>wrote:
>
>> Related to https://issues.apache.org/jira/browse/SHIRO-175 and some of
>> the Shiro extensions we (Tynamo.org) have made, I'd like to change
>> @Target(ElementType.METHOD) of all annotations to
>> @Target({ElementType.TYPE, ElementType.METHOD}). The obvious semantics
>> of attaching a security annotation to a class is that the same
>> permission would apply to all method invocations in that class.
>>
>> SHIRO-175 suggests that we'd deprecate the existing annotations in
>> favor of RequiresAnyXXX and RequiresAllXXX annotations, forcing to
>> target 1.1 release for the issue resolution. We could also keep the
>> existing annotations and pick either ANY or ALL semantics in order to
>> avoid deprecating anything - not sure which one is better. The current
>> semantics are unclear, the Javadoc implies ANY for RequiresRoles but
>> ALL for RequiresPermissions.
>>
>> Here's what I'd suggest:
>> - 1.0.1 change @Target to apply to TYPEs as well, fix ambiguity of
>> current annotations (especially if we decide to keep them)
>> - 1.1.0 resolve SHIRO-175 and add RequiresAnyXXX and RequiresAllXXX
>> annotations (and deprecate existing annotations if we choose to do so)
>>
>> I can handle implementing the changes. Any comments?
>>
>> Kalle
>>
>

Re: Proposed changes to Shiro annotations

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

2010/6/28 Willi Schönborn <sc...@cosmocode.de>:
> Correct we if i am wrong but i think you can omit the new String[] stuff
> like this:
> @RequiresRoles({"dev",  "teamLead"})
> which would look/read so much better.
> If there is only one role it should be possible to write:
> @RequiresRoles("dev")

Yes, that's just standard annotations.

Kalle

Re: Proposed changes to Shiro annotations

[Willi Schönborn <sc...@cosmocode.de>]

On 25/06/2010 23:18, Les Hazlewood wrote:
> Definitely some good ideas here.
>
> My initial concern is that 1.0.1 should be backwards and forwards compatible
> w/ 1.0.0 and 1.0.3 respectively per traditional APR compatibility
> guidelines.  If we as a project want to retain that compatibility policy,
> shouldn't these changes go into 1.1? (I personally think the compatibility
> guidelines are a sound way to go about this kind of stuff, barring some
> other reasonable alternative.)
>
> As to the Annotations, Jeremy and I have talked a number of times (long ago)
> about adding expression support to annotations using a proper grammar.
> Something like:
>
> @SecurityCheck("perm(user:read:{0}.id)&&  (role[admin] || role[developer])")
> public void someSecureMethod(User user) { ... }
>
> where {0} represents the first method argument, {1} the second, etc.
>
> I don't know if we'll get this in the codebase any time soon, but I wanted
> to surface it again to get the creative juices flowing again.  Food for
> thought.
>
> As for the existing annotations, I believe they're both supposed to be
> aggregating - i.e. 'ALL' for both of them.  My personal preference is that
> we keep the existing annotations and have retain 'ALL' semantics.
>   @RequiresRoles(new String[] { "dev", "teamLead"}) to me definitely reads as
>    
Correct we if i am wrong but i think you can omit the new String[] stuff 
like this:
@RequiresRoles({"dev",  "teamLead"})
which would look/read so much better.
If there is only one role it should be possible to write:
@RequiresRoles("dev")
> I should have both the 'dev' and 'teamLead' roles in order for the method to
> execute.  @RequiresAnyRoles (again, to me) sounds like a good alternative
> for 'OR' semantics.
>
> Thoughts?
>
> Les
>
> On Fri, Jun 25, 2010 at 1:24 PM, Kalle Korhonen
> <ka...@gmail.com>wrote:
>
>    
>> Related to https://issues.apache.org/jira/browse/SHIRO-175 and some of
>> the Shiro extensions we (Tynamo.org) have made, I'd like to change
>> @Target(ElementType.METHOD) of all annotations to
>> @Target({ElementType.TYPE, ElementType.METHOD}). The obvious semantics
>> of attaching a security annotation to a class is that the same
>> permission would apply to all method invocations in that class.
>>
>> SHIRO-175 suggests that we'd deprecate the existing annotations in
>> favor of RequiresAnyXXX and RequiresAllXXX annotations, forcing to
>> target 1.1 release for the issue resolution. We could also keep the
>> existing annotations and pick either ANY or ALL semantics in order to
>> avoid deprecating anything - not sure which one is better. The current
>> semantics are unclear, the Javadoc implies ANY for RequiresRoles but
>> ALL for RequiresPermissions.
>>
>> Here's what I'd suggest:
>> - 1.0.1 change @Target to apply to TYPEs as well, fix ambiguity of
>> current annotations (especially if we decide to keep them)
>> - 1.1.0 resolve SHIRO-175 and add RequiresAnyXXX and RequiresAllXXX
>> annotations (and deprecate existing annotations if we choose to do so)
>>
>> I can handle implementing the changes. Any comments?
>>
>> Kalle
>>
>>      
>    

Re: Proposed changes to Shiro annotations

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

Definitely some good ideas here.

My initial concern is that 1.0.1 should be backwards and forwards compatible
w/ 1.0.0 and 1.0.3 respectively per traditional APR compatibility
guidelines.  If we as a project want to retain that compatibility policy,
shouldn't these changes go into 1.1? (I personally think the compatibility
guidelines are a sound way to go about this kind of stuff, barring some
other reasonable alternative.)

As to the Annotations, Jeremy and I have talked a number of times (long ago)
about adding expression support to annotations using a proper grammar.
Something like:

@SecurityCheck("perm(user:read:{0}.id) && (role[admin] || role[developer])")
public void someSecureMethod(User user) { ... }

where {0} represents the first method argument, {1} the second, etc.

I don't know if we'll get this in the codebase any time soon, but I wanted
to surface it again to get the creative juices flowing again.  Food for
thought.

As for the existing annotations, I believe they're both supposed to be
aggregating - i.e. 'ALL' for both of them.  My personal preference is that
we keep the existing annotations and have retain 'ALL' semantics.
 @RequiresRoles(new String[] { "dev", "teamLead"}) to me definitely reads as
I should have both the 'dev' and 'teamLead' roles in order for the method to
execute.  @RequiresAnyRoles (again, to me) sounds like a good alternative
for 'OR' semantics.

Thoughts?

Les

On Fri, Jun 25, 2010 at 1:24 PM, Kalle Korhonen
<ka...@gmail.com>wrote:

> Related to https://issues.apache.org/jira/browse/SHIRO-175 and some of
> the Shiro extensions we (Tynamo.org) have made, I'd like to change
> @Target(ElementType.METHOD) of all annotations to
> @Target({ElementType.TYPE, ElementType.METHOD}). The obvious semantics
> of attaching a security annotation to a class is that the same
> permission would apply to all method invocations in that class.
>
> SHIRO-175 suggests that we'd deprecate the existing annotations in
> favor of RequiresAnyXXX and RequiresAllXXX annotations, forcing to
> target 1.1 release for the issue resolution. We could also keep the
> existing annotations and pick either ANY or ALL semantics in order to
> avoid deprecating anything - not sure which one is better. The current
> semantics are unclear, the Javadoc implies ANY for RequiresRoles but
> ALL for RequiresPermissions.
>
> Here's what I'd suggest:
> - 1.0.1 change @Target to apply to TYPEs as well, fix ambiguity of
> current annotations (especially if we decide to keep them)
> - 1.1.0 resolve SHIRO-175 and add RequiresAnyXXX and RequiresAllXXX
> annotations (and deprecate existing annotations if we choose to do so)
>
> I can handle implementing the changes. Any comments?
>
> Kalle
>