Implement fetching, caching and retrieval of external policy data

Implement fetching, caching and retrieval of external policy data

This CL adds CloudExternalDataManagerBase, a fully functional
implementation of the ExternalDataManager interface that allows external
data referenced by policies to be fetched, verified, cached and retrieved.
The CL also extends policy_templates.json to allow policies referencing
external data to be specified.

There is no code in Chrome that instantiates CloudExternalDataManagerBase
yet. Follow-up CLs will create CloudExternalDataManagerBase instances for
desktop users, Chrome OS users and Chrome OS device-local accounts.

BUG=256635
TEST=New unit test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215888

[bartfab (slow), 2013-07-18 17:16:12]

Hi Joao,

Could you take a look please?

[Joao da Silva, 2013-07-19 11:15:15]

lgtm.

We may revisit some of this code & the policy-for-components code later, since
there's so much in common.

https://codereview.chromium.org/19462006/diff/1/chrome/app/policy/syntax_chec...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

https://codereview.chromium.org/19462006/diff/1/chrome/app/policy/syntax_chec...
chrome/app/policy/syntax_check_policy_template_json.py:318: 
This newline is strange, do you mind fixing it here and similar ones above?

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_external_data_manager_base.h (right):

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_external_data_manager_base.h:32: explicit
CloudExternalDataManagerBase(
No need to be explicit

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/cloud/c...
File chrome/browser/policy/cloud/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/cloud/c...
chrome/browser/policy/cloud/cloud_external_data_manager_base_unittest.cc:74:
void ClearFetchAttempted();
IIUC this class is meant to prevent creation of fetchers for unknown URLs, and
to check if that ever happened. If so then it would suffice to
ADD_FATAL_FAILURE() in CreateURLFetcher(), return NULL from there, and spin the
loop on TearDown to make sure no unexpected fetchers were started.

Also, these two are always used together in the tests, maybe combine into one?
(in case you don't take the suggestion above)

   bool GetFetchAttemptedAndClear()

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/externa...
File chrome/browser/policy/external_data_manager.h (right):

https://codereview.chromium.org/19462006/diff/1/chrome/browser/policy/externa...
chrome/browser/policy/external_data_manager.h:28: // does not exist on the
server), the |callback| will never be invoked.
Document what happens when Fetch is called multiple times for the same policy:

- does it fetch the external data twice?
- does it reuse previously fetched data?
- do all the callbacks get eventually called back?

https://codereview.chromium.org/19462006/diff/1/chrome/tools/build/generate_p...
File chrome/tools/build/generate_policy_source.py (right):

https://codereview.chromium.org/19462006/diff/1/chrome/tools/build/generate_p...
chrome/tools/build/generate_policy_source.py:74: self.max_size =
policy.get('max_size', 0)
Document "max_size" in the template

[bartfab (slow), 2013-07-19 13:06:29]

I addressed your comments and also made sure to exclude the test on Android.

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/app/policy/synt...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/app/policy/synt...
chrome/app/policy/syntax_check_policy_template_json.py:318: 
On 2013/07/19 11:15:15, Joao da Silva wrote:
> This newline is strange, do you mind fixing it here and similar ones above?

Done.

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
File chrome/browser/policy/cloud/cloud_external_data_manager_base.h (right):

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_external_data_manager_base.h:32: explicit
CloudExternalDataManagerBase(
On 2013/07/19 11:15:15, Joao da Silva wrote:
> No need to be explicit

Done.

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
File chrome/browser/policy/cloud/cloud_external_data_manager_base_unittest.cc
(right):

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_external_data_manager_base_unittest.cc:74:
void ClearFetchAttempted();
On 2013/07/19 11:15:15, Joao da Silva wrote:
> IIUC this class is meant to prevent creation of fetchers for unknown URLs, and
> to check if that ever happened. If so then it would suffice to
> ADD_FATAL_FAILURE() in CreateURLFetcher(), return NULL from there, and spin
the
> loop on TearDown to make sure no unexpected fetchers were started.
> 
> Also, these two are always used together in the tests, maybe combine into one?
> (in case you don't take the suggestion above)
> 
>    bool GetFetchAttemptedAndClear()

Done.

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
File chrome/browser/policy/external_data_manager.h (right):

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/browser/policy/...
chrome/browser/policy/external_data_manager.h:28: // does not exist on the
server), the |callback| will never be invoked.
On 2013/07/19 11:15:15, Joao da Silva wrote:
> Document what happens when Fetch is called multiple times for the same policy:
> 
> - does it fetch the external data twice?
> - does it reuse previously fetched data?
> - do all the callbacks get eventually called back?

I would not want to document this in ExternalDataManager as this is a generic
interface for which we may have platform-specific implementations in the future.
On some platforms, we may want to fetch the data every time it is requested
(e.g. Windows, if data is referenced via UNC paths), on others, we may want to
cache it (e.g. cloud policy, where data is referenced via URLs).

I added a comment to CloudExternalDataManagerBase, which is the cloud-specific
implementation with well-defined caching behavior.

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/tools/build/gen...
File chrome/tools/build/generate_policy_source.py (right):

https://chromiumcodereview.appspot.com/19462006/diff/1/chrome/tools/build/gen...
chrome/tools/build/generate_policy_source.py:74: self.max_size =
policy.get('max_size', 0)
On 2013/07/19 11:15:15, Joao da Silva wrote:
> Document "max_size" in the template

Done.

[commit-bot: I haz the power, 2013-08-06 11:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/19462006/27001

[commit-bot: I haz the power, 2013-08-06 14:00:11]

Change committed as 215888