On the user-defined-authn branch: In the interaction diagram, correct the

position in the handshake sequence where pipelining is optionally turned
off on a connection. Add the option to reset pipelining on two more
callbacks; current internal schemes sometimes restore it during the 'handle'
phase, and sometimes (notably spnego) during the response validation phase.
This makes the API for user-defined implementations more flexible at the
cost of having to deal with far too long argument lists. Ain't no fun if
it's less than six parameters, right?

* serf.h: Update the authentication interaction diagram.
  (serf_authn_handle_func_t): Add the reset_pipelining argument and update
   the docstring to hopefully make it clear what that argument means.
  (serf_authn_setup_request_func_t): Add the reset_pipelining argument.
  (serf_authn_validate_response_func_t): Update the docstring.

* auth/auth_user_defined.c
  (struct authn_baton_wrapper::pipelining_was_reset): Rename from the shorter
   but more ambiguous pipelining_reset.
  (validate_handler): Add docstring.
  (maybe_reset_pipelining): New. Now common code extracted from one of
   the callbacks.
  (serf__authn_user__handle,
   serf__authn_user__setup_request): Implement the reset_pipelining argument.
  (serf__authn_user__validate_response): Call maybe_reset_pipelining.

* test/test_auth.c
  (user_authn_handle,
   user_authn_setup_request): Implement the reset_pipelining argument.


git-svn-id: https://svn.apache.org/repos/asf/serf/branches/user-defined-authn@1926858 13f79535-47bb-0310-9956-ffa450edef68

Author: brainy

auth/auth_user_defined.c

@@ -53,13 +53,14 @@ struct authn_baton_wrapper {
     int pipelining;
 
     /* Was pipelining reset to its previous value?. */
-    bool pipelining_reset;
+    bool pipelining_was_reset;
 
     /* The user-defined scheme's per-connection baton. */
     void *user_authn_baton;
 };
 
 
+/* Common callback parameter validation. */
 typedef apr_status_t (*callback_fn_t)();
 static apr_status_t validate_handler(serf_config_t *config,
                                      const serf__authn_scheme_t *scheme,
@@ -89,6 +90,34 @@ static apr_status_t validate_handler(serf_config_t *config,
 }
 
 
+/* Reset request pipelining on the connection if required. */
+static void maybe_reset_pipelining(const serf__authn_scheme_t *scheme,
+                                   struct authn_baton_wrapper *authn_baton,
+                                   serf_connection_t *conn,
+                                   apr_status_t status,
+                                   int reset_pipelining)
+{
+    if (!reset_pipelining
+        || status != APR_SUCCESS
+        || authn_baton->pipelining_was_reset
+        || (scheme->user_flags & SERF_AUTHN_FLAG_PIPE))
+    {
+        serf__log(LOGLVL_DEBUG, LOGCOMP_AUTHN, __FILE__, conn->config,
+                  "User-defined scheme %s: no pipelining change for %s\n",
+                  scheme->name, conn->host_url);
+        return;
+    }
+
+    serf__log(LOGLVL_DEBUG, LOGCOMP_AUTHN, __FILE__, conn->config,
+              "User-defined scheme %s: pipelining reset to %s for %s\n",
+              scheme->name,
+              authn_baton->pipelining ? "on" : "off",
+              conn->host_url);
+    serf__connection_set_pipelining(conn, authn_baton->pipelining);
+    authn_baton->pipelining_was_reset = true;
+}
+
+
 apr_status_t
 serf__authn_user__init_conn(const serf__authn_scheme_t *scheme,
                             int code,
@@ -135,9 +164,9 @@ serf__authn_user__init_conn(const serf__authn_scheme_t *scheme,
                   "User-defined scheme %s: pipelining off for %s\n",
                   scheme->name, conn->host_url);
         authn_baton->pipelining = serf__connection_set_pipelining(conn, 0);
-        authn_baton->pipelining_reset = false;
+        authn_baton->pipelining_was_reset = false;
     } else {
-        authn_baton->pipelining_reset = true;
+        authn_baton->pipelining_was_reset = true;
     }
     return status;
 }
