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

[Junio C Hamano <gitster@pobox.com>]

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?

	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.