Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12518027:
Protocol / client plugin changes to support interactively asking for a PIN (Closed)
|
Created:
7 years, 9 months ago by rmsousa Modified:
7 years, 9 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionProtocol / client plugin changes to support interactively asking for a PIN.
This has a few special cases, to be able to deal with all sorts of combinations of Me2Me/It2Me/old-plugin/old-webapp/old-host.
The idea is: A webapp that supports asking for PINs asynchronously will explicitly notify the plugin of that. Older webapps (or It2Me) will send the passphrase directly on connect.
The negotiating authenticator, instead of immediately trying to send the first SPAKE message, first sends a message with the supported methods to the host, and only when the host replies with the specific method it tries to create the authenticator. If there is a PinFetcher interface, it tries to use a PinClientAuthenticator (a thin layer on top of V2Authenticator that takes care of asynchronously asking for PIN), otherwise it uses V2Authenticator directly with the pre-provided pass phrase.
This also adds support for authenticators that can't be created in a particular state (e.g. ones for which the first message must go in one particular direction). The NegotiatingAuthenticator takes care of sending blank messages/ignoring those messages as appropriate.
BUG=115899
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190056
Patch Set 1 #
Total comments: 28
Patch Set 2 : Reviewer comments #Patch Set 3 : Missing remoting.gyp #Patch Set 4 : Changed Pin-related objects to callbacks #
Total comments: 39
Patch Set 5 : Pin -> Secret, comment clarifications. #
Total comments: 25
Patch Set 6 : Reviewer comments #
Total comments: 6
Patch Set 7 : Fix comments #
Messages
Total messages: 17 (0 generated)
See https://codereview.chromium.org/12867004/ for the associated WebApp changes. Please take a look at the NegotiatingAuthenticator changes, and the protocol summary I put in the comments for that class. I feel like it's somewhat overengineered right now, and that it could be simplified by restricting the protocol in some ways: For example: By making it explicitly so that the first message *from the negotiating authenticator* (i.e. the session-initiate) is from client to host. Right now it's always true but the code is not restricted to that, so there's a few cases that we need to deal with even though they never happen in practice (e.g. the client itself having to pick a method from a host-supplied list of methods). Furthermore, by making it required that (from now on) the first message from the client contains only the supported methods, and that it is always the host that picks a method based on those (that will never change in the course of the negotiation). This makes it more deterministic than the current model where the client may or may not pick a method and send something in the first message, that may or may not be discarded by the host. We still have to deal with those cases for the current SPAKE methods (because of older clients), but it'll make our lives easier to not have to deal with them for future methods. Also, by dictating that protocols that require user interaction must do so while processing a message (i.e. the first message of a protocol must not require user interaction). That is doable for SPAKE (we can always wait for the message from the host before asking for the PIN), and for the token authentication (we must wait for URLs from the host, etc), but might not be always true.
> This also adds support for authenticators that > can't be created in a particular state (e.g. ones > for which the first message must go in one > particular direction). The NegotiatingAuthenticator > takes care of sending blank messages/ignoring those > messages as appropriate. Why do you need this? Would it be better to fix low-level authenticators so it's possible to create them in any state (V2Authenticator already supports it)? https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo... remoting/client/plugin/chromoting_instance.cc:139: "onPinFetched useAsyncPinDialog"; Don't need two separate features https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo... remoting/client/plugin/chromoting_instance.cc:465: // TODO(rmsousa): Move pin fetcher code to a separate class. Isn't it already in a separate class? https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper... File remoting/client/plugin/pepper_pin_fetcher.h (right): https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper... remoting/client/plugin/pepper_pin_fetcher.h:25: private: nit: add empty line before private: https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper... remoting/client/plugin/pepper_pin_fetcher.h:26: base::WeakPtr<ChromotingInstance> parent_; parent_ is ambiguous. Call it plugin_ or plugin_instance https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper... remoting/client/plugin/pepper_pin_fetcher.h:29: DISALLOW_COPY_AND_ASSIGN(PepperPinFetcher); nit: empty line https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:247: if (pin_fetcher_factory_) { Do we really need to distinguish between these two cases here? Can ChromotingInstance just always pass the PIN fetcher - it would just implement it differently depending on how the PIN is fetched. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:251: pin_fetcher_factory_->CreatePinFetcher(), preferred_initial_state, Why do we need the factory interface? Is there any reason you would need multiple fetchers? All fetchers are the same, so you don't need more than one instance, hence no need to have the factory. Am I missing something? https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:259: authentication_tag_, shared_secret_), preferred_initial_state); nit: move the last parameter to a separate line for readability. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_... File remoting/protocol/pin_client_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_... remoting/protocol/pin_client_authenticator.cc:37: DCHECK(underlying_); What if this method is called before PIN is fetched? https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_... File remoting/protocol/pin_client_authenticator.h (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_... remoting/protocol/pin_client_authenticator.h:21: class PinClientAuthenticator : public Authenticator { Please add a comment to explain what this is for. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher.h File remoting/protocol/pin_fetcher.h (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher... remoting/protocol/pin_fetcher.h:13: class PinFetcher { Do we need this interface at all? There is only one method, so you can just use base::Callback<void(const PinFetchedCallback& pin_fetched_callback)>
> This also adds support for authenticators that
> can't be created in a particular state (e.g. ones
> for which the first message must go in one
> particular direction). The NegotiatingAuthenticator
> takes care of sending blank messages/ignoring those
> messages as appropriate.
Why do you need this? Would it be better to fix low-level authenticators so it's
possible to create them in any state (V2Authenticator already supports it)?
That's easy for V2 because it's symmetrical. For other protocols, the first
(useful) message must go in a particular direction (e.g. for third-party-auth it
always goes from host to client). To start in the opposite direction, we
basically need to send a blank message that the other side will then ignore.
That can either be done in the low-level authenticator implementation (as in the
pending version of the third-party-auth protocol CL), or be done by the
negotiating authenticator (as Wez suggested in that review thread). In either
case, it's just a couple "if (wrong_state) { send/drop blank message }" blocks.
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo...
File remoting/client/plugin/chromoting_instance.cc (right):
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:139: "onPinFetched
useAsyncPinDialog";
On 2013/03/17 21:29:21, sergeyu wrote:
> Don't need two separate features
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/chromo...
remoting/client/plugin/chromoting_instance.cc:465: // TODO(rmsousa): Move pin
fetcher code to a separate class.
On 2013/03/17 21:29:21, sergeyu wrote:
> Isn't it already in a separate class?
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper...
File remoting/client/plugin/pepper_pin_fetcher.h (right):
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper...
remoting/client/plugin/pepper_pin_fetcher.h:25: private:
On 2013/03/17 21:29:21, sergeyu wrote:
> nit: add empty line before private:
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper...
remoting/client/plugin/pepper_pin_fetcher.h:26:
base::WeakPtr<ChromotingInstance> parent_;
On 2013/03/17 21:29:21, sergeyu wrote:
> parent_ is ambiguous. Call it plugin_ or plugin_instance
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/client/plugin/pepper...
remoting/client/plugin/pepper_pin_fetcher.h:29:
DISALLOW_COPY_AND_ASSIGN(PepperPinFetcher);
On 2013/03/17 21:29:21, sergeyu wrote:
> nit: empty line
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating...
File remoting/protocol/negotiating_authenticator.cc (right):
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:247: if (pin_fetcher_factory_) {
On 2013/03/17 21:29:21, sergeyu wrote:
> Do we really need to distinguish between these two cases here? Can
> ChromotingInstance just always pass the PIN fetcher - it would just implement
it
> differently depending on how the PIN is fetched.
I think that would get ugly in the upper layers, when we have to deal with older
clients. (But I'm open to doing it that way)
An older client (or It2Me) would just call connect() with the sharedSecret in
the parameters. To make a separate pin_fetcher_ implementation we'd need to, in
the plugin connect() call, check that asyncPinFetching is not enabled, and if
it's not, save the sharedSecret, so that a later FetchPin would return it.
ChromotingInstance has no "connection" object, so that data would live
"globally" in the ChromotingInstance object. But then we need to clean up that
data at the right times, so that we don't accidentally use it.
Finally, Pinfetcher is kind of a transition object. It may eventually evolve to
something that allows you to pair a host (and name that pairing), etc, etc. None
of that will make sense for IT2Me. I feel it's cleaner to handle IT2Me like it
really is - a mode where the passphrase is given at connect time, and that's it.
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:251:
pin_fetcher_factory_->CreatePinFetcher(), preferred_initial_state,
On 2013/03/17 21:29:21, sergeyu wrote:
> Why do we need the factory interface? Is there any reason you would need
> multiple fetchers? All fetchers are the same, so you don't need more than one
> instance, hence no need to have the factory. Am I missing something?
It's to manage the lifetime for the callback appropriately - If PinFetcher is a
shared instance pointer, I would need to make sure that it won't call the
callback after the authenticator is deleted (e.g. by making
PinClientAuthenticator a weakptr, or adding an explicit Cancel call). I also
need to make sure that if we ever switch between two PinClientAuthenticators
instances, there's no garbage left in the PinFetcher between the two. Putting
all* state in a scoped_ptr<PinFetcher> makes this simpler - it's owned by
PinClientAuthenticator, so everything will be cleaned up if it's destroyed.
* Not exactly all state - there's still an assumption in ChromotingInstance that
there's only one live PinFetcher, but if we needed to change that it's just a
matter of assinging PinFetcher ids, and changign from a pointer to a map.
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating...
remoting/protocol/negotiating_authenticator.cc:259: authentication_tag_,
shared_secret_), preferred_initial_state);
On 2013/03/17 21:29:21, sergeyu wrote:
> nit: move the last parameter to a separate line for readability.
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_...
File remoting/protocol/pin_client_authenticator.cc (right):
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_...
remoting/protocol/pin_client_authenticator.cc:37: DCHECK(underlying_);
On 2013/03/17 21:29:21, sergeyu wrote:
> What if this method is called before PIN is fetched?
That's illegal (hence the DCHECK).
NegotatingAuthenticator will be in a PROCESSING_MESSAGE state. In that state,
JingleSession will reject any incoming messages. Also, the call to
GetNextMessage() is inside the resume_callback, so it'll also only be called
when the PIN is received. Those are the only points in which the
NegotiatingAuthenticator checks the underlying state().
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_...
File remoting/protocol/pin_client_authenticator.h (right):
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_client_...
remoting/protocol/pin_client_authenticator.h:21: class PinClientAuthenticator :
public Authenticator {
On 2013/03/17 21:29:21, sergeyu wrote:
> Please add a comment to explain what this is for.
Done.
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher.h
File remoting/protocol/pin_fetcher.h (right):
https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher...
remoting/protocol/pin_fetcher.h:13: class PinFetcher {
On 2013/03/17 21:29:21, sergeyu wrote:
> Do we need this interface at all? There is only one method, so you can just
use
> base::Callback<void(const PinFetchedCallback& pin_fetched_callback)>
Kinda... there's also an implicit Cancel() (in the destructor). And we may also
want to add some more stuff here (like pin dialog options, etc). I think it's
worth having a low-level object corresponding to the webapp PIN dialog.
I'm still not convinced we need the new interfaces. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:247: if (pin_fetcher_factory_) { On 2013/03/18 21:07:26, rmsousa wrote: > On 2013/03/17 21:29:21, sergeyu wrote: > > Do we really need to distinguish between these two cases here? Can > > ChromotingInstance just always pass the PIN fetcher - it would just implement > it > > differently depending on how the PIN is fetched. > > I think that would get ugly in the upper layers, when we have to deal with older > clients. (But I'm open to doing it that way) > > An older client (or It2Me) would just call connect() with the sharedSecret in > the parameters. To make a separate pin_fetcher_ implementation we'd need to, in > the plugin connect() call, check that asyncPinFetching is not enabled, and if > it's not, save the sharedSecret, so that a later FetchPin would return it. > ChromotingInstance has no "connection" object, so that data would live > "globally" in the ChromotingInstance object. But then we need to clean up that > data at the right times, so that we don't accidentally use it. > > Finally, Pinfetcher is kind of a transition object. It may eventually evolve to > something that allows you to pair a host (and name that pairing), etc, etc. None > of that will make sense for IT2Me. I feel it's cleaner to handle IT2Me like it > really is - a mode where the passphrase is given at connect time, and that's it. I think this is just a question of whether we should put the logic of handling differences between different clients. We could either put it here or in the plugin layer. I think it belongs to the plugin. It will be possible to remove it later once all clients support the feature. Putting that logic here makes it harder to do. In general any nullable parameters add complexity (at least when reading the code) and it's better to avoid when possible. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:251: pin_fetcher_factory_->CreatePinFetcher(), preferred_initial_state, On 2013/03/18 21:07:26, rmsousa wrote: > On 2013/03/17 21:29:21, sergeyu wrote: > > Why do we need the factory interface? Is there any reason you would need > > multiple fetchers? All fetchers are the same, so you don't need more than one > > instance, hence no need to have the factory. Am I missing something? > > It's to manage the lifetime for the callback appropriately - If PinFetcher is a > shared instance pointer, I would need to make sure that it won't call the > callback after the authenticator is deleted (e.g. by making > PinClientAuthenticator a weakptr, or adding an explicit Cancel call). I also > need to make sure that if we ever switch between two PinClientAuthenticators > instances, there's no garbage left in the PinFetcher between the two. Putting > all* state in a scoped_ptr<PinFetcher> makes this simpler - it's owned by > PinClientAuthenticator, so everything will be cleaned up if it's destroyed. > The issue with possible lifetime mismatch is easy to solve. You could just use ref-counted callback target or weak pointers. > * Not exactly all state - there's still an assumption in ChromotingInstance that > there's only one live PinFetcher, but if we needed to change that it's just a > matter of assinging PinFetcher ids, and changign from a pointer to a map. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher.h File remoting/protocol/pin_fetcher.h (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher... remoting/protocol/pin_fetcher.h:13: class PinFetcher { On 2013/03/18 21:07:26, rmsousa wrote: > On 2013/03/17 21:29:21, sergeyu wrote: > > Do we need this interface at all? There is only one method, so you can just > use > > base::Callback<void(const PinFetchedCallback& pin_fetched_callback)> > > Kinda... there's also an implicit Cancel() (in the destructor). And we may also > want to add some more stuff here (like pin dialog options, etc). I think it's > worth having a low-level object corresponding to the webapp PIN dialog. But what does the cancel do? Would it hide the PIN prompt (I didn't see anything in the plugin interface for that). If it doesn't hide PIN prompt, then I don't think we need to worry about cancellation (again you could use weak pointers for to make sure we safely ignore pin_fetcher_callback when the handler is destroyed.)
I removed the PinFetcher* classes, and moved everything to callbacks. At this point, PinClientAuthenticator became so trivial that having it as a separate object was just adding unnecessary complication (because of the asynchronous construction), so I just put that logic directly in NegotiatingAuthenticator. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:247: if (pin_fetcher_factory_) { On 2013/03/19 02:44:53, sergeyu wrote: > On 2013/03/18 21:07:26, rmsousa wrote: > > On 2013/03/17 21:29:21, sergeyu wrote: > > > Do we really need to distinguish between these two cases here? Can > > > ChromotingInstance just always pass the PIN fetcher - it would just > implement > > it > > > differently depending on how the PIN is fetched. > > > > I think that would get ugly in the upper layers, when we have to deal with > older > > clients. (But I'm open to doing it that way) > > > > An older client (or It2Me) would just call connect() with the sharedSecret in > > the parameters. To make a separate pin_fetcher_ implementation we'd need to, > in > > the plugin connect() call, check that asyncPinFetching is not enabled, and if > > it's not, save the sharedSecret, so that a later FetchPin would return it. > > ChromotingInstance has no "connection" object, so that data would live > > "globally" in the ChromotingInstance object. But then we need to clean up that > > data at the right times, so that we don't accidentally use it. > > > > Finally, Pinfetcher is kind of a transition object. It may eventually evolve > to > > something that allows you to pair a host (and name that pairing), etc, etc. > None > > of that will make sense for IT2Me. I feel it's cleaner to handle IT2Me like it > > really is - a mode where the passphrase is given at connect time, and that's > it. > > > I think this is just a question of whether we should put the logic of handling > differences between different clients. We could either put it here or in the > plugin layer. I think it belongs to the plugin. It will be possible to remove it > later once all clients support the feature. Putting that logic here makes it > harder to do. In general any nullable parameters add complexity (at least when > reading the code) and it's better to avoid when possible. > Done. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/negotiating... remoting/protocol/negotiating_authenticator.cc:251: pin_fetcher_factory_->CreatePinFetcher(), preferred_initial_state, On 2013/03/19 02:44:53, sergeyu wrote: > On 2013/03/18 21:07:26, rmsousa wrote: > > On 2013/03/17 21:29:21, sergeyu wrote: > > > Why do we need the factory interface? Is there any reason you would need > > > multiple fetchers? All fetchers are the same, so you don't need more than > one > > > instance, hence no need to have the factory. Am I missing something? > > > > It's to manage the lifetime for the callback appropriately - If PinFetcher is > a > > shared instance pointer, I would need to make sure that it won't call the > > callback after the authenticator is deleted (e.g. by making > > PinClientAuthenticator a weakptr, or adding an explicit Cancel call). I also > > need to make sure that if we ever switch between two PinClientAuthenticators > > instances, there's no garbage left in the PinFetcher between the two. Putting > > all* state in a scoped_ptr<PinFetcher> makes this simpler - it's owned by > > PinClientAuthenticator, so everything will be cleaned up if it's destroyed. > > > > The issue with possible lifetime mismatch is easy to solve. You could just use > ref-counted callback target or weak pointers. > > > * Not exactly all state - there's still an assumption in ChromotingInstance > that > > there's only one live PinFetcher, but if we needed to change that it's just a > > matter of assinging PinFetcher ids, and changign from a pointer to a map. > Done. https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher.h File remoting/protocol/pin_fetcher.h (right): https://codereview.chromium.org/12518027/diff/1/remoting/protocol/pin_fetcher... remoting/protocol/pin_fetcher.h:13: class PinFetcher { On 2013/03/19 02:44:53, sergeyu wrote: > On 2013/03/18 21:07:26, rmsousa wrote: > > On 2013/03/17 21:29:21, sergeyu wrote: > > > Do we need this interface at all? There is only one method, so you can just > > use > > > base::Callback<void(const PinFetchedCallback& pin_fetched_callback)> > > > > Kinda... there's also an implicit Cancel() (in the destructor). And we may > also > > want to add some more stuff here (like pin dialog options, etc). I think it's > > worth having a low-level object corresponding to the webapp PIN dialog. > > But what does the cancel do? Would it hide the PIN prompt (I didn't see anything > in the plugin interface for that). If it doesn't hide PIN prompt, then I don't > think we need to worry about cancellation (again you could use weak pointers for > to make sure we safely ignore pin_fetcher_callback when the handler is > destroyed.) Done.
Some initial comments. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:247: weak_factory_.GetWeakPtr(), preferred_initial_state, resume_callback)); This is a strange way to arrange things. Why can't we create the V2Authenticator synchronously and pass it the PIN-fetcher callback to use? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:259: resume_callback.Run(); Why does this need a |resume_callback|? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.h (right): https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:24: typedef base::Callback<void(const std::string& pin)> PinFetchedCallback; nit: SecretFetchedCallback - PIN is specific to Me2Me. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:26: FetchPinCallback; nit: Ditto here. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:33: // (clients may also pick a method and send its first message here). nit: also -> additionally https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:35: // (if a supported method/message was sent by a client, it is processed). nit: second "a" -> "the" https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:41: // The details: This entire block is rather verbose. It looks like it could be simplified considerably. Key points seem to be: * Some Autheticators start with a client->host message, others with host->client. * Some are symmetric, and can start in either direction. * Only sequential exchanges are supported (i.e. no simultaneous send/receive). * When an Authenticator is created it is given a hint as to whether client or host initiated is preferred, which it may ignore. * NegotiatingAuthenticator can use this to optimistically initiate a chosen Authenticator during the negotiation handshake. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:44: // resume processing when done. This seems to be an inherent property of the Authenticator interface. Why are you mentioning it here? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:47: // receives a callback. If the authenticator has received a message to What receives a callback? The Create*() function? Or something else? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:48: // process, the message processing code must also be on that callback. Not sure what "... must also be on that callback" means here? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:53: // initial state, but it may ignore it, and the negotiating authenticator nit: "but it may ignore it" -> "which the authenticator may ignore" https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:55: // to send, and ignoring such blank message on the receiving end. This What does it mean to send a "blank message"? You mean we actually add an extra empty stanza, or that we just don't include the stanza at all? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:57: // both ends, or name an explicit direction to start. This sentence is confusing and I don't think you need it - it wouldn't make sense for an authentication method to support both starting directions but for the peer not to. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:63: // the host picks a method from the client's list, it's final. Why do we not return to the handshake and let the host choose another method if it wants? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:65: // mix of webapp, client plugin and host, for both Me2Me and IT2Me. This is a good point, although it's not "any" version mix, just the currently-supported protocols. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:99: // |resume_callback| is called when the authenticator is ready. Where does the Authenticator end up? |current_authenticator_|? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:103: // Processes a message in the current authenticator. |message| and nit: Suggest "Calls |current_authenticator_| to process |message|, passing the supplied |resume_callback|." https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:108: // Updates |state_| to reflect the current underlying authenticator state. Why does this need a |resume_callback|? What is it looking at that "reflects the state"? Is it just updating |state_| based on |current_authenticator_->state()|? https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:113: void OnPinFetched( Why is this called OnPinFetched if it actually creates a V2 authenticator?
https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:247: weak_factory_.GetWeakPtr(), preferred_initial_state, resume_callback)); On 2013/03/20 01:36:31, Wez wrote: > This is a strange way to arrange things. > > Why can't we create the V2Authenticator synchronously and pass it the > PIN-fetcher callback to use? The V2Authenticator is unusable until it has a secret to use. We need to do something asynchronous somewhere, and only when it finishes can we continue processing the message. Our options are: * Put the asynchronous call inside the authenticator constructor. This is how it was organized in the previous patchset (see pin_client_authenticator). The problem with that is that if the pin fetch calls resume_callback synchronously, resume_callback will run before current_authenticator_ is assigned (aside from that, doing that kind of work in the constructor is kind of icky). * Make the constructor return an unusable/uninitialized object, then use a separate Init() method to start the asynchronous pin fetch. I started with these approaches, but then I realized that PinClientAuthenticator's only purpose was to exist in an unusable state until the pin fetch finished, and the rest of its code after that was done was mostly proxying stuff to v2authenticator, so I decided to remove all this cruft and just fetch the pin and directly construct a v2authenticator. It kinda breaks encapsulation that negotiating_authenticator knows how to fetch the secret, but this object already knew low-level stuff like how to hash the secret, as well as all of the v2authenticator-specific parameters, so we might as well fetch the secret here too. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:259: resume_callback.Run(); On 2013/03/20 01:36:31, Wez wrote: > Why does this need a |resume_callback|? All of this is happening while the negotiatingauthenticator is processing a message. The full sequence is: 1) JingleSession calls ProcessMessage with the message received from the peer, and a callback to be called when processing is done. 2) NegotiatingAuthenticator selects the method specified in the message, and starts creating the appropriate authenticator. (... async steps ...) 3) Once the authenticator is ready, it finally uses it to process the message received from the host. (... possibly more async steps ...) 4) once underlying message processing is finished, this object's state is updated to reflect the underlying state. 5) We call the original resume_callback received from the JingleSession ProcessMessage call. The resume_callback here encapsulates steps 3, 4, 5. (or 4, 5, depending on which state the authenticator was created on) https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.h (right): https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:24: typedef base::Callback<void(const std::string& pin)> PinFetchedCallback; On 2013/03/20 01:36:31, Wez wrote: > nit: SecretFetchedCallback - PIN is specific to Me2Me. Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:26: FetchPinCallback; On 2013/03/20 01:36:31, Wez wrote: > nit: Ditto here. Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:33: // (clients may also pick a method and send its first message here). On 2013/03/20 01:36:31, Wez wrote: > nit: also -> additionally Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:35: // (if a supported method/message was sent by a client, it is processed). On 2013/03/20 01:36:31, Wez wrote: > nit: second "a" -> "the" Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:35: // (if a supported method/message was sent by a client, it is processed). On 2013/03/20 01:36:31, Wez wrote: > nit: second "a" -> "the" Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:41: // The details: On 2013/03/20 01:36:31, Wez wrote: > This entire block is rather verbose. It looks like it could be simplified > considerably. Key points seem to be: These only describe the 3rd and 4th bulletpoint. > * When an Authenticator is created it is given a hint as to whether client or > host initiated is preferred, which it may ignore. > * NegotiatingAuthenticator can use this to optimistically initiate a chosen > Authenticator during the negotiation handshake. These are not really related. Some authenticators can start in any state, and some authenticators can be initiated during the handshake, but these are orthogonal properties. The PIN authenticator can be created in any state, but we don't want to ask the user for a PIN until we know we need to. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:44: // resume processing when done. On 2013/03/20 01:36:31, Wez wrote: > This seems to be an inherent property of the Authenticator interface. Why are > you mentioning it here? This text is describing the authentication negotiation protocol as a whole, not just this object. That property is relevant because this object must deal appropriately with being the middle-man in that situation (i.e. it must keep its own state consistent when that happens). https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:47: // receives a callback. If the authenticator has received a message to On 2013/03/20 01:36:31, Wez wrote: > What receives a callback? The Create*() function? Or something else? CreateAuthenticator. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:48: // process, the message processing code must also be on that callback. On 2013/03/20 01:36:31, Wez wrote: > Not sure what "... must also be on that callback" means here? If the authenticator is creating an authenticator to process a message, the callback passed to CreateAuthenticator must include a call to ProcessMessage in the newly created authenticator. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:53: // initial state, but it may ignore it, and the negotiating authenticator On 2013/03/20 01:36:31, Wez wrote: > nit: "but it may ignore it" -> "which the authenticator may ignore" Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:55: // to send, and ignoring such blank message on the receiving end. This On 2013/03/20 01:36:31, Wez wrote: > What does it mean to send a "blank message"? You mean we actually add an extra > empty stanza, or that we just don't include the stanza at all? An <authenticator> stanza is part of the protocol, and must be always be included. If there is no message to be sent, that stanza is blank. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:57: // both ends, or name an explicit direction to start. On 2013/03/20 01:36:31, Wez wrote: > This sentence is confusing and I don't think you need it - it wouldn't make > sense for an authentication method to support both starting directions but for > the peer not to. Not quite... The V2 authentication is a good example - now that we only want to ask the client for a pin if we need to, it would have been convenient to declare that the client side can only start in the WAITING_MESSAGE state (i.e. we only ask for PIN after we receive the first SPAKE message). At the same time, the host can still start in any state, since it doesn't need to prompt the user. If we did that, however, it would break the blank message mechanism (the host side would try to process that blank message). https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:63: // the host picks a method from the client's list, it's final. On 2013/03/20 01:36:31, Wez wrote: > Why do we not return to the handshake and let the host choose another method if > it wants? I'm not sure what you mean here. This is just describing the existing protocol (both in this change, and in the previous code). The sequence is: 1) Client sends host supported-methods="A,B,C" (and maybe a message with method="A") 2) Host picks one method, and sends either a reply with method="A", or a new message with method="B". 3) Client continues with the method the host selected (and never sending the supported-methods again). I suppose we could, in a future change, let the client decide it doesn't really want the method that the host picked, and try again with, say, supported-methods="C" method="C". There are specific cases that won't work with older host code, but whoever implements it can work around that. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:65: // mix of webapp, client plugin and host, for both Me2Me and IT2Me. On 2013/03/20 01:36:31, Wez wrote: > This is a good point, although it's not "any" version mix, just the > currently-supported protocols. What do you mean currently-supported protocols? This class describes a single ("meta-")protocol. By any version mix I mean that any combination of old/new host, old/new plugin, old/new webapp must work, both for IT2Me and Me2Me. Any change to this class must be verified to work in all 16 combinations of current and previous code (although, hopefully, the (old, old, old, *) combinations can be assumed to work...) https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:99: // |resume_callback| is called when the authenticator is ready. On 2013/03/20 01:36:31, Wez wrote: > Where does the Authenticator end up? |current_authenticator_|? Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:103: // Processes a message in the current authenticator. |message| and On 2013/03/20 01:36:31, Wez wrote: > nit: Suggest "Calls |current_authenticator_| to process |message|, passing the > supplied |resume_callback|." Done. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:108: // Updates |state_| to reflect the current underlying authenticator state. On 2013/03/20 01:36:31, Wez wrote: > Why does this need a |resume_callback|? What is it looking at that "reflects the > state"? Is it just updating |state_| based on > |current_authenticator_->state()|? Yes, it's updating the negotiating_authenticator's state and rejection_reason to match the ones in which the current_authenticator ended up after ProcessMessage finished. It's receiving a resume_callback because it's in the middle of a stack of callbacks. The Session will give this object a resume_callback on its ProcessMessage. This object will call the underlying ProcessMessage, and after that finishes, it must update its state, then call the original resume_callback received from the Session. So UpdateState is used as the underlying's resume_callback, and wraps around the original resume_callback received from the Session. The same applies for when this is used as a callback from CreateAuthenticator - CreateAuthentciator is called from inside this object's ProcessMessage - once CreateAuthenticator finishes, it must update the state and call the originally received resume_callback. https://codereview.chromium.org/12518027/diff/18001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:113: void OnPinFetched( On 2013/03/20 01:36:31, Wez wrote: > Why is this called OnPinFetched if it actually creates a V2 authenticator? Done. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:142: void OnPinFetched(const std::string& pin); OnPinFetched and use_async_pin_dialog_ below are specifically related to the PIN dialog (and not any secret), so I kept the PIN nomenclature.
https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:697: VLOG(1) << nit: operator << should be wrapped when used for IO. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:137: void FetchSecretFromDialog( this and the following method do not need to be public. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:139: static void FetchSecretFromString( nit: static methods should be declared above non-static ones. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:142: void OnPinFetched(const std::string& pin); On 2013/03/20 04:54:54, rmsousa wrote: > OnPinFetched and use_async_pin_dialog_ below are specifically related to the PIN > dialog (and not any secret), so I kept the PIN nomenclature. sgtm https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:148: } else { nit: add return above and then you won't need "else" Similarly you can avoid the else case below. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:163: void NegotiatingAuthenticator::ProcessMessageInternal( Move this below so order of functions here matches order in .h file. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:212: DCHECK_NE(state_, REJECTED); here state can be either WAITING_MESSAGE or ACCEPTED. It's better to use positive filter here. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:235: nit: remove empty line https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:243: resume_callback.Run(); Do you need to set state_ here to something other than PROCESSING_MESSAGE? JingleSession::ProcessAuthenticationStep() has a DCHECK for it. I wonder why unittests didn't catch it. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:259: resume_callback.Run(); Set state_ to MESSAGE_READY? https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.h (right): https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:28: // This class provides a meta-authenticator that allows clients and hosts that Thanks for adding these comments! https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:42: // * CreateAuthenticator may be asynchronous (i.e. require user interaction nit: CreateAuthenticator(), and similarly add () for other methods in this comment
https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:697: VLOG(1) << On 2013/03/20 05:49:05, sergeyu wrote: > nit: operator << should be wrapped when used for IO. Done. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:137: void FetchSecretFromDialog( On 2013/03/20 05:49:05, sergeyu wrote: > this and the following method do not need to be public. Actually none of the methods in this block do - they're all called from this class's own message processing. Regardless of that, FetchSecretFrom* still don't belong in this group, so I moved them. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:139: static void FetchSecretFromString( On 2013/03/20 05:49:05, sergeyu wrote: > nit: static methods should be declared above non-static ones. Done. https://codereview.chromium.org/12518027/diff/30002/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:142: void OnPinFetched(const std::string& pin); On 2013/03/20 05:49:05, sergeyu wrote: > On 2013/03/20 04:54:54, rmsousa wrote: > > OnPinFetched and use_async_pin_dialog_ below are specifically related to the > PIN > > dialog (and not any secret), so I kept the PIN nomenclature. > > sgtm Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.cc (right): https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:148: } else { On 2013/03/20 05:49:05, sergeyu wrote: > nit: add return above and then you won't need "else" > > Similarly you can avoid the else case below. Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:163: void NegotiatingAuthenticator::ProcessMessageInternal( On 2013/03/20 05:49:05, sergeyu wrote: > Move this below so order of functions here matches order in .h file. Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:212: DCHECK_NE(state_, REJECTED); On 2013/03/20 05:49:05, sergeyu wrote: > here state can be either WAITING_MESSAGE or ACCEPTED. It's better to use > positive filter here. Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:235: On 2013/03/20 05:49:05, sergeyu wrote: > nit: remove empty line Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:243: resume_callback.Run(); On 2013/03/20 05:49:05, sergeyu wrote: > Do you need to set state_ here to something other than PROCESSING_MESSAGE? > JingleSession::ProcessAuthenticationStep() has a DCHECK for it. I wonder why > unittests didn't catch it. The callbacks passed in here all call UpdateState (either directly or through ProcessMessageInternal). It's only at that time (*after* all async processing is done) that it's safe to change the state from PROCESSING_MESSAGE I don't think we have unittests for the Session -> NegotiatingAuthenticator integration. NegotiatingAuthenticator tests run the flow manually (AuthenticatorTestBase), and Session tests use FakeAuthenticator https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.cc:259: resume_callback.Run(); On 2013/03/20 05:49:05, sergeyu wrote: > Set state_ to MESSAGE_READY? Same applies, there's an UpdateState in the callback https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator.h (right): https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:28: // This class provides a meta-authenticator that allows clients and hosts that On 2013/03/20 05:49:05, sergeyu wrote: > Thanks for adding these comments! Done. https://codereview.chromium.org/12518027/diff/30002/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator.h:42: // * CreateAuthenticator may be asynchronous (i.e. require user interaction On 2013/03/20 05:49:05, sergeyu wrote: > nit: CreateAuthenticator(), and similarly add () for other methods in this > comment Done.
LGTM. Thanks! https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:288: LOG(ERROR) << "Invalid connect() data."; nit: "sharedSecret no specified in connect()." https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:460: void ChromotingInstance::FetchSecretFromString( nit: // static https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:153: // Immediately calls |secret_fetched_callback| with |shared_secret| Explain what this is used for. Also need period at the end of the sentence.
https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:288: LOG(ERROR) << "Invalid connect() data."; On 2013/03/20 20:33:40, sergeyu wrote: > nit: "sharedSecret no specified in connect()." Done. https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.cc:460: void ChromotingInstance::FetchSecretFromString( On 2013/03/20 20:33:40, sergeyu wrote: > nit: // static Done. https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/12518027/diff/35001/remoting/client/plugin/ch... remoting/client/plugin/chromoting_instance.h:153: // Immediately calls |secret_fetched_callback| with |shared_secret| On 2013/03/20 20:33:40, sergeyu wrote: > Explain what this is used for. Also need period at the end of the sentence. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12518027/40001
Retried try job too often on win_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12518027/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12518027/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12518027/40001
Message was sent while issue was closed.
Change committed as 190056 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
