Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492643003:
[Blimp Net] Add EngineAuthHandler. (Closed)
|
Created:
5 years ago by haibinlu Modified:
5 years ago CC:
cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, maniscalco, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, Sriram Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blimp Net] Add EngineAuthHandler.
It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler.
BUG=567817
Committed: https://crrev.com/9fa0095471e61a1de43d902974567355585157fe
Cr-Commit-Position: refs/heads/master@{#363880}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 32
Patch Set 4 : Addresses Kevin's comments. #
Total comments: 14
Patch Set 5 : #
Total comments: 32
Patch Set 6 : Addresses Wez's comments #
Total comments: 7
Patch Set 7 : Addresses comments and adds unit tests #
Total comments: 32
Patch Set 8 : Adds a test case for failed authentication due to timeout #Patch Set 9 : Addresses wez's comments #
Total comments: 8
Patch Set 10 : rebase #
Dependent Patchsets: Messages
Total messages: 36 (13 generated)
Description was changed from ========== [Blimp Net] Add EngineAuthHandler. BUG= ========== to ========== [Blimp Net] Add EngineAuthHandler. It authenticates one connection only and hand it over to underlying connection_handler once it is authenticated. It manages its own lifetime, and deletes itself under one of the conditions below: * the connection is authenticated. * the connection gets into error state. * timeout on waiting for START_CONNECTION message. BUG= ==========
haibinlu@chromium.org changed reviewers: + kmarshall@chromium.org, wez@chromium.org
not ready for review - just want design input
https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/blimp_me... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/blimp_me... blimp/common/proto/blimp_message.proto:68: optional StartConnectionMessage start_connection = 1005; Let's either move these into a general "protocol control" message namespace as discussed offline. https://codereview.chromium.org/1492643003/diff/1/blimp/net/engine_auth_handl... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/1/blimp/net/engine_auth_handl... blimp/net/engine_auth_handler.h:22: // Authenticates one connection and hand it over to |connection_handler| Not sure that this makes sense; it's possible for multiple remote parties to try to connect to the engine at once, so even though only one of them will succeed in connecting, they all need to have authentication processed for them. Basically I'd expect the EngineAuthHandler to take the supplied BlimpConnection and to create a per-connection BlimpMessageProcessor for it, that will receive an authentication message from it and either close it or hand it off to the |connection_handler|. Make sense?
https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... File blimp/common/proto/start_connection.proto (right): https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... blimp/common/proto/start_connection.proto:13: optional string auth_token = 1; Elsewhere we've called this client_token. I suggest using that name here for consistency. https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... blimp/common/proto/start_connection.proto:14: optional int32 protocol_version = 2; Elsewhere this is defined as a string.
Description was changed from ========== [Blimp Net] Add EngineAuthHandler. It authenticates one connection only and hand it over to underlying connection_handler once it is authenticated. It manages its own lifetime, and deletes itself under one of the conditions below: * the connection is authenticated. * the connection gets into error state. * timeout on waiting for START_CONNECTION message. BUG= ========== to ========== [Blimp Net] Add EngineAuthHandler. It authenticates connections and hands them over to underlying connection_handler once authenticated. BUG= ==========
Ready for review. will add unit test. https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/blimp_me... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/blimp_me... blimp/common/proto/blimp_message.proto:68: optional StartConnectionMessage start_connection = 1005; On 2015/12/02 01:52:52, Wez wrote: > Let's either move these into a general "protocol control" message namespace as > discussed offline. Done. https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... File blimp/common/proto/start_connection.proto (right): https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... blimp/common/proto/start_connection.proto:13: optional string auth_token = 1; On 2015/12/02 17:09:35, maniscalco wrote: > Elsewhere we've called this client_token. I suggest using that name here for > consistency. Done. https://codereview.chromium.org/1492643003/diff/1/blimp/common/proto/start_co... blimp/common/proto/start_connection.proto:14: optional int32 protocol_version = 2; On 2015/12/02 17:09:35, maniscalco wrote: > Elsewhere this is defined as a string. use int per offline discussion. https://codereview.chromium.org/1492643003/diff/1/blimp/net/engine_auth_handl... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/1/blimp/net/engine_auth_handl... blimp/net/engine_auth_handler.h:22: // Authenticates one connection and hand it over to |connection_handler| On 2015/12/02 01:52:52, Wez wrote: > Not sure that this makes sense; it's possible for multiple remote parties to try > to connect to the engine at once, so even though only one of them will succeed > in connecting, they all need to have authentication processed for them. > > Basically I'd expect the EngineAuthHandler to take the supplied BlimpConnection > and to create a per-connection BlimpMessageProcessor for it, that will receive > an authentication message from it and either close it or hand it off to the > |connection_handler|. > > Make sense? Done.
https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. This won't be used bidirectionally? https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:12: message ProtocolControlMessage { Add an inner Type enum to distinguish this message? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/connection_ha... blimp/net/connection_handler.h:17: // Implementation must attach incoming message processor and error observer That seems like an impl detail. How about "The receiving object is responsible for synchronously attaching its necessary delegates and observers to |connection| during the call to HandleConnection." https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:17: const int kAuthTimeoutDurationInSeconds = 2; Add a comment https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:17: const int kAuthTimeoutDurationInSeconds = 2; This timeout is far too short to deal with bufferbloated networks - maybe do 10? Make it a double? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:20: EngineAuthHandler::Authenticator::Authenticator( Move Authenticator impl below EngineAuthHandler? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:30: DCHECK(connection_); Unnecessary b/c only EngineAuthHandler can call it, and its connection is already DCHECKED. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:35: timer_.reset(new base::OneShotTimer()); Nit: no parans needed https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:37: base::TimeDelta::FromSeconds(kAuthTimeoutDurationInSeconds), FromSecondsD? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:40: bool isAuthenticated)>; No camelCase for variable names. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:55: // ConnectionErrorObserver implementation. Move overrides to the bottom of the methods https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:65: // Invoked when timeout on waiting for auth message. nit: on timeout while https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:75: scoped_ptr<base::OneShotTimer> timer_; Move this out of a scoped_ptr? It's never going to be used more than once, so no need to allocate it as a pointer. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:92: std::vector<scoped_ptr<Authenticator>> pending_auths_; What's the purpose of tracking the pending authentication? I'm not seeing cases in the code where we put this vector to good use. Can we postpone centralized management of Authenticators until we have a solid use case? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:93: Add WeakPtrFactory<> for callbacks to Authenticator.
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492643003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492643003/40001
https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:12: message ProtocolControlMessage { On 2015/12/03 19:11:36, Kevin M wrote: > Add an inner Type enum to distinguish this message? what are other types? https://codereview.chromium.org/1492643003/diff/40001/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/connection_ha... blimp/net/connection_handler.h:17: // Implementation must attach incoming message processor and error observer On 2015/12/03 19:11:36, Kevin M wrote: > That seems like an impl detail. > > How about > > "The receiving object is responsible for synchronously attaching its necessary > delegates and observers to |connection| during the call to HandleConnection." Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:17: const int kAuthTimeoutDurationInSeconds = 2; On 2015/12/03 19:11:36, Kevin M wrote: > Add a comment Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:17: const int kAuthTimeoutDurationInSeconds = 2; On 2015/12/03 19:11:36, Kevin M wrote: > This timeout is far too short to deal with bufferbloated networks - maybe do 10? > > Make it a double? Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:20: EngineAuthHandler::Authenticator::Authenticator( On 2015/12/03 19:11:36, Kevin M wrote: > Move Authenticator impl below EngineAuthHandler? Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:30: DCHECK(connection_); On 2015/12/03 19:11:36, Kevin M wrote: > Unnecessary b/c only EngineAuthHandler can call it, and its connection is > already DCHECKED. Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:35: timer_.reset(new base::OneShotTimer()); On 2015/12/03 19:11:36, Kevin M wrote: > Nit: no parans needed removed https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:37: base::TimeDelta::FromSeconds(kAuthTimeoutDurationInSeconds), On 2015/12/03 19:11:36, Kevin M wrote: > FromSecondsD? Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:40: bool isAuthenticated)>; On 2015/12/03 19:11:36, Kevin M wrote: > No camelCase for variable names. Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:55: // ConnectionErrorObserver implementation. On 2015/12/03 19:11:36, Kevin M wrote: > Move overrides to the bottom of the methods Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:65: // Invoked when timeout on waiting for auth message. On 2015/12/03 19:11:36, Kevin M wrote: > nit: on timeout while Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:75: scoped_ptr<base::OneShotTimer> timer_; On 2015/12/03 19:11:36, Kevin M wrote: > Move this out of a scoped_ptr? It's never going to be used more than once, so no > need to allocate it as a pointer. Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:92: std::vector<scoped_ptr<Authenticator>> pending_auths_; On 2015/12/03 19:11:36, Kevin M wrote: > What's the purpose of tracking the pending authentication? I'm not seeing cases > in the code where we put this vector to good use. Can we postpone centralized > management of Authenticators until we have a solid use case? Wez@, what do you think? Since we are not doing anything special about these authenticators right now, I am fine either way. It is actually clean for this object to own these authenticators, and no need for weakptr factory.
https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. On 2015/12/03 19:11:36, Kevin M wrote: > This won't be used bidirectionally? ? https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:12: message ProtocolControlMessage { On 2015/12/03 22:28:35, haibinlu wrote: > On 2015/12/03 19:11:36, Kevin M wrote: > > Add an inner Type enum to distinguish this message? > > what are other types? e.g. checkpoint acks, session recovery message. We'll add these later, but we can go with a single-entry type enum for now. ISTM that a minimal message structure won't save us time even in the short term; we'll have to refactor it in a matter of a couple weeks to support other subtypes. https://codereview.chromium.org/1492643003/diff/60001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. ISTM that "protocol control" will be bidirectional and used for all protocol-related messaging, not just authentication. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:69: DCHECK(connection_); Not needed if connection_ is const https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:79: DCHECK(connection_); Not needed https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:80: InvokeAuthCallback(false); Log |error|? Anything we can do to interpret it or propagate it up? https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:86: DCHECK(connection_); Not needed https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:99: if (auth_callback_.is_null()) { DCHECK for this case, there aren't any cases where this should happen (fired timers are ignored if the timer was Stopped first.) https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:69: scoped_ptr<BlimpConnection> connection_; Can you make this a const?
https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. On 2015/12/03 23:12:05, Kevin M wrote: > On 2015/12/03 19:11:36, Kevin M wrote: > > This won't be used bidirectionally? > > ? Done. https://codereview.chromium.org/1492643003/diff/40001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:12: message ProtocolControlMessage { On 2015/12/03 23:12:05, Kevin M wrote: > On 2015/12/03 22:28:35, haibinlu wrote: > > On 2015/12/03 19:11:36, Kevin M wrote: > > > Add an inner Type enum to distinguish this message? > > > > what are other types? > > e.g. checkpoint acks, session recovery message. We'll add these later, but we > can go with a single-entry type enum for now. > > ISTM that a minimal message structure won't save us time even in the short term; > we'll have to refactor it in a matter of a couple weeks to support other > subtypes. Done. https://codereview.chromium.org/1492643003/diff/60001/blimp/common/proto/prot... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/common/proto/prot... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. On 2015/12/03 23:12:05, Kevin M wrote: > ISTM that "protocol control" will be bidirectional and used for all > protocol-related messaging, not just authentication. Done. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:69: DCHECK(connection_); On 2015/12/03 23:12:05, Kevin M wrote: > Not needed if connection_ is const Acknowledged. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:79: DCHECK(connection_); On 2015/12/03 23:12:05, Kevin M wrote: > Not needed Done. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:80: InvokeAuthCallback(false); On 2015/12/03 23:12:05, Kevin M wrote: > Log |error|? Anything we can do to interpret it or propagate it up? added log. but I donot think we need to propagate it up, how would engine act on such error? https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:86: DCHECK(connection_); On 2015/12/03 23:12:05, Kevin M wrote: > Not needed Done. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:99: if (auth_callback_.is_null()) { On 2015/12/03 23:12:05, Kevin M wrote: > DCHECK for this case, there aren't any cases where this should happen (fired > timers are ignored if the timer was Stopped first.) Done. https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/60001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:69: scoped_ptr<BlimpConnection> connection_; On 2015/12/03 23:12:05, Kevin M wrote: > Can you make this a const? can not. autheticator will give the ownership of the connection back in the auth callback so that EngineAuthHandler can hand it over to the next connection handler.
https://codereview.chromium.org/1492643003/diff/80001/blimp/common/proto/blim... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/common/proto/blim... blimp/common/proto/blimp_message.proto:43: PROTOCOL_CONTROL = 6; I'd prefer that this be something like: // Namespace for internal protocol book-keeping messages. INTERNAL = 1; ... TAB_CONTROL = 6; and re-name the existing Control namespace to TabControl, accordingly. I realise this breaks protocol compatibility but that's not really an issue right now! https://codereview.chromium.org/1492643003/diff/80001/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/connection_ha... blimp/net/connection_handler.h:19: // HandleConnection. I'm not keen on this - it means that all ConnectionHandlers have to know what "necessary delegates and observers" are - if we added some new "necessary delegate" then old ConnectionHandler implementations should break if this comment holds true. Why is it problematic to have a temporarily-unattached |connection|, or to wait to attach handlers, etc, to a connection until later on? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:34: base::Unretained(authenticator.get()))); Why do we have a separate Start() call, rather than passing the OnAuthResult callback directly to the Authenticator constructor? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:85: if (message->type() == BlimpMessage::PROTOCOL_CONTROL) { If you called SetIncomingMessageProcessor(null) on the connection here then that should be sufficient to avoid further messages being pumped until a new processor is attached to the connection, right? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:88: callback.Run(net::OK); Why are you only acknowledging the message in case of authentication success? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:102: DVLOG(1) << "Connection authentication timeout"; If this is specific to timeout, it should be called OnAuthenticationTimeout, surely? Or just bind the timer directly to InvokeAuthCallback(false)? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:24: // Authenticates connections and hand them over to |connection_handler| s/hand/hands Suggest: "... passes successfully authenticated connections to |connection_handler|." https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:26: class BLIMP_NET_EXPORT EngineAuthHandler : public ConnectionHandler { Suggest AuthenticatingConnectionHandler or ConnectionAuthenticator or AuthenticatingHandler or EngineAuthenticationHandler. :) https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:40: bool authenticated)>; Why do we need a special AuthCallback? Surely the EngAuthHandler just needs to provide a way for the Authenticator to remove itself from the authenticating_ list, and let the Authenticator either drop or hand-off the connection itself? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:47: public BlimpMessageProcessor { Why does this need to be defined in the header, rather than the cc? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:58: void FailAuthetication(); Typo https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:61: // Used to implement reconnection logic on unexpected disconnections. Why is this class implementing reconnection logic? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:80: // Callback when |authenticator| has the authentication result. Why? What does it do? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:83: bool authenticated); You could simplify this to: OnConnectionAuthenticated(Authenticator* authenticator, scoped_ptr<BlimpConnection> connection) with a null connection indicating failure, for example. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:92: std::vector<scoped_ptr<Authenticator>> pending_auths_; nit: Suggesting just |authenticating_| as the name, or |authenticators_|. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler_unittest.cc:50: ASSERT_FALSE(false); Can haz test?
PTAL. I used BlimpConnection.SetIncomingMessageProcessor(nullptr) in this patch, just to show that it is a nice API to have. https://codereview.chromium.org/1492643003/diff/80001/blimp/common/proto/blim... File blimp/common/proto/blimp_message.proto (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/common/proto/blim... blimp/common/proto/blimp_message.proto:43: PROTOCOL_CONTROL = 6; On 2015/12/04 01:35:37, Wez wrote: > I'd prefer that this be something like: > > // Namespace for internal protocol book-keeping messages. > INTERNAL = 1; > > ... > > TAB_CONTROL = 6; > > and re-name the existing Control namespace to TabControl, accordingly. > > I realise this breaks protocol compatibility but that's not really an issue > right now! Could we refactor this in a different CL? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/connection_ha... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/connection_ha... blimp/net/connection_handler.h:19: // HandleConnection. On 2015/12/04 01:35:38, Wez wrote: > I'm not keen on this - it means that all ConnectionHandlers have to know what > "necessary delegates and observers" are - if we added some new "necessary > delegate" then old ConnectionHandler implementations should break if this > comment holds true. > > Why is it problematic to have a temporarily-unattached |connection|, or to wait > to attach handlers, etc, to a connection until later on? I do not like this either. It is the internal implementation detail of BlimpConnection that forces such requirement: Because BlimpConnection's incoming processor can not be set to nullptr, when a BlimpConnection is hand over from one connection handler to another, the connection may deliver message to an obsolete message processor; requiring a synchronous updating incoming message processor prevents this from happening. I prefer to restore the ability to set BlimpConnection's incoming processor to nullptr, so that auth handler can use it to stop receiving further messages. Kevin has different opinion on this. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.cc (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:34: base::Unretained(authenticator.get()))); On 2015/12/04 01:35:38, Wez wrote: > Why do we have a separate Start() call, rather than passing the OnAuthResult > callback directly to the Authenticator constructor? removed https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:85: if (message->type() == BlimpMessage::PROTOCOL_CONTROL) { On 2015/12/04 01:35:38, Wez wrote: > If you called SetIncomingMessageProcessor(null) on the connection here then that > should be sufficient to avoid further messages being pumped until a new > processor is attached to the connection, right? Added, though need to restore the ability to SetIncomingMessageProcessor to null in BlimpConnection. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:88: callback.Run(net::OK); On 2015/12/04 01:35:38, Wez wrote: > Why are you only acknowledging the message in case of authentication success? Done. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.cc:102: DVLOG(1) << "Connection authentication timeout"; On 2015/12/04 01:35:38, Wez wrote: > If this is specific to timeout, it should be called OnAuthenticationTimeout, > surely? Or just bind the timer directly to InvokeAuthCallback(false)? https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler.h (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:24: // Authenticates connections and hand them over to |connection_handler| On 2015/12/04 01:35:38, Wez wrote: > s/hand/hands > > Suggest: "... passes successfully authenticated connections to > |connection_handler|." Done. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:26: class BLIMP_NET_EXPORT EngineAuthHandler : public ConnectionHandler { On 2015/12/04 01:35:38, Wez wrote: > Suggest AuthenticatingConnectionHandler or ConnectionAuthenticator or > AuthenticatingHandler or EngineAuthenticationHandler. :) Done. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:40: bool authenticated)>; On 2015/12/04 01:35:38, Wez wrote: > Why do we need a special AuthCallback? Surely the EngAuthHandler just needs to > provide a way for the Authenticator to remove itself from the authenticating_ > list, and let the Authenticator either drop or hand-off the connection itself? removed. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:47: public BlimpMessageProcessor { On 2015/12/04 01:35:38, Wez wrote: > Why does this need to be defined in the header, rather than the cc? Done. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:58: void FailAuthetication(); On 2015/12/04 01:35:38, Wez wrote: > Typo changed method name as suggested below. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:61: // Used to implement reconnection logic on unexpected disconnections. On 2015/12/04 01:35:38, Wez wrote: > Why is this class implementing reconnection logic? removed. copy&paste issue https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:80: // Callback when |authenticator| has the authentication result. On 2015/12/04 01:35:38, Wez wrote: > Why? What does it do? removed. https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:83: bool authenticated); On 2015/12/04 01:35:38, Wez wrote: > You could simplify this to: > > OnConnectionAuthenticated(Authenticator* authenticator, > scoped_ptr<BlimpConnection> connection) > > with a null connection indicating failure, for example. done https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler.h:92: std::vector<scoped_ptr<Authenticator>> pending_auths_; On 2015/12/04 01:35:38, Wez wrote: > nit: Suggesting just |authenticating_| as the name, or |authenticators_|. removed https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... File blimp/net/engine_auth_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/80001/blimp/net/engine_auth_h... blimp/net/engine_auth_handler_unittest.cc:50: ASSERT_FALSE(false); On 2015/12/04 01:35:38, Wez wrote: > Can haz test? I'll finish the test once Kevin lands the CL to make BlimpConnection an interface (which enables us to easy mock a connection).
Discussed this with Wez. We'll add DCHECKs and comments to the MessagePump to ensure that the incoming message processor can only be set/unset during certain periods, that way we don't have to fiddle with buffering and whatnot. https://codereview.chromium.org/1492643003/diff/100001/blimp/net/connection_h... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/100001/blimp/net/connection_h... blimp/net/connection_handler.h:17: // The receiving object is responsible for synchronously attaching its Remove this comment https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.cc (right): https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:53: base::WeakPtr<ConnectionHandler> connection_handler_; This isn't used anymore; remove it and all references to it https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:68: timer_.Start(FROM_HERE, Constructors shouldn't be responsible for doing actual work - put this in a Start() method. https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:48: Plz implement
haibinlu@chromium.org changed reviewers: + maniscalco@chromium.org
PTAL https://codereview.chromium.org/1492643003/diff/100001/blimp/net/connection_h... File blimp/net/connection_handler.h (right): https://codereview.chromium.org/1492643003/diff/100001/blimp/net/connection_h... blimp/net/connection_handler.h:17: // The receiving object is responsible for synchronously attaching its On 2015/12/04 19:12:05, Kevin M (no reviews) wrote: > Remove this comment Done. https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.cc (right): https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:53: base::WeakPtr<ConnectionHandler> connection_handler_; On 2015/12/04 19:12:05, Kevin M (no reviews) wrote: > This isn't used anymore; remove it and all references to it It is used to take a connection when it is authenticated. https://codereview.chromium.org/1492643003/diff/100001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:68: timer_.Start(FROM_HERE, On 2015/12/04 19:12:05, Kevin M (no reviews) wrote: > Constructors shouldn't be responsible for doing actual work - put this in a > Start() method. Per wez's previous comment, Start method is removed.
https://codereview.chromium.org/1492643003/diff/120001/blimp/common/create_bl... File blimp/common/create_blimp_message.h (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/common/create_bl... blimp/common/create_blimp_message.h:43: StartConnectionMessage** start_connection_message); Presumably we want to allow the caller to pass in the client_token and version number, at least? Note that I don't think this overloading of CreateBlimpMessage() actually meets style-guide requirements, in general; there are situations when we will want to create several messages in sequence, at which point the overloading may make it less clear which is being called. We should consider renaming these CreateCompositorMessage, CreateInputMessage, etc. https://codereview.chromium.org/1492643003/diff/120001/blimp/common/proto/pro... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/common/proto/pro... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. nit: an authenticated connection https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.cc (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:21: // Expect authentication to be done in 10 seconds. nit: I think you can be more explicit "Expect Client to send the StartConnection within ten seconds of becoming connected." https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:26: // * the connection gets into error state. nit: into an error state https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:27: // * timeout on waiting for auth message. nit: the auth message does not arrive within a reasonable time. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:36: // Processes authentication result and delete this object. nit: delete->deletes nit: You can also write "this object" as "|this|" in comments of this sort. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:39: // Invoked on timeout while waiting for auth message. nit: Reword this similarly to the processes one, above, e.g. "Handles timeout waiting for auth message, and deletes |this|." https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:52: // Handler for authenticated connections. nit: Handler to pass successfully authentication connections to. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:56: base::OneShotTimer timer_; nit: timeout_ or timeout_timer_? https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:69: base::TimeDelta::FromSecondsD(kAuthTimeoutDurationInSeconds), nit: Why are we using FromSecondsD() here, rather than having an int64 auth-timeout duration constant? Do we need sub-second granularity..? https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:76: timer_.Stop(); nit: We are about to delete |this|, which will delete |timer_|, which will StopAndAbandon() it - do we need to explicitly Stop() it here? https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:93: DVLOG(1) << "Connection error before authenticated"; nit: Log the actual error code. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:103: OnConnectionAuthenticated(false); nit: Perhaps a DVLOG(1) here, as you have above, would be appropriate? https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:106: callback.Run(net::OK); nit: We should update MessageProcessor (separate CL) to clarify that it's OK for ProcessMessage() to invoke the callback synchronously. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.h (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.h:32: // Handler for authenticated connections. nit: Suggest "Used to abandon pending authentication events if |this| is deleted." https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:75: TEST_F(EngineAuthenticationHandlerTest, AuthenticationFails) { Add tests that simulate a timeout, and that simulate a network error? Also, a test that verifies that if you delete the authenticating handler and then have authentication complete then the internal Authenticator object doesn't cause things to explode would be good.
PTAL https://codereview.chromium.org/1492643003/diff/120001/blimp/common/create_bl... File blimp/common/create_blimp_message.h (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/common/create_bl... blimp/common/create_blimp_message.h:43: StartConnectionMessage** start_connection_message); On 2015/12/08 00:31:41, Wez wrote: > Presumably we want to allow the caller to pass in the client_token and version > number, at least? > > Note that I don't think this overloading of CreateBlimpMessage() actually meets > style-guide requirements, in general; there are situations when we will want to > create several messages in sequence, at which point the overloading may make it > less clear which is being called. We should consider renaming these > CreateCompositorMessage, CreateInputMessage, etc. Update new method name. Shall we use separate CL for update other method names? https://codereview.chromium.org/1492643003/diff/120001/blimp/common/proto/pro... File blimp/common/proto/protocol_control.proto (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/common/proto/pro... blimp/common/proto/protocol_control.proto:11: // Contains Client->Engine information to establish a authenticated connection. On 2015/12/08 00:31:41, Wez wrote: > nit: an authenticated connection Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.cc (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:21: // Expect authentication to be done in 10 seconds. On 2015/12/08 00:31:41, Wez wrote: > nit: I think you can be more explicit "Expect Client to send the StartConnection > within ten seconds of becoming connected." Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:26: // * the connection gets into error state. On 2015/12/08 00:31:41, Wez wrote: > nit: into an error state Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:27: // * timeout on waiting for auth message. On 2015/12/08 00:31:41, Wez wrote: > nit: the auth message does not arrive within a reasonable time. Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:36: // Processes authentication result and delete this object. On 2015/12/08 00:31:41, Wez wrote: > nit: delete->deletes > > nit: You can also write "this object" as "|this|" in comments of this sort. Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:39: // Invoked on timeout while waiting for auth message. On 2015/12/08 00:31:41, Wez wrote: > nit: Reword this similarly to the processes one, above, e.g. "Handles timeout > waiting for auth message, and deletes |this|." Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:52: // Handler for authenticated connections. On 2015/12/08 00:31:41, Wez wrote: > nit: Handler to pass successfully authentication connections to. Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:56: base::OneShotTimer timer_; On 2015/12/08 00:31:41, Wez wrote: > nit: timeout_ or timeout_timer_? Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:69: base::TimeDelta::FromSecondsD(kAuthTimeoutDurationInSeconds), On 2015/12/08 00:31:41, Wez wrote: > nit: Why are we using FromSecondsD() here, rather than having an int64 > auth-timeout duration constant? Do we need sub-second granularity..? No, we do not right now. Kevin prefered FromSecondsD. Updated to FromSeconds. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:76: timer_.Stop(); On 2015/12/08 00:31:41, Wez wrote: > nit: We are about to delete |this|, which will delete |timer_|, which will > StopAndAbandon() it - do we need to explicitly Stop() it here? right. no need to stop timer here. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:93: DVLOG(1) << "Connection error before authenticated"; On 2015/12/08 00:31:41, Wez wrote: > nit: Log the actual error code. Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:103: OnConnectionAuthenticated(false); On 2015/12/08 00:31:41, Wez wrote: > nit: Perhaps a DVLOG(1) here, as you have above, would be appropriate? Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.cc:106: callback.Run(net::OK); On 2015/12/08 00:31:41, Wez wrote: > nit: We should update MessageProcessor (separate CL) to clarify that it's OK for > ProcessMessage() to invoke the callback synchronously. Acknowledged. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler.h (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler.h:32: // Handler for authenticated connections. On 2015/12/08 00:31:41, Wez wrote: > nit: Suggest "Used to abandon pending authentication events if |this| is > deleted." Done. https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/120001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:75: TEST_F(EngineAuthenticationHandlerTest, AuthenticationFails) { On 2015/12/08 00:31:41, Wez wrote: > Add tests that simulate a timeout, and that simulate a network error? Also, a > test that verifies that if you delete the authenticating handler and then have > authentication complete then the internal Authenticator object doesn't cause > things to explode would be good. Done.
Do we have a bug # for this? Also, suggest making the CL description a little more complete, e.g explain that it adds a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler.
Description was changed from ========== [Blimp Net] Add EngineAuthHandler. It authenticates connections and hands them over to underlying connection_handler once authenticated. BUG= ========== to ========== [Blimp Net] Add EngineAuthHandler. It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler. BUG=567817 ==========
Updated description and added a bug. PTAL
lgtm https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... blimp/common/create_blimp_message.cc:49: scoped_ptr<BlimpMessage> output(new BlimpMessage); nit: I'd suggest putting using blank lines here to break this function into three obvious logical blocks: 1. creating the BlimpMessage & setting the type, 2. creating the protocol control message & setting type and 3. creating the start-connection message within it. https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... File blimp/common/create_blimp_message_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... blimp/common/create_blimp_message_unittest.cc:45: int protocol_version = 1; nit: These can both be const, I think; the string can probably be const char*, and will be implicitly converted when necessary. https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:100: TEST_F(EngineAuthenticationHandlerTest, TimeOut) { nit: Timeout or TimesOut https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:111: TEST_F(EngineAuthenticationHandlerTest, AuthHandlerDeleteFirst) { nit: Delete -> Deleted
https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... File blimp/common/create_blimp_message.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... blimp/common/create_blimp_message.cc:49: scoped_ptr<BlimpMessage> output(new BlimpMessage); On 2015/12/08 22:59:59, Wez wrote: > nit: I'd suggest putting using blank lines here to break this function into > three obvious logical blocks: 1. creating the BlimpMessage & setting the type, > 2. creating the protocol control message & setting type and 3. creating the > start-connection message within it. Done. https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... File blimp/common/create_blimp_message_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/common/create_bl... blimp/common/create_blimp_message_unittest.cc:45: int protocol_version = 1; On 2015/12/08 22:59:59, Wez wrote: > nit: These can both be const, I think; the string can probably be const char*, > and will be implicitly converted when necessary. Done. https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... File blimp/net/engine_authentication_handler_unittest.cc (right): https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:100: TEST_F(EngineAuthenticationHandlerTest, TimeOut) { On 2015/12/08 22:59:59, Wez wrote: > nit: Timeout or TimesOut Done. https://codereview.chromium.org/1492643003/diff/160001/blimp/net/engine_authe... blimp/net/engine_authentication_handler_unittest.cc:111: TEST_F(EngineAuthenticationHandlerTest, AuthHandlerDeleteFirst) { On 2015/12/08 22:59:59, Wez wrote: > nit: Delete -> Deleted Done.
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1492643003/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492643003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492643003/180001
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1492643003/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492643003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492643003/180001
Message was sent while issue was closed.
Description was changed from ========== [Blimp Net] Add EngineAuthHandler. It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler. BUG=567817 ========== to ========== [Blimp Net] Add EngineAuthHandler. It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler. BUG=567817 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Blimp Net] Add EngineAuthHandler. It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler. BUG=567817 ========== to ========== [Blimp Net] Add EngineAuthHandler. It is a ConnectionHandler which expects each new connection to supply a StartConnection message with a valid client authentication token, verifies it, and if successful then hands the connection off to the main ConnectionHandler. BUG=567817 Committed: https://crrev.com/9fa0095471e61a1de43d902974567355585157fe Cr-Commit-Position: refs/heads/master@{#363880} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9fa0095471e61a1de43d902974567355585157fe Cr-Commit-Position: refs/heads/master@{#363880} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
