Take a quick pass through the refactored authz code. No real big changes here.

* modules/aaa/mod_authz_host.c: Tweak locations of some comments; reflow
  long lines; correct and simplify (I think) ap_some_auth_required logic.
* modules/aaa/mod_auth.h: Remove AUTHZ_DECLINED for now; reflow long lines;
  improve comments; remove some no longer useful comments
* modules/aaa/mod_authz_user.c: Reflow long lines and remove unneeded variable.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/authz-dev@351902 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/modules/aaa/mod_auth.h b/modules/aaa/mod_auth.h
index 43c372bd831be9fd88a71c3a226d67c0bcfe81ff..48d585494eda0704550b20de0d5024ffe3d0651b 100644 (file)
--- a/modules/aaa/mod_auth.h
+++ b/modules/aaa/mod_auth.h
@@ -16,7 +16,7 @@
 
 /**
  * @file  mod_auth.h
- * @brief uthentication Extension Module for Apache
+ * @brief Authentication and Authorization Extension for Apache
  *
  * @defgroup MOD_AUTH mod_auth
  * @ingroup  APACHE_MODS
@@ -38,7 +38,7 @@ extern "C" {
 #define AUTHZ_PROVIDER_GROUP "authz"
 #define AUTHN_DEFAULT_PROVIDER "file"
 #define AUTHZ_DEFAULT_PROVIDER "valid-user"
-    
+
 #define AUTHZ_GROUP_NOTE "authz_group_note"
 #define AUTHN_PROVIDER_NAME_NOTE "authn_provider_name"
 #define AUTHZ_PROVIDER_NAME_NOTE "authz_provider_name"
@@ -53,7 +53,6 @@ typedef enum {
 
 typedef enum {
     AUTHZ_DENIED,
-    AUTHZ_DECLINED,
     AUTHZ_GRANTED,
     AUTHZ_GENERAL_ERROR
 } authz_status;
@@ -63,7 +62,7 @@ typedef struct {
      * if we can validate this user/password combination.
      */
     authn_status (*check_password)(request_rec *r, const char *user,
-                                  const char *password);
+                                   const char *password);
 
     /* Given a user and realm, expected to return AUTH_USER_FOUND if we
      * can find a md5 hash of 'user:realm:password'
@@ -83,9 +82,11 @@ struct authn_provider_list {
 
 typedef struct {
     /* Given a request_rec, expected to return AUTH_GRANTED
-    * if we can authorize user access.
-    */
-    authz_status (*check_authorization)(request_rec *r, apr_int64_t method_mask, const char *require_line);
+     * if we can authorize user access.
+     */
+    authz_status (*check_authorization)(request_rec *r,
+                                        apr_int64_t method_mask,
+                                        const char *require_line);
 } authz_provider;
 
 /* A linked-list of authn providers. */
@@ -95,24 +96,12 @@ struct authz_provider_list {
     const char *provider_name;
     const authz_provider *provider;
     authz_provider_list *next;
-    /** Where the require line is in the config file. */
+    /** If a Limit method is in effect, this field will be set */
     apr_int64_t method_mask;
-    /** The complete string from the command line */
+    /** String following 'require <provider>' from config file */
     char *requirement;
 };
 
-/* Need to add an enum for authz_status.  Convert one of the authorization
-   modules to deal with the new require directive.
-*/
-
-
-#if 0
-typedef struct {
-    /* For a given user, return a hash of all groups the user belongs to.  */
-    apr_hash_t * (*get_user_groups)(request_rec *r, const char *user);
-} authz_provider;
-#endif
-
 #ifdef __cplusplus
 }
 #endif

diff --git a/modules/aaa/mod_authz_host.c b/modules/aaa/mod_authz_host.c
index 6ccbb84f384286035dae71aa8c356286da21b05e..39ad360d9d26ad5ca2b5798319cbb92aefc419b4 100644 (file)
--- a/modules/aaa/mod_authz_host.c
+++ b/modules/aaa/mod_authz_host.c
@@ -96,7 +96,7 @@ static void *merge_authz_host_dir_config(apr_pool_t *a, void *basev, void *newv)
     authz_host_dir_conf *base = (authz_host_dir_conf *)basev;
     authz_host_dir_conf *new = (authz_host_dir_conf *)newv;
     authz_host_dir_conf *conf;
