>Hi Yves, > >Thanks for the patch and your efforts! > >Do you have a rebased version of this patch for me to try?
I am not sureŠ I tried and got conflicts, then was told by git my branch was out of sync, then redid a "git pull --rebase upstream" and got a bunch of conflicts againŠ Solved them, committed but only saw two changed files. But I see some "15 minutes ago" changes on GitHub from others than me. So... I'm new at git, so I may need some guidance here. Sorry :( Check out what's on https://github.com/ExpediaInc/curl. >Also, I would like >to see some test cases/examples of how this is used. That will help me do >a >full review. Still, I've read the patch and here are my comments for now: What's a good format for the test cases/examples. Would something showing typical use help, or do you mean automated tests? That'd require adding OAuth 2.0 to the sample server too. An example of use is: # Make an unauthenticated request to our server, and see how we get a 401 with a request to authenticate with HTTP MAC % curl -LsS -H 'Accept: application/json; charset=utf-8' http://hostname/service/v1/resources -D - HTTP/1.1 401 Unauthorized Server: Apache-Coyote/1.1 WWW-Authenticate: MAC error="Authorization header is missing" Content-Type: text/html;charset=utf-8 Content-Length: 1099 Date: Fri, 08 Feb 2013 01:07:44 GMT [Š] # Get a token from our oauth2server % curl -LsS -H 'Accept: application/json; charset=utf-8' http://127.0.0.1/oauth2server/tokens -d /dev/null -D - HTTP/1.0 200 OK Date: Fri, 08 Feb 2013 01:05:29 GMT Server: WSGIServer/0.1 Python/2.7.2 Content-Length: 214 Content-Type: application/json {"token_type": "mac", "mac_key": "LBy8nZzvidRT8N9lTQmfE4t6ONd4AAsS1GZNkyh0nzMqasXDjt0jJHjOPyBOpEQL", "mac_algorithm": "hmac-sha-256", "access_token": "qrw6241j8U", "expires_in": 3600, "refresh_token": "2TEKVY9jrL"} # Let's store a token (we're getting a new one here, of course, since we're POSTing again) % curl -Lss -H 'Accept: application/json; charset=utf-8' http://127.0.0.1/oauth2server/tokens -d /dev/null -D - >token.json # Make an authenticated request, and just show the generated Authorization header for illustration purposes: % curl -LsS -H 'Accept: application/json; charset=utf-8' http://hostname/service/v1/resources -D - \ --oauth2 token.json -v 2>&1 sed -n 's/.*\(Authorization: .*\)/\1/p' Authorization: MAC id="qrw6241j8U", ts="1360285918", nonce="029j47951", mac="x+WII3djVZqCRsEn1OP0jmFuGFWR7N6mJSvmU4/PUak=" # Make the same request this time showing the headers and beginning of the response again % curl -LsS -H 'Accept: application/json; charset=utf-8' http://hostname/service/v1/resources -D - --oauth2 token.json HTTP/1.1 200 OK Server: Apache-Coyote/1.1 Content-Type: application/json Date: Fri, 08 Feb 2013 01:13:11 GMT { [Š] } % >My first reaction to for example the Curl_output_mac() function is that >it is >a lot of magic functionality with very little comments. I'll add comments. It's a straightforward implementation of the internet draft so I guess if you read the draft along it's pretty obvious. I'll make it stand on its own though. > >Curl_output_mac() should also not have to figure out the default port >numbers >based on the conn->handler pointer, it can just use conn->remote_port. Sweet. I changed that. Thanks! >When you #define macros, I think you should define them outside of the >funtions so that the definition is clearly not part of the function. I >can't >see why you define and use CURL_OUTPUT_BEARER_CONV as its only used once. Honestly? That's because it's copied straight from http_digest.c. I honestly don't understand that back-to-ASCII conversion there and just got it from the file I inspired myself from (including the #define inside the function, which I thought was a curl idiom). Maybe it's not even needed? If there are ASCII/non-ASCII issues to be aware of, how is it dealt with? I need to add some code to validate that some values only include characters in these ranges: %x20-21 / %x23-5B / %x5D-7E (any printable ASCII character except for <"> and <\>) and haven't done that yet. >There's a bunch of case sensitive string comparisons. That seems a little >unorthodox in protocol land. Are you sure they're not case insensitive in >the >protocol? > >Why the new curlx_tvgettimeofday() implementation? curlx_tvnow() is there >already, and if it isn't good enough I figure that's what should fixed. >Or am >I missing something? curlx_tvnow() is not good enough because on Windows for example its beginning time is relative to a system event (last reboot for example). This works great for what curl does with it: computing durations by subtracting the values returned by two calls. However, HTTP MAC requires a monotically increasing function with a fixed epoch for EACH client (I quote: "The value MUST be a positive integer set by the client when making each request to the number of seconds elapsed from a fixed point in time.") Again, not knowing curl well, I didn't want at that point to mess up with curlx_tvnow(), especially if that had consequences on platforms I do not use. If a fixed Epoch is okay for curlx_tvnow() on Windows, then yes you could make the curlx_tvgettimeofday() code the implementation for Windows in order to simplify. Speaking of timeŠ In my patch, the code I wrote was made to "break" a non-strictly compliant HTTP MAC implementation by default (like the one my team initially deliveredŠ). I have changed this so that by default we use the Unix Epoch as a reference so that a slightly non-compliant HTTP MAC validation (one that only works with that Epoch) still works. >You're suggesting three new functions to the libcurl API but there's no >docs >for them. I'm not at all happy with new functions dedicated solely for a >particular auth method for a particular protocol... I would of course add docs. You are talking about the functions in oauth2.h here I am assuming. I am fine not exposing them in the library. The parsing and freeing could stay in the tool directory. This being said, they are quite convenient for someone who is just getting started, as they do a lot of work (parsing tokens in both JSON and application/x-www-url-encoded formats) that is required once someone gets a token. Parsing them may not be obvious for the casual user of the libcurl library. BUT I am certainly fine either wayŠ It could be moved into the tool (src) directory until someone screams for a builtin parser... If however the convenience functions make sense, we could only have two by removing the curl_parse_oauth2_token_file function. It's basically a file to memory globber. I don't even use it myself I believe (the tool use its builtin reader to set the token in config, and it's parsed later on from that string in tool_operate.c). >Several of the functions are define with the starting brace on the right >side >of the function intead of in column 0 on the first line of the function. Sorry. I thought all such issues had been caught by the build. I'll look for those. >Your patch modifies unrelated code in getparameter() where you've changed >a >bunch of calls to str2unum() etc. Those changes may be fine, but how are >they >oauth2 related? getparameter() and str2unumŠ Never heard of that. Maybe my patch caught some other changes by accident as I was rebasing/trying to keep up with the base. *OR* (I just looked) maybe I did a rename of "err" to "rc" in a (Emacs) region with a heavy hand because all those changes as I can see them are about renaming an "err" variable to "rc." If so, I apologize. YA >-- > > / daniel.haxx.se >------------------------------------------------------------------- >List admin: http://cool.haxx.se/list/listinfo/curl-library >Etiquette: http://curl.haxx.se/mail/etiquette.html ------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
