On 5 June 2025 10:03:51 pm IST, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>Aditya Garg <gargaditya08@xxxxxxxx> 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 :(.
>> 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.
