Adds the OAuthRequestSigner class to facilitate OAuth integration

Adds the OAuthRequestSigner class to facilitate OAuth integration

BUG=86223
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89632

[Rick Campbell, 2011-06-15 20:08:26]

Please take a look.

This patch adds the OAuthRequestSigner class, which computes appropriate values
for oauth_signature parameters.

[Mattias Nissler (ping if slow), 2011-06-16 17:20:34]

couple of style nits (non-scoring)

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:26: static const std::string
OAuthConsumerKeyLabel = "oauth_consumer_key";
I thought we have a policy to not non-POD statics? Should these rather be const
char[] constants? Also, the convention is to prefix these with k as you did
above.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:89: void
OAuthParameterEncode(std::ostream& output,
static

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:116: void
OAuthParameterDecode(std::ostream& output,
shouldn't all these helper functions either be static or in an anonymous
namespace?

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:198: time(&now);
use base/time.h for portability?

[Nicolas Zea, 2011-06-16 18:59:27]

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:22: static const int
kMinNonceLength = 15;
Include the static globals in the anonymous namespace too?

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:46: static std::string get_result
= "GET";
Mattias's comment applies here and with the other static local variables below
too, user static const char [] instead
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...).

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:89: void
OAuthParameterEncode(std::ostream& output,
stream's are only allowed to be used for logging unfortunately :(
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...).

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:313: }
"}  // namespace"

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.h (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.h:11: #include "base/basictypes.h"
nit, line between stl and other includes

[Rick Campbell, 2011-06-17 16:57:54]

Please take another look.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:22: static const int
kMinNonceLength = 15;
On 2011/06/16 18:59:27, nzea wrote:
> Include the static globals in the anonymous namespace too?

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:26: static const std::string
OAuthConsumerKeyLabel = "oauth_consumer_key";
On 2011/06/16 17:20:34, Mattias Nissler wrote:
> I thought we have a policy to not non-POD statics? Should these rather be
const
> char[] constants? Also, the convention is to prefix these with k as you did
> above.

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:46: static std::string get_result
= "GET";
On 2011/06/16 18:59:27, nzea wrote:
> Mattias's comment applies here and with the other static local variables below
> too, user static const char [] instead
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...).

