Chromium Code Reviews Chromium Code Reviews
Issue
830193002:
Using PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac. (Closed)
DescriptionUsing PolicyServiceWatcher instead of PolicyWatcherLinux/Win/Mac.
This changelist removes PolicyWatcherLinux/Win/Mac from
src/remoting/host/policy_hack (which had most of the code copied from
various classes that are now refactored into src/components/policy) and
replaces these classes with PolicyServiceWatcher that wraps a
PolicyService from src/components/policy. PolicyServiceWatcher is based
on a mostly unchanged PolicyWatcherChromeOS, which has been made
applicable to other OS-es, by enabling wrapping an *owned* PolicyService
(and by adding code that creates the owned PolicyService).
This changelist focuses on making minimal changes that:
1) preserve current behavior of PolicyWatcher
2) replace PolicyWatcherLinux/Win/Mac with PolicyServiceWatcher
The focused/minimal nature of the changelist means that some undesirable
or questionable aspects of PolicyWatcher remain untouched. For example:
A) src/remoting/host/policy_hack uses same policy dir path on Chrome
and Chromium (unlike src/chrome/browser/policy)
B) PolicyWatcher still contains quite a bit of business logic
(i.e. for filling out default values or using special fallback
values in case of incorrect value types)
C) PolicyWatcher's users consume policies via base::DictionaryValue
rather than policy::PolicyMap
D) Users of PolicyWatcher have to be aware of threading gotchas (i.e.
It2MeHost::OnPolicyUpdate has to account for the fact that on
ChromeOS, PolicyWatcher's callbacks are called on browser's UI thread
rather than on Chromoting's network_task_runner)
The changelist fails to preserve the following behavior of PolicyWatcher:
*) Policy parsing errors are not surfaced anymore to users of PolicyWatcher
Not retaining this behavior is probably okay because:
i) If the security lock down response to parsing errors is desirable, then we
should detect and surface this from component/policy, so that all Chrome
policies benefit (i.e. security-sensitive AlwaysAuthorizePlugins,
AudioCaptureAllowed or URLBlacklist)
ii) Code reuse from the current changelist allows detecting currently
undetected policy authoring problems in Chromoting scenarios (i.e.
by making it possible to validate policy value types via policy::Schema).
iii) Current policy-errors behavior is far from bullet-proof:
- Misspelled policy names are inherently undetectable
- Invalid types of policy values are ignored on Windows and Mac
(as mentioned above in (ii) and tracked by crbug.com/427513)
- Up until recently (until changelist here:
https://chromium.googlesource.com/chromium/src.git/+/142f8e1d3e4b75c901f33cb771cd848019630c1e)
malformed json files prevented chromoting host from starting but did
*not* stop an already running host (only prevented the already running
host from ever refreshing policies from other, still parseable files).
iv) Reuniting remoting/host/policy_hack with components/policy is automatically
bringing in other features (i.e. detecting policy updates on Mac).
The changelist has been tested by manually dumping and verifying policy
contents via the newly added unit test on Linux, Windows and Mac.
BUG=368321
Committed: https://crrev.com/d5a6bda8b9cb6db9ee89e20e889916072a738ed4
Cr-Commit-Position: refs/heads/master@{#311994}
Patch Set 1 #Patch Set 2 : Fixed building for Chrome OS. #
Total comments: 44
Patch Set 3 : Addressed code review feedback from Mattias. #
Total comments: 16
Patch Set 4 : Addressed 2nd round of feedback from Mattias. #Patch Set 5 : Reuploading with lower "similarity". #
Total comments: 16
Patch Set 6 : Addressed code review feedback from Sergey. #
Total comments: 6
Patch Set 7 : Addressed code review feedback from Sergey. #
Total comments: 2
Patch Set 8 : Fixed the incorrect edit of BUILD.gn. #Patch Set 9 : Rebasing... #Messages
Total messages: 24 (6 generated)
|
