Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316231:
Improve curtain mode error message. (Closed)
|
Created:
8 years ago by Jamie Modified:
8 years 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:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImprove curtain mode error message.
BUG=161158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170397
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reinstate RejectAuthenticatingClient. #
Total comments: 4
Patch Set 3 : Reviewer comments. #
Messages
Total messages: 7 (0 generated)
ptal https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:939: host_->RejectAuthenticatingClient(); There's a comment on this method saying that it should only be used from OnClientAuthenticated(), so I don't think it's the right thing to use here, although it's not the cause of the bogus error message. https://codereview.chromium.org/11316231/diff/1/remoting/webapp/client_screen.js File remoting/webapp/client_screen.js (right): https://codereview.chromium.org/11316231/diff/1/remoting/webapp/client_screen... remoting/webapp/client_screen.js:263: showConnectError_(remoting.Error.UNEXPECTED); I'm not particularly happy with this fix. I would prefer that the host closed the session with an error message (preferably one specific to curtain mode). I'm not sure it's worth the effort though, given that we don't expect this to happen very often. Let me know if you disagree. The reasons for giving an invalid access code error in this situation are no longer valid. It was introduced in https://codereview.chromium.org/7511001 if you're interested.
drive-by https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:939: host_->RejectAuthenticatingClient(); On 2012/11/28 23:52:32, Jamie wrote: > There's a comment on this method saying that it should only be used from > OnClientAuthenticated(), so I don't think it's the right thing to use here, > although it's not the cause of the bogus error message. Right now this is only called when activating the curtain mode failed (so perhaps OnCurtainModeActivationFailed would be more explicit), which is ultimately done from an OnClientAuthenticated callback from CurtainingHostObserver. The problem with using OnDisconnectRequested() here is that it immediately disconnects the session, and calls the OnClientDisconnected callback in all HostObservers. This means that if it's called from an OnClientAuthenticated handler, some HostObservers will receive the OnClientDisconnected callback *before* the OnClientAuthenticated (and the CurtainingHostObserver will call OnClientDisconnected *from* an OnClientAuthenticated callstack) I'd prefer to change the messaging on the client side from "the PIN was wrong" to something on the lines of "the host rejected your connection, due to a wrong PIN or a host security policy", since there could be more policy-related reasons why a host rejects a client during authentication (username policy, etc). https://codereview.chromium.org/11316231/diff/1/remoting/webapp/client_screen.js File remoting/webapp/client_screen.js (right): https://codereview.chromium.org/11316231/diff/1/remoting/webapp/client_screen... remoting/webapp/client_screen.js:263: showConnectError_(remoting.Error.UNEXPECTED); On 2012/11/28 23:52:32, Jamie wrote: > I'm not particularly happy with this fix. I would prefer that the host closed > the session with an error message (preferably one specific to curtain mode). I'm > not sure it's worth the effort though, given that we don't expect this to happen > very often. Let me know if you disagree. > > The reasons for giving an invalid access code error in this situation are no > longer valid. It was introduced in https://codereview.chromium.org/7511001 if > you're interested. This seems like a valid change regardless of the host-side curtain mode change (as long as we remove the example from the comment)
https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:939: host_->RejectAuthenticatingClient(); On 2012/11/29 00:11:14, rmsousa wrote: > On 2012/11/28 23:52:32, Jamie wrote: > > There's a comment on this method saying that it should only be used from > > OnClientAuthenticated(), so I don't think it's the right thing to use here, > > although it's not the cause of the bogus error message. > Right now this is only called when activating the curtain mode failed (so > perhaps OnCurtainModeActivationFailed would be more explicit), which is > ultimately done from an OnClientAuthenticated callback from > CurtainingHostObserver. You're right. I'd forgotten that we are effectively calling it in the right context, it's just not obvious from a cursory inspection of the code, and it feels brittle to rely on it. > > The problem with using OnDisconnectRequested() here is that it immediately > disconnects the session, and calls the OnClientDisconnected callback in all > HostObservers. This means that if it's called from an OnClientAuthenticated > handler, some HostObservers will receive the OnClientDisconnected callback > *before* the OnClientAuthenticated (and the CurtainingHostObserver will call > OnClientDisconnected *from* an OnClientAuthenticated callstack) Yuck. Okay, I'll switch back to calling RejectAuthenticatingClient. Thanks for the drive-by. > I'd prefer to change the messaging on the client side from "the PIN was wrong" > to something on the lines of "the host rejected your connection, due to a wrong > PIN or a host security policy", since there could be more policy-related reasons > why a host rejects a client during authentication (username policy, etc). Since the most likely reason a user might see this message is that they've entered an invalid PIN, I don't want to make the error message more complicated.
LGTM with one nit and one suggestions. https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/11316231/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:939: host_->RejectAuthenticatingClient(); On 2012/11/29 01:05:58, Jamie wrote: > On 2012/11/29 00:11:14, rmsousa wrote: > > I'd prefer to change the messaging on the client side from "the PIN was wrong" > > to something on the lines of "the host rejected your connection, due to a > wrong > > PIN or a host security policy", since there could be more policy-related > reasons > > why a host rejects a client during authentication (username policy, etc). > > Since the most likely reason a user might see this message is that they've > entered an invalid PIN, I don't want to make the error message more complicated. +1. A message that says "maybe your entered incorrect PIN or maybe something else is wrong" is not helpful and we should avoid it. https://codereview.chromium.org/11316231/diff/5001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11316231/diff/5001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:947: << " Closing connection."; nit: indentation. You can also remove "<<" here. https://codereview.chromium.org/11316231/diff/5001/remoting/webapp/client_scr... File remoting/webapp/client_screen.js (right): https://codereview.chromium.org/11316231/diff/5001/remoting/webapp/client_scr... remoting/webapp/client_screen.js:260: // For example, it does this if it fails to activate curtain mode. Since Would it be better if the host was sending exact error code in this case instead of just closing the connection? Or is the curtain failure is not frequent enough to worry about it?
fyi https://codereview.chromium.org/11316231/diff/5001/remoting/host/remoting_me2... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/11316231/diff/5001/remoting/host/remoting_me2... remoting/host/remoting_me2me_host.cc:947: << " Closing connection."; On 2012/11/30 01:09:39, sergeyu wrote: > nit: indentation. You can also remove "<<" here. Done. https://codereview.chromium.org/11316231/diff/5001/remoting/webapp/client_scr... File remoting/webapp/client_screen.js (right): https://codereview.chromium.org/11316231/diff/5001/remoting/webapp/client_scr... remoting/webapp/client_screen.js:260: // For example, it does this if it fails to activate curtain mode. Since On 2012/11/30 01:09:39, sergeyu wrote: > Would it be better if the host was sending exact error code in this case instead > of just closing the connection? Or is the curtain failure is not frequent enough > to worry about it? That would definitely be an improvement, and I think we should do it in the longer term. I couldn't see how to implement that without substantial surgery, which I don't think this warrants right now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/11316231/8001
Message was sent while issue was closed.
Change committed as 170397 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
