Use Authenticator interface in Session and SessionManager

Use Authenticator interface in Session and SessionManager.

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

[Sergey Ulanov, 2011-11-24 00:39:46]

This is ready for review now.

[Wez, 2011-11-25 06:54:10]

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

http://codereview.chromium.org/8619011/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:98: void SetSharedSecret(const std::string&
shared_secret);
Will you be removing this in favour of having the application supply the
AuthenticatorFactory to use?

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/content_de...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/content_de...
remoting/protocol/content_description.cc:165: //      Authentication message
created by Authenticator implementation.
nit: The word "authentication" seems superfluous here.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:159: virtual const std::string&
initiator_token() OVERRIDE;
These next few lines all need removing, too?

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:177: std::string initiator_token_;
And these fields?

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.h:62: void Initiate();
nit: Initiate() -> SendSessionInit[iate](), or just Init()?

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.h:42: void set_allow_local_ips(bool
allow_local_ips);
Can we just remove this?  I don't think we have any use for it?

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:83: authenticator_->GetNextMessage()));
We should only call GetNextMessage() if the authenticator is in the right state,
I think?

[Sergey Ulanov, 2011-11-28 18:55:15]

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

http://codereview.chromium.org/8619011/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:98: void SetSharedSecret(const std::string&
shared_secret);
On 2011/11/25 06:54:11, Wez wrote:
> Will you be removing this in favour of having the application supply the
> AuthenticatorFactory to use?

Most probably yes.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/content_de...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/content_de...
remoting/protocol/content_description.cc:165: //      Authentication message
created by Authenticator implementation.
On 2011/11/25 06:54:11, Wez wrote:
> nit: The word "authentication" seems superfluous here.

Done.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:159: virtual const std::string&
initiator_token() OVERRIDE;
On 2011/11/25 06:54:11, Wez wrote:
> These next few lines all need removing, too?

Done.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:177: std::string initiator_token_;
On 2011/11/25 06:54:11, Wez wrote:
> And these fields?

Done.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.h:62: void Initiate();
On 2011/11/25 06:54:11, Wez wrote:
> nit: Initiate() -> SendSessionInit[iate](), or just Init()?

Done.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.h:42: void set_allow_local_ips(bool
allow_local_ips);
On 2011/11/25 06:54:11, Wez wrote:
> Can we just remove this?  I don't think we have any use for it?

Done.

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session.cc:83: authenticator_->GetNextMessage()));
On 2011/11/25 06:54:11, Wez wrote:
> We should only call GetNextMessage() if the authenticator is in the right
state,
> I think?
Authenticators for outgoing connections are expected to be in MESSAGE_READY
state when they are created. I've added DCHECK for this.

[Sergey Ulanov, 2011-11-28 21:36:16]

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.h (right):

http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.h:42: void set_allow_local_ips(bool
allow_local_ips);
On 2011/11/28 18:55:16, sergeyu wrote:
> On 2011/11/25 06:54:11, Wez wrote:
> > Can we just remove this?  I don't think we have any use for it?
> 
> Done.

Actually we do need this to allow connections from local host - mainly for
testing. I've returned it back.

[Wez, 2011-11-28 21:41:22]

On 2011/11/28 21:36:16, sergeyu wrote:
>
http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
> File remoting/protocol/jingle_session_manager.h (right):
> 
>
http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
> remoting/protocol/jingle_session_manager.h:42: void set_allow_local_ips(bool
> allow_local_ips);
> On 2011/11/28 18:55:16, sergeyu wrote:
> > On 2011/11/25 06:54:11, Wez wrote:
> > > Can we just remove this?  I don't think we have any use for it?
> > 
> > Done.
> 
> Actually we do need this to allow connections from local host - mainly for
> testing. I've returned it back.

Why do we not just always allow localhost candidates to be generated?

[Sergey Ulanov, 2011-11-28 21:57:16]

On 2011/11/28 21:41:22, Wez wrote:
> On 2011/11/28 21:36:16, sergeyu wrote:
> >
>
http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
> > File remoting/protocol/jingle_session_manager.h (right):
> > 
> >
>
http://codereview.chromium.org/8619011/diff/6001/remoting/protocol/jingle_ses...
> > remoting/protocol/jingle_session_manager.h:42: void set_allow_local_ips(bool
> > allow_local_ips);
> > On 2011/11/28 18:55:16, sergeyu wrote:
> > > On 2011/11/25 06:54:11, Wez wrote:
> > > > Can we just remove this?  I don't think we have any use for it?
> > > 
> > > Done.
> > 
> > Actually we do need this to allow connections from local host - mainly for
> > testing. I've returned it back.
> 
> Why do we not just always allow localhost candidates to be generated?

You are right, we don't need this method we always set allow_local_ips to true
anyway, and there is no reason not to allow local IPs. But it is not related to
this CL. I'll create a separate CL to remove this method.

[Wez, 2011-11-29 00:27:14]

lgtm

[commit-bot: I haz the power, 2011-11-29 21:50:17]

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

[commit-bot: I haz the power, 2011-11-29 21:50:19]

Can't process patch for file remoting/protocol/jingle_session.h.
File's status is None, patchset upload is incomplete.