Wire in OAuth2 support into non-sandboxed connections in libjingle.

Wire in OAuth2 support into non-sandboxed connections in libjingle.

BUG=none
TEST=can connect w/o ClientLogin token.

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

[awong, 2011-05-25 01:06:13]

More OAuth2 fun :)

[Wez, 2011-05-25 03:57:46]

Vroom vroom.

LGTM with nits.

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.cc
File remoting/base/auth_token_util.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.c...
remoting/base/auth_token_util.cc:18: // TODO(ajwong): Remove this defaulting
once all webclients are migrated.
Mark this with a bug-#?

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.h
File remoting/base/auth_token_util.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.h...
remoting/base/auth_token_util.h:16: std::string* auth_service);
|auth_service|->|service| to match the description in the comment (or vice
versa).
We may be the first folks to hit this issue, but others (e.g. Cloud Print) are
presumably going to have similar need - should this be somewhere where others
can find and use it?

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util_u...
File remoting/base/auth_token_util_unittest.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util_u...
remoting/base/auth_token_util_unittest.cc:1: // Use of this source code is
governed by a BSD-style license that can be
Missing Copyright line.

http://codereview.chromium.org/7008003/diff/1/remoting/base/constants.h
File remoting/base/constants.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/constants.h#newco...
remoting/base/constants.h:13: // TODO(ajwong): Remove this once we've killed off
XmppToken usage.
Mark this with a bug-#?

http://codereview.chromium.org/7008003/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.h:81: //   //
"serivce:token". For example, "oauth2:5/aBd123".
Typo: serivce.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_config.h
File remoting/host/host_config.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_config.h#new...
remoting/host/host_config.h:24: // Auth service used to authenticate in XMPP
network.
Should "in" really be "in" and not "to"?

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:62: // Auth token should be in the format
"service:token".  And example
Typo: And

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:64: void connect(string uid, string
auth_token_with_service);
If these were called |auth_service_and_token| the parts would be in the same
order as the name implies.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:395: if (auth_token_with_service.empty()) {
Why do this test here; the empty token case post-parse will catch this case, the
only difference being that right now it'll default the auth-service.

http://codereview.chromium.org/7008003/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7008003/diff/1/remoting/remoting.gyp#newcode239
remoting/remoting.gyp:239: 'base/capture_data.h',
Duplicate capture_data.h.

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:41: if (remoting.getItem(XMPP_LOGIN_NAME)) {
It's a bit confusing that XMPP_LOGIN_NAME contains a field called "email"...

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:310: tryConnect(accessCode);
Is there any possibility of this getting stuck in a loop if something goes
wrong?

[awong, 2011-05-25 16:56:56]

done.

Commit queuing. Thanks! :)

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.cc
File remoting/base/auth_token_util.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.c...
remoting/base/auth_token_util.cc:18: // TODO(ajwong): Remove this defaulting
once all webclients are migrated.
On 2011/05/25 03:57:46, Wez wrote:
> Mark this with a bug-#?

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.h
File remoting/base/auth_token_util.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util.h...
remoting/base/auth_token_util.h:16: std::string* auth_service);
On 2011/05/25 03:57:46, Wez wrote:
> |auth_service|->|service| to match the description in the comment (or vice
> versa).
> We may be the first folks to hit this issue, but others (e.g. Cloud Print) are
> presumably going to have similar need - should this be somewhere where others
> can find and use it?

I'm not sure this is the right way to generalize yet, so I'd rather not push up.
I'll drop them a not so that they know we've gotten OAuth2 working.

