threads / patch / 46747

v4, 4 partsimap-send: add wrapper to get server credentials if needed

Subject: [PATCH v4 2/4] imap-send: add wrapper to get server credentials if needed

## tl;dr

7 messages between Sep 14, 2017 and Sep 15, 2017. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Nicolas Morey-Chaisemartin· Sep 14, 2017, 07:50 UTC · lore

[PATCH v4 0/4] imap-send: Fix and enable curl by default

Changes since v3:
- Fix return code in patch #1
- Reword patch#4
Nicolas Morey-Chaisemartin (4):
  imap-send: return with error if curl failed
  imap-send: add wrapper to get server credentials if needed
  imap_send: setup_curl: retreive credentials if not set in config file
  imap-send: use curl by default when possible
 imap-send.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 20 deletions(-)
-- 
2.14.1.461.g503560879
Nicolas Morey-Chaisemartin· Sep 14, 2017, 07:52 UTC · re: Nicolas Morey-Chaisemartin · lore
Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
Show changes to imap-send.c +20 −14
diff --git a/imap-send.c b/imap-send.c
index b5e332420a..1b8fbbd545 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)
+{
+	if (srvc->user && srvc->pass)
+		return;
+
+	cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+	cred->host = xstrdup(srvc->host);
+
+	cred->username = xstrdup_or_null(srvc->user);
+	cred->password = xstrdup_or_null(srvc->pass);
+
+	credential_fill(cred);
+
+	if (!srvc->user)
+		srvc->user = xstrdup(cred->username);
+	if (!srvc->pass)
+		srvc->pass = xstrdup(cred->password);
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
 		}
 #endif
 		imap_info("Logging in...\n");
-		if (!srvc->user || !srvc->pass) {
-			cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
-			cred.host = xstrdup(srvc->host);
-
-			cred.username = xstrdup_or_null(srvc->user);
-			cred.password = xstrdup_or_null(srvc->pass);
-
-			credential_fill(&cred);
-
-			if (!srvc->user)
-				srvc->user = xstrdup(cred.username);
-			if (!srvc->pass)
-				srvc->pass = xstrdup(cred.password);
-		}
+		server_fill_credential(srvc, &cred);
 
 		if (srvc->auth_method) {
 			struct imap_cmd_cb cb;

-- 
2.14.1.461.g503560879
Nicolas Morey-Chaisemartin· Sep 14, 2017, 07:52 UTC · re: Nicolas Morey-Chaisemartin · lore

[PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file

Up to this point, the curl mode only supported getting the username and password from the gitconfig file while the legacy mode could also fetch them using the credential API.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
Show changes to imap-send.c +16 −2
diff --git a/imap-send.c b/imap-send.c
index 1b8fbbd545..7e39993d95 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
-static CURL *setup_curl(struct imap_server_conf *srvc)
+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
 	CURL *curl;
 	struct strbuf path = STRBUF_INIT;
@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (!curl)
 		die("curl_easy_init failed");
 
+	server_fill_credential(&server, cred);
 	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
 	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	struct buffer msgbuf = { STRBUF_INIT, 0 };
 	CURL *curl;
 	CURLcode res = CURLE_OK;
+	struct credential cred = CREDENTIAL_INIT;
 
-	curl = setup_curl(server);
+	curl = setup_curl(server, &cred);
 	curl_easy_setopt(curl, CURLOPT_READDATA, &msgbuf);
 
 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
@@ -1496,6 +1498,18 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
 
+	if (cred.username)
+		if (res == CURLE_OK)
+			credential_approve(&cred);
+#if LIBCURL_VERSION_NUM >= 0x070d01
+		else if (res == CURLE_LOGIN_DENIED)
+#else
+		else
+#endif
+			credential_reject(&cred);
+
+	credential_clear(&cred);
+
 	return res != CURLE_OK;
 }
 #endif

-- 
2.14.1.461.g503560879
Junio C Hamano· Sep 15, 2017, 04:44 UTC · re: Nicolas Morey-Chaisemartin · lore

Re: [PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file

Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
Show 5 quoted lines
> +	if (cred.username)
> +		if (res == CURLE_OK)
> +			credential_approve(&cred);
> +#if LIBCURL_VERSION_NUM >= 0x070d01
> +		else if (res == CURLE_LOGIN_DENIED)

A slight tangent. This is in line with the way in which we do conditional compilation to work with different versions of libCurl, but we recently had discussion on modernizing these version based conditional compilation to use feature based one in another topic. We may want to switch to

	#if defined(CURLE_LOGIN_DENIED)
		...

(cf. https://public-inbox.org/git/cover.1502462884.git.tgc@jupiterrise.com/ the entire thread).

No need to change _this_ patch in this series, but something to keep in mind planning for a future follow-up work to clean things up.

Thanks.
Junio C Hamano· Sep 15, 2017, 04:50 UTC · re: Nicolas Morey-Chaisemartin · lore

Re: [PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file

Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
Show 13 quoted lines
>  
> +	if (cred.username)
> +		if (res == CURLE_OK)
> +			credential_approve(&cred);
> +#if LIBCURL_VERSION_NUM >= 0x070d01
> +		else if (res == CURLE_LOGIN_DENIED)
> +#else
> +		else
> +#endif
> +			credential_reject(&cred);
> +
> +	credential_clear(&cred);
> +

As my copy of GCC seemed to be worried about readers getting confused by the if/else cascade, I'd place an extra pair of braces around this, i.e.

	if (cred.username) {
		if (res == CURLE_OK)
			credential_approve(&cred);
		else /* or "else if DENIED" */
			credential_reject(&cred);
	}
	credential_clear(&cred);
while queuing this patch.
Nicolas Morey-Chaisemartin· Sep 14, 2017, 07:52 UTC · re: Nicolas Morey-Chaisemartin · lore

[PATCH v4 4/4] imap-send: use curl by default when possible

Set curl as the runtime default when it is available. When linked against older curl versions (< 7_34_0) or without curl, use the legacy imap implementation.

The goal is to validate feature parity between the legacy and the curl implementation, deprecate the legacy implementation later on and in the long term, hopefully drop it altogether.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to imap-send.c +3 −3
diff --git a/imap-send.c b/imap-send.c
index 7e39993d95..af1e1576bd 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,11 +35,11 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
+#if defined(USE_CURL_FOR_IMAP_SEND)
+/* Always default to curl if it's available. */
 #define USE_CURL_DEFAULT 1
 #else
-/* strictly opt in */
+/* We don't have curl, so continue to use the historical implementation */
 #define USE_CURL_DEFAULT 0
 #endif
 

-- 
2.14.1.461.g503560879
Nicolas Morey-Chaisemartin· Sep 14, 2017, 07:51 UTC · re: Nicolas Morey-Chaisemartin · lore

[PATCH v4 1/4] imap-send: return with error if curl failed

curl_append_msgs_to_imap always returned 0, whether curl failed or not. Return a proper status so git imap-send will exit with an error code if something wrong happened.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to imap-send.c +1 −1
diff --git a/imap-send.c b/imap-send.c
index b2d0b849bb..b5e332420a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
 
-	return 0;
+	return res != CURLE_OK;
 }
 #endif
 

-- 
2.14.1.461.g503560879

← back to recent threads