Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2854005:
Enable policy support on the Linux platform. (Closed)
|
Created:
10 years, 6 months ago by markusheintz Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
DescriptionEnable policy support on the Linux platform.
On the Linux platform policies are read from a configuration file directory. Depending whether the chromium build is branded or not the configuration file directory is:
/etc/opt/chromium or
/etc/opt/chrome
The configuration file directory will contain two sub-subdirectories: policies/managed
policies/recommended
The sub-directory policies/managed will contain all managed policies that are enforced on the user.
The sub-directory policies/recommended will contain policies for recommended setting that can be changed by a user.
BUG=47278
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50589
Patch Set 1 #
Total comments: 14
Patch Set 2 : Worked on Mattias code-review comments. #
Total comments: 6
Patch Set 3 : Fix lint errors and some other nits. #Patch Set 4 : Add unittest for chrome_path[.h,.cc] changes #
Total comments: 14
Patch Set 5 : Worked on code review feeback. #
Total comments: 6
Patch Set 6 : Worked on codereview feedback and rm chrome_path_unittest for now. #Patch Set 7 : Modify gypi file after removing chrome_paths_unittest. #
Messages
Total messages: 13 (0 generated)
@Danno: On more CL :) @Even: I'd like to get your input if this is a sensible approach for the linux platform. Also I'm not sure if using the path_service like I did it is a good idea. @all: Further more I plan to add a #define entry to the gyp file that would allow changing the configuration file directory at compile time. This would allow distributors to change the configuration file directory at their taste.
http://codereview.chromium.org/2854005/diff/1/2 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/1/2#newcode109 chrome/browser/pref_service.cc:109: // TODO(markusheintz): Test if the path exists I think you actually don't want to test whether the path exists, since it is perfectly valid for the directory to be missing but created later. Once we have runtime reloading, the policy mechanism should pick up the now existing policy definition. http://codereview.chromium.org/2854005/diff/1/2#newcode114 chrome/browser/pref_service.cc:114: } How about recommended prefs? Shouldn't you be initializing another ConfigDirPolicyProvider for the recommended directory here? http://codereview.chromium.org/2854005/diff/1/3 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/1/3#newcode92 chrome/common/chrome_paths.cc:92: case chrome::DIR_CONFIGURATION_FILES: Indentation http://codereview.chromium.org/2854005/diff/1/3#newcode97 chrome/common/chrome_paths.cc:97: //#endif I guess you want to remove the comments here :) http://codereview.chromium.org/2854005/diff/1/3#newcode100 chrome/common/chrome_paths.cc:100: break; Seems this whole block is in the middle of the chrome::DIR_LOGS label. Furthermore, I get the impression it does not belong here, since the first switch is meant to resolve aliases (comment says so), no? And you have essentially the same logic in the second switch below. http://codereview.chromium.org/2854005/diff/1/3#newcode305 chrome/common/chrome_paths.cc:305: return false; You don't want to create the directory, but we should still return true since the path has been successfully resolved, no? http://codereview.chromium.org/2854005/diff/1/4 File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/2854005/diff/1/4#newcode41 chrome/common/chrome_paths.h:41: DIR_CONFIGURATION_FILES, // Directory for policy configuration Comment does not match how the directory is used. Maybe something like "System-wide readonly configuration directory"?
In addition to the stuff Mattias pointed out: * Lint should run through cleanly for chrome/common/chrome_paths.cc * Any chance that you can add unit tests for the new chrome_paths.cc functionality? I know, there's currently aren't any, however, they might have caught the case that Mattias points out.
http://codereview.chromium.org/2854005/diff/1/2 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/1/2#newcode109 chrome/browser/pref_service.cc:109: // TODO(markusheintz): Test if the path exists Great! I wasn't exactly sure about that and wanted to ask you. On 2010/06/16 09:24:34, mnissler wrote: > I think you actually don't want to test whether the path exists, since it is > perfectly valid for the directory to be missing but created later. Once we have > runtime reloading, the policy mechanism should pick up the now existing policy > definition. http://codereview.chromium.org/2854005/diff/1/2#newcode114 chrome/browser/pref_service.cc:114: } On 2010/06/16 09:24:34, mnissler wrote: > How about recommended prefs? Shouldn't you be initializing another > ConfigDirPolicyProvider for the recommended directory here? Done for the Linux platform only. Added DummyPrefStores for all others platforms. http://codereview.chromium.org/2854005/diff/1/3 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/1/3#newcode92 chrome/common/chrome_paths.cc:92: case chrome::DIR_CONFIGURATION_FILES: Block removed. On 2010/06/16 09:24:34, mnissler wrote: > Indentation http://codereview.chromium.org/2854005/diff/1/3#newcode97 chrome/common/chrome_paths.cc:97: //#endif Block removed. On 2010/06/16 09:24:34, mnissler wrote: > I guess you want to remove the comments here :) http://codereview.chromium.org/2854005/diff/1/3#newcode100 chrome/common/chrome_paths.cc:100: break; This block is not supposed to be here. It's a far outdated version of some work in progress. Probably a past accident. On 2010/06/16 09:24:34, mnissler wrote: > Seems this whole block is in the middle of the chrome::DIR_LOGS label. > Furthermore, I get the impression it does not belong here, since the first > switch is meant to resolve aliases (comment says so), no? And you have > essentially the same logic in the second switch below. http://codereview.chromium.org/2854005/diff/1/3#newcode305 chrome/common/chrome_paths.cc:305: return false; "If (!file_util::PathExists(cur)) " means that the path does not exist. Or am I wrong :) ? => return false since we could not resolve the path On 2010/06/16 09:24:34, mnissler wrote: > You don't want to create the directory, but we should still return true since > the path has been successfully resolved, no? http://codereview.chromium.org/2854005/diff/1/4 File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/2854005/diff/1/4#newcode41 chrome/common/chrome_paths.h:41: DIR_CONFIGURATION_FILES, // Directory for policy configuration On 2010/06/16 09:24:34, mnissler wrote: > Comment does not match how the directory is used. Maybe something like > "System-wide readonly configuration directory"? Done.
lint seems to still be seriously upset, too. http://codereview.chromium.org/2854005/diff/8001/9001 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/8001/9001#newcode103 chrome/browser/pref_service.cc:103: recommended_prefs_provider = new ConfigurationPolicyProviderWin(); I think this should be the dummy provider, since we don't have a real solution to this on windows, yet. http://codereview.chromium.org/2854005/diff/8001/9001#newcode107 chrome/browser/pref_service.cc:107: recommended_prefs_provider = new ConfigurationPolicyProviderWin(); should likely be the dummy provider. certainly shouldn't be the windows provider. does this even compile? http://codereview.chromium.org/2854005/diff/8001/9003 File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/2854005/diff/8001/9003#newcode41 chrome/common/chrome_paths.h:41: DIR_CONFIGURATION_FILES, // Directory for system-wide read-only is this new directory just for policy, or are there other configuration files that do/will go there? if not, it probably should have a more meaningful policy name, like "DIR_POLICY_FILES" or "DIR_CONFIGURATION_POLICY_FILES".
http://codereview.chromium.org/2854005/diff/8001/9001 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/8001/9001#newcode103 chrome/browser/pref_service.cc:103: recommended_prefs_provider = new ConfigurationPolicyProviderWin(); On 2010/06/16 13:56:35, danno wrote: > I think this should be the dummy provider, since we don't have a real solution > to this on windows, yet. Done. http://codereview.chromium.org/2854005/diff/8001/9001#newcode107 chrome/browser/pref_service.cc:107: recommended_prefs_provider = new ConfigurationPolicyProviderWin(); On 2010/06/16 13:56:35, danno wrote: > should likely be the dummy provider. certainly shouldn't be the windows > provider. does this even compile? Done. http://codereview.chromium.org/2854005/diff/8001/9003 File chrome/common/chrome_paths.h (right): http://codereview.chromium.org/2854005/diff/8001/9003#newcode41 chrome/common/chrome_paths.h:41: DIR_CONFIGURATION_FILES, // Directory for system-wide read-only I'm thinking about this right now. And I tent to agree with you. On 2010/06/16 13:56:35, danno wrote: > is this new directory just for policy, or are there other configuration files > that do/will go there? if not, it probably should have a more meaningful policy > name, like "DIR_POLICY_FILES" or "DIR_CONFIGURATION_POLICY_FILES".
Chromium should use /etc/chromium, chrome should use /opt/google/chrome/etc or something like that. I think distros can just carry a patch if they want to change it (brevity due to phone) On Jun 16, 2010 1:57 AM, <markusheintz@google.com> wrote: > Reviewers: danno, Evan Martin, > > Message: > @Danno: On more CL :) > > @Even: I'd like to get your input if this is a sensible approach for the > linux > platform. Also I'm not sure if using the path_service like I did it is a > good > idea. > > @all: Further more I plan to add a #define entry to the gyp file that would > allow changing the configuration file directory at compile time. This would > allow distributors to change the configuration file directory at their > taste. > > Description: > Enable policy support on the Linux platform. > > On the Linux platform policies are read from a configuration file directory. > Depending whether the chromium build is branded or not the configuration > file > directory is: > /etc/opt/chromium or > /etc/opt/chrome > > The configuration file directory will contain two sub-subdirectories: > policies/managed > policies/recommended > > The sub-directory policies/managed will contain all managed policies that > are > enforced on the user. > > The sub-directory policies/recommended will contain policies for recommended > setting that can be changed by a user. > > Please review this at http://codereview.chromium.org/2854005/show > > SVN Base: http://src.chromium.org/git/chromium.git > > Affected files: > M chrome/browser/pref_service.cc > M chrome/common/chrome_paths.h > M chrome/common/chrome_paths.cc > >
On Thu, Jun 17, 2010 at 7:21 AM, Evan Martin <evan@chromium.org> wrote: > Chromium should use /etc/chromium, chrome should use /opt/google/chrome/etc > or something like that. I think distros can just carry a patch if they want > to change it > Note that the filesystem hierachy standard mandates /etc/opt/ for software installed in /opt: http://www.pathname.com/fhs/pub/fhs-2.3.html#ETCOPTCONFIGURATIONFILESFOROPT <http://www.pathname.com/fhs/pub/fhs-2.3.html#ETCOPTCONFIGURATIONFILESFOROPT> > (brevity due to phone) > > On Jun 16, 2010 1:57 AM, <markusheintz@google.com> wrote: > > Reviewers: danno, Evan Martin, > > > > Message: > > @Danno: On more CL :) > > > > @Even: I'd like to get your input if this is a sensible approach for the > > linux > > platform. Also I'm not sure if using the path_service like I did it is a > > good > > idea. > > > > @all: Further more I plan to add a #define entry to the gyp file that > would > > allow changing the configuration file directory at compile time. This > would > > allow distributors to change the configuration file directory at their > > taste. > > > > Description: > > Enable policy support on the Linux platform. > > > > On the Linux platform policies are read from a configuration file > directory. > > Depending whether the chromium build is branded or not the configuration > > file > > directory is: > > /etc/opt/chromium or > > /etc/opt/chrome > > > > The configuration file directory will contain two sub-subdirectories: > > policies/managed > > policies/recommended > > > > The sub-directory policies/managed will contain all managed policies that > > > are > > enforced on the user. > > > > The sub-directory policies/recommended will contain policies for > recommended > > setting that can be changed by a user. > > > > Please review this at http://codereview.chromium.org/2854005/show > > > > SVN Base: http://src.chromium.org/git/chromium.git > > > > Affected files: > > M chrome/browser/pref_service.cc > > M chrome/common/chrome_paths.h > > M chrome/common/chrome_paths.cc > > > > >
http://codereview.chromium.org/2854005/diff/18001/19001 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/18001/19001#newcode112 chrome/browser/pref_service.cc:112: if (success) { Why not collapse these three lines into if (PathService::Get(...)) { ? http://codereview.chromium.org/2854005/diff/18001/19001#newcode124 chrome/browser/pref_service.cc:124: // The ConfigurationPolicyPrefStore takes the ownership of the passed If you're fixing the grammar, it should be "takes ownership", without the "the". :) http://codereview.chromium.org/2854005/diff/18001/19003 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/18001/19003#newcode289 chrome/common/chrome_paths.cc:289: cur = * new FilePath(FILE_PATH_LITERAL("/etc/opt/chrome/policies")); Doesn't this immediately leak? Should be cur = FilePath(...) http://codereview.chromium.org/2854005/diff/18001/19003#newcode291 chrome/common/chrome_paths.cc:291: cur = * new FilePath(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); Thanks for catching me on the FHS thing! http://codereview.chromium.org/2854005/diff/18001/19005 File chrome/common/chrome_paths_unittest.cc (right): http://codereview.chromium.org/2854005/diff/18001/19005#newcode5 chrome/common/chrome_paths_unittest.cc:5: #if (!defined OS_MACOSX) && defined(OS_POSIX) Maybe just wrap this around the test on line 15 or so? It's not that chrome_paths can't be tested on other platforms, it's just that the only test we have is currently for these specific platforms. http://codereview.chromium.org/2854005/diff/18001/19005#newcode13 chrome/common/chrome_paths_unittest.cc:13: }; You don't need to declare this; just use TEST(...) below rather than TEST_F(). The first param to TEST() can stay the same and it will compile. http://codereview.chromium.org/2854005/diff/18001/19005#newcode27 chrome/common/chrome_paths_unittest.cc:27: # endif s/ //
http://codereview.chromium.org/2854005/diff/18001/19001 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/18001/19001#newcode112 chrome/browser/pref_service.cc:112: if (success) { On 2010/06/21 15:38:45, Evan Martin wrote: > Why not collapse these three lines into > if (PathService::Get(...)) { > ? Done. http://codereview.chromium.org/2854005/diff/18001/19001#newcode124 chrome/browser/pref_service.cc:124: // The ConfigurationPolicyPrefStore takes the ownership of the passed On 2010/06/21 15:38:45, Evan Martin wrote: > If you're fixing the grammar, it should be "takes ownership", without the "the". > :) Done. :) http://codereview.chromium.org/2854005/diff/18001/19003 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/18001/19003#newcode289 chrome/common/chrome_paths.cc:289: cur = * new FilePath(FILE_PATH_LITERAL("/etc/opt/chrome/policies")); On 2010/06/21 15:38:45, Evan Martin wrote: > Doesn't this immediately leak? > Should be > cur = FilePath(...) Done. http://codereview.chromium.org/2854005/diff/18001/19003#newcode291 chrome/common/chrome_paths.cc:291: cur = * new FilePath(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); On 2010/06/21 15:38:45, Evan Martin wrote: > Thanks for catching me on the FHS thing! :) Thanks to Mattias. http://codereview.chromium.org/2854005/diff/18001/19005 File chrome/common/chrome_paths_unittest.cc (right): http://codereview.chromium.org/2854005/diff/18001/19005#newcode5 chrome/common/chrome_paths_unittest.cc:5: #if (!defined OS_MACOSX) && defined(OS_POSIX) On 2010/06/21 15:38:45, Evan Martin wrote: > Maybe just wrap this around the test on line 15 or so? It's not that > chrome_paths can't be tested on other platforms, it's just that the only test we > have is currently for these specific platforms. Done. http://codereview.chromium.org/2854005/diff/18001/19005#newcode13 chrome/common/chrome_paths_unittest.cc:13: }; On 2010/06/21 15:38:45, Evan Martin wrote: > You don't need to declare this; just use TEST(...) below rather than TEST_F(). > The first param to TEST() can stay the same and it will compile. Done. http://codereview.chromium.org/2854005/diff/18001/19005#newcode27 chrome/common/chrome_paths_unittest.cc:27: # endif On 2010/06/21 15:38:45, Evan Martin wrote: > s/ // Done.
LGTM with few remaining nits fixed http://codereview.chromium.org/2854005/diff/27001/15004 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/27001/15004#newcode116 chrome/browser/pref_service.cc:116: // error no configuration dir found. Consider making this comment more clear (not quite sure what it's saying; perhaps it's superfluous anyway). http://codereview.chromium.org/2854005/diff/27001/15006 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/27001/15006#newcode291 chrome/common/chrome_paths.cc:291: cur = FilePath(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); /etc/chromium http://codereview.chromium.org/2854005/diff/27001/15008 File chrome/common/chrome_paths_unittest.cc (right): http://codereview.chromium.org/2854005/diff/27001/15008#newcode15 chrome/common/chrome_paths_unittest.cc:15: FilePath expected_path(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); /etc/chromium
I made a judgement call an removed the chrome_path_unittest.cc because it has some issues that I think are not worth solving right now. I cannot create the /etc/chromium directories on the try bots and hence this test will always fail. Re-factoring the code to inject a different path just for testing doesn't seem right to me. The result is low and the effort hight. Good ideas are welcome otherwise I dismiss the unittest. http://codereview.chromium.org/2854005/diff/27001/15004 File chrome/browser/pref_service.cc (right): http://codereview.chromium.org/2854005/diff/27001/15004#newcode116 chrome/browser/pref_service.cc:116: // error no configuration dir found. I decided to delete the comment since it is not adding any additional information. I think the "If-else" is self explanatory in this case. On 2010/06/21 19:26:34, Evan Martin wrote: > Consider making this comment more clear (not quite sure what it's saying; > perhaps it's superfluous anyway). http://codereview.chromium.org/2854005/diff/27001/15006 File chrome/common/chrome_paths.cc (right): http://codereview.chromium.org/2854005/diff/27001/15006#newcode291 chrome/common/chrome_paths.cc:291: cur = FilePath(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); On 2010/06/21 19:26:34, Evan Martin wrote: > /etc/chromium Done. http://codereview.chromium.org/2854005/diff/27001/15008 File chrome/common/chrome_paths_unittest.cc (right): http://codereview.chromium.org/2854005/diff/27001/15008#newcode15 chrome/common/chrome_paths_unittest.cc:15: FilePath expected_path(FILE_PATH_LITERAL("/etc/opt/chromium/policies")); On 2010/06/21 19:26:34, Evan Martin wrote: > /etc/chromium Done.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
