threads / patch / 37229

patchimap-send: clarify CRAM-MD5 vs LOGIN documentation

Subject: [PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

## tl;dr

12 messages between Jul 28, 2014 and Aug 3, 2014. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Tony Finch· Jul 28, 2014, 10:38 UTC · lore
Explicitly mention that leaving imap.authMethod unset makes
git imap-send use the basic IMAP plaintext LOGIN command.
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/git-imap-send.txt +2 −1
diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.0.3.dirty
Junio C Hamano· Jul 30, 2014, 19:28 UTC · re: Tony Finch · lore

Re: [PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

Tony Finch <dot@dotat.at> writes:
Show 20 quoted lines
> Explicitly mention that leaving imap.authMethod unset makes
> git imap-send use the basic IMAP plaintext LOGIN command.
> ---
>  Documentation/git-imap-send.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 875d283..770cbe8 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -76,7 +76,8 @@ imap.preformattedHTML::
>
>  imap.authMethod::
>  	Specify authenticate method for authentication with IMAP server.
> -	Current supported method is 'CRAM-MD5' only.
> +	Current supported method is 'CRAM-MD5' only. If this is not set
> +	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
>
>  Examples
>  ~~~~~~~~

Both patches make sense to me, but can you please sign-off your patches? See Documentation/SubmittingPatches for details.

Thanks.
Tony Finch· Jul 31, 2014, 08:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

Junio C Hamano <gitster@pobox.com> wrote:
>
> Both patches make sense to me, but can you please sign-off your
> patches?
Oops, sorry about that. Re-roll on its way...
Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
Thames, Dover: Southwest 4 or 5, increasing 6 at times. Slight or moderate.
Fair. Good.
Tony Finch· Jul 31, 2014, 08:14 UTC · re: Tony Finch · lore

[PATCH 1/2] imap-send: clarify CRAM-MD5 vs LOGIN documentation

Explicitly mention that leaving imap.authMethod unset makes git imap-send use the basic IMAP plaintext LOGIN command.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/git-imap-send.txt +2 −1
diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.1.0.rc0.229.gaee38de
Tony Finch· Jul 31, 2014, 08:14 UTC · re: Tony Finch · lore

[PATCH 2/2] imap-send: create target mailbox if it is missing

Some MUAs delete their "drafts" folder when it is empty, so git imap-send should be able to create it if necessary.

This change checks that the folder exists immediately after login and tries to create it if it is missing.

There was some vestigial code to handle a [TRYCREATE] response from the server when an APPEND target is missing. However this code never ran (the create and trycreate flags were never set) and when I tried to make it run I found that the code had already thrown away the contents of the message it was trying to append.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 imap-send.c | 56 +++++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)
Show changes to imap-send.c +25 −31
diff --git a/imap-send.c b/imap-send.c
index 524fbab..5e4a24e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -128,7 +128,6 @@ struct imap_cmd_cb {
 	char *data;
 	int dlen;
 	int uid;
-	unsigned create:1, trycreate:1;
 };

 struct imap_cmd {
@@ -714,8 +713,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 {
 	struct imap *imap = ctx->imap;
-	struct imap_cmd *cmdp, **pcmdp, *ncmdp;
-	char *cmd, *arg, *arg1, *p;
+	struct imap_cmd *cmdp, **pcmdp;
+	char *cmd, *arg, *arg1;
 	int n, resp, resp2, tag;

 	for (;;) {
@@ -801,30 +800,9 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (!strcmp("OK", arg))
 				resp = DRV_OK;
 			else {
-				if (!strcmp("NO", arg)) {
-					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
-						p = strchr(cmdp->cmd, '"');
-						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						/* not waiting here violates the spec, but a server that does not
-						   grok this nonetheless violates it too. */
-						cmdp->cb.create = 0;
-						if (!(ncmdp = issue_imap_cmd(ctx, &cmdp->cb, "%s", cmdp->cmd))) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						free(cmdp->cmd);
-						free(cmdp);
-						if (!tcmd)
-							return 0;	/* ignored */
-						if (cmdp == tcmd)
-							tcmd = ncmdp;
-						continue;
-					}
+				if (!strcmp("NO", arg))
 					resp = RESP_NO;
-				} else /*if (!strcmp("BAD", arg))*/
+				else /*if (!strcmp("BAD", arg))*/
 					resp = RESP_BAD;
 				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
 					 memcmp(cmdp->cmd, "LOGIN", 5) ?
@@ -833,7 +811,6 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 			if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
 				resp = resp2;
-		normal:
 			if (cmdp->cb.done)
 				cmdp->cb.done(ctx, cmdp, resp);
 			free(cmdp->cb.data);
@@ -944,7 +921,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }

-static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
 	struct imap_store *ctx;
@@ -1156,6 +1133,25 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
 		credential_approve(&cred);
 	credential_clear(&cred);

+	/* check the target mailbox exists */
+	ctx->name = folder;
+	switch (imap_exec(ctx, NULL, "EXAMINE \"%s\"", ctx->name)) {
+	case RESP_OK:
+		/* ok */
+		break;
+	case RESP_BAD:
+		fprintf(stderr, "IMAP error: could not check mailbox\n");
+		goto bail;
+	case RESP_NO:
+		if (imap_exec(ctx, NULL, "CREATE \"%s\"", ctx->name) == RESP_OK) {
+			imap_info("Created missing mailbox\n");
+		} else {
+			fprintf(stderr, "IMAP error: could not create missing mailbox\n");
+			goto bail;
+		}
+		break;
+	}
+
 	ctx->prefix = "";
 	return ctx;

@@ -1219,7 +1215,6 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)

 	box = ctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
@@ -1418,14 +1413,13 @@ int main(int argc, char **argv)
 	}

 	/* write it to the imap server */
-	ctx = imap_open_store(&server);
+	ctx = imap_open_store(&server, imap_folder);
 	if (!ctx) {
 		fprintf(stderr, "failed to open store\n");
 		return 1;
 	}

 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;
-- 
2.1.0.rc0.229.gaee38de
Junio C Hamano· Jul 31, 2014, 18:57 UTC · re: Tony Finch · lore

Re: [PATCH 2/2] imap-send: create target mailbox if it is missing

Tony Finch <dot@dotat.at> writes:
Show 14 quoted lines
> Some MUAs delete their "drafts" folder when it is empty, so
> git imap-send should be able to create it if necessary.
>
> This change checks that the folder exists immediately after
> login and tries to create it if it is missing.
>
> There was some vestigial code to handle a [TRYCREATE] response
> from the server when an APPEND target is missing. However this
> code never ran (the create and trycreate flags were never set)
> and when I tried to make it run I found that the code had already
> thrown away the contents of the message it was trying to append.
>
> Signed-off-by: Tony Finch <dot@dotat.at>
> ---
The basic idea looks good, but I have doubts on one point.
Show 26 quoted lines
> diff --git a/imap-send.c b/imap-send.c
> index 524fbab..5e4a24e 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1156,6 +1133,25 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
>  		credential_approve(&cred);
>  	credential_clear(&cred);
>
> +	/* check the target mailbox exists */
> +	ctx->name = folder;
> +	switch (imap_exec(ctx, NULL, "EXAMINE \"%s\"", ctx->name)) {
> +	case RESP_OK:
> +		/* ok */
> +		break;
> +	case RESP_BAD:
> +		fprintf(stderr, "IMAP error: could not check mailbox\n");
> +		goto bail;
> +	case RESP_NO:
> +		if (imap_exec(ctx, NULL, "CREATE \"%s\"", ctx->name) == RESP_OK) {
> +			imap_info("Created missing mailbox\n");
> +		} else {
> +			fprintf(stderr, "IMAP error: could not create missing mailbox\n");
> +			goto bail;
> +		}
> +		break;
> +	}

At any and all the existing places that "goto bail" in the function, we know we failed to authenticate. I think they are all sensible places to call credential_reject().

On the other hand, at this point before you try to "check the target mailbox exists", we have authenticated sucessfully, we know the credential used was good, and called credential_approve() to mark it as such. I do agree that you would want to signal an error to the caller upon these two failures, but I do not think you want to "goto bail" and reject the credential. The error you observed in the new codepath is caused by something else, not authentication failure, and in such a case you do not want to cause the credential helper to evict the user/pass pair from the keyring, no?

Thanks.
Tony Finch· Jul 31, 2014, 19:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] imap-send: create target mailbox if it is missing