-    
+
     /* Create this conf by duplicating the base, replacing elements
     * (or creating copies for merging) where new-> values exist.
     */
@@ -234,6 +234,7 @@ static const char *add_authz_provider(cmd_parms *cmd, void *config,
     authz_provider_list *newp;
 
     newp = apr_pcalloc(cmd->pool, sizeof(authz_provider_list));
+    /* XXX: Split this out to the name and then the rest of the directive. */
     newp->provider_name = apr_pstrdup(cmd->pool, arg);
     newp->requirement = apr_pstrdup(cmd->pool, arg);
     newp->method_mask = cmd->limited;
@@ -242,19 +243,21 @@ static const char *add_authz_provider(cmd_parms *cmd, void *config,
     newp->provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
                                         newp->provider_name, "0");
 
+    /* by the time the config file is used, the provider should be loaded
+     * and registered with us.
+     */
     if (newp->provider == NULL) {
-        /* by the time they use it, the provider should be loaded and
-        registered with us. */
         return apr_psprintf(cmd->pool,
                             "Unknown Authz provider: %s",
                             newp->provider_name);
     }
 
+    /* if the provider doesn't provide the appropriate function, reject it */
     if (!newp->provider->check_authorization) {
-        /* if it doesn't provide the appropriate function, reject it */
         return apr_psprintf(cmd->pool,
-                            "The '%s' Authz provider is not supported by any of the "
-                                    "loaded authorization modules", newp->provider_name);
+                            "The '%s' Authz provider is not supported by any "
+                            "of the loaded authorization modules ",
+                            newp->provider_name);
     }
 
     /* Add it to the list now. */
@@ -284,7 +287,8 @@ static const command_rec authz_host_cmds[] =
     AP_INIT_ITERATE2("deny", allow_cmd, NULL, OR_LIMIT,
                      "'from' followed by hostnames or IP-address wildcards"),
     AP_INIT_RAW_ARGS("Require", add_authz_provider, NULL, OR_AUTHCFG,
-                     "Selects which authenticated users or groups may access a protected space"),
+                     "Selects which authenticated users or groups may access "
+                     "a protected space"),
     {NULL}
 };
 /*
@@ -430,7 +434,7 @@ static int check_dir_access(request_rec *r)
 static int authorize_user(request_rec *r)
 {
     authz_host_dir_conf *conf = ap_get_module_config(r->per_dir_config,
-            &authz_host_module);
+                                                     &authz_host_module);
     authz_status auth_result;
     authz_provider_list *current_provider;
 
@@ -439,31 +443,39 @@ static int authorize_user(request_rec *r)
         const authz_provider *provider;
 
         /* For now, if a provider isn't set, we'll be nice and use the file
-        * provider.
-        */
+         * 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 provider configured");
+                              "No default authz provider configured");
                 auth_result = AUTHZ_GENERAL_ERROR;
                 break;
             }
-            apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, AUTHZ_DEFAULT_PROVIDER);
+            apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE,
+                           AUTHZ_DEFAULT_PROVIDER);
         }
         else {
             provider = current_provider->provider;
-            apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE, current_provider->provider_name);
+            apr_table_setn(r->notes, AUTHZ_PROVIDER_NAME_NOTE,
+                           current_provider->provider_name);
         }
 
 
-        auth_result = provider->check_authorization(r, current_provider->method_mask, current_provider->requirement);
+        auth_result = provider->check_authorization(r,
+                        current_provider->method_mask,
+                        current_provider->requirement);
 
         apr_table_unset(r->notes, AUTHZ_PROVIDER_NAME_NOTE);
 
         /* Something occured. Stop checking. */
+        /* XXX: We need to figure out what the implications of multiple
+         * require directives are.  Must all satisfy?  Can we leverage
+         * satisfy here then?
+         */
         if (auth_result != AUTHZ_DENIED) {
             break;
         }
