Remove old Authentication code that we don't use or need.

Remove old Authentication code that we don't use or need.

Also removed some auth-related protobufs that we don't currenly use.

BUG=None
TEST=IT2Me still works.


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

[Wez, 2011-11-03 00:25:02]

http://codereview.chromium.org/8351084/diff/2001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.cc:239: case
protocol::ConnectionToHost::CONNECTED:
Is there any point in keeping this case?

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

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:523: client->get()));
nit: It seems a little strange that we go through
ProcessPreAuthentication->OnAuthorizationComplete->AddAuthenticatedClient, since
authentication and authorization mean different things.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:398: TEST_F(ChromotingHostTest,
CurtainModeFail) {
Assuming that we retain the idea of curtain mode, we'll need to test that it
doesn't get triggered if the EKE handshake fails, so perhaps we need to keep
this around?

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:413: .InSequence(s2);
How come these have changed?

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session.h:32: // Called to signal that local login has
succeeded and ChromotingHost can
We don't have local login any more, so what does this really mean?

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session.h:53: // Set the authenticated flag or log a
failure message as appropriate.
Out-of-date comment.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:40:
.WillRepeatedly(ReturnRef(default_screen_size_));
Is this fixing up an existing issue with the test, i.e. not CL-related?

http://codereview.chromium.org/8351084/diff/2001/remoting/proto/internal.proto
File remoting/proto/internal.proto (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/proto/internal.prot...
remoting/proto/internal.proto:21: optional BeginSessionResponse
begin_session_response = 4;
Best to mention in the CL description that this protobuf change affects a
message-type that we don't currently use.

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

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:25: // messages handlers here when necessary.
nit: use control -> use the control, messages -> message.

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

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:25: // messages handlers here when necessary.
typo nits as for ClientStub.

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/protocol_m...
File remoting/protocol/protocol_mock_objects.h (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/protocol_m...
remoting/protocol/protocol_mock_objects.h:86: const base::Closure& done));
Ah, this is paired with the EXPECT_CALL() change?

[Sergey Ulanov, 2011-11-03 01:21:12]

Addressed all comments. Also fixed backward-compatibility issue with older
clients.

http://codereview.chromium.org/8351084/diff/2001/remoting/client/plugin/peppe...
File remoting/client/plugin/pepper_view.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/client/plugin/peppe...
remoting/client/plugin/pepper_view.cc:239: case
protocol::ConnectionToHost::CONNECTED:
On 2011/11/03 00:25:02, Wez wrote:
> Is there any point in keeping this case?
Yes, to avoid compiler warnings, otherwise I would have to add default: case..

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

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:523: client->get()));
On 2011/11/03 00:25:02, Wez wrote:
> nit: It seems a little strange that we go through
> ProcessPreAuthentication->OnAuthorizationComplete->AddAuthenticatedClient,
since
> authentication and authorization mean different things.

