Notify calling web-app when Host plugin becomes connected to a client.

Notify calling web-app when Host plugin becomes connected to a client.

BUG=85110
TEST=


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

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

[Wez, 2011-06-08 19:11:11]

Small CL to add the "connected" state in the Host plugin.

[Wez, 2011-06-09 20:26:29]

On 2011/06/08 19:11:11, Wez wrote:
> Small CL to add the "connected" state in the Host plugin.

Added code to stop the Host from accepting more connections once there is a
client connected.

[Wez, 2011-06-10 17:21:25]

On 2011/06/09 20:26:29, Wez wrote:
> On 2011/06/08 19:11:11, Wez wrote:
> > Small CL to add the "connected" state in the Host plugin.
> 
> Added code to stop the Host from accepting more connections once there is a
> client connected.

Ping!

[Jamie, 2011-06-10 18:00:38]

LGTM, but please take a look at my comments below.

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:284: // reject the connection immediately.
What's the concern here? If we're worried that a bad guy might have obtained the
access code and try to connect, then I'd be tempted to go further and disconnect
everyone. After all, who's to say that the bad guy didn't get there first?

Also, is this really core to getting the correct state into the web-app? It
feels orthogonal to it, and would probably be better as a separate CL.

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:437: // TODO(wez): CurtainMode shouldn't be
driven directly by ChromotingHost.
+1

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:126: // TODO(wez): ChromotingHost shouldn't need
to know about Me2Mom.
+1, although I don't think that half the lines in this CL needed to document the
fact :)

http://codereview.chromium.org/7134023/diff/1007/remoting/host/host_status_ob...
File remoting/host/host_status_observer.h (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/host_status_ob...
remoting/host/host_status_observer.h:27: virtual void
OnAuthenticatedClientsChanged(int authenticated_clients) = 0;
Optional: I think that this method is never implemented if the other three are,
and vice versa. Perhaps a separate interface would be better.

[Wez, 2011-06-13 20:30:35]

I've moved the AccessCode handling change to its own CL, and addressed the
comments with this one.

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:284: // reject the connection immediately.
On 2011/06/10 18:00:38, Jamie wrote:
> What's the concern here? If we're worried that a bad guy might have obtained
the
> access code and try to connect, then I'd be tempted to go further and
disconnect
> everyone. After all, who's to say that the bad guy didn't get there first?
> 
> Also, is this really core to getting the correct state into the web-app? It
> feels orthogonal to it, and would probably be better as a separate CL.

The Access Code is one-shot by design, so from a security perspective,
regardless of whether you can think of a good exploit for it, it should be
implemented as one-shot to avoid nasty surprises due to the more lax implemented
vs designed behaviour.

In practice, the issue is that the time between the Access Code being generated
and being used is likely to be an order of magnitude smaller than the duration
of a session, so leaving the Access Code open for the duration of the session
increases the attack window by that order of magnitude, too.

I agree that this is really a separate change, though; I'll pull it out (you
win!), but you have to review it, please (you lose!).

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:437: // TODO(wez): CurtainMode shouldn't be
driven directly by ChromotingHost.
On 2011/06/10 18:00:38, Jamie wrote:
> +1

As requested, I've removed the dups of this comment...

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:126: // TODO(wez): ChromotingHost shouldn't need
to know about Me2Mom.
On 2011/06/10 18:00:38, Jamie wrote:
> +1, although I don't think that half the lines in this CL needed to document
the
> fact :)

You're no fun. ;)

http://codereview.chromium.org/7134023/diff/1007/remoting/host/host_status_ob...
File remoting/host/host_status_observer.h (right):

http://codereview.chromium.org/7134023/diff/1007/remoting/host/host_status_ob...
remoting/host/host_status_observer.h:27: virtual void
OnAuthenticatedClientsChanged(int authenticated_clients) = 0;
On 2011/06/10 18:00:38, Jamie wrote:
> Optional: I think that this method is never implemented if the other three
are,
> and vice versa. Perhaps a separate interface would be better.

That certainly makes sense, but currently HostStatusObserver is RefCounted.  I'd
like to fix it to be a pure observer interface before splitting it up.

[commit-bot: I haz the power, 2011-06-16 18:36:53]

Change committed as 89370

[commit-bot: I haz the power, 2011-06-21 02:10:05]

Can't apply patch for file remoting/host/register_support_host_request.cc.
While running patch -p1 --forward --force;
patching file remoting/host/register_support_host_request.cc
Hunk #1 FAILED at 78.
1 out of 1 hunk FAILED -- saving rejects to file
remoting/host/register_support_host_request.cc.rej

[commit-bot: I haz the power, 2011-06-21 02:54:54]

Can't apply patch for file remoting/host/host_plugin.cc.
While running patch -p1 --forward --force;
patching file remoting/host/host_plugin.cc
Hunk #1 FAILED at 152.
Hunk #2 FAILED at 375.
2 out of 2 hunks FAILED -- saving rejects to file
remoting/host/host_plugin.cc.rej

[commit-bot: I haz the power, 2011-06-21 19:00:38]

Change committed as 89873