httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jus...@erenkrantz.com>
Subject Re: svn commit: r351547 - in /httpd/httpd/branches/authz-dev: include/http_core.h modules/aaa/mod_authz_host.c server/core.c server/request.c
Date Fri, 02 Dec 2005 05:53:07 GMT
On Fri, Dec 02, 2005 at 01:19:11AM -0000, bnicholes@apache.org wrote:
> Author: bnicholes
> Date: Thu Dec  1 17:19:07 2005
> New Revision: 351547
> 
> URL: http://svn.apache.org/viewcvs?rev=351547&view=rev
> Log:
> Reimplement ap_some_auth_required as an optional function since the data has moved to
mod_authz_host yet it is still needed by the request handler
> 
> Modified:
>     httpd/httpd/branches/authz-dev/include/http_core.h
>     httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c
>     httpd/httpd/branches/authz-dev/server/core.c
>     httpd/httpd/branches/authz-dev/server/request.c

In general, woo-hoo.  =)

> Modified: httpd/httpd/branches/authz-dev/include/http_core.h
> URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/authz-dev/include/http_core.h?rev=351547&r1=351546&r2=351547&view=diff
> ==============================================================================
> --- httpd/httpd/branches/authz-dev/include/http_core.h (original)
> +++ httpd/httpd/branches/authz-dev/include/http_core.h Thu Dec  1 17:19:07 2005
> @@ -687,6 +687,7 @@
>  
>  APR_DECLARE_OPTIONAL_FN(const apr_array_header_t *, authz_host_ap_requires,
>                          (request_rec *r));
> +APR_DECLARE_OPTIONAL_FN(int, authz_some_auth_required, (request_rec *r));

I'm not quite clear on why we need to have an optional function here.  (I'm
probably missing something obvious here.)

>  /*
>  APR_DECLARE_OPTIONAL_FN(const char *, authz_host_ap_auth_type, (request_rec *r));
>  APR_DECLARE_OPTIONAL_FN(const char *, authz_host_ap_auth_name, (request_rec *r));
> 
> Modified: httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c
> URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c?rev=351547&r1=351546&r2=351547&view=diff
> ==============================================================================
> --- httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c (original)
> +++ httpd/httpd/branches/authz-dev/modules/aaa/mod_authz_host.c Thu Dec  1 17:19:07 2005

I know you probably don't want to hear this, but this module has nothing to
do with hosts any more.  Want to reconsider a mod_authz_require or
something like that?  ;-)

But, there's no way we should have the authz provider lookup code stuck
inside of mod_authz_host. 

> @@ -520,6 +520,46 @@
>      return conf->ap_requires;
>  }
>  
> +static int authz_some_auth_required(request_rec *r)
> +{
> +    authz_host_dir_conf *conf = ap_get_module_config(r->per_dir_config,
> +            &authz_host_module);
> +    authz_provider_list *current_provider;
> +    int req_authz = 1;

Shouldn't this be defaulting to 0?  If there aren't any providers active or
interested, why return 1?

> +    current_provider = conf->providers;
> +    do {
> +        const authz_provider *provider;
> +
> +        /* For now, if a provider isn't set, we'll be nice and use the file
> +        * provider.
> +        */
> +        if (!current_provider) {
> +            provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
> +                                          AUTHZ_DEFAULT_PROVIDER, "0");
> +
> +            if (!provider || !provider->check_authorization) {
> +                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
> +                              "No Authz providers configured.  Assmuming no authorization
required.");
> +                req_authz = 0;
> +                break;
> +            }

Considering we already do a check at config/registration time, do we really
need to repeat this check at request-time?

> +        }
> +        else {
> +            provider = current_provider->provider;
> +        }
> +
> +        if (current_provider->method_mask & (AP_METHOD_BIT << r->method_number))
{
> +            req_authz = 1;
> +            break;
> +        }
> +    
> +        current_provider = current_provider->next;
> +    } while (current_provider);

The current version of the branch has a bunch of dead-code on it for this
loop.  We should strip it to the bare minimum:

...
req_authz = 0;
provider = conf->providers;
while (provider) {
  if (provider->method_mask & (AP_METHOD_BIT << r->method_number)) {
    req_authz = 1;
    break;
  }
  provider = provider->next;
}

return req_authz;
...

Yet, something unsettles me about having to do this check at all.  However,
my thoughts haven't yet coalesced and I'm falling asleep at the keyboard.
I'll try to collect my thoughts on this and follow up later.  -- justin

Mime
View raw message