Junio C Hamano <gitster@pobox.com> wrote:
>
> The basic idea looks good, but I have doubts on one point.

Thanks for spotting the mistake in the error handling. I'll send an update with a fix.

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
South Utsire: Southwesterly 4 or 5, occasionally 6 at first in south, backing
southeasterly later. Moderate. Thundery showers. Good.
Tony Finch· Aug 1, 2014, 08:15 UTC · re: Tony Finch · lore

[PATCH v3 1/2] imap-send: clarify CRAM-MD5 vs LOGIN documentation

Explicitly mention that leaving imap.authMethod unset makes git imap-send use the basic IMAP plaintext LOGIN command.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to Documentation/git-imap-send.txt +2 −1
diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.1.0.rc0.60.gb5b18e5
Tony Finch· Aug 1, 2014, 08:15 UTC · re: Tony Finch · lore

[PATCH v3 2/2] imap-send: create target mailbox if it is missing

Some MUAs delete their "drafts" folder when it is empty, so git imap-send should be able to create it if necessary.

This change checks that the folder exists immediately after login and tries to create it if it is missing.

There was some vestigial code to handle a [TRYCREATE] response from the server when an APPEND target is missing. However this code never ran (the create and trycreate flags were never set) and when I tried to make it run I found that the code had already thrown away the contents of the message it was trying to append.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 imap-send.c | 57 ++++++++++++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 31 deletions(-)
Show changes to imap-send.c +26 −31
diff --git a/imap-send.c b/imap-send.c
index 524fbab..a29d03e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -128,7 +128,6 @@ struct imap_cmd_cb {
 	char *data;
 	int dlen;
 	int uid;
-	unsigned create:1, trycreate:1;
 };

 struct imap_cmd {
@@ -714,8 +713,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 {
 	struct imap *imap = ctx->imap;
-	struct imap_cmd *cmdp, **pcmdp, *ncmdp;
-	char *cmd, *arg, *arg1, *p;
+	struct imap_cmd *cmdp, **pcmdp;
+	char *cmd, *arg, *arg1;
 	int n, resp, resp2, tag;

 	for (;;) {
@@ -801,30 +800,9 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (!strcmp("OK", arg))
 				resp = DRV_OK;
 			else {
-				if (!strcmp("NO", arg)) {
-					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
-						p = strchr(cmdp->cmd, '"');
-						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						/* not waiting here violates the spec, but a server that does not
-						   grok this nonetheless violates it too. */
-						cmdp->cb.create = 0;
-						if (!(ncmdp = issue_imap_cmd(ctx, &cmdp->cb, "%s", cmdp->cmd))) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						free(cmdp->cmd);
-						free(cmdp);
-						if (!tcmd)
-							return 0;	/* ignored */
-						if (cmdp == tcmd)
-							tcmd = ncmdp;
-						continue;
-					}
+				if (!strcmp("NO", arg))
 					resp = RESP_NO;
-				} else /*if (!strcmp("BAD", arg))*/
+				else /*if (!strcmp("BAD", arg))*/
 					resp = RESP_BAD;
 				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
 					 memcmp(cmdp->cmd, "LOGIN", 5) ?
@@ -833,7 +811,6 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 			if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
 				resp = resp2;
-		normal:
 			if (cmdp->cb.done)
 				cmdp->cb.done(ctx, cmdp, resp);
 			free(cmdp->cb.data);
@@ -944,7 +921,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }

-static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
 	struct imap_store *ctx;
@@ -1156,6 +1133,25 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
 		credential_approve(&cred);
 	credential_clear(&cred);

+	/* check the target mailbox exists */
+	ctx->name = folder;
+	switch (imap_exec(ctx, NULL, "EXAMINE \"%s\"", ctx->name)) {
+	case RESP_OK:
+		/* ok */
+		break;
+	case RESP_BAD:
+		fprintf(stderr, "IMAP error: could not check mailbox\n");
+		goto out;
+	case RESP_NO:
+		if (imap_exec(ctx, NULL, "CREATE \"%s\"", ctx->name) == RESP_OK) {
+			imap_info("Created missing mailbox\n");
+		} else {
+			fprintf(stderr, "IMAP error: could not create missing mailbox\n");
+			goto out;
+		}
+		break;
+	}
+
 	ctx->prefix = "";
 	return ctx;

@@ -1164,6 +1160,7 @@ bail:
 		credential_reject(&cred);
 	credential_clear(&cred);

+ out:
 	imap_close_store(ctx);
 	return NULL;
 }
@@ -1219,7 +1216,6 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)

 	box = ctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