@@ -156,6 +185,7 @@ serf__authn_user__handle(const serf__authn_scheme_t *scheme,
     serf__authn_info_t *const authn_info = get_authn_info(code, conn);
     serf_context_t *const ctx = conn->ctx;
     struct authn_baton_wrapper *authn_baton;
+    int reset_pipelining = 0;
     char *username, *password;
     apr_pool_t *scratch_pool;
     apr_hash_t *auth_param;
@@ -222,7 +252,8 @@ serf__authn_user__handle(const serf__authn_scheme_t *scheme,
         username = password = NULL;
     }
 
-    status = scheme->user_handle_func(scheme->user_baton,
+    status = scheme->user_handle_func(&reset_pipelining,
+                                      scheme->user_baton,
                                       authn_baton->user_authn_baton, code,
                                       auth_hdr, auth_param,
                                       SERF__HEADER_FROM_CODE(code),
@@ -231,6 +262,7 @@ serf__authn_user__handle(const serf__authn_scheme_t *scheme,
                                       pool, scratch_pool);
 
   cleanup:
+    maybe_reset_pipelining(scheme, authn_baton, conn, status, reset_pipelining);
     apr_pool_destroy(scratch_pool);
     return status;
 }
@@ -249,6 +281,7 @@ serf__authn_user__setup_request(const serf__authn_scheme_t *scheme,
     const int peer_id = SERF__CODE_FROM_PEER(peer);
     serf__authn_info_t *const authn_info = get_authn_info(peer_id, conn);
     struct authn_baton_wrapper *authn_baton;
+    int reset_pipelining = 0;
     apr_pool_t *scratch_pool;
     apr_status_t status;
 
@@ -271,11 +304,14 @@ serf__authn_user__setup_request(const serf__authn_scheme_t *scheme,
 
     authn_baton = authn_info->baton;
     apr_pool_create(&scratch_pool, conn->pool);
-    status = scheme->user_setup_request_func(scheme->user_baton,
+    status = scheme->user_setup_request_func(&reset_pipelining,
+                                             scheme->user_baton,
                                              authn_baton->user_authn_baton,
                                              conn, request,
                                              method, uri, hdrs_bkt,
                                              scratch_pool);
+
+    maybe_reset_pipelining(scheme, authn_baton, conn, status, reset_pipelining);
     apr_pool_destroy(scratch_pool);
     return status;
 }
@@ -334,19 +370,7 @@ serf__authn_user__validate_response(const serf__authn_scheme_t *scheme,
                                                  request, response,
                                                  scratch_pool);
 
-    /* Reset pipelining if the scheme requires it. */
-    if (status == APR_SUCCESS && reset_pipelining
-        && !authn_baton->pipelining_reset
-        && !(scheme->user_flags & SERF_AUTHN_FLAG_PIPE)) {
-        serf__log(LOGLVL_DEBUG, LOGCOMP_AUTHN, __FILE__, conn->config,
-                  "User-defined scheme %s: pipelining reset to %s for %s\n",
-                  scheme->name,
-                  authn_baton->pipelining ? "on" : "off",
-                  conn->host_url);
-        serf__connection_set_pipelining(conn, authn_baton->pipelining);
-        authn_baton->pipelining_reset = true;
-    }
-
+    maybe_reset_pipelining(scheme, authn_baton, conn, status, reset_pipelining);
     apr_pool_destroy(scratch_pool);
     return status;
 }

serf.h

@@ -962,19 +962,22 @@ serf_bucket_t *serf_request_bucket_request_create(
  *    |                         +<---- authenticate <----+ (401 or 407)
  *    +<------ init-conn <------+                        |
  *    |                         |                        |
+ *    |     (pipelining off) <--+ (optional)             |
  *    |    (get credentials) <--+ (optional)             |
  *    |                         |                        |
  *    +<-------- handle <-------+                        |
  *    |                         |                        |
- *    |     (pipelining off) <--+ (optional)             |
+ *    |   (reset pipelining) <--+ (optional)             |
  *    |                         |                        |
  *    +<---- setup-requiest <---+                        |
+ *    |                         |                        |
+ *    |   (reset pipelining) <--+ (optional)             |
  *    |                         +---> request + authn -->+
  *    |                         |                        |
  *    |                         +<------ response <------+
  *    +<-- validate-response <--+                        |
  *    |                         |                        |
- *    |      (pipelining on) <--+ (optional)             |
+ *    |   (reset pipelining) <--+ (optional)             |
  *    |                         |                        |
  * ```
  */
@@ -1077,12 +1080,22 @@ typedef apr_status_t
  * @a request is the pending request and @a response is the response that
  * caused this callback to be called.
  *
+ * If the scheme flag @c SERF_AUTHN_FLAG_PIPE is *not* set, return a boolean
+ * value in @a reset_pipelining to indicate whether pipelining on @a conn
+ * should be restored to the value before the init-conn callback was invoked.
+ * The value of this flag is set to @c false by the caller, so the
+ * implementation does not have to modify it if the pipelining state should
+ * remain unchanged. This parameter has the same meaning in the
+ * serf_authn_setup_request_func_t and serf_authn_validate_response_func_t
+ * callbacks and will only take effect the first time its value @c true.
+ *
  * Use @a scratch_pool for temporary allocations.
  *
  * @since New in 1.4.
  */
 typedef apr_status_t
-(*serf_authn_handle_func_t)(void *baton,
+(*serf_authn_handle_func_t)(int *reset_pipelining,
+                            void *baton,
                             void *authn_baton,
                             int code,
                             const char *authn_header,
@@ -1106,12 +1119,15 @@ typedef apr_status_t
  * @a method and @a uri are the requests attributes and @a headers are the
  * request headers where the credentials are usually set.
  *
+ * For the meaning of @a reset_pipelining, see serf_authn_handle_func_t.
+ *
  * Use @a scratch_pool for temporary allocations.
  *
  * @since New in 1.4.
  */
 typedef apr_status_t
-(*serf_authn_setup_request_func_t)(void *baton,
+(*serf_authn_setup_request_func_t)(int *reset_pipelining,
+                                   void *baton,
                                    void *authn_baton,
                                    serf_connection_t *conn,
                                    serf_request_t *request,
@@ -1134,9 +1150,7 @@ typedef apr_status_t
  * response header. This argument will be NULL @a response does  not contain
  * one of those headers.
  *
- * If the scheme flag @c SERF_AUTHN_FLAG_PIPE is *not* set, return a boolean
- * value in @a reset_pipelining to indicate whether pipelining on @a conn should
- * be restored to the value before the init-conn callback was invoked.
+ * For the meaning of @a reset_pipelining, see serf_authn_handle_func_t.
  *
  * Use @a scratch_pool for temporary allocations.
  *

test/test_auth.c

@@ -733,7 +733,8 @@ static apr_status_t user_authn_get_realm(const char **realm_name,
     return APR_SUCCESS;
 }
 
-static apr_status_t user_authn_handle(void *baton,
+static apr_status_t user_authn_handle(int *reset_pipelining,
+                                      void *baton,
                                       void *authn_baton,
                                       int code,
                                       const char *authn_header,
@@ -753,10 +754,12 @@ static apr_status_t user_authn_handle(void *baton,
     ab->header = apr_pstrdup(result_pool, response_header);
     ab->value = apr_pstrcat(result_pool, b->name, " ", password, NULL);
 
+    *reset_pipelining = 1;
     return APR_SUCCESS;
 }
 
-static apr_status_t user_authn_setup_request(void *baton,
+static apr_status_t user_authn_setup_request(int *reset_pipelining,
+                                             void *baton,
                                              void *authn_baton,
                                              serf_connection_t *conn,
                                              serf_request_t *request,
@@ -777,6 +780,8 @@ static apr_status_t user_authn_setup_request(void *baton,
               ab->header, ab->value);
 
     serf_bucket_headers_setn(headers, ab->header, ab->value);
+
+    *reset_pipelining = 1;
     return APR_SUCCESS;
 }