Done.  Also changed return type to not be a reference.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:89: void
OAuthParameterEncode(std::ostream& output,
On 2011/06/16 17:20:34, Mattias Nissler wrote:
> static

This is within an anonymous namespace.  Not *exactly* static, but effectively
the same.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:89: void
OAuthParameterEncode(std::ostream& output,
On 2011/06/16 18:59:27, nzea wrote:
> stream's are only allowed to be used for logging unfortunately :(
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...).

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:91: static std::string kHexDigits
= "0123456789ABCDEF";
Changed to const char[] and moved to top of anonymous namespace with other
constants.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:116: void
OAuthParameterDecode(std::ostream& output,
On 2011/06/16 17:20:34, Mattias Nissler wrote:
> shouldn't all these helper functions either be static or in an anonymous
> namespace?

Yes, they are within an anonymous namespace.  I've also added // namespace after
the closing brace now.  I could wrap them individually to be more clear if you
think that's appropriate, though I'd prefer to keep the block.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:116: void
OAuthParameterDecode(std::ostream& output,
The decode routine is not used (and implemented incorrectly -- a copy of
Encode), so I've removed it completely.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:118: static std::string
kHexDigits = "0123456789ABCDEF";
Changed to const char[]

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:198: time(&now);
On 2011/06/16 17:20:34, Mattias Nissler wrote:
> use base/time.h for portability?

I'm going for Time::NowFromSystemTime().ToTimeT() despite, brettw's note that
"this should be removed once everybody starts using the |Time| class".  The
OAuth spec requires seconds-since-epoch and this appears to be the best way to
get it using the Time class.

Lemme know if I'm missing a better way.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:313: }
On 2011/06/16 18:59:27, nzea wrote:
> "}  // namespace"

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.h (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.h:11: #include "base/basictypes.h"
On 2011/06/16 18:59:27, nzea wrote:
> nit, line between stl and other includes

Done.

[Mattias Nissler (ping if slow), 2011-06-17 17:10:03]

More nits. regarding the actual functionality, I cannot say anything since I my
OAuth knowledge is very limited. Feel free to commit after fixing my comments
and getting a thumbs up from somebody who feels comfortable to do so.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:116: void
OAuthParameterDecode(std::ostream& output,
On 2011/06/17 16:57:54, Rick Campbell wrote:
> On 2011/06/16 17:20:34, Mattias Nissler wrote:
> > shouldn't all these helper functions either be static or in an anonymous
> > namespace?
> 
> Yes, they are within an anonymous namespace.  I've also added // namespace
after
> the closing brace now.  I could wrap them individually to be more clear if you
> think that's appropriate, though I'd prefer to keep the block.

No, that's fine, I missed the namespace declaration I my first pass.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:198: time(&now);
On 2011/06/17 16:57:54, Rick Campbell wrote:
> On 2011/06/16 17:20:34, Mattias Nissler wrote:
> > use base/time.h for portability?
> 
> I'm going for Time::NowFromSystemTime().ToTimeT() despite, brettw's note that
> "this should be removed once everybody starts using the |Time| class".  The
> OAuth spec requires seconds-since-epoch and this appears to be the best way to
> get it using the Time class.
> 
> Lemme know if I'm missing a better way.

Some time ago, I solved this by (Time::NowFromSystemTime() -
Time::UnixEpoch()).InSeconds(). It's a bit verbose, but on the plus side it's
very obvious what you're doing. YMMV.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer_unittest.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:15:
GURL("https://www.google.com/accounts/o8/GetOAuthToken");
nit: You can just call the constructor directly, getting rid of the assignment.
Also below.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:74: 
nit: excess whitespace

http://codereview.chromium.org/7171023/diff/8001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:10: class
OAuthRequestSignerTest : public testing::Test {
If you have an empty fixture, you can just use the TEST() macro instead of
TEST_F() for the test cases and remove the fixture.

[Rick Campbell, 2011-06-17 18:53:44]

Please take another look

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:198: time(&now);
On 2011/06/17 17:10:04, Mattias Nissler wrote:
> On 2011/06/17 16:57:54, Rick Campbell wrote:
> > On 2011/06/16 17:20:34, Mattias Nissler wrote:
> > > use base/time.h for portability?
> > 
> > I'm going for Time::NowFromSystemTime().ToTimeT() despite, brettw's note
that
> > "this should be removed once everybody starts using the |Time| class".  The
> > OAuth spec requires seconds-since-epoch and this appears to be the best way
to
> > get it using the Time class.
> > 
> > Lemme know if I'm missing a better way.
> 
> Some time ago, I solved this by (Time::NowFromSystemTime() -
> Time::UnixEpoch()).InSeconds(). It's a bit verbose, but on the plus side it's
> very obvious what you're doing. YMMV.

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer_unittest.cc (right):

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:15:
GURL("https://www.google.com/accounts/o8/GetOAuthToken");
On 2011/06/17 17:10:04, Mattias Nissler wrote:
> nit: You can just call the constructor directly, getting rid of the
assignment.
> Also below.

Done.

http://codereview.chromium.org/7171023/diff/2001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:74: 
On 2011/06/17 17:10:04, Mattias Nissler wrote:
> nit: excess whitespace

Done.

http://codereview.chromium.org/7171023/diff/8001/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer_unittest.cc:10: class
OAuthRequestSignerTest : public testing::Test {
On 2011/06/17 17:10:04, Mattias Nissler wrote:
> If you have an empty fixture, you can just use the TEST() macro instead of
> TEST_F() for the test cases and remove the fixture.

Done.

[Nicolas Zea, 2011-06-17 19:00:23]

LGTM

[Rick Campbell, 2011-06-17 22:32:10]

That last change with base::Time caused the Mac build to break, so I added a
static case from int64 to long int within the StringPrintf.  If there's no
objection, I'll dcommit when the try bots complete.

[Nicolas Zea, 2011-06-17 22:38:40]

http://codereview.chromium.org/7171023/diff/8013/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/8013/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:156: "%ld",
See base/format_macros.h. I think you want the "PRId64" macro.

[Rick Campbell, 2011-06-17 23:04:04]

Thanks.  PRId64 is in.  Without objection, I'll dcommit after try jobs complete.

http://codereview.chromium.org/7171023/diff/8013/chrome/common/net/gaia/oauth...
File chrome/common/net/gaia/oauth_request_signer.cc (right):

http://codereview.chromium.org/7171023/diff/8013/chrome/common/net/gaia/oauth...
chrome/common/net/gaia/oauth_request_signer.cc:156: "%ld",
On 2011/06/17 22:38:40, nzea wrote:
> See base/format_macros.h. I think you want the "PRId64" macro.

Done.