Updated comment.

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util_u...
File remoting/base/auth_token_util_unittest.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/auth_token_util_u...
remoting/base/auth_token_util_unittest.cc:1: // Use of this source code is
governed by a BSD-style license that can be
On 2011/05/25 03:57:46, Wez wrote:
> Missing Copyright line.

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/base/constants.h
File remoting/base/constants.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/base/constants.h#newco...
remoting/base/constants.h:13: // TODO(ajwong): Remove this once we've killed off
XmppToken usage.
On 2011/05/25 03:57:46, Wez wrote:
> Mark this with a bug-#?

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.h:81: //   //
"serivce:token". For example, "oauth2:5/aBd123".
On 2011/05/25 03:57:46, Wez wrote:
> Typo: serivce.

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_config.h
File remoting/host/host_config.h (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_config.h#new...
remoting/host/host_config.h:24: // Auth service used to authenticate in XMPP
network.
On 2011/05/25 03:57:46, Wez wrote:
> Should "in" really be "in" and not "to"?

sure.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:62: // Auth token should be in the format
"service:token".  And example
On 2011/05/25 03:57:46, Wez wrote:
> Typo: And

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:64: void connect(string uid, string
auth_token_with_service);
On 2011/05/25 03:57:46, Wez wrote:
> If these were called |auth_service_and_token| the parts would be in the same
> order as the name implies.

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:395: if (auth_token_with_service.empty()) {
On 2011/05/25 03:57:46, Wez wrote:
> Why do this test here; the empty token case post-parse will catch this case,
the
> only difference being that right now it'll default the auth-service.

cause I'm silly? :)

Fixed.

http://codereview.chromium.org/7008003/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7008003/diff/1/remoting/remoting.gyp#newcode239
remoting/remoting.gyp:239: 'base/capture_data.h',
On 2011/05/25 03:57:46, Wez wrote:
> Duplicate capture_data.h.

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:41: if (remoting.getItem(XMPP_LOGIN_NAME)) {
On 2011/05/25 03:57:46, Wez wrote:
> It's a bit confusing that XMPP_LOGIN_NAME contains a field called "email"...

Yes...I'm not going to change this for now.  We're going to overhaul the storage
of everything soonish.  For right now, the logic is a bit tangled and I want to
easily revert to XMPP-land by just re-exposing the div.

[commit-bot: I haz the power, 2011-05-25 16:59:51]

Presubmit check for 7008003-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  remoting/base/auth_token_util_unittest.cc

Presubmit checks took 1.3s to calculate.

[Jamie, 2011-05-25 17:04:52]

http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc
File remoting/client/client_util.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc#...
remoting/client/client_util.cc:53: } else if (arg == "--service") {
Would it be more consistent to use the colon-separated form as the value for
--token?

Actually, come to think of it, why are we using a colon-separated string anyway?
Why not specify the service as a separate parameter in the connect() API as
well?

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:62: // Auth token should be in the format
"service:token".  And example
Should we name the parameter explicitly in the comment? ie,
|auth_token_with_service| instead of "Auth token".

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:403: if (auth_token.empty()) {
Do we need a check for an empty service? The string ":blah" would produce one I
think.

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.html
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.h...
remoting/webapp/me2mom/choice.html:45: <div id="xmpp_div" style="display:none;">
Does this div ever get shown?

[awong, 2011-05-25 17:07:31]

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:310: tryConnect(accessCode);
On 2011/05/25 03:57:46, Wez wrote:
> Is there any possibility of this getting stuck in a loop if something goes
> wrong?

Good point.  Added simple break for now.

[commit-bot: I haz the power, 2011-05-25 17:27:20]

Commit queue patch verification failed

[awong, 2011-05-25 18:00:01]

http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc
File remoting/client/client_util.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc#...
remoting/client/client_util.cc:53: } else if (arg == "--service") {
On 2011/05/25 17:04:52, Jamie wrote:
> Would it be more consistent to use the colon-separated form as the value for
> --token?

Yes...I just wasn't clear on what script might be using this, etc.  So I didn't
want to change the existing API yet.
 
> Actually, come to think of it, why are we using a colon-separated string
anyway?
> Why not specify the service as a separate parameter in the connect() API as
> well?

I thought about it, but decided that since we hadn't really settled on a plan
for the connect() API call, to do it this way for now.  This maintains backwards
compat for the time being.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:62: // Auth token should be in the format
"service:token".  And example
On 2011/05/25 17:04:52, Jamie wrote:
> Should we name the parameter explicitly in the comment? ie,
> |auth_token_with_service| instead of "Auth token".

Done.

http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:403: if (auth_token.empty()) {
On 2011/05/25 17:04:52, Jamie wrote:
> Do we need a check for an empty service? The string ":blah" would produce one
I
> think.

I'm intentionally not doing that for now so I don't break all our existing
javascript.

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.html
File remoting/webapp/me2mom/choice.html (right):

http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.h...
remoting/webapp/me2mom/choice.html:45: <div id="xmpp_div" style="display:none;">
On 2011/05/25 17:04:52, Jamie wrote:
> Does this div ever get shown?

nope.  I didn't quite want to delete it completely until we were sure the OAuth2
stuff worked. This way, someone can take the exist webapp, pop open the
inspector, and just enable the XMPP div to get the tokens.  It should go away
soon though (eg., next week).

[jamiewalch, 2011-05-25 20:49:00]

LGTM.

On Wed, May 25, 2011 at 11:00 AM,  <ajwong@chromium.org> wrote:
>
> http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc
> File remoting/client/client_util.cc (right):
>
>
http://codereview.chromium.org/7008003/diff/1/remoting/client/client_util.cc#...
> remoting/client/client_util.cc:53: } else if (arg == "--service") {
> On 2011/05/25 17:04:52, Jamie wrote:
>>
>> Would it be more consistent to use the colon-separated form as the
>
> value for
>>
>> --token?
>
> Yes...I just wasn't clear on what script might be using this, etc.  So I
> didn't want to change the existing API yet.
>
>> Actually, come to think of it, why are we using a colon-separated
>
> string anyway?
>>
>> Why not specify the service as a separate parameter in the connect()
>
> API as
>>
>> well?
>
> I thought about it, but decided that since we hadn't really settled on a
> plan for the connect() API call, to do it this way for now.  This
> maintains backwards compat for the time being.
>
> http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc
> File remoting/host/host_plugin.cc (right):
>
>
http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
> remoting/host/host_plugin.cc:62: // Auth token should be in the format
> "service:token".  And example
> On 2011/05/25 17:04:52, Jamie wrote:
>>
>> Should we name the parameter explicitly in the comment? ie,
>> |auth_token_with_service| instead of "Auth token".
>
> Done.
>
>
http://codereview.chromium.org/7008003/diff/1/remoting/host/host_plugin.cc#ne...
> remoting/host/host_plugin.cc:403: if (auth_token.empty()) {
> On 2011/05/25 17:04:52, Jamie wrote:
>>
>> Do we need a check for an empty service? The string ":blah" would
>
> produce one I
>>
>> think.
>
> I'm intentionally not doing that for now so I don't break all our
> existing javascript.
>
>
http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.html
> File remoting/webapp/me2mom/choice.html (right):
>
>
http://codereview.chromium.org/7008003/diff/1/remoting/webapp/me2mom/choice.h...
> remoting/webapp/me2mom/choice.html:45: <div id="xmpp_div"
> style="display:none;">
> On 2011/05/25 17:04:52, Jamie wrote:
>>
>> Does this div ever get shown?
>
> nope.  I didn't quite want to delete it completely until we were sure
> the OAuth2 stuff worked. This way, someone can take the exist webapp,
> pop open the inspector, and just enable the XMPP div to get the tokens.
> It should go away soon though (eg., next week).
>
> http://codereview.chromium.org/7008003/
>