Aditya GargShow 8 quoted lines
> On 6 Jun 2025, at 12:18 AM, Junio C Hamano <gitster@pobox.com> wrote: > Aditya Garg <gargaditya08@live.com> 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?
Ok so I was wrong here. The warning actually comes when we compile WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and will attract waddress warnings,
And NOT_CONSTANT doesn't help here.
Show 78 quoted lines
>
> if (NOT_CONSTANT(!auth_oauthbearer)) {
> ... skip the thing ...
> }
>
>
>>>> 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?
>
>> 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");
>
> Or the code is not protecting this code path so control can reach
> with auth_method set to CRAM_MD5 here (e.g. when built without
> OpenSSL)? If so, replace BUG("message") with die(_("message"))
> above.
>
> On the other hand, if you are trying to fall back to plain password
> when other unhandled methods are specified, I would expect that the
> code to read more like:
>
> if (srvc->auth_method &&
> (!strcmp(srvc->auth_method, "XOAUTH2") ||
> !strcmp(srvc->auth_method, "OAUTHBEARER")))
> ;
> else {
> if (srvc->auth_method)
> warning("auth method %s not supported,
> falling back to plain password",
> srvc->auth_method);
> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> }
>
> I cannot quite tell which one you meant, but I am guessing that the
> former is the case from your explanation.
>
> Thanks.