On 1/9/11 3:08 PM, "Kalle Korhonen" <[email protected]> wrote:
> On Fri, Jan 7, 2011 at 9:45 AM, Les Hazlewood <[email protected]> wrote:
>> This is mostly an artifact of an old codebase. The idea was
>> originally that a Realm should have the ability to respond to both
>> authorization and authentication operations for a data source. The
>> Authorizer interface included all possible authorization options, so
>> by having the Realm extend that interface, we guaranteed that a Realm
>> could react to any authorization operation if it needed to. A Realm
>> implementation could return false for all Authorizer methods if it
>> didn't want or need to do authorization, so there was no harm in
>> imposing that interface (at the risk of being less than convenient).
>
> Sure, but an authenticatingRealm simply shouldn't react to any
> authorization operations. There's pretty bad code smell that an
> interface needs extend another interface, especially given that it's
> the smaller interface that is extending a larger one.
>
>> I wouldn't classify AuthenticatingRealm as 'useless' - just not
>> convenient (the authentication workflow it does provide is still worth
>> using IMO). Most people choose to use AuthorizingRealm which
>> basically implements all of those methods and is considered more
>> 'convenient' to get around the pain of stubbing the methods.
>
> Useless as in no point using it since using AuthorizingRealm would be
> simpler. You are right about AuthorizingRealm, but if you just want to
> implement authentication, you *shouldn't* and shouldn't need to
> implement those operations at all.
>
>> But, I do see your point, and very much agree that there is a better
>> way to deal with this, and that if we can clean this up for 2.x, I
>> definitely support it. In fact, I've long envisioned an authorization
>> API that is much more coarse-grained, e.g.:
>
> Actually, this was very simple. I already made the change (instead of
> Realm interface extending Authorizer, AuthorizingRealm implements
> Authorizer) and the only place where realm was expected to be
> authorizing was in ModularRealm (plus an AuthorizingRealm unit test).
> As such, I don't see any reason for not putting it in for 1.2.x. I'm
> gonna create an issue for it and commit.
>
Looking at the changes made to ModularRealmAuthorizer, it appears that now,
if I wish to implement a Realm that provides Authorization, I am required to
extend AuthorizingRealm? This seems a bit short-sighted. I'm not
particularly crazy about the inherent change in what "Realm" means, but
wouldn't it at least make more sense for ModularRealmAuthorizer to check if
the realm is an instance of Authorizer?
>> AuthorizationResponse response =
>> realm.isAuthorized(AuthorizationRequest request);
>> This would allow supporting a multitude of authorization scenarios,
>> and because of the very coarse-grained nature of the method, the
>> Request/Response APIs could change and/or be modified over time
>> without much impact on the Realm implementations. We could also
>> formulate very robust authorization checks via a Criteria API (e.g.
>> role = 'blah' and in group 'foo' except with permission 'bar', etc.)
>
> Sure, this all all about changing authorization mechanism and would
> like need to wait for 2.x.
>
> Kalle
