[remoting host] Host decides if URL forwarder is supported [chromium/src : main]
Yuwei Huang (Gerrit)
Attention is currently required from: Joe Downing.
Patch set 1:Commit-Queue +1
3 comments:
Patchset:
PTAL!
File remoting/host/remote_open_url_constants.h:
Patch Set #1, Line 24:
IsRemoteOpenUrlSupportedIt feels a bit weird to be in a "constants" file, but I'm a bit hesitant to introduce a new "util" file just for this method..
File remoting/host/remoting_me2me_host.cc:
Patch Set #1, Line 1583:
IsRemoteOpenUrlSupportedA downside with this approach is that IsRemoteOpenUrlSupported() will only be called once here, so it won't pick up changes to the registry unless the user restarts the service. I'm not sure how puppet is going to set that registry value, but if it doesn't restart the host after setting the value, then it may not take effect..
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Attention is currently required from: Yuwei Huang.
6 comments:
File remoting/host/installer/win/chromoting.wxs:
<!-- RegisteredApplications -->
<!-- This will be deployed directly to Googlers: -->
<!--
<RegistryKey Id="remote_open_url_registered_applications_reg_key"
Root="HKLM"
Key="SOFTWARE\RegisteredApplications"
Action="create">
<RegistryValue Type="string"
Name="$(var.UrlForwarderName)"
Value="SOFTWARE\$(var.ChromotingKeyPath)\UrlForwarder\Capabilities"/>
</RegistryKey>
-->I think this blob should be removed since the requirement is already documented in a .cc file and this is effectively dead code since it is never tested. I'd also remove the comment about how the registry key will be applied internally.
File remoting/host/remote_open_url_constants.h:
Patch Set #1, Line 24:
IsRemoteOpenUrlSupported
It feels a bit weird to be in a "constants" file, but I'm a bit hesitant to introduce a new "util" f […]
I did a quick spot check and it looks like there are ~23 files in //remoting/host that are associated with this feature (remote_open_* and url_forwarder_configurator_*).
Can you move these into a sub-folder (I think a single feature directory would be fine for both (I'd guess url_forwarding is probably more meaningful but feel free to choose whatever name is most intuitive to you).
If the files are moved into a sub-dir, I don't think having a constants and helpers/utils file would be too bad, I do see the concern with polluting the //remoting/host directory with it though.
The move can happen in a follow-up CL if you agree that would help clean things up in the /host dir.
File remoting/host/remote_open_url_constants.cc:
Patch Set #2, Line 69:
fined(OS_WIN)Don't we want to check |is_googler| for Windows as well?
which is currently only deployed to
// Googlers' devices.Can we make this more generic:
"which must be applied out of band to enable the feature."Failed to open key HKLM\\" << kRegisteredApplicationsKeyName
<< ", result: " << reWDYT about updating this message to indicate that this means the URL forwarding capability cannot be determined (or essentially that the host doesn't support it because the key couldn't be opened).
File remoting/host/remoting_me2me_host.cc:
Patch Set #1, Line 1583:
IsRemoteOpenUrlSupported
A downside with this approach is that IsRemoteOpenUrlSupported() will only be called once here, so i […]
I was thinking that we would query the registry key for each connection.
Could you do an initial check on platform version/is_googler here to set the environment options and then when the host is generating it's capability set, check the registry key (on Windows) if the environment says that it is supported?
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Yuwei Huang (Gerrit)
Attention is currently required from: Joe Downing.
Patch set 3:Commit-Queue +1
8 comments:
Patchset:
PTAL
File remoting/host/installer/win/chromoting.wxs:
<!-- RegisteredApplications -->
<!-- This will be deployed directly to Googlers: -->
<!--
<RegistryKey Id="remote_open_url_registered_applications_reg_key"
Root="HKLM"
Key="SOFTWARE\RegisteredApplications"
Action="create">
<RegistryValue Type="string"
Name="$(var.UrlForwarderName)"
Value="SOFTWARE\$(var.ChromotingKeyPath)\UrlForwarder\Capabilities"/>
</RegistryKey>
-->
I think this blob should be removed since the requirement is already documented in a . […]
Done
File remoting/host/remote_open_url_constants.h:
Patch Set #1, Line 24:
IsRemoteOpenUrlSupported
I did a quick spot check and it looks like there are ~23 files in //remoting/host that are associate […]
Done. Will move them to a subdir in a future CL.
Patch Set #1, Line 24:
IsRemoteOpenUrlSupported
I did a quick spot check and it looks like there are ~23 files in //remoting/host that are associate […]
Done
File remoting/host/remote_open_url_constants.cc:
Patch Set #2, Line 69:
fined(OS_WIN)Don't we want to check |is_googler| for Windows as well?
I think that not checking this can allow external users or Googlers using a non-google account to test or enable the feature (say b/183135000#comment68) the reason we have to check it on Linux is that we can't make puppet apply the host config in a cascading manner.
Now I think about it, I think we could have done something similar for Linux, say we deploy the crd-url-forwarder.desktop file[1] to Googlers only, and use the existence of that file to determine if the feature is supported. It's hard to make that switch now though, since the feature has already been up and running.
which is currently only deployed to
// Googlers' devices.
Can we make this more generic: […]
Done
Failed to open key HKLM\\" << kRegisteredApplicationsKeyName
<< ", result: " << re
WDYT about updating this message to indicate that this means the URL forwarding capability cannot be […]
Done
File remoting/host/remoting_me2me_host.cc:
Patch Set #1, Line 1583:
IsRemoteOpenUrlSupported
I was thinking that we would query the registry key for each connection. […]
Done.
If we view set_enable_remote_open_url() as setting the preference to enable the feature, and view IsRemoteOpenUrlSupported() as determining if the platform supports the future, then I think it makes more sense to move the windows version check to IsRemoteOpenUrlSupported().
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Attention is currently required from: Yuwei Huang.
Patch set 3:Code-Review +1
2 comments:
Patchset:
lgtm in general but I would like to discuss how we roll out the feature externally (i.e. should this be gated to a subset of users until we are 100% sure we are ready).
File remoting/host/remote_open_url_constants.cc:
Patch Set #2, Line 69:
fined(OS_WIN)
I think that not checking this can allow external users or Googlers using a non-google account to te […]
That is part of it, the reason I recall was that originally you were going to put an is_googler check in the client and only expose the UI in that case. Moving it to the host was reasonable as there is no external release deadline and we can ensure the feature is working internally, make any necessary fixes, then remove the is_googler check if we decide to.
I would like to be consistent between the two platforms so my preference is that we leave the check in for both (until we feel ready to release externally).
I'm not a fan of allowing unsupported features to be enabled before we are ready for release. If for example a blog is written about how to enable the feature, then we change the impl (for whatever reason), we can communicate that to internal users (or update them dynamically) but we can't do the same for external users. Now our product looks like it did something wrong because someone figured out how to enable something that we didn't expect them to enable.
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Yuwei Huang (Gerrit)
Patch set 4:Commit-Queue +2
2 comments:
Patchset:
lgtm in general but I would like to discuss how we roll out the feature externally (i.e. […]
I'd imagine that we will need to change back to client side negotiation in that case. Though I think we should just forget about rolling it out externally and move on given the current configuration and planning :')
File remoting/host/remote_open_url_constants.cc:
Patch Set #2, Line 69:
fined(OS_WIN)
That is part of it, the reason I recall was that originally you were going to put an is_googler chec […]
Done
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Attention is currently required from: Yuwei Huang.
Patch set 4:Code-Review +1
1 comment:
Patchset:
I'd imagine that we will need to change back to client side negotiation in that case. […]
Possibly, it depends on the rollout plan. Either way I would agree that it's a future issue. I think we should be consistent in how the flag is applied and whether we ever expect external folks to be able to enable this.
To view, visit change 3154847. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[remoting host] Host decides if URL forwarder is supported
1. Add registry keys for URL forwarding back to the wxs, except the
RegisteredApplications value, which will be deployed by puppet.
2. Make the host determine if the feature is enabled by either checking
the RegisteredApplications value exist (Windows), or checking if the
user is a Googler. The client can be later modified to always enable
the feature, as the support will be fully controlled by the host.
Bug: b:183135000
Change-Id: I597e14d85b3b1897136aed789888495000c05a61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3154847
Commit-Queue: Yuwei Huang <yuw...@chromium.org>
Reviewed-by: Joe Downing <joe...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#920975}
---
M remoting/host/BUILD.gn
M remoting/host/desktop_environment_options.h
M remoting/host/desktop_session_proxy.cc
M remoting/host/installer/win/chromoting.wxs
M remoting/host/me2me_desktop_environment.cc
A remoting/host/remote_open_url_util.cc
A remoting/host/remote_open_url_util.h
M remoting/host/remoting_me2me_host.cc
M remoting/host/win/url_forwarder_configurator_main.cc
9 files changed, 150 insertions(+), 90 deletions(-)