From 1bbcc224ccfdccd99b1221d83150a8ffb8059ffd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:23 -0400 Subject: http: refactor options to http_get_* Over time, the http_get_strbuf function has grown several optional parameters. We now have a bitfield with multiple boolean options, as well as an optional strbuf for returning the content-type of the response. And a future patch in this series is going to add another strbuf option. Treating these as separate arguments has a few downsides: 1. Most call sites need to add extra NULLs and 0s for the options they aren't interested in. 2. The http_get_* functions are actually wrappers around 2 layers of low-level implementation functions. We have to pass these options through individually. 3. The http_get_strbuf wrapper learned these options, but nobody bothered to do so for http_get_file, even though it is backed by the same function that does understand the options. Let's consolidate the options into a single struct. For the common case of the default options, we'll allow callers to simply pass a NULL for the options struct. The resulting code is often a few lines longer, but it ends up being easier to read (and to change as we add new options, since we do not need to update each call site). Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- http.h | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'http.h') diff --git a/http.h b/http.h index d77c1b54f2..40404b4836 100644 --- a/http.h +++ b/http.h @@ -125,11 +125,16 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url, extern char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix); -/* Options for http_request_*() */ -#define HTTP_NO_CACHE 1 -#define HTTP_KEEP_ERROR 2 +/* Options for http_get_*() */ +struct http_get_options { + unsigned no_cache:1, + keep_error:1; -/* Return values for http_request_*() */ + /* If non-NULL, returns the content-type of the response. */ + struct strbuf *content_type; +}; + +/* Return values for http_get_*() */ #define HTTP_OK 0 #define HTTP_MISSING_TARGET 1 #define HTTP_ERROR 2 @@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex, * * If the result pointer is NULL, a HTTP HEAD request is made instead of GET. */ -int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options); +int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options); extern int http_fetch_ref(const char *base, struct ref *ref); -- cgit v1.2.1 From 2501aff8b7516115c409cb34cc50305cdde40a47 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:31:45 -0400 Subject: http: hoist credential request out of handle_curl_result When we are handling a curl response code in http_request or in the remote-curl RPC code, we use the handle_curl_result helper to translate curl's response into an easy-to-use code. When we see an HTTP 401, we do one of two things: 1. If we already had a filled-in credential, we mark it as rejected, and then return HTTP_NOAUTH to indicate to the caller that we failed. 2. If we didn't, then we ask for a new credential and tell the caller HTTP_REAUTH to indicate that they may want to try again. Rejecting in the first case makes sense; it is the natural result of the request we just made. However, prompting for more credentials in the second step does not always make sense. We do not know for sure that the caller is going to make a second request, and nor are we sure that it will be to the same URL. Logically, the prompt belongs not to the request we just finished, but to the request we are (maybe) about to make. In practice, it is very hard to trigger any bad behavior. Currently, if we make a second request, it will always be to the same URL (even in the face of redirects, because curl handles the redirects internally). And we almost always retry on HTTP_REAUTH these days. The one exception is if we are streaming a large RPC request to the server (e.g., a pushed packfile), in which case we cannot restart. It's extremely unlikely to see a 401 response at this stage, though, as we would typically have seen it when we sent a probe request, before streaming the data. This patch drops the automatic prompt out of case 2, and instead requires the caller to do it. This is a few extra lines of code, and the bug it fixes is unlikely to come up in practice. But it is conceptually cleaner, and paves the way for better handling of credentials across redirects. Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- http.h | 1 + 1 file changed, 1 insertion(+) (limited to 'http.h') diff --git a/http.h b/http.h index 40404b4836..17116abc57 100644 --- a/http.h +++ b/http.h @@ -102,6 +102,7 @@ extern void http_cleanup(void); extern int active_requests; extern int http_is_verbose; extern size_t http_post_buffer; +extern struct credential http_auth; extern char curl_errorstr[CURL_ERROR_SIZE]; -- cgit v1.2.1 From 78868962c03e5bdddad5c5d02c1d5a0c72a7ac26 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:32:02 -0400 Subject: http: provide effective url to callers When we ask curl to access a URL, it may follow one or more redirects to reach the final location. We have no idea this has happened, as curl takes care of the details and simply returns the final content to us. The final URL that we ended up with can be accessed via CURLINFO_EFFECTIVE_URL. Let's make that optionally available to callers of http_get_*, so that they can make further decisions based on the redirection. Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- http.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'http.h') diff --git a/http.h b/http.h index 17116abc57..974ede7736 100644 --- a/http.h +++ b/http.h @@ -133,6 +133,12 @@ struct http_get_options { /* If non-NULL, returns the content-type of the response. */ struct strbuf *content_type; + + /* + * If non-NULL, returns the URL we ended up at, including any + * redirects we followed. + */ + struct strbuf *effective_url; }; /* Return values for http_get_*() */ -- cgit v1.2.1 From c93c92f30977adb2eb385a851f9f5e9975da7d5e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 28 Sep 2013 04:34:05 -0400 Subject: http: update base URLs when we see redirects If a caller asks the http_get_* functions to go to a particular URL and we end up elsewhere due to a redirect, the effective_url field can tell us where we went. It would be nice to remember this redirect and short-cut further requests for two reasons: 1. It's more efficient. Otherwise we spend an extra http round-trip to the server for each subsequent request, just to get redirected. 2. If we end up with an http 401 and are going to ask for credentials, it is to feed them to the redirect target. If the redirect is an http->https upgrade, this means our credentials may be provided on the http leg, just to end up redirected to https. And if the redirect crosses server boundaries, then curl will drop the credentials entirely as it follows the redirect. However, it, it is not enough to simply record the effective URL we saw and use that for subsequent requests. We were originally fed a "base" url like: http://example.com/foo.git and we want to figure out what the new base is, even though the URLs we see may be: original: http://example.com/foo.git/info/refs effective: http://example.com/bar.git/info/refs Subsequent requests will not be for "info/refs", but for other paths relative to the base. We must ask the caller to pass in the original base, and we must pass the redirected base back to the caller (so that it can generate more URLs from it). Furthermore, we need to feed the new base to the credential code, so that requests to credential helpers (or to the user) match the URL we will be requesting. This patch teaches http_request_reauth to do this munging. Since it is the caller who cares about making more URLs, it seems at first glance that callers could simply check effective_url themselves and handle it. However, since we need to update the credential struct before the second re-auth request, we have to do it inside http_request_reauth. Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- http.h | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'http.h') diff --git a/http.h b/http.h index 974ede7736..12ca5c892d 100644 --- a/http.h +++ b/http.h @@ -139,6 +139,14 @@ struct http_get_options { * redirects we followed. */ struct strbuf *effective_url; + + /* + * If both base_url and effective_url are non-NULL, the base URL will + * be munged to reflect any redirections going from the requested url + * to effective_url. See the definition of update_url_from_redirect + * for details. + */ + struct strbuf *base_url; }; /* Return values for http_get_*() */ -- cgit v1.2.1 From 0972ccd97cc8f913ea828a1e03ef3652fc1ff514 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 Oct 2013 02:35:31 -0400 Subject: http: return curl's AUTHAVAIL via slot_results Callers of the http code may want to know which auth types were available for the previous request. But after finishing with the curl slot, they are not supposed to look at the curl handle again. We already handle returning other information via the slot_results struct; let's add a flag to check the available auth. Note that older versions of curl did not support this, so we simply return 0 (something like "-1" would be worse, as the value is a bitflag and we might accidentally set a flag). This is sufficient for the callers planned in this series, who only trigger some optional behavior if particular bits are set, and can live with a fake "no bits" answer. Signed-off-by: Jeff King --- http.h | 1 + 1 file changed, 1 insertion(+) (limited to 'http.h') diff --git a/http.h b/http.h index d77c1b54f2..81d4843293 100644 --- a/http.h +++ b/http.h @@ -54,6 +54,7 @@ struct slot_results { CURLcode curl_result; long http_code; + long auth_avail; }; struct active_request_slot { -- cgit v1.2.1 From 01689909eb9f8366583c44b325d8d9ba259a2538 Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Mon, 31 Mar 2014 15:11:46 -0700 Subject: comments: fix misuses of "nor" Signed-off-by: Justin Lebar Signed-off-by: Junio C Hamano --- http.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'http.h') diff --git a/http.h b/http.h index cd37d5881c..a766066dfd 100644 --- a/http.h +++ b/http.h @@ -13,8 +13,7 @@ /* * We detect based on the cURL version if multi-transfer is * usable in this implementation and define this symbol accordingly. - * This is not something Makefile should set nor users should pass - * via CFLAGS. + * This shouldn't be set by the Makefile or by the user (e.g. via CFLAGS). */ #undef USE_CURL_MULTI -- cgit v1.2.1