As I mentioned in my last commit, it still needs some clean up.
Please, Please feel free to jump in and clean it up wherever you see the
need. I don't have all of the answers to why things were done the way
they were before and if we still need to do it that way now or is there
a better way. In this branch I have been trying to maintain backward
API compatibility as much as it makes sense. But there are questions I
have about the need of some APIs such as ap_requires() and
ap_some_auth_required(), etc. These are points that I need feedback on
and if we can eliminate them, like as was discussed with AuthType, then
lets do it.
>>> On 12/1/2005 at 10:53:07 pm, in message
<[EMAIL PROTECTED]>, [EMAIL PROTECTED]
wrote:
>
=============================================================================
> =
>> --- 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.)
The optional functions are here because the data moved but core still
needs it in order to process the request. If we can eliminate the need
for core to require this information, then lets do it and get rid of the
optional functions. The authz_host_ap_requires() optional function
(used in the ap_requires() API) can probably be removed in the clean up
since the "Require" line is no longer handled like it was before. In
other words, there isn't a need to keep or retrieve an array of
"Require" lines that must be parsed at request time. The only purpose
for the ap_require() function was to get the array of "Require" lines
for a particular dir_config.
>
=============================================================================
> =
>> --- 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.
>
I'm not opposed to creating a new mod_authz_require module if it is
necessary. My only concern is auth_module overload. It just seems
wrong to have to load 5-6 different authx_modules just to get simple
authentication working. I chose to move the "Require" directive into
mod_authz_host simply thinking that "host" would refer to non-authz
specific module functionality (ie. a catch-all for global authz
functionality). But I can be easily convinced to put "Require" along
with "AuthName" somewhere else if needed. I need opinions and feedback
on this issue please.
>> @@ -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?
>
True, it should default to 0.
>> + 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?
This is another area that needs some clean up. This is kind of a cut
and paste left-over from the authnprovider stuff. If a "Require" line
does not exist, in other words no authz provider was requested, should
we attempt to default to something like 'valid-user' in the same way
that the authn stuff defaults to 'file'? As it stands now, attempting
to default would never happen because of the call to
ap_some_auth_required() during the request processing that skips authz
checking if there isn't a "Require" line present.
>
>> + }
>> + 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:
Not sure what dead code you are referring to, but by all means, strip
it out if it isn't needed.
>
> ...
> 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
The 'method_mask' check is there so that if the "Require" line is
enclosed in a <Limit> block, we only apply authorization for the
appropriate methods. We may be able to consolidate this to the
auth_checker hook function before the providers are called and then
eliminate this check in the authz providers.
Like I mentioned before, Please, Please jump in and help out here. I
haven't got all of the answers, I just want to see this functionality
implemented and hopefully improve the authn/authz handling.
Brad
