Hi Kyle, On Fri, 23 Aug 2013, Kyle L. Huff wrote:
> In the attached 4 commits I have implemented OAUTH 2.0[1] > Bearer Token[2] support to SMTP and IMAP. Many thanks for your efforts here. Do you know if OAUTH 2.0 is supported in POP3 as well? > This implementation allows for authentication to an SMTP or > IMAP server using only the username and an OAUTH 2.0[1] > Bearer token[2]. Here is the feedback from my initial review (It appears to be long but most suggestions are fairly minor and revolve around the order of definitions and functions and the indentation of parameters): * There is extra whitespace in line 1557 of curl.h * There is a small typo in curl_sasl.h "he user name" should be "the user name" * The parameters of the parameters in lines 56, 57, 68 of curl_sasl.h should be indented by two more characters to align with the first parameter on line 55 * Could you add a reference to RFC6749 to the top of cur_sasl.c? * Could you align the [in] / [out] parameter specifiers in the sasl function comments to be like the other functions - I would suggest aligning with "user" and "bearer" * Following the recent changes to use heap based buffers would you be so kind to allocate your xoauth buffer dynamically and specify the correct size in the malloc() - alternatively you might want to consider using aprintf() insterad, rather than using memcpy() type functions - this will also reduce the need for the separate allocation? * Could you move the definition of SMTP_AUTH_XOAUTH2 to between the SMTP_NTLM_TYPE2MSG and SMTP_AUTH_FINAL definitions as I have tried to base this list loosely on the sasl mechanism definitions - although I appreciate that LOGIN and PLAIN are the wrong way round at present? * Could you place the RFC reference in smtp.c in the correct numeric order - up one line to before the Draft reference? * Could you move the check for XOAUTH in smtp_endofresp() to after NTLM to match the order in which the mechanism constants are defined? * The order of the SMTP states in state() doesn't match the order in the header file and will give incorrect output in debug builds * It looks from the code in smtp_perform_authenticate() (as well as that in the IMAP code) that "XOAUTH" is supported in the AUTH initial response command - If this is the case then a) state2 should be set to SMTP_AUTH_FINAL to allow for checking of the response code afterwards and b) the 3rd and 4th parameters should be indented to match the 1st. If not then there is no need to perform the call to Curl_sasl_create_xoauth2_message() there. * Could you move smtp_state_auth_xoauth2_resp() to after the ntlm functions? * The indentation of the 2nd and 3rd parameters should match the 1st * Could you move the case for SMTP_AUTH_XOAUTH2 to after SMTP_AUTH_NTLM_TYPE2MSG in smtp_statemach_act() * I'm not sure the check for conn->xoauth2_bearer is best placed in smtp_statemach_act() as it a) doesn't allow for returning an error code and b) isn't checked when the initial response is used - suggest moving it into smtp_state_auth_xoauth2_resp() * The preferred mechanism check in smtp_parse_url_options() should match the order of the mechanism definitions * Could you move the definition of IMAP_AUTH_XOAUTH2 to between the IMAP_NTLM_TYPE2MSG and IMAP_AUTH_FINAL definitions as a) the same reason I gave above for the SMTP definitions and 2) it currently doesn't match the SMTP definitions which I have tried to keep in sync where possible? * The indentation of the parameters to Curl_sasl_create_xoauth2_message() in imap_perform_authenticate() is not correct * Could you move the check for XOAUTH in imap_state_capability_resp () to after NTLM to match the order in which the mechanism constants are defined? * Could you move imap_state_auth_xoauth2_resp() to after the ntlm functions? * The indentation of the 2nd and 3rd parameter to imap_state_auth_xoauth2_resp() is not correct * Could you move the case for IMAP_AUTHENTICATE_XOAUTH2 to after IMAP_AUTHENTICATE_NTLM_TYPE2MSG in imap_statemach_act() * Suggest moving the check for conn->xoauth2_bearer to imap_state_auth_xoauth2_resp() as per smtp suggestion * The preferred mechanism check in imap_parse_url_options() should match the order of the mechanism definitions * Suggest aligning the comment to xoauth2_bearer variable in tool_cfgable.h to match the "use_metalink" variable * In tool_getparam.c you have added a check for config->xoauth2_bearer in case 'u' (line 1639) but the check for err is still outside that if statement * Suggest adding "(IMAP and SMTP)" to the line you added to tool_help.c to indicate to the user that it is only supported for these protocols The most important items in this list were the lack of setting state2 in smtp_perform_authenticate() and the use of a stack based buffer in the sasl code. Additionally, could you please split patch 1 into two separate patches: one for adding bearer token support and the second for adding the support to SMTP as that will help keep things separate when I come to commit your changes which I will do one at a time allowing the auto builds to pick up anything I may have missed. Many thanks Steve ------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