The original intent was to call authorization the step to verify with directory
that this particular Gaia account is authorized to access the host - but it was
never implemented properly.
Renamed OnAuthorizationComplete to OnAuthenticationComplete.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:398: TEST_F(ChromotingHostTest,
CurtainModeFail) {
On 2011/11/03 00:25:02, Wez wrote:
> Assuming that we retain the idea of curtain mode, we'll need to test that it
> doesn't get triggered if the EKE handshake fails, so perhaps we need to keep
> this around?

Done.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:413: .InSequence(s2);
On 2011/11/03 00:25:02, Wez wrote:
> How come these have changed?
They haven't changed. I just added them to fix gunit warnings about
uninteresting calls.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session.h:32: // Called to signal that local login has
succeeded and ChromotingHost can
On 2011/11/03 00:25:02, Wez wrote:
> We don't have local login any more, so what does this really mean?
changed to authentication. I'm going to refactor this code more in the next CL.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session.h:53: // Set the authenticated flag or log a
failure message as appropriate.
On 2011/11/03 00:25:02, Wez wrote:
> Out-of-date comment.

Done.

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/8351084/diff/2001/remoting/host/client_session...
remoting/host/client_session_unittest.cc:40:
.WillRepeatedly(ReturnRef(default_screen_size_));
On 2011/11/03 00:25:02, Wez wrote:
> Is this fixing up an existing issue with the test, i.e. not CL-related?
yes, I cleaned up some minor warnings in the tests when when debugging tests for
this CL.

http://codereview.chromium.org/8351084/diff/2001/remoting/proto/internal.proto
File remoting/proto/internal.proto (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/proto/internal.prot...
remoting/proto/internal.proto:21: optional BeginSessionResponse
begin_session_response = 4;
On 2011/11/03 00:25:02, Wez wrote:
> Best to mention in the CL description that this protobuf change affects a
> message-type that we don't currently use.
Done. Also see the last version of this CL: I added BeginSessionResponse as it
is currently send by the host.

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

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/client_stu...
remoting/protocol/client_stub.h:25: // messages handlers here when necessary.
On 2011/11/03 00:25:02, Wez wrote:
> nit: use control -> use the control, messages -> message.

Done.

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

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/host_stub....
remoting/protocol/host_stub.h:25: // messages handlers here when necessary.
On 2011/11/03 00:25:02, Wez wrote:
> typo nits as for ClientStub.

Done.

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/protocol_m...
File remoting/protocol/protocol_mock_objects.h (left):

http://codereview.chromium.org/8351084/diff/2001/remoting/protocol/protocol_m...
remoting/protocol/protocol_mock_objects.h:86: const base::Closure& done));
On 2011/11/03 00:25:02, Wez wrote:
> Ah, this is paired with the EXPECT_CALL() change?
No. NotifyResolution was removed from ClientStub long time ago.