@@ -1418,14 +1414,13 @@ int main(int argc, char **argv)
 	}

 	/* write it to the imap server */
-	ctx = imap_open_store(&server);
+	ctx = imap_open_store(&server, imap_folder);
 	if (!ctx) {
 		fprintf(stderr, "failed to open store\n");
 		return 1;
 	}

 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;
-- 
2.1.0.rc0.60.gb5b18e5
Junio C Hamano· Aug 1, 2014, 22:30 UTC · re: Tony Finch · lore

Re: [PATCH v3 2/2] imap-send: create target mailbox if it is missing

Tony Finch <dot@dotat.at> writes:
> Some MUAs delete their "drafts" folder when it is empty, so
> git imap-send should be able to create it if necessary.
Thanks for an update; queued.
Ramsay Jones· Aug 2, 2014, 13:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/2] imap-send: create target mailbox if it is missing

On 01/08/14 23:30, Junio C Hamano wrote:
Show 6 quoted lines
> Tony Finch <dot@dotat.at> writes:
> 
>> Some MUAs delete their "drafts" folder when it is empty, so
>> git imap-send should be able to create it if necessary.
> 
> Thanks for an update; queued.
BTW, this provokes a warning from gcc (v4.8.2) for me:
        CC imap-send.o
    imap-send.c:561:25: warning: ‘issue_imap_cmd’ defined but not used [-Wunused-function]
     static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
                             ^
It seems the change to get_cmd_result(), in commit 7ebac713, removes
the only remaining (two) calls to issue_imap_cmd().
HTH

ATB, Ramsay Jones

Junio C Hamano· Aug 3, 2014, 06:38 UTC · re: Ramsay Jones · lore

Re: [PATCH v3 2/2] imap-send: create target mailbox if it is missing

Yup. I think I also queued a fix-up on top. Thanks for reporting.

On Sat, Aug 2, 2014 at 6:44 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:
Show 24 quoted lines
> On 01/08/14 23:30, Junio C Hamano wrote:
>> Tony Finch <dot@dotat.at> writes:
>>
>>> Some MUAs delete their "drafts" folder when it is empty, so
>>> git imap-send should be able to create it if necessary.
>>
>> Thanks for an update; queued.
>
> BTW, this provokes a warning from gcc (v4.8.2) for me:
>
>         CC imap-send.o
>     imap-send.c:561:25: warning: ‘issue_imap_cmd’ defined but not used [-Wunused-function]
>      static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
>                              ^
> It seems the change to get_cmd_result(), in commit 7ebac713, removes
> the only remaining (two) calls to issue_imap_cmd().
>
> HTH
>
> ATB,
> Ramsay Jones
>
>
>

← back to recent threads