Modify ProtocolHandlerRegistry to make room for policy

Modify ProtocolHandlerRegistry to make room for policy

This CL modifies ProtocolHandlerRegistry to enable addition of policy for registering and ignoring protocol handlers. The current implementation does not allow it since it saves the registered handlers, which would result in policy leak when directly mapped, i.e., protocol handler registered via policy would be saved to disk and mixed with user preference.

BUG=116119
TBR=koz,jochen

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

[kaliamoorthi, 2014-05-20 12:56:15]

Hi Jochen,

I am changing protocol handler to add a policy for it.

Its a WIP. I wanted to get my approach verified.

PTAL.

[jochen (gone - plz use gerrit), 2014-05-21 09:20:57]

as discussed offline, somebody more familiar with protocol handlers should look
at this (tony/koz)

[kaliamoorthi, 2014-05-21 09:35:06]

Hi Tony/James,

I am changing protocol handler to add a policy for it.

Its a WIP. I wanted to get my approach verified.

PTAL.

[tony, 2014-05-21 19:39:53]

I defer to koz.

[koz (OOO until 15th September), 2014-05-22 00:50:21]

The approach looks ok to me. Can you please add some tests to exercise this
logic?

[koz (OOO until 15th September), 2014-05-22 00:51:31]

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:446:
std::vector<const base::DictionaryValue*> registered_handlers =
It might be worth introducing some helper functions here, e.g.

RegisterHandlersFromPref(prefs::kPolicyRegisteredProtocolHandlers, true);
IgnoreHandlersFromPref(prefs::kPolicyIgnoredProtocolHandlers, true);

RegisterHandlersFromPref(prefs::kRegisteredProtocolHandlers, false);
IgnoreHandlersFromPref(prefs::kIgnoredProtocolHandlers, false);

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:617: Save();
Save() should be called after removing handler from ignored_protocol_handlers_
below.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:921: return
(std::find(list.begin(), list.end(), handler) != list.end());
nit: no need for parens

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.h:291: const bool
policy_managed);
Can you replace the bool here with an enum, e.g.

enum HandlerSource {
  USER,
  POLICY,
}

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.h:320:
ProtocolHandlerMultiMap user_protocol_handlers_;
I think this comment could be a bit clearer, e.g.

These maps track the source of protocol handler registrations for the purposes
of disallowing the removal of handlers that are registered by policy. Every
entry in protocol_handlers_ should exist in at least one of the user/policy
maps. Likewise for ignored_protocol_handlers_.

[kaliamoorthi, 2014-05-28 08:40:20]

Thanks for the feedback. I'll send a new revision that fixes these comments
along with the unit test for the change.

On 2014/05/22 00:51:31, koz wrote:
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> chrome/browser/custom_handlers/protocol_handler_registry.cc:446:
> std::vector<const base::DictionaryValue*> registered_handlers =
> It might be worth introducing some helper functions here, e.g.
> 
> RegisterHandlersFromPref(prefs::kPolicyRegisteredProtocolHandlers, true);
> IgnoreHandlersFromPref(prefs::kPolicyIgnoredProtocolHandlers, true);
> 
> RegisterHandlersFromPref(prefs::kRegisteredProtocolHandlers, false);
> IgnoreHandlersFromPref(prefs::kIgnoredProtocolHandlers, false);
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> chrome/browser/custom_handlers/protocol_handler_registry.cc:617: Save();
> Save() should be called after removing handler from ignored_protocol_handlers_
> below.
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> chrome/browser/custom_handlers/protocol_handler_registry.cc:921: return
> (std::find(list.begin(), list.end(), handler) != list.end());
> nit: no need for parens
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> File chrome/browser/custom_handlers/protocol_handler_registry.h (right):
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> chrome/browser/custom_handlers/protocol_handler_registry.h:291: const bool
> policy_managed);
> Can you replace the bool here with an enum, e.g.
> 
> enum HandlerSource {
>   USER,
>   POLICY,
> }
> 
>
https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
> chrome/browser/custom_handlers/protocol_handler_registry.h:320:
> ProtocolHandlerMultiMap user_protocol_handlers_;
> I think this comment could be a bit clearer, e.g.
> 
> These maps track the source of protocol handler registrations for the purposes
> of disallowing the removal of handlers that are registered by policy. Every
> entry in protocol_handlers_ should exist in at least one of the user/policy
> maps. Likewise for ignored_protocol_handlers_.

[kaliamoorthi, 2014-05-30 10:01:09]

The revision includes unit tests and addresses your old comments.

PTAL.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:446:
std::vector<const base::DictionaryValue*> registered_handlers =
On 2014/05/22 00:51:31, koz wrote:
> It might be worth introducing some helper functions here, e.g.
> 
> RegisterHandlersFromPref(prefs::kPolicyRegisteredProtocolHandlers, true);
> IgnoreHandlersFromPref(prefs::kPolicyIgnoredProtocolHandlers, true);
> 
> RegisterHandlersFromPref(prefs::kRegisteredProtocolHandlers, false);
> IgnoreHandlersFromPref(prefs::kIgnoredProtocolHandlers, false);

Done.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:617: Save();
On 2014/05/22 00:51:31, koz wrote:
> Save() should be called after removing handler from ignored_protocol_handlers_
> below.

Please note that only user_ignored_protocol_handlers are saved.
ignored_protocol_handlers which is the in memory merged representation is not
saved, since it includes handlers from policy.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:921: return
(std::find(list.begin(), list.end(), handler) != list.end());
On 2014/05/22 00:51:31, koz wrote:
> nit: no need for parens

Done.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.h:291: const bool
policy_managed);
On 2014/05/22 00:51:31, koz wrote:
> Can you replace the bool here with an enum, e.g.
> 
> enum HandlerSource {
>   USER,
>   POLICY,
> }

Done.

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.h:320:
ProtocolHandlerMultiMap user_protocol_handlers_;
On 2014/05/22 00:51:31, koz wrote:
> I think this comment could be a bit clearer, e.g.
> 
> These maps track the source of protocol handler registrations for the purposes
> of disallowing the removal of handlers that are registered by policy. Every
> entry in protocol_handlers_ should exist in at least one of the user/policy
> maps. Likewise for ignored_protocol_handlers_.

Done.

[koz (OOO until 15th September), 2014-06-01 23:44:28]

lgtm, thanks

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/293983004/diff/1/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:617: Save();
On 2014/05/30 10:01:09, kaliamoorthi wrote:
> On 2014/05/22 00:51:31, koz wrote:
> > Save() should be called after removing handler from
ignored_protocol_handlers_
> > below.
> 
> Please note that only user_ignored_protocol_handlers are saved.
> ignored_protocol_handlers which is the in memory merged representation is not
> saved, since it includes handlers from policy.

Ah, my bad. In that case feel free to revert to how you had the code before.

https://codereview.chromium.org/293983004/diff/20001/chrome/browser/custom_ha...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/293983004/diff/20001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:972:
GetProtocolHandlerValueWithDefault("p1", "http://p1u2.com/%s", true));
Cool tests! One nit: Would this be easier to read with the URL string literals
replaced with constants?

[kaliamoorthi, 2014-06-02 09:47:30]

Thanks for the review. Your final comments are addressed.

https://codereview.chromium.org/293983004/diff/20001/chrome/browser/custom_ha...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/293983004/diff/20001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:972:
GetProtocolHandlerValueWithDefault("p1", "http://p1u2.com/%s", true));
On 2014/06/01 23:44:29, koz wrote:
> Cool tests! One nit: Would this be easier to read with the URL string literals
> replaced with constants?

Done.

[kaliamoorthi, 2014-06-02 09:48:50]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-06-02 09:49:44]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/293983004/40001

[commit-bot: I haz the power, 2014-06-02 12:50:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-02 12:52:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 12:52:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[kaliamoorthi, 2014-06-02 13:12:37]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-06-02 13:13:03]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/293983004/40001

[commit-bot: I haz the power, 2014-06-02 13:16:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-02 13:19:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 13:19:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[kaliamoorthi, 2014-06-02 13:21:52]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-06-02 13:22:40]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/293983004/40001

[commit-bot: I haz the power, 2014-06-02 13:26:45]

Change committed as 274223