Update Windows It2Me to allow remote users to interact with elevated windows

Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
Committed: https://crrev.com/a89fa1aab1ef6fda1e88e2482c867594157d89e4
Cr-Commit-Position: refs/heads/master@{#416778}

[joedow, 2016-07-27 03:58:30]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 03:58:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 04:46:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 04:46:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-07-28 02:45:23]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 02:45:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 03:40:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 03:40:15]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-08-30 21:20:40]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 21:21:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 22:05:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 22:05:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-08-31 18:31:20]

Patchset #3 (id:40001) has been deleted

[joedow, 2016-08-31 18:31:39]

Patchset #2 (id:20001) has been deleted

[joedow, 2016-08-31 18:33:33]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 18:34:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-08-31 19:55:16]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 19:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-08-31 19:56:13]

joedow@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, sergeyu@chromium.org

[joedow, 2016-08-31 19:56:13]

PTAL!

Thanks,
Joe

[Devlin, 2016-08-31 20:13:03]

Sergey's a much better reviewer for this than I am; moving myself -> cc

[Devlin, 2016-08-31 20:13:15]

Description was changed from

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
==========

to

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
==========

[Devlin, 2016-08-31 20:13:15]

rdevlin.cronin@chromium.org changed reviewers:
- rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-08-31 21:18:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 21:18:10]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-08-31 22:03:32]

joedow@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[joedow, 2016-08-31 22:03:33]

rdevlin.cronin@, I added you as an owner for the ChromeOS change (which I don't
think Sergey can LGTM).  Could you take a look at that part of the change?

Thanks,
Joe

[Devlin, 2016-08-31 22:39:56]

On 2016/08/31 22:03:33, joedow wrote:
> rdevlin.cronin@, I added you as an owner for the ChromeOS change (which I
don't
> think Sergey can LGTM).  Could you take a look at that part of the change?
> 
> Thanks,
> Joe

https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/extens...

sergeyu is an OWNER of Chrome's native messaging API, too. :)

[Sergey Ulanov, 2016-08-31 23:00:17]

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.h (right):

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.h:230: policy::PolicyService*
get_policy_service() { return policy_service_; }
This also looks out of place here. Factories should create things, but here you
piggy-back on it to pass PolicyService to It2MeNativeMessagingHost. Maybe just
pass PolicyService* to It2MeNativeMessagingHost explicitly? Then it could be
removed from here and replaced with PolicyWatcher parameter in CreateIt2MeHost,
but that's optional.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:86: base::Unretained(this)),
I don't think it's safe to use Unretained() here because policy_watcher_ will
call this closure on a thread different from the one on which
It2MeNativeMessagingHost is destroyed.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:186: if (needs_elevation_ &&
allow_elevated_host_) {
Add a comment here to explain what happens here.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:188:
response->SetBoolean("result", false);
Does the app support this field right now?

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:433:
/*parent_window_handle=*/0,
Why is this not parameter passed to the elevated process? maybe explain it in
the comments?

[joedow, 2016-09-02 21:56:31]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 21:57:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-02 21:58:58]

joedow@chromium.org changed reviewers:
- rdevlin.cronin@chromium.org

[joedow, 2016-09-02 21:58:59]

Addressed feedback, PTAL!

Removing rdevlin -> sorry I re-added you, it was due to git cl upload's
suggestion :P

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.h (right):

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.h:230: policy::PolicyService*
get_policy_service() { return policy_service_; }
On 2016/08/31 23:00:16, Sergey Ulanov wrote:
> This also looks out of place here. Factories should create things, but here
you
> piggy-back on it to pass PolicyService to It2MeNativeMessagingHost. Maybe just
> pass PolicyService* to It2MeNativeMessagingHost explicitly? Then it could be
> removed from here and replaced with PolicyWatcher parameter in
CreateIt2MeHost,
> but that's optional.

Great suggestion

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:86: base::Unretained(this)),
On 2016/08/31 23:00:17, Sergey Ulanov wrote:
> I don't think it's safe to use Unretained() here because policy_watcher_ will
> call this closure on a thread different from the one on which
> It2MeNativeMessagingHost is destroyed.

Done.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:186: if (needs_elevation_ &&
allow_elevated_host_) {
On 2016/08/31 23:00:16, Sergey Ulanov wrote:
> Add a comment here to explain what happens here.

Done.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:188:
response->SetBoolean("result", false);
On 2016/08/31 23:00:16, Sergey Ulanov wrote:
> Does the app support this field right now?

I seem to remember testing this in a previous iteration but I think it is better
to be consistent so I will use the same error mechanism as the rest of this
file.

https://codereview.chromium.org/2179353004/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:433:
/*parent_window_handle=*/0,
On 2016/08/31 23:00:17, Sergey Ulanov wrote:
> Why is this not parameter passed to the elevated process? maybe explain it in
> the comments?

Done.

[commit-bot: I haz the power, 2016-09-02 22:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 22:51:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[joedow, 2016-09-02 23:06:34]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 23:07:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Ulanov, 2016-09-02 23:21:15]

lgtm

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.h:116: bool policy_received_ =
false;
nit: make this const and remove default initializer. That way the compiler will
verify that it gets initialized in the constructor. Same for policy_service.

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host_main.cc:108:
CHECK(CurrentProcessHasUiAccess());
Do we actually need to crash the process in this case? Maybe just log an error?

[commit-bot: I haz the power, 2016-09-02 23:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:46:00]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-09-06 22:42:13]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 22:42:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-06 22:51:49]

Patchset #7 (id:160001) has been deleted

[joedow, 2016-09-06 22:52:00]

Thanks!

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.h (right):

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.h:116: bool policy_received_ =
false;
On 2016/09/02 23:21:14, Sergey Ulanov wrote:
> nit: make this const and remove default initializer. That way the compiler
will
> verify that it gets initialized in the constructor. Same for policy_service.

Acknowledged.  |policy_received_| is modified so I can't make it const.  I can
mark |policy_service_| const, but I would need to use const_cast to remove the
const'ness before using it.

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host_main.cc (right):

https://codereview.chromium.org/2179353004/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host_main.cc:108:
CHECK(CurrentProcessHasUiAccess());
On 2016/09/02 23:21:14, Sergey Ulanov wrote:
> Do we actually need to crash the process in this case? Maybe just log an
error?

I chose to use a CHECK here as this functionality only works on official builds
which we don't generally run in debug mode.  I'd really hate to miss this
problem because It2Me appeared to work but was silently failing.

I made this decision before we decided to use a policy to control the behavior
though, I think it is now more likely that we would test without the policy set
and miss this bug and then start seeing crashes after we ship.  I will change
this to a log statement to make it easier to debug.

[joedow, 2016-09-06 22:52:23]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-09-06 22:52:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2179353004/#ps180001
(title: "Removing the CHECK assertion and replacing it with LOG(ERROR)
instead.")

[commit-bot: I haz the power, 2016-09-06 22:53:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:49:16]

Description was changed from

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
==========

to

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
==========

[commit-bot: I haz the power, 2016-09-06 23:49:17]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-09-06 23:51:34]

Description was changed from

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185
==========

to

==========
Update Windows It2Me to allow remote users to interact with elevated windows

This changes updates the existing It2Me host for windows to use a second
process It2Me process (with the uiaccess flag in its manifest) to allow
the remote user to interact with elevated windows on the desktop.

BUG=617185

Committed: https://crrev.com/a89fa1aab1ef6fda1e88e2482c867594157d89e4
Cr-Commit-Position: refs/heads/master@{#416778}
==========

[commit-bot: I haz the power, 2016-09-06 23:51:35]

Patchset 7 (id:??) landed as
https://crrev.com/a89fa1aab1ef6fda1e88e2482c867594157d89e4
Cr-Commit-Position: refs/heads/master@{#416778}