[Wez, 2011-11-03 18:14:36]

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

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:383: if (client->authenticated()) {
Why remove the conditional?

http://codereview.chromium.org/8351084/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:523: client->get()));
On 2011/11/03 01:21:12, sergeyu wrote:
> On 2011/11/03 00:25:02, Wez wrote:
> > nit: It seems a little strange that we go through
> > ProcessPreAuthentication->OnAuthorizationComplete->AddAuthenticatedClient,
> since
> > authentication and authorization mean different things.
> 
> The original intent was to call authorization the step to verify with
directory
> that this particular Gaia account is authorized to access the host - but it
was
> never implemented properly.
> Renamed OnAuthorizationComplete to OnAuthenticationComplete.

Strictly speaking, we should probably talk about authorized clients rather than
authenticated ones for the most part, since authorization implies authentication
and not vice versa, but that's a naming nit for a follow-up CL.

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

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:149: context_->main_message_loop()->PostTask(
In future will we put the EKE exchange before OnConnectionOpened even gets
invoked, or after?

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:150: FROM_HERE,
base::Bind(&ChromotingHost::ProcessPreAuthentication, this,
ProcessPreAuthentication really needs a better name; one for a follow-up CL,
though, I think.

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

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:103: // TODO(sergeyu): Move this to
ClientSession.
Surely OnConnectionOpened needs to remain on ChromotingHost?

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto
File remoting/proto/control.proto (right):

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:13: // BeginSessionDepricated is send from host to
client when session
typo: Depricated -> Deprecated
typo: send -> sent
typo: ...when _the_ session...

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:14: // starts. Legacy clients expect to receive
this message in the
typo: in -> at

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:15: // beginning of each session so the host sends
this message. Current
nit: lose "so the host..."?

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:16: // client ignores it.
typo: client -> clients, ignores -> ignore

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:19: // version (Chrome 17 is released).
Release of M17 doesn't guarantee that all clients are upgraded, though, so I
don't think we can necessarily do that.

http://codereview.chromium.org/8351084/diff/7002/remoting/protocol/connection...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/8351084/diff/7002/remoting/protocol/connection...
remoting/protocol/connection_to_host.h:53: // renamed AUTHENTICATED to
CONNECTED?
With the EKE handshake in place we'll have a separation between "connected" and
"authenticated" again - we'll be accepting the session, then exchanging
authentication stanzas before we hook up the channels, during which time we're
connected but not authenticated?

I think it would make sense to call the
connected-and-authenticated-and-authorized state "connected", and to either have
an "authenticating" state after "connecting", or even to fold that into
"connecting".

[Sergey Ulanov, 2011-11-03 19:11:54]

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

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:149: context_->main_message_loop()->PostTask(
On 2011/11/03 18:14:37, Wez wrote:
> In future will we put the EKE exchange before OnConnectionOpened even gets
> invoked, or after?
We will have OnSessionAuthenticated() in ClientSession::EventHandler which is
called by ClientSession after session is authenticated.

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:150: FROM_HERE,
base::Bind(&ChromotingHost::ProcessPreAuthentication, this,
On 2011/11/03 18:14:37, Wez wrote:
> ProcessPreAuthentication really needs a better name; one for a follow-up CL,
> though, I think.
I'm going to remove it in the next CL.

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

http://codereview.chromium.org/8351084/diff/7002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:103: // TODO(sergeyu): Move this to
ClientSession.
On 2011/11/03 18:14:37, Wez wrote:
> Surely OnConnectionOpened needs to remain on ChromotingHost?
No, why? New connections are accepted in OnIncomingSession()

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto
File remoting/proto/control.proto (right):

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:13: // BeginSessionDepricated is send from host to
client when session
On 2011/11/03 18:14:37, Wez wrote:
> typo: Depricated -> Deprecated
> typo: send -> sent
> typo: ...when _the_ session...

Done.

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:14: // starts. Legacy clients expect to receive
this message in the
On 2011/11/03 18:14:37, Wez wrote:
> typo: in -> at

Done.

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:15: // beginning of each session so the host sends
this message. Current
On 2011/11/03 18:14:37, Wez wrote:
> nit: lose "so the host..."?

Done.

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:16: // client ignores it.
On 2011/11/03 18:14:37, Wez wrote:
> typo: client -> clients, ignores -> ignore

Done.

http://codereview.chromium.org/8351084/diff/7002/remoting/protocol/connection...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/8351084/diff/7002/remoting/protocol/connection...
remoting/protocol/connection_to_host.h:53: // renamed AUTHENTICATED to
CONNECTED?
On 2011/11/03 18:14:37, Wez wrote:
> With the EKE handshake in place we'll have a separation between "connected"
and
> "authenticated" again - we'll be accepting the session, then exchanging
> authentication stanzas before we hook up the channels, during which time we're
> connected but not authenticated?
> 
> I think it would make sense to call the
> connected-and-authenticated-and-authorized state "connected", and to either
have
> an "authenticating" state after "connecting", or even to fold that into
> "connecting".

I agree.

[Wez, 2011-11-03 20:39:38]

lgtm

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto
File remoting/proto/control.proto (right):

http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
remoting/proto/control.proto:19: // version (Chrome 17 is released).
On 2011/11/03 18:14:37, Wez wrote:
> Release of M17 doesn't guarantee that all clients are upgraded, though, so I
> don't think we can necessarily do that.

I think we may have to wait to remove this until we move to NaCl, i.e. until we
control the plugin version in the web-app rather than the browser.

[Sergey Ulanov, 2011-11-04 22:01:23]

On 2011/11/03 20:39:38, Wez wrote:
> lgtm
> 
> http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto
> File remoting/proto/control.proto (right):
> 
>
http://codereview.chromium.org/8351084/diff/7002/remoting/proto/control.proto...
> remoting/proto/control.proto:19: // version (Chrome 17 is released).
> On 2011/11/03 18:14:37, Wez wrote:
> > Release of M17 doesn't guarantee that all clients are upgraded, though, so I
> > don't think we can necessarily do that.
> 
> I think we may have to wait to remove this until we move to NaCl, i.e. until
we
> control the plugin version in the web-app rather than the browser.

I think we can never guarantee that all clients are upgraded, even when we
release it as NaCl. Just removed reference to M17.

[commit-bot: I haz the power, 2011-11-04 22:01:59]

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

[commit-bot: I haz the power, 2011-11-04 22:55:39]

Try job failure for 8351084-22001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-04 22:58:43]

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

[commit-bot: I haz the power, 2011-11-04 23:47:28]

Try job failure for 8351084-26001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-04 23:51:42]

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

[commit-bot: I haz the power, 2011-11-05 01:19:32]

Change committed as 108759