Enforce the RemoteAccessHostRequireCurtain policy on all platforms.

Enforce the RemoteAccessHostRequireCurtain policy on all platforms.

The host now always creates the CurtainingHostObserver, and configures it
based upon policy.  The platform specific CurtainMode implementations then
handle attempts to activate the curtain according to availability; Windows
and Mac hosts will currently reject connections and Linux hosts will reject
connections if running against the console X server.

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

[Wez, 2012-11-06 23:50:59]

PTAL ASAP!

[rmsousa, 2012-11-07 00:19:16]

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac.cc
File remoting/host/curtain_mode_mac.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac...
remoting/host/curtain_mode_mac.cc:75: if (getuid() == 0) {
IIRC (Jamie might have more details) with the current implementation of Mac
curtain mode enabled we actually need to shutdown in this case - otherwise we'll
end up with two hosts running (when the curtain is activated for a host, a
second host process will get started on the login screen, and we need to
shutdown that *second* process, so it doesn't compete with the real one).

We can comment this code in its original place (based on the affected
implementation being disabled), but we can't really move it here, since (1) we
need to call HostProcess::Shutdown(), and (2) if this is the process that's
running on the login screen, we need to run this code on the process's
initialization, it has nothing to do with *this process* activating the curtain.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:718:
curtaining_host_observer_->SetEnableCurtaining(curtain_required_);
Note that the RestartHost immediately after this will kick out all the remote
clients. So doing this immediately here and also curtaining the local console
seems a bit redundant.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:808: curtaining_host_observer_.reset(new
CurtainingHostObserver(
You need to SetEnableCurtaining again in the new object.

[Jamie, 2012-11-07 00:27:49]

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac.cc
File remoting/host/curtain_mode_mac.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac...
remoting/host/curtain_mode_mac.cc:75: if (getuid() == 0) {
On 2012/11/07 00:19:16, rmsousa wrote:
> IIRC (Jamie might have more details) with the current implementation of Mac
> curtain mode enabled we actually need to shutdown in this case - otherwise
we'll
> end up with two hosts running (when the curtain is activated for a host, a
> second host process will get started on the login screen, and we need to
> shutdown that *second* process, so it doesn't compete with the real one).
> 
> We can comment this code in its original place (based on the affected
> implementation being disabled), but we can't really move it here, since (1) we
> need to call HostProcess::Shutdown(), and (2) if this is the process that's
> running on the login screen, we need to run this code on the process's
> initialization, it has nothing to do with *this process* activating the
curtain.

Renato is correct. This is not an error activating the curtain--it's a start-up
condition that requires us to quit.

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.cc (left):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.cc:26: // to the HostStatusObserver
interface to address this.
Please reinstate this TODO in some form. If you think it's worth adding the list
of clients to this class for now, then that's fine, but I think the correct
long-term solution is to centralize that logic somewhere.

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.h (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.h:28: // Enables/disabled curtaining when
one or more clients are connected.
Nit: s/disabled/disables/

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.h:30: void SetEnableCurtaining(bool
enable);
Check with Renato, or see his previous CL around curtain mode. I think I
suggested moving the enable state into the observer, but he pointed out it
causes a problem in a corner case.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:717: LOG(INFO) << "Updated curtain
policy.";
While you're here, it would be useful to log the new value.

[rmsousa, 2012-11-07 00:33:25]

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.h (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.h:30: void SetEnableCurtaining(bool
enable);
On 2012/11/07 00:27:49, Jamie wrote:
> Check with Renato, or see his previous CL around curtain mode. I think I
> suggested moving the enable state into the observer, but he pointed out it
> causes a problem in a corner case.

Having it here is fine, as long as there's another copy somewhere permanent (in
this change, remoting_me2me_host) to be reapplied when the host observer is
deleted/recreated.

[Jamie, 2012-11-07 00:41:11]

lgtm once my comments are addressed.

[Wez, 2012-11-07 02:37:32]

PTAL

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac.cc
File remoting/host/curtain_mode_mac.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtain_mode_mac...
remoting/host/curtain_mode_mac.cc:75: if (getuid() == 0) {
On 2012/11/07 00:27:49, Jamie wrote:
> On 2012/11/07 00:19:16, rmsousa wrote:
> > IIRC (Jamie might have more details) with the current implementation of Mac
> > curtain mode enabled we actually need to shutdown in this case - otherwise
> we'll
> > end up with two hosts running (when the curtain is activated for a host, a
> > second host process will get started on the login screen, and we need to
> > shutdown that *second* process, so it doesn't compete with the real one).
> > 
> > We can comment this code in its original place (based on the affected
> > implementation being disabled), but we can't really move it here, since (1)
we
> > need to call HostProcess::Shutdown(), and (2) if this is the process that's
> > running on the login screen, we need to run this code on the process's
> > initialization, it has nothing to do with *this process* activating the
> curtain.
> 
> Renato is correct. This is not an error activating the curtain--it's a
start-up
> condition that requires us to quit.

Aha; it wasn't clear to me from the comment that this is an implementation
artefact.

I've restored it to its original location and updated the comment to try to make
that clearer.

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.cc (left):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.cc:26: // to the HostStatusObserver
interface to address this.
On 2012/11/07 00:27:49, Jamie wrote:
> Please reinstate this TODO in some form. If you think it's worth adding the
list
> of clients to this class for now, then that's fine, but I think the correct
> long-term solution is to centralize that logic somewhere.

Most of our uses of HostObserver will disappear into DesktopEnvironment[Factory]
in due course, so I don't think that optimization is worthwhile.

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.h (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.h:28: // Enables/disabled curtaining when
one or more clients are connected.
On 2012/11/07 00:27:49, Jamie wrote:
> Nit: s/disabled/disables/

Done.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:717: LOG(INFO) << "Updated curtain
policy.";
On 2012/11/07 00:27:49, Jamie wrote:
> While you're here, it would be useful to log the new value.

I'll be following up shortly to add an ignore-policies policy, so I'll add
printout of policy values there.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:717: LOG(INFO) << "Updated curtain
policy.";
On 2012/11/07 00:27:49, Jamie wrote:
> While you're here, it would be useful to log the new value.

I've update the logging of all the policy values, rather than single this one
out for special treatment.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:718:
curtaining_host_observer_->SetEnableCurtaining(curtain_required_);
On 2012/11/07 00:19:16, rmsousa wrote:
> Note that the RestartHost immediately after this will kick out all the remote
> clients. So doing this immediately here and also curtaining the local console
> seems a bit redundant.

It'll curtain the local console iff there's a connection active to it, which is
correct wrt the intent of the policy.

http://codereview.chromium.org/11275178/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:808: curtaining_host_observer_.reset(new
CurtainingHostObserver(
On 2012/11/07 00:19:16, rmsousa wrote:
> You need to SetEnableCurtaining again in the new object.

Done.

[rmsousa, 2012-11-07 02:44:10]

lgtm

[Jamie, 2012-11-07 22:28:01]

lgtm

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.cc (left):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.cc:26: // to the HostStatusObserver
interface to address this.
On 2012/11/07 02:37:32, Wez wrote:
> On 2012/11/07 00:27:49, Jamie wrote:
> > Please reinstate this TODO in some form. If you think it's worth adding the
> list
> > of clients to this class for now, then that's fine, but I think the correct
> > long-term solution is to centralize that logic somewhere.
> 
> Most of our uses of HostObserver will disappear into
DesktopEnvironment[Factory]
> in due course, so I don't think that optimization is worthwhile.

Fair enough, but we'll still want to centralize the handling of first/last
client state, so I think a TODO is still valuable.

[Wez, 2012-11-07 22:33:06]

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
File remoting/host/curtaining_host_observer.cc (left):

http://codereview.chromium.org/11275178/diff/1/remoting/host/curtaining_host_...
remoting/host/curtaining_host_observer.cc:26: // to the HostStatusObserver
interface to address this.
On 2012/11/07 22:28:02, Jamie wrote:
> On 2012/11/07 02:37:32, Wez wrote:
> > On 2012/11/07 00:27:49, Jamie wrote:
> > > Please reinstate this TODO in some form. If you think it's worth adding
the
> > list
> > > of clients to this class for now, then that's fine, but I think the
correct
> > > long-term solution is to centralize that logic somewhere.
> > 
> > Most of our uses of HostObserver will disappear into
> DesktopEnvironment[Factory]
> > in due course, so I don't think that optimization is worthwhile.
> 
> Fair enough, but we'll still want to centralize the handling of first/last
> client state, so I think a TODO is still valuable.

No, we won't; this way of handling "curtaining" will eventually be replaced in
the daemon model.  Let's discuss offline. :)

[commit-bot: I haz the power, 2012-11-07 22:33:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/11275178/6002

[commit-bot: I haz the power, 2012-11-12 21:53:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/11275178/15002

[commit-bot: I haz the power, 2012-11-13 04:42:48]

Change committed as 167304