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 <kalle.o.korho...@gmail.com> wrote: > On Thu, Jul 29, 2010 at 1:01 PM, Les Hazlewood <lhazlew...@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 >
