Block event processing on host/client until the client has authenticated.

Block event processing on host/client until the client has authenticated.

Input events:
* Client will not send them
* Host will not process them

Control events:
* Client will only process BeginSessionResponse
* Host will only process BeginSessionRequest
All other control messages will be ignored.

BUG=72466
TEST=manual+tests

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

[garykac, 2011-03-03 03:43:37]

Nothing earth-shattering here. I played with a few variants before settling on
having the connection_to_host|client own this. And finally had time to test the
client and host sides individually.

I'm not sure I'm content with the SetEnable/Enabled names on the stubs, but
SetClientAuthenticated (used on the connection) seemed a bit verbose.

I'm open to suggestions for better/more consistent names.

[garykac, 2011-03-04 00:29:09]

On 2011/03/03 03:43:37, garykac wrote:
> Nothing earth-shattering here. I played with a few variants before settling on
> having the connection_to_host|client own this. And finally had time to test
the
> client and host sides individually.
> 
> I'm not sure I'm content with the SetEnable/Enabled names on the stubs, but
> SetClientAuthenticated (used on the connection) seemed a bit verbose.
> 
> I'm open to suggestions for better/more consistent names.

Adding alpha...

[dmac, 2011-03-04 01:36:27]

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stub.h
File remoting/protocol/client_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:26: virtual ~ClientStub() {}
as this is no longer pure virtual, may have to move the d'tor out of line

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:38: void SetEnable(bool enable) {
actually the var name should probably be enabled, and this should be
set_enabled(bool enabled).

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:44: bool Enabled() {
just enabled()

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:79:
//ConnectionToClient::ConnectionToClient() {
get rid of this then?

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:80: //ConnectionToClient();
should we get rid of this then?

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:93: // will be silently ignored.
will they be silently ignored? or will LOGs occur?

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:25: virtual ~HostStub() {};
since this is no longer pure virtual, move out of line?

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:37: void SetEnable(bool enable) {
actually the var name should probably be enabled, and this should be
set_enabled(bool enabled).

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub...
remoting/protocol/input_stub.h:22: virtual ~InputStub() {}
same deal with other stubs

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub...
remoting/protocol/input_stub.h:30: void SetEnable(bool enable) {
same comment about enabled

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/protocol_m...
File remoting/protocol/protocol_mock_objects.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/protocol_m...
remoting/protocol/protocol_mock_objects.h:88: Task*bdone));
bdone?

[dmac, 2011-03-04 01:36:40]

sorry I didn't get the review done earlier.

[ajwong, 2011-03-04 01:45:31]

we do need to out-of-line any non-pure-virtual functions, and any
constructors/destructors for classes that aren't pure interfaces.

Otherwise clang-bot will go red.

-Albert

On Thu, Mar 3, 2011 at 5:36 PM, <dmaclach@chromium.org> wrote:

> sorry I didn't get the review done earlier.
>
>
> http://codereview.chromium.org/6594138/
>

[garykac, 2011-03-04 05:33:23]

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stub.h
File remoting/protocol/client_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:26: virtual ~ClientStub() {}
On 2011/03/04 01:36:27, dmac wrote:
> as this is no longer pure virtual, may have to move the d'tor out of line

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:38: void SetEnable(bool enable) {
On 2011/03/04 01:36:27, dmac wrote:
> actually the var name should probably be enabled, and this should be
> set_enabled(bool enabled).

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:44: bool Enabled() {
On 2011/03/04 01:36:27, dmac wrote:
> just enabled()

SetEnabled() and enabled()?

or would set_enabled() be better?

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:79:
//ConnectionToClient::ConnectionToClient() {
On 2011/03/04 01:36:27, dmac wrote:
> get rid of this then?

oops

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:80: //ConnectionToClient();
On 2011/03/04 01:36:27, dmac wrote:
> should we get rid of this then?

oops

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:93: // will be silently ignored.
On 2011/03/04 01:36:27, dmac wrote:
> will they be silently ignored? or will LOGs occur?

They should LOG, so I removed "silently"

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:25: virtual ~HostStub() {};
On 2011/03/04 01:36:27, dmac wrote:
> since this is no longer pure virtual, move out of line?

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:37: void SetEnable(bool enable) {
On 2011/03/04 01:36:27, dmac wrote:
> actually the var name should probably be enabled, and this should be
> set_enabled(bool enabled).

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub...
remoting/protocol/input_stub.h:22: virtual ~InputStub() {}
On 2011/03/04 01:36:27, dmac wrote:
> same deal with other stubs

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/input_stub...
remoting/protocol/input_stub.h:30: void SetEnable(bool enable) {
On 2011/03/04 01:36:27, dmac wrote:
> same comment about enabled

Done.

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/protocol_m...
File remoting/protocol/protocol_mock_objects.h (right):

http://codereview.chromium.org/6594138/diff/3015/remoting/protocol/protocol_m...
remoting/protocol/protocol_mock_objects.h:88: Task*bdone));
On 2011/03/04 01:36:27, dmac wrote:
> bdone?

Done.

[Wez, 2011-03-04 12:09:37]

Some drive-by comments. :)

http://codereview.chromium.org/6594138/diff/12001/remoting/client/chromoting_...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/client/chromoting_...
remoting/client/chromoting_client.cc:238:
connection_->SetClientAuthenticated(msg->success());
I'd recommend SetClientAuthenticated(bool) -> if (bool) OnClientAuthenticated().
 Whether the client is authenticated should only ever go false->true.

http://codereview.chromium.org/6594138/diff/12001/remoting/client/input_handl...
File remoting/client/input_handler.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/client/input_handl...
remoting/client/input_handler.cc:26: if (stub && stub->enabled()) {
Why does the InputStub and/or InputHandler even exist if we haven't yet
authenticated?

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
File remoting/protocol/client_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:25: ClientStub();
Is there a more descriptive name we can attached to this, and make it more
uniform with the related Input and Video classes? e.g. ClientControl,
ClientInput, ClientVideo

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:39: 
Move from SetEnabled(bool) to OnAuthenticated()?

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:42: bool enabled();
Do we need this?

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:90: }
Why do we store whether the "stubs" are enabled/disabled in each stub, and then
enforce whether they accept messages in a different class?  That seems kinda
complicated.
Could we instead have a completed separate Dispatcher prior to authentication,
and once authenticated switch to the main Dispatcher, so that there is a
completely clear separation of code paths before and after authentication?

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub...
remoting/protocol/host_stub.h:40: // I.e., should we be processing control
events?
As HostStub

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub...
remoting/protocol/host_stub.h:47: 
As HostStub.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:33: 
Similar comments to ClientStub. :)

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:35: // I.e., are we processing input events?
As ClientStub.

[garykac, 2011-03-04 20:28:02]

http://codereview.chromium.org/6594138/diff/12001/remoting/client/chromoting_...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/client/chromoting_...
remoting/client/chromoting_client.cc:238:
connection_->SetClientAuthenticated(msg->success());
On 2011/03/04 12:09:38, Wez wrote:
> I'd recommend SetClientAuthenticated(bool) -> if (bool)
OnClientAuthenticated().
>  Whether the client is authenticated should only ever go false->true.

Done.

http://codereview.chromium.org/6594138/diff/12001/remoting/client/input_handl...
File remoting/client/input_handler.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/client/input_handl...
remoting/client/input_handler.cc:26: if (stub && stub->enabled()) {
On 2011/03/04 12:09:38, Wez wrote:
> Why does the InputStub and/or InputHandler even exist if we haven't yet
> authenticated?

On host side: input_stub if part of the DesktopEnvironment owned by
ChromotingHost so it would be awkward to create it after authentication.

On client side: I now delay creation until authentication.

We still need |enabled| for the host, but I removed the check here.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
File remoting/protocol/client_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:25: ClientStub();
On 2011/03/04 12:09:38, Wez wrote:
> Is there a more descriptive name we can attached to this, and make it more
> uniform with the related Input and Video classes? e.g. ClientControl,
> ClientInput, ClientVideo

I agree that it would be nice to have better names, but I'll defer that to a
separate CL.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:39: 
On 2011/03/04 12:09:38, Wez wrote:
> Move from SetEnabled(bool) to OnAuthenticated()?

Done.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/client_st...
remoting/protocol/client_stub.h:42: bool enabled();
On 2011/03/04 12:09:38, Wez wrote:
> Do we need this?

The message dispatchers make use of this to cut off processing the input or
control packets.  These dispatchers don't currently have access to the
connection_to_host/client, so it's convenient to have it here.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:90: }
On 2011/03/04 12:09:38, Wez wrote:
> Why do we store whether the "stubs" are enabled/disabled in each stub, and
then
> enforce whether they accept messages in a different class?  That seems kinda
> complicated.
> Could we instead have a completed separate Dispatcher prior to authentication,
> and once authenticated switch to the main Dispatcher, so that there is a
> completely clear separation of code paths before and after authentication?

My initial approach was to do everything at the stub level, but it was suggested
that it would be better to:
(1) cut off processing as early as possible and
(2) have 1 central place to enable/disable, namely connection_to_host (or
client).

Ideally, I would have liked to cut off message decoding, but I can't do that for
the control channel.  For the input channel, if I defer creation of the input
stub (as you suggest elsewhere) then that would address that.

I'm not sure a separate dispatcher is needed since all it does is replace an
if-statement and it spreads the message handling across multiple files. But I'm
open to arguments (although I don't think it's worth doing for this CL).

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub...
remoting/protocol/host_stub.h:40: // I.e., should we be processing control
events?
On 2011/03/04 12:09:38, Wez wrote:
> As HostStub

Done.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/host_stub...
remoting/protocol/host_stub.h:47: 
On 2011/03/04 12:09:38, Wez wrote:
> As HostStub.

Done.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:33: 
On 2011/03/04 12:09:38, Wez wrote:
> Similar comments to ClientStub. :)

Done.

http://codereview.chromium.org/6594138/diff/12001/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:35: // I.e., are we processing input events?
On 2011/03/04 12:09:38, Wez wrote:
> As ClientStub.

Done.

[dmac, 2011-03-04 20:32:17]

LGTM with nits

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/host_stub.cc
File remoting/protocol/host_stub.cc (right):

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/host_stub....
remoting/protocol/host_stub.cc:15: : authenticated_(false) {
indent four? nit does this actually need to be wrapped?

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/input_stub.cc
File remoting/protocol/input_stub.cc (right):

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/input_stub...
remoting/protocol/input_stub.cc:15: : authenticated_(false) {
does it need wrap? should it be indented four?

[garykac, 2011-03-04 20:35:43]

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/host_stub.cc
File remoting/protocol/host_stub.cc (right):

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/host_stub....
remoting/protocol/host_stub.cc:15: : authenticated_(false) {
On 2011/03/04 20:32:17, dmac wrote:
> indent four? nit does this actually need to be wrapped?

Moved onto single line.

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/input_stub.cc
File remoting/protocol/input_stub.cc (right):

http://codereview.chromium.org/6594138/diff/9005/remoting/protocol/input_stub...
remoting/protocol/input_stub.cc:15: : authenticated_(false) {
On 2011/03/04 20:32:17, dmac wrote:
> does it need wrap? should it be indented four?

Moved onto single line.