Properly handle accounts that don't have GMail.

Properly handle accounts that don't have GMail account.

1. Me2MeHostAuthenticatorFactory now verifies that the bare 
JID of the remote client matches bare JID of the host. 
Previously it was comparing it with the user's email, 
which may be different from JID.
2. GaiaOAuthClient now fetches user's email.
3. SignalingConnector verifies that user's email matches 
the expected value stored in xmpp_login. If it doesn't, 
then the auth token was generated for a different account 
and the host treats it as an authentication error.

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

[Wez, 2012-05-15 23:14:07]

Before I get into code-review, could you beef up the description to explain what
it means to "properly" handle these accounts?  Thanks!

[Sergey Ulanov, 2012-05-15 23:33:32]

On 2012/05/15 23:14:07, Wez wrote:
> Before I get into code-review, could you beef up the description to explain
what
> it means to "properly" handle these accounts?  Thanks!

Done

[Wez, 2012-05-16 01:21:20]

To address the immediate issue this looks fine, but it takes us in the direction
of being tied to access control based on the OAuth token identity, rather than
having potential to allow other users access, which seems wrong.  It also seems
preferable to do the OAuth token / email verification at configuration time,
rather than run-time, if possible. What do you think?

[Sergey Ulanov, 2012-05-16 17:53:15]

On 2012/05/16 01:21:20, Wez wrote:
> To address the immediate issue this looks fine, but it takes us in the
direction
> of being tied to access control based on the OAuth token identity, rather than
> having potential to allow other users access, which seems wrong. 

I don't think this change precludes sharing support. Simple way to add support
for sharing would be to add client_email parameter in
AuthenticatorFactory::CreateAuthenticator() and check it against the access
list. We will still need to allow connections from the same JID for connections
from the owner.

> It also seems
> preferable to do the OAuth token / email verification at configuration time,
> rather than run-time, if possible. What do you think?

It would be a bigger change to do verification at configuration time, and I
think it would have to be platform-specific.

[Wez, 2012-05-16 21:23:20]

On 2012/05/16 17:53:15, sergeyu wrote:
> On 2012/05/16 01:21:20, Wez wrote:
> > To address the immediate issue this looks fine, but it takes us in the
> direction
> > of being tied to access control based on the OAuth token identity, rather
than
> > having potential to allow other users access, which seems wrong. 
> 
> I don't think this change precludes sharing support. Simple way to add support
> for sharing would be to add client_email parameter in
> AuthenticatorFactory::CreateAuthenticator() and check it against the access
> list. We will still need to allow connections from the same JID for
connections
> from the owner.

Not if we can do lookups in the other direction, e.g. for an ACL containing a
list of Google account email addresses, look up the corresponding JID prefixes;
I take it that that's not possible?

> > It also seems
> > preferable to do the OAuth token / email verification at configuration time,
> > rather than run-time, if possible. What do you think?
> 
> It would be a bigger change to do verification at configuration time, and I
> think it would have to be platform-specific.

It would be cross-platform code, I think, but it would need to be driven by the
platform-specific configuration components, yes.

[Sergey Ulanov, 2012-05-16 21:33:21]

On Wed, May 16, 2012 at 2:23 PM, <wez@chromium.org> wrote:

> On 2012/05/16 17:53:15, sergeyu wrote:
>
>> On 2012/05/16 01:21:20, Wez wrote:
>> > To address the immediate issue this looks fine, but it takes us in the
>> direction
>> > of being tied to access control based on the OAuth token identity,
>> rather
>>
> than
>
>> > having potential to allow other users access, which seems wrong.
>>
>
>  I don't think this change precludes sharing support. Simple way to add
>> support
>> for sharing would be to add client_email parameter in
>> AuthenticatorFactory::**CreateAuthenticator() and check it against the
>> access
>> list. We will still need to allow connections from the same JID for
>>
> connections
>
>> from the owner.
>>
>
> Not if we can do lookups in the other direction, e.g. for an ACL
> containing a
> list of Google account email addresses, look up the corresponding JID
> prefixes;
> I take it that that's not possible?


For incoming session-initiate requests we cat get google:email:from that
specifies the email of the sender and we can use it for ACL. Problem is
that this attribute is added by GTalk only when message is send to a
different user, so we cannot use it here, when the client and host are
connected with the same account.
I don't think there is an easy way to find a JID that corresponds to
account with a specific e-mail.


>
>  > It also seems
>> > preferable to do the OAuth token / email verification at configuration
>> time,
>> > rather than run-time, if possible. What do you think?
>>
>
>  It would be a bigger change to do verification at configuration time, and
>> I
>> think it would have to be platform-specific.
>>
>
> It would be cross-platform code, I think, but it would need to be driven
> by the
> platform-specific configuration components, yes.
>

Right. It would still be more complicated change compared to this one, and
I don't see any big downsides of the approach I implemented here.



>
>
http://codereview.chromium.**org/10332187/<http://codereview.chromium.org/103...
>

[Wez, 2012-05-16 23:00:06]

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
File remoting/host/gaia_oauth_client.cc (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:21: const char kUserInfoUrl[] =
"https://www.googleapis.com/oauth2/v1/userinfo";
It seems wrong for the GaiaOAuth2Url to be a parameter to the class but the
UserInfo URL not to be?

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:26: const char kEmailValue[] = "email";
nit: Keep this definition with the URL definition, above, since it's used for a
different type of request?

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:132: void
GaiaOAuthClient::Core::FetchUserInfo() {
Since this fetches the UserInfo and then notifies the delegate, the name should
reflect the fact, I think.
Alternatively, FetchUserInfo() could accept a callback that gets notified when
the request completes, and you'd use Bind() to associate the token & expiry with
that?

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:146: scoped_ptr<Value>
message_value(base::JSONReader::Read(response));
nit: Indentation.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:150:
static_cast<DictionaryValue*>(message_value.release()));
Can't use AsDictionaryValue() here?

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
File remoting/host/gaia_oauth_client.h (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.h:40: virtual void OnRefreshTokenResponse(const
std::string& email,
nit: email -> user_email?

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:105: "The OAuth token was issued for a
different account.";
Suggest "OAuth token and email address do not refer to the same account."

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
remoting/host/signaling_connector.h:74: virtual void
OnRefreshTokenResponse(const std::string& email,
nit: email -> user_email?

[Sergey Ulanov, 2012-05-17 00:38:11]

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
File remoting/host/gaia_oauth_client.cc (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:21: const char kUserInfoUrl[] =
"https://www.googleapis.com/oauth2/v1/userinfo";
On 2012/05/16 23:00:07, Wez wrote:
> It seems wrong for the GaiaOAuth2Url to be a parameter to the class but the
> UserInfo URL not to be?
Made them both configurable.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:26: const char kEmailValue[] = "email";
On 2012/05/16 23:00:07, Wez wrote:
> nit: Keep this definition with the URL definition, above, since it's used for
a
> different type of request?

Done.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:132: void
GaiaOAuthClient::Core::FetchUserInfo() {
On 2012/05/16 23:00:07, Wez wrote:
> Since this fetches the UserInfo and then notifies the delegate, the name
should
> reflect the fact, I think.
> Alternatively, FetchUserInfo() could accept a callback that gets notified when
> the request completes, and you'd use Bind() to associate the token & expiry
with
> that?

Renamed it to FetchUserInfoAndInvokeCallback(), but don't think it's much better
this way.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:146: scoped_ptr<Value>
message_value(base::JSONReader::Read(response));
On 2012/05/16 23:00:07, Wez wrote:
> nit: Indentation.

Done.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.cc:150:
static_cast<DictionaryValue*>(message_value.release()));
On 2012/05/16 23:00:07, Wez wrote:
> Can't use AsDictionaryValue() here?

Yes, I can, but it makes this code shorter only when we put assignment in the if
condition, which is less readable. It's also consistent with
OnAuthTokenFetchComplete().

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
File remoting/host/gaia_oauth_client.h (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/gaia_oauth_cl...
remoting/host/gaia_oauth_client.h:40: virtual void OnRefreshTokenResponse(const
std::string& email,
On 2012/05/16 23:00:07, Wez wrote:
> nit: email -> user_email?

Done.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:105: "The OAuth token was issued for a
different account.";
On 2012/05/16 23:00:07, Wez wrote:
> Suggest "OAuth token and email address do not refer to the same account."

Done.

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10332187/diff/3001/remoting/host/signaling_con...
remoting/host/signaling_connector.h:74: virtual void
OnRefreshTokenResponse(const std::string& email,
On 2012/05/16 23:00:07, Wez wrote:
> nit: email -> user_email?

Done.

[Sergey Ulanov, 2012-05-17 21:08:22]

ping

[Wez, 2012-05-17 22:36:19]

lgtm

[commit-bot: I haz the power, 2012-05-18 02:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10332187/11002

[commit-bot: I haz the power, 2012-05-18 04:31:14]

Change committed as 137824