[cgiapp] Authentication/Authorization prerun interaction

[Cees Hek]

On Thu, Jan 12, 2012 at 5:18 AM, Lyle Brooks <brooks at deseret.com> wrote:
> Quoting Cees Hek (ceeshek at gmail.com):
>> On Wed, Jan 11, 2012 at 5:40 AM, Lyle Brooks <brooks at deseret.com> wrote:
>> The problem here is that Authorization does not necessarily have
>> anything to do with Authentication.  You could authorize someone based
>> on the IP address that they came from for instance.
>
> Yes, I can understand that point.  However, I would "think" that the
> case where Authorization is used in conjunction with Authentication
> would often be a far more prevalent 'use case'.  No hard data to back
> this up...just my gut feel.

Yes, I would agree with that.

>> The quick solution is to short circuit your code so that authorization
>> checks are only executed when a user is authenticated.  You didn't
>> provide any code so I can't be sure if that is easy or not...
>
> There's not much code to provide.  I pretty much use the modules by
> taking all the defaults.
...
> In the Application's cgiapp_init() method I have two calls..
>
> $self->authen->protected_runmodes( qw( display process ) );
>
> $self->authorization->authz_runmodes(
>    display => 'admin',
>    process => 'admin',
> );

I tend to do authorization the following way:


__PACKAGE__->authz('group')->config(
    DRIVER => [ 'Generic', sub {
       my ($username,$group) = @_;
       return MyApp->user($username)->in_group($group);
    } ],
);

# Only administrators can access anything in this module
__PACKAGE__->add_callback('prerun', sub {
    my $self = shift;
    return $self->forbidden unless
__PACKAGE__->authz('group')->authorize('administrator');
});

-or-

# Only administrators can run this runmode
sub admin_something {
  my $self = shift;
  return $self->forbidden unless
__PACKAGE__->authz('group')->authorize('administrator');
  ...
}

Authentication is handled in Apache using Apache2::AuthCookie which is
probably why I can't come across this problem before.

> Seems to me in this very basic case, that an unauthenticated user
> should be given the opportunity to authenticate.  As it now stands,
> the "Forbidden" page is returned...and the user (even one who would
> be authorized to access the runmodes) is never the opportunity to
> authenticate...and hence then be judged to be authorized).
>
> This is why I argue that a change should be made to allow the
> authentication (via the prerun mode) be given precedence over the
> authorization's Forbidden response.

Yup I agree

> >From my reading of the CGI::Application's documention on the
> use of prerun_mode(), its purpose is to allow for a new runmode
> to supercede the requested runmode....in which case, it seems
> like it should as a byproduct also set $self->{__CURRENT_RUNMODE}
> In CGI::Application::prerun_mode() replace the line
>
> $self->{__PRERUN_MODE} = $prerun_mode;
>
> with
>
> $self->{__PRERUN_MODE} = $self->{__CURRENT_RUNMODE} = $prerun_mode;
>
> (which I think would address my issue).

You could test that easily by copying the prerun method into your
application and adding your patch.

I think this is probably the most appropriate way to solve this issue.
 The plugin should be able to ask CGI::Application 'What is the
runmode that you are about to execute", without having to do multiple
checks.

> I don't what impact that would have on existing code written
> for CGI::Application.

Neither do I, but I would suspect it would be minimal (does anyone
else have any comments on this?)

> If that approach turned out to be inappropriate, then I would
> in CGI::Application::Plugin::Authorization::prerun_callback()
>
> the line
>
> if ($rule = $auth->is_authz_runmode($self->get_current_runmode)) {
>
> be replaced with something like...
>
>
> my $candidate_runmode = $self->prerun_mode() || $self->get_current_runmode();
> if ($rule = $auth->is_authz_runmode($candidate_runmode)) {

This brings me back to my comment above.  Why should the plugin have
to check in multiple places to find out what runmode is about to be
executed?

Cheers,

Cees