Realm extends Authorizer, why?

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

Why does a Realm extend Authorizer? Shouldn't only the
AuthorizingRealm implement Authorizer? This makes the
AuthenticatingRealm fairly useless since you need to implement a bunch
of authorizing operations even if your realm is not supposed to do
anything more than just authenticate. There's probably a good reason
for Authorizer being the top level interface rather than Realm, but
it's just not immediately obvious for me. Even then, I hope we could
straighten this out in a future release since it just doesn't seem
right to me. Les, care to comment, I bet you have a pretty good
insight into this.

Kalle

Re: Realm extends Authorizer, why?

[Jared Bunting <ja...@digitalreasoning.com>]

On 1/11/11 1:22 PM, "Kalle Korhonen" <ka...@gmail.com> wrote:

> On Mon, Jan 10, 2011 at 2:22 PM, Les Hazlewood <lh...@apache.org> wrote:
>> Can we guarantee that if a user upgrades from 1.1 to 1.2 their
>> existing code and/or config won't break due to this change?  If not, I
>> question the need for this to occur right now.  Sure, it's not an
>> ideal thing having Realm extend Authorizer, but if it breaks code, I
>> don't know what the hurry would be.
> 
> Hey Les, sure, I did think about it. I can only "virtually guarantee"
> - as you say, most users have extended AuthorizationRealm anyway where
> everything works as before. Only if they've implemented Realm
> directly, they'd have to explicitly Authorizer as the other
> implemented interface. I don't think you can create configuration only
> in a way that would break this. I'm of the opinion that any code smell
> should be addressed as soon as possible to prevent it from becoming a
> bigger issue. This is easy to document in the upgrade notes since we
> didn't change any of the actual interfaces. It fails fast and is easy
> to correct. If we didn't do it now, it would have to wait for till
> unplanned and unscheduled 2.x and for such a small upgrade issue, I
> think it's much better to encourage users to upgrade and keep up with
> the api changes in small steps.
> 

This will also break any code written against the Realm interface that
attempts to invoke those authorization methods on it.

>> The fact that Realms have always been able to perform both
>> authentication and authorization has been true for a very long time.
>> Removing it for a minor release sounds a little hasty to me.  I'm not
>> questioning the change - just the timing.
> 
> But the AuthenticatingRealm doesn't imply that. You know an
> authenticating only realm and an ability to differentiate between
> authenticating only and authorizing realms makes sense. We could of
> course keep the other changes I made and add Authorizer back to the
> top - but since there's no way to indicate it as deprecated (unless we
> introduced a new base interface replacing Realm, which would be far
> worse), it seems to me repositioning the interface in the inheritance
> hierarchy is still a much better option.
> 
> Kalle

Re: Realm extends Authorizer, why?

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

On Mon, Jan 10, 2011 at 2:22 PM, Les Hazlewood <lh...@apache.org> wrote:
> Can we guarantee that if a user upgrades from 1.1 to 1.2 their
> existing code and/or config won't break due to this change?  If not, I
> question the need for this to occur right now.  Sure, it's not an
> ideal thing having Realm extend Authorizer, but if it breaks code, I
> don't know what the hurry would be.

Hey Les, sure, I did think about it. I can only "virtually guarantee"
- as you say, most users have extended AuthorizationRealm anyway where
everything works as before. Only if they've implemented Realm
directly, they'd have to explicitly Authorizer as the other
implemented interface. I don't think you can create configuration only
in a way that would break this. I'm of the opinion that any code smell
should be addressed as soon as possible to prevent it from becoming a
bigger issue. This is easy to document in the upgrade notes since we
didn't change any of the actual interfaces. It fails fast and is easy
to correct. If we didn't do it now, it would have to wait for till
unplanned and unscheduled 2.x and for such a small upgrade issue, I
think it's much better to encourage users to upgrade and keep up with
the api changes in small steps.

> The fact that Realms have always been able to perform both
> authentication and authorization has been true for a very long time.
> Removing it for a minor release sounds a little hasty to me.  I'm not
> questioning the change - just the timing.

But the AuthenticatingRealm doesn't imply that. You know an
authenticating only realm and an ability to differentiate between
authenticating only and authorizing realms makes sense. We could of
course keep the other changes I made and add Authorizer back to the
top - but since there's no way to indicate it as deprecated (unless we
introduced a new base interface replacing Realm, which would be far
worse), it seems to me repositioning the interface in the inheritance
hierarchy is still a much better option.

Kalle

Re: Realm extends Authorizer, why?

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

Hi Kalle,

Can we guarantee that if a user upgrades from 1.1 to 1.2 their
existing code and/or config won't break due to this change?  If not, I
question the need for this to occur right now.  Sure, it's not an
ideal thing having Realm extend Authorizer, but if it breaks code, I
don't know what the hurry would be.

The fact that Realms have always been able to perform both
authentication and authorization has been true for a very long time.
Removing it for a minor release sounds a little hasty to me.  I'm not
questioning the change - just the timing.

Regards,

Les

Re: Realm extends Authorizer, why?

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

On Sun, Jan 9, 2011 at 1:55 PM, Jared Bunting
<ja...@digitalreasoning.com> wrote:
> 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?

Duh. Of course, thanks, give a me a second.

Kalle

Re: Realm extends Authorizer, why?

[Jared Bunting <ja...@digitalreasoning.com>]

On 1/9/11 3:08 PM, "Kalle Korhonen" <ka...@gmail.com> wrote:

> On Fri, Jan 7, 2011 at 9:45 AM, Les Hazlewood <lh...@apache.org> 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

Re: Realm extends Authorizer, why?

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

On Fri, Jan 7, 2011 at 9:45 AM, Les Hazlewood <lh...@apache.org> 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.

> 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

Re: Realm extends Authorizer, why?

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

Hi Kalle,

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).

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.

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.:

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.)

I don't know if a request/response + Criteria solution is a good idea
yet - it would likely add a level of complexity that most Realm
implementors may not want to see, but it would provide a tremendous
amount of flexibility that may justify it - especially if we're trying
to provide a definitive authorization API.  It's worth discussing
anyhow.

It also seems like Authentication should mirror this same approach,
for the same benefits, e.g.:

AuthenticationResponse response = subject.login(AuthenticationRequest request);

This would allow a much cleaner way of performing multi-stage
authentication (something I need to solve myself), where the first
authentication attempt is successful, but the response dictates that
another (different) type of authentication be used to complete the
process.  I've seen this need in the banking/financial industry, and
the only way to solve it now is to do a lot of custom coding in realms
and use AuthenticationExceptions to trigger different workflows
(yuck).  It also mirrors SAML's structure and could work in
conjunction with that as well.

Anyway, this is probably a lot more than what you were asking for, but
I figured now was a good time to 'get it out' to see what others think
about it since it stems from the same concern represented by your
question.

Cheers,

Les


On Fri, Jan 7, 2011 at 12:18 AM, Kalle Korhonen
<ka...@gmail.com> wrote:
> Why does a Realm extend Authorizer? Shouldn't only the
> AuthorizingRealm implement Authorizer? This makes the
> AuthenticatingRealm fairly useless since you need to implement a bunch
> of authorizing operations even if your realm is not supposed to do
> anything more than just authenticate. There's probably a good reason
> for Authorizer being the top level interface rather than Realm, but
> it's just not immediately obvious for me. Even then, I hope we could
> straighten this out in a future release since it just doesn't seem
> right to me. Les, care to comment, I bet you have a pretty good
> insight into this.
>
> Kalle