> On 6 Jun 2025, at 8:58 AM, Aditya Garg <gargaditya08@xxxxxxxx> wrote:
>
>
>
>> On 6 June 2025 12:18:25 am IST, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Aditya Garg <gargaditya08@xxxxxxxx> writes:
>>
>>> 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 :(.
>>
>> Sounds like a good place to use NOT_CONSTANT(), it seems?
>>
>> if (NOT_CONSTANT(!auth_oauthbearer)) {
>> ... skip the thing ...
>> }
>>
>>
>
> Ok
>
>>>>> 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.
>>
>> Which makes it sound like if (!srvc->auth_method) is enough?
>>
>
> No. If the user specifies PLAIN or LOGIN then it's not enough.
>
>>> OAUTHBEARER and XOAUTH2 use CURLOPT_XOAUTH2_BEARER
>>> in curl, which can use either of them based on what server says.
>>
>> That is what we can read from the updated code.
>>
>> The question is what happens when the user sets srvc->auth_method to
>> something other than NULL (unused---use plain password), "XOAUTH2"
>> or "OAUTHBEARER".
>>
>> If the answer to that question is ...
>>
>>> Other auth methods
>>> are not supported yet in this code, and this is the reason CRAM_MD5 is supported
>>> by only OpenSSL.
>>
>> ... "with srvc->auth_method set to other methods like CRAM_MD5, the
>> control would never enter this codepath, as they are implemented
>> elsewhere", then I think it would make more sense to write the above
>> like this:
>>
>> if (!srvc->auth_method)
>> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>> else if (strcmp(srvc->auth_method, "XOAUTH2") &&
>> strcmp(srvc->auth_method, "OAUTHBEARER"))
>> BUG("we only support XOAUTH2 and OAUTHBEARER in this codepath");
>>
>
> We can implement this, but:
>
> 1. It will fail if user specifies PLAIN or LOGIN as auth method.
>
> 2. We have this in the code as well:
>
> if (srvc->auth_method) {
> struct strbuf auth = STRBUF_INIT;
> strbuf_addstr(&auth, "AUTH=");
> strbuf_addstr(&auth, srvc->auth_method);
> curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
> strbuf_release(&auth);
> }
>
> Which basically means that if a user specifies an auth method,
> curl will try to use SMTP AUTH command with that method.
> So ideally, this should have worked for OAUTHBEAER and XOAUTH2
>
> But the problem with that would be a) we would need to format
> the access token as per the specifications of these mechanisms.
> and b) curl simply says these methods are not supported when
> we try with that.
>
> I filed a bug report regarding this and they were not really clear
> on whether CURLOPT_LOGIN_OPTIONS is meant for PLAIN only or should work like this with other methods too.
>
> But, the docs indicate it's for PLAIN auth only.
>
> So, considering the fact that the original code for imap-send
> was setting CURLOPT_LOGIN_OPTIONS
> unconditionally and
> was running the AUTH command even if auth was set to CRAM-MD5
> or whatever, I just preferred to not change that behaviour since I
> may cause some regression. There is a tiny possibility that CRAM-MD5
> may work, but I don't really have any free SMTP server which uses
SMTP was a typo. It's IMAP.
