Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication

From: Aditya Garg <gargaditya08@live.com>
Date: Jun 5, 2025, 17:16 UTC
Message-ID: <PN3PR01MB9597EA16029BEBBF4B966212B86FA@PN3PR01MB9597.INDPRD01.PROD.OUTLOOK.COM>
In-Reply-To: <xmqqy0u687ww.fsf@gitster.g>

On 5 June 2025 10:03:51 pm IST, Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

>Aditya Garg <gargaditya08@live.com> writes:
>
>> +			} else if (!strcmp(srvc->auth_method, "OAUTHBEARER")) {
>> +				if (!CAP(AUTH_OAUTHBEARER)) {
>> +					fprintf(stderr, "You specified "
>> +						"OAUTHBEARER as authentication method, "
>> +						"but %s doesn't support it.\n", srvc->host);
>> +					goto bail;
>> +				}
>> +
>> +				#ifdef NO_OPENSSL
>> +				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
>> +					"with OpenSSL library, but its support has not been compiled in.");
>> +				goto bail;
>> +				#endif
>
>Ugly.  Can we avoid #ifdef/#endif in the middle of such a main flow
>of the logic?  Hiding such ugliness by indenting the #ifdef/#endif
>directives as if they are just one of the code lines is doubly ugly.
>

RESENDING AS PLAIN TEXT

Your suggestion in a previous review said:

           if (!auth_oauthbearer) {
               ... we do not support ...
               goto bail;
           }

Might look less ugly, but will result in a compiler warning that this will always be true if compiled with NO_OPENSSL. If you are fine with that, good. Else tbh I am out of ideas :(.

Show 17 quoted lines

>>  	server_fill_credential(srvc, cred);
>>  	curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>> -	curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>> +
>> +	if (!srvc->auth_method ||
>> +	    (strcmp(srvc->auth_method, "XOAUTH2") &&
>> +	    strcmp(srvc->auth_method, "OAUTHBEARER")))
>> +		curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>
>Can we clarify this part, possibly with an in-code comment?
>
>"Unless XOAUTH2 or OAUTHBEARER, use the password" sounds a bit
>strange.  What about methods other than these two that are not a
>plain simple password authentication?  Will we remember extending
>this code when we add yet another one to exclude it like XOAUTH2 and
>OAUTHBEARER are excluded with this patch?
>

Let me answer this first. CURLOPT_PASSWORD is for plain or login type authentication, and if srvc->auth_method is not defined, curl's behaviour defaults to them. OAUTHBEARER and XOAUTH2 use CURLOPT_XOAUTH2_BEARER in curl, which can use either of them based on what server says. Other auth methods are not supported yet in this code, and this is the reason CRAM_MD5 is supported by only OpenSSL.