Add a policy to control clipboard size limit for Chrome Remote Desktop [chromium/src : main]
Joe Downing (Gerrit)
Attention is currently required from: Jamie Walch.
1 comment:
Patchset:
PTAL!
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Jamie Walch (Gerrit)
Attention is currently required from: Joe Downing.
Patch set 2:Code-Review +1
4 comments:
Patchset:
LGTM with some suggested wording simplifications.
File components/policy/resources/policy_templates.json:
Patch Set #2, Line 2710:
2147483647I could see someone trying to work around the WebRTC message size limitation by setting this to 2147483647, which won't work. It's difficult to set a true maximum here, because it depends on the WebRTC implementation, and it's a bit awkward to phrase the description to document that limitation so this is just FYI, unless you want to change anything.
Patch Set #2, Line 2722:
effectivelyNo need for "effectively" either, I'd say.
Patch Set #2, Line 2722:
not exceed this value for each event. If an event contains more data than is allowed, the payload willI think you can cut everything from "not" to "will" here without losing any information.
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
4 comments:
Patchset:
Thanks!
File components/policy/resources/policy_templates.json:
I could see someone trying to work around the WebRTC message size limitation by setting this to 2147 […]
I agree with this but don't have a better solution. Setting a maximum value is required so I can't drop the field and I didn't want to add WebRTC impl details into the policy if I could avoid it (especially if we decide to fix the message dropping behavior).
I'll add a blurb that the actual message size is subject to data channel restrictions as that should help clarify the behavior for the folks setting the policy.
Patch Set #2, Line 2722:
effectivelyNo need for "effectively" either, I'd say.
Done
Patch Set #2, Line 2722:
not exceed this value for each event. If an event contains more data than is allowed, the payload willI think you can cut everything from "not" to "will" here without losing any information.
Done
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Attention is currently required from: Maksim Ivanov.
1 comment:
Patchset:
Adding emaxx@ for policy review. Thanks!
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Ivanov (Gerrit)
Attention is currently required from: Joe Downing.
4 comments:
File chrome/test/data/policy/policy_test_cases.json:
Patch Set #3, Line 587:
"reason_for_missing_test": "TODO(crbug.com/1213429) add test case"Please add a test for this new policy - that bug is tracking backfilling tests for existing policies, not the new ones.
File components/policy/resources/policy_templates.json:
8 MiB?
Just trying to understand... Is the default behavior - an infinite limit, i.e., 2147483647? (after truncating it to the data channel size)
So up to you whether to replace this with "2147483647", or keep this "none" to designate a special behavior that different from any other value.
File remoting/host/it2me/it2me_host.cc:
Patch Set #3, Line 372:
if (policies->GetInteger(policy::key::kRemoteAccessHostClipboardSizeBytes,Just in case - I don't think there's any validation of min/max values at this point. Normally these kind of things are validated at the point of transforming the policy value into a pref, which is usually done via a handler at configuration_policy_handler_list_factory.cc.
So if int is 64-bit, this code will accept much larger values than 2147483647 that's specified in policy_templates.json. Unless I'm missing something.
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Attention is currently required from: Maksim Ivanov.
5 comments:
Patchset:
PTAL! Thanks!
File chrome/test/data/policy/policy_test_cases.json:
Patch Set #3, Line 587:
"reason_for_missing_test": "TODO(crbug.com/1213429) add test case"
Please add a test for this new policy - that bug is tracking backfilling tests for existing policies […]
I was planning on submitting a separate CL to take care of all of these but AFAICT this isn't something that's needed for our component. None of these policies are read from or exposed through the browser so I'm not sure what would be specified here.
If you have an example where the policy has a test case but with no prefs or there is a specific format to use, please let me know. For now I've marked them as N/A.
File components/policy/resources/policy_templates.json:
8 MiB?
Argh, yeah I was thinking bits instead of bytes : P
Just trying to understand... Is the default behavior - an infinite limit, i.e. […]
Yeah the default is that we (CRD) won't enforce any limits. None seemed correct for this behavior but LMK if something else is more appropriate.
File remoting/host/it2me/it2me_host.cc:
Patch Set #3, Line 372:
if (policies->GetInteger(policy::key::kRemoteAccessHostClipboardSizeBytes,
Just in case - I don't think there's any validation of min/max values at this point. […]
None of these are exposed from the browser so I don't think prefs come into play (LMK if that's incorrect).
Our PolicyWatcher (https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/policy_watcher.h) relies on the PolicyService and AsyncPolicyLoader which handle schema validation (if one of values is entered incorrectly, then the host will not run).
I've verified that setting a value larger than max (e.g. 2147483648) is handled and the host logs an error:
ERROR 11676 20200 15:25:16-417 ../../remoting/host/policy_watcher.cc 263 Invalid policy contents: RemoteAccessHostClipboardSizeBytes: Invalid value for integerINFORMATION 11676 26140 15:25:16-418 ../../remoting/host/remoting_me2me_host.cc 1807 SendHostOfflineReason: sending POLICY_READ_ERROR.
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Ivanov (Gerrit)
Attention is currently required from: Joe Downing.
Patch set 4:Code-Review +1
4 comments:
Patchset:
Thanks!
File chrome/test/data/policy/policy_test_cases.json:
Patch Set #3, Line 587:
"reason_for_missing_test": "TODO(crbug.com/1213429) add test case"
I was planning on submitting a separate CL to take care of all of these but AFAICT this isn't someth […]
Ack - If there's no handling in the browser, then, indeed, this .json file won't be able to test it.
File components/policy/resources/policy_templates.json:
Yeah the default is that we (CRD) won't enforce any limits. […]
Ack
File remoting/host/it2me/it2me_host.cc:
Patch Set #3, Line 372:
if (policies->GetInteger(policy::key::kRemoteAccessHostClipboardSizeBytes,
None of these are exposed from the browser so I don't think prefs come into play (LMK if that's inco […]
Ack - I didn't know that the remoting code applies the schema validation itself (in PolicyWatcher::NormalizePolicies(), as it turns out to be). Good to know.
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Joe Downing (Gerrit)
Patch set 5:Commit-Queue +2
1 comment:
Patchset:
Thanks!
To view, visit change 3243274. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Add a policy to control clipboard size limit for Chrome Remote Desktop
Change-Id: I7cf6cac423a2a1b33ac1fa701c282a47aebcbeb0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3243274
Commit-Queue: Joe Downing <joe...@chromium.org>
Reviewed-by: Maksim Ivanov <em...@chromium.org>
Reviewed-by: Jamie Walch <jamie...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#935899}
---
M chrome/test/data/policy/policy_test_cases.json
M remoting/host/desktop_environment_options.h
M tools/metrics/histograms/enums.xml
M remoting/host/desktop_environment_options.cc
M remoting/host/remoting_me2me_host.cc
M remoting/host/policy_watcher_unittest.cc
M remoting/host/it2me/it2me_host.cc
M remoting/host/it2me/it2me_host.h
M components/policy/resources/policy_templates.json
M remoting/host/policy_watcher.cc
10 files changed, 126 insertions(+), 31 deletions(-)