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