On Thu, Jul 29, 2010 at 11:06 AM, Les Hazlewood <lhazlew...@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