@@ -479,11 +491,6 @@ static int authorize_user(request_rec *r)
     if (auth_result != AUTHZ_GRANTED) {
         int return_code;
 
-/* XXX need to deal with DECLINED vs DENIED.  DECLINED may not even
-   be needed since we are only going to call registered require providers.
-   I assume that it will deal with passing from one provider to the next
-   according to the order and the Authz_xxx_Authoritative directives.
-*/
         switch (auth_result) {
             case AUTHZ_DENIED:
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -493,15 +500,16 @@ static int authorize_user(request_rec *r)
                 break;
             case AUTHZ_GENERAL_ERROR:
             default:
-            /* We'll assume that the module has already said what its error
-                * was in the logs.
-            */
+                /* We'll assume that the module has already said what its
+                 * error was in the logs.
+                 */
                 return_code = HTTP_INTERNAL_SERVER_ERROR;
                 break;
         }
 
         /* If we're returning 403, tell them to try again. */
         if (return_code == HTTP_UNAUTHORIZED) {
+            /* XXX: Why is this a basic auth failure? */
             ap_note_basic_auth_failure (r);
         }
         return return_code;
@@ -525,38 +533,20 @@ 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;
+    int req_authz = 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");
+    while (current_provider) {
 
-            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;
-/*            }*/
-        }
-        else {
-            provider = current_provider->provider;
-        }
-
-        if (current_provider->method_mask & (AP_METHOD_BIT << r->method_number)) {
+        /* Does this provider config apply for this method */
+        if (current_provider->method_mask &
+                (AP_METHOD_BIT << r->method_number)) {
             req_authz = 1;
             break;
         }
-    
+
         current_provider = current_provider->next;
-    } while (current_provider);
+    }
 
     return req_authz;
 }
@@ -593,7 +583,7 @@ static void register_hooks(apr_pool_t *p)
     */
 
     /* This can be access checker since we don't require r->user to be set. */
-    ap_hook_access_checker(check_dir_access,NULL,NULL,APR_HOOK_MIDDLE);
+    ap_hook_access_checker(check_dir_access, NULL, NULL, APR_HOOK_MIDDLE);
     ap_hook_auth_checker(authorize_user, NULL, NULL, APR_HOOK_MIDDLE);
 }
 

diff --git a/modules/aaa/mod_authz_user.c b/modules/aaa/mod_authz_user.c
index 15efaa47d222e1ddb2b7e3fed911e166add054db..de8aada83dfba038f592ecf03f379c50f6f2dc76 100644 (file)
--- a/modules/aaa/mod_authz_user.c
+++ b/modules/aaa/mod_authz_user.c
@@ -117,9 +117,10 @@ static int check_user_access(request_rec *r)
 }
 #endif
 
-static authz_status user_check_authorization(request_rec *r, apr_int64_t method_mask, const char *require_line)
+static authz_status user_check_authorization(request_rec *r,
+                                             apr_int64_t method_mask,
+                                             const char *require_line)
 {
-    char *user = r->user;
     int m = r->method_number;
     const char *t, *w;
 
@@ -131,11 +132,11 @@ static authz_status user_check_authorization(request_rec *r, apr_int64_t method_
     w = ap_getword_white(r->pool, &t);
     if (!strcasecmp(w, "user")) {
         /* And note that there are applicable requirements
-        * which we consider ourselves the owner of.
-        */
+         * which we consider ourselves the owner of.
+         */
         while (t[0]) {
             w = ap_getword_conf(r->pool, &t);
-            if (!strcmp(user, w)) {
+            if (!strcmp(r->user, w)) {
                 return AUTHZ_GRANTED;
             }
         }
@@ -143,11 +144,11 @@ static authz_status user_check_authorization(request_rec *r, apr_int64_t method_
 
     ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                   "access to %s failed, reason: user '%s' does not meet "
-                          "'require'ments for user to be allowed access",
-                  r->uri, user);
+                  "'require'ments for user to be allowed access",
+                  r->uri, r->user);
 
     ap_note_auth_failure(r);
-    return AUTHZ_GENERAL_ERROR;
+    return AUTHZ_DENIED;
 }
 
 static authz_status validuser_check_authorization(request_rec *r, apr_int64_t method_mask, const char *require_line)