Hi Kyle,

On Sun, 25 Aug 2013, Kyle L. Huff wrote:

> > Many thanks for your efforts here.
>
> My pleasure! It almost seems like writing it was less effort than
> your review. (and maybe that is evident with all the white-space
> issues I missed...)

Hehehe - don't worry about the white space issues that's all minor stuff
really. The quality of the patches, in my opinion, and your attention to
following the existing coding style is excellent.

> > Do you know if OAUTH 2.0 is supported in POP3 as well?
>
> POP3 might support OAUTH 2.0, but I have not found a provider
> that has implemented it. I know that presently the google POP3
> service does not support OAUTH 2.0.

I couldn't find much on the subject myself, from a quick search, but I think
I found a mail client / api that had implemented it and it should be pretty
straight forward to implement it here as well.

> > case 'u' (line 1639) but the check for err is still outside that if
> > statement
>
> I was trying to avoid an error if no password was specified, since
> the bearer token is the password. I have since changed it to only
> evaluate err if the bearer token is not present. I still don't like this
> though, because it only works if the user has specified the
> --bearer parameter before the --user parameter. Perhaps there
> is way I am not aware of to look ahead in the options provided to
> determine if the --bearer parameter has been specified, negating
> the need for a password.

Mmmm... I don't know of a way of doing this from that function myself and it
does worries me that --bearer needs to be specified before -u to avoid the
user being prompted for the password :(

Perhaps the call to checkpasswd() could be moved to after all the parameters
have been got ??

You also need to bear in mind that checkpasswd() doesn't just check for the
password - it also looks for the optional login options via the ; separator
which is currently used to specify the preferred authentication mechanism
(in addition to in the URL). As such we should support "-bearer mybearer -u
steve;AUTH=XOAUTH2" as a valid command line to curl. If bearer was passed
into this function then it could be added to the check that then requests
the password from the user.

We should probably rename the function in that respect, as well, but I can
do that after I commit your changes - I was being a little lazy with my
changes in v7.31 :(

> All of the items you mentioned have been corrected
> and the only notes for any of the items you listed are
> added above. When I wrote the stack based buffer in
> the sasl file, I was without internet access using an
> older version of cURL source -- it felt dirty at the time
> but I tried to match the conventions used elsewhere in
> the same file. I should have checked those areas after
> I pulled in the more recent commits.

I'm pretty happy with the patches myself - although I did want to ask /
point out the following before committing:

* Is it necessary to obtain the length of user and bearer (ulen and blen) in
Curl_sasl_create_xoauth2_message() in order to calculate the length that is
passed to Curl_base64_encode() - could this be strlen(xoauth) or have I
missed something there?
* Setting outlen and outptr are not, strictly speaking, needed when
returning failure
* The alignment of the parameters to the function are also 2 characters out
in the .c file

> Thank you very much for your attention to this project and the
> related patches.

You're welcome and thank you again for your work here.

Kind Regards

Steve
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-08-25