Adding a DownloadRestrictions group policy.

Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for trivial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797
Review-Url: https://codereview.chromium.org/2674973003
Cr-Commit-Position: refs/heads/master@{#479465}
Committed: https://chromium.googlesource.com/chromium/src/+/4e9c44b9e4a05ba422bbe2f5d62b12d83071b94c

[MAD, 2017-02-03 20:00:40]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 20:01:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 20:06:25]

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

[commit-bot: I haz the power, 2017-02-03 20:06:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[MAD, 2017-02-03 20:07:58]

mad@chromium.org changed reviewers:
+ jochen@chromium.org, pastarmovj@chromium.org

[MAD, 2017-02-03 20:07:59]

As mentioned in the temporary title, this is not a complete CL to be reviewed,
it's mainly functioning code that I would like to validate as an approach to
implement a download restrictions policy. I still need to add support for bad
download URLs (though I don't know how yet), and add some more tests.

But most importantly, I don't really like the way I handle the cases where we
want to block file downloads that are marked as [potentially] bad by
safebrowsing. The only way I found to make it work is to replave the dangerous
flag with a file_block flag when calling OnContentCheckCompleted in the
appropriate cases. I wish there was another way of doing this but couldn't find
one. I feel we might need to add a BlockDownload API on the DownloadItem class,
but I'm not quite sure how to implement it safely. So before I embarck on this
journey, I would like to get your opinion about it.

Thanks!

BYE
MAD

[MAD, 2017-02-03 20:08:25]

Description was changed from

==========
INCOMPLETE CL to valide ideas about "Adding a DownloadREstrictions group
policy."

BUG=683797
==========

to

==========
INCOMPLETE CL to validate ideas about "Adding a DownloadREstrictions group
policy."

BUG=683797
==========

[MAD, 2017-02-03 20:19:02]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 20:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 20:36:17]

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

[commit-bot: I haz the power, 2017-02-03 20:36:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[MAD, 2017-02-03 20:39:27]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 20:40:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 21:33:52]

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

[commit-bot: I haz the power, 2017-02-03 21:33:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pastarmovj, 2017-02-06 07:45:00]

I see Asanka is already in the cc list but I think he will do great job being on
the reviewers lists. He helped me a lot implementing changes to the download
logic recently.

I don't know enough about the download code to offer much advice with the
questions you have and can only help as a policy code reviewer here.

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:1707: 'caption': '''All
downloads allowed, except for those that carry SafeBrowsing warnings.''',
I know this CL is only a prototype still but I will put some comments here that
are more nits than actual issues.

The caption is too long and won't show well in the policy editor on windows. I
would go with a short cation e.g. "Allow all downloads" and then have a
paragraph about what each option does explained in the description below.

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:1733: 'label': '''Allow
download restrictions''',
Download restrictions" might be a better name imho.

[MAD, 2017-02-06 19:41:32]

mad@chromium.org changed reviewers:
+ asanka@chromium.org

[MAD, 2017-02-06 19:41:33]

Updated the policy template descriptions to address comments, and also specified
the subtleties of download VS save/print. Is this what you meant pastramovj@?

Also moved asanka@ from CC to Reviewers, and started looking at a new option
(not uploaded the code yet), where we could modify the semantic of the cancel
method to also support policy blocked on top of user cancels or system
shutdowns.

Let me know if you have other comments or suggestions about the general
approach.

Thanks!

BYE
MAD

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:1707: 'caption': '''All
downloads allowed, except for those that carry SafeBrowsing warnings.''',
On 2017/02/06 07:45:00, pastarmovj wrote:
> I know this CL is only a prototype still but I will put some comments here
that
> are more nits than actual issues.
> 
> The caption is too long and won't show well in the policy editor on windows. I
> would go with a short cation e.g. "Allow all downloads" and then have a
> paragraph about what each option does explained in the description below.

Done.

https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:1733: 'label': '''Allow
download restrictions''',
On 2017/02/06 07:45:00, pastarmovj wrote:
> Download restrictions" might be a better name imho.

Done.

[pastarmovj, 2017-02-07 06:52:32]

On 2017/02/06 19:41:33, MAD wrote:
> Updated the policy template descriptions to address comments, and also
specified
> the subtleties of download VS save/print. Is this what you meant pastramovj@?
> 
> Also moved asanka@ from CC to Reviewers, and started looking at a new option
> (not uploaded the code yet), where we could modify the semantic of the cancel
> method to also support policy blocked on top of user cancels or system
> shutdowns.
> 
> Let me know if you have other comments or suggestions about the general
> approach.
> 
> Thanks!
> 
> BYE
> MAD
> 
>
https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
> File components/policy/resources/policy_templates.json (right):
> 
>
https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
> components/policy/resources/policy_templates.json:1707: 'caption': '''All
> downloads allowed, except for those that carry SafeBrowsing warnings.''',
> On 2017/02/06 07:45:00, pastarmovj wrote:
> > I know this CL is only a prototype still but I will put some comments here
> that
> > are more nits than actual issues.
> > 
> > The caption is too long and won't show well in the policy editor on windows.
I
> > would go with a short cation e.g. "Allow all downloads" and then have a
> > paragraph about what each option does explained in the description below.
> 
> Done.
> 
>
https://codereview.chromium.org/2674973003/diff/40001/components/policy/resou...
> components/policy/resources/policy_templates.json:1733: 'label': '''Allow
> download restrictions''',
> On 2017/02/06 07:45:00, pastarmovj wrote:
> > Download restrictions" might be a better name imho.
> 
> Done.

Thanks, this description looks good. The comment about "Save as" and "Print" is
ok too.

[MAD, 2017-02-07 20:38:12]

Description was changed from

==========
INCOMPLETE CL to validate ideas about "Adding a DownloadREstrictions group
policy."

BUG=683797
==========

to

==========
INCOMPLETE CL to validate ideas about "Adding a DownloadRestrictions group
policy."

BUG=683797
==========

[MAD, 2017-02-07 21:25:10]

Patchset #6 (id:100001) has been deleted

[MAD, 2017-02-07 21:26:13]

Patch 6 (as compared to patch 5) proposes an alternate solution using a more
explicit call to BLOCK the download and introduces a new BLOCKED state.

Any preference? Any other alternative?

Thanks!

BYE
MAD

[asanka, 2017-02-09 03:35:49]

Sorry about the delay.

I'd suggest looking at a couple of options:

* If we want to block downloads by source and do so silently, implement it as a
ResourceThrottle that's added on by
ChromeResourceDispatcherHostDelegate::DownloadStarting(). This is the point
where //chrome side code can kill a download before it's created.

* If we want to block downloads by source, but want all such downloads to appear
on the shelf with an interrupt reason, then we can do so at
ChromeDownloadManagerDelegate::DetermineDownloadTarget(). This is the place
where //chrome decides what to do about a download. I have a pending CL at
https://codereview.chromium.org/2453633006 which will allow
DetermineDownloadTarget to interrupt a download with a specific reason (I'll
start kicking some people and get it reviewed in the next few days).

* If we want to block the download based on source + danger level, then we need
to do so after the SafeBrowsing check has completed. I'd be okay with adding
something on OnContentCheckCompleted() that will cause the download to be
interrupted with an arbitrary terminal interrupt reason.

Either way, the UI can then pick up the pieces and figure out how to represent
the interrupted download. There are //chrome specific side-channels in
DownloadItemModel which can be used to communicate metadata like the specific
policy that resulted in the download getting blocked. These might then be used
to guide the behavior of "Learn more" etc...



Adding new states to the DownloadItem is a bit of nuclear option that should
only be necessary if the blocked state is going to interact with an active
network request. Since that the SafeBrowsing check is only completed after the
active portion completes, this level of complexity is not needed. We are either
blocking the download at the start, or at the very end.

I'm assuming here that the download is not recoverable once it's blocked. Is
this correct?

[MAD, 2017-02-09 14:52:44]

Excellent! Thanks...

So patch set 6 is NOT what we want (it's the one adding the BLOCKED state), but
patch set 5 is close. 

I think (though I may be wrong) that we don't want to block silently. We want
the user to see the UI mentioning that the download was blocked. Also, I think
we can't avoid it when the blocking is caused by a safe browsing warning, which
only get completed once the UI was already created, right?

I'm not sure how important it is to specify why it was blocked, but if I
understand correctly, it shouldn't be hard to add a specific interrupt reason
for admin policy blocking, and update DownloadItemModel accordingly. But I think
this could be done in another CL.

When you say blocking downloads by source, do you mean the download URL? This is
the case I currently don't support since at the time safebrowsing identifies the
download URL as bad, I couldn't find ways to block the download. Will this be
easier with your pending CL?

In patch set 5, I basically: 
- Add an API to the download manager delegate to express whether ALL downloads
should be blocked (i.e., option 3 of the new policy described in the bug).
- In the download manager implementation, use this new API to set the file
blocked interrupt reason within StartDownloadWithId.
- For options 1 and 2, a new check is added in
ChromeDownloadManagerDelegate::IsDownloadReadyForCompletion to let
OnContentCheckCompleted know if want to block the download.
- The part that is missing (that might be easier to do when your pending CL
lands) is when the chrome download manager delegate identifies a bad download
URL in CheckDownloadUrlDone.

Thanks again! :-)

BYE
MAD

[MAD, 2017-02-09 15:10:25]

Patchset #7 (id:140001) has been deleted

[asanka, 2017-02-09 19:24:46]

On 2017/02/09 at 14:52:44, mad wrote:
> Excellent! Thanks...
> 
> So patch set 6 is NOT what we want (it's the one adding the BLOCKED state),
but patch set 5 is close. 
> 
> I think (though I may be wrong) that we don't want to block silently. We want
the user to see the UI mentioning that the download was blocked. Also, I think
we can't avoid it when the blocking is caused by a safe browsing warning, which
only get completed once the UI was already created, right?

For the SafeBrowsing case, yes. The download would already be on the shelf. The
silent blocking is only viable if downloads are entirely disabled (say, some
sort of kiosk mode). The browser would essentially act as if the downloads
feature doesn't exist. For enterprise policy controls, I don't know what the
right answer is. I'm generally in favor having some feedback even if that
feedback is that their action got blocked.

> 
> I'm not sure how important it is to specify why it was blocked, but if I
understand correctly, it shouldn't be hard to add a specific interrupt reason
for admin policy blocking, and update DownloadItemModel accordingly. But I think
this could be done in another CL.

Yup. As mentioned elsewhere we can associate arbitrary metadata with a blocked
download via DownloadItemModel. It's not limited to an interrupt reason.

> 
> When you say blocking downloads by source, do you mean the download URL? This
is the case I currently don't support since at the time safebrowsing identifies
the download URL as bad, I couldn't find ways to block the download. Will this
be easier with your pending CL?

By source == based on URL. I'm picturing that enterprises may want to do things
like block downloads from the intranet but allow downloads from within their
intranet or web email client (think attachments). And yes, the pending CL would
make it easier to implement blocking by source along with an on-shelf UI. It
would make it possible for
ChromeDownloadManagerDelegate::DetermineDownloadTarget to respond with an
interrupt reason.

> In patch set 5, I basically: 
> - Add an API to the download manager delegate to express whether ALL downloads
should be blocked (i.e., option 3 of the new policy described in the bug).
> - In the download manager implementation, use this new API to set the file
blocked interrupt reason within StartDownloadWithId.
> - For options 1 and 2, a new check is added in
ChromeDownloadManagerDelegate::IsDownloadReadyForCompletion to let
OnContentCheckCompleted know if want to block the download.
> - The part that is missing (that might be easier to do when your pending CL
lands) is when the chrome download manager delegate identifies a bad download
URL in CheckDownloadUrlDone.
> 
> Thanks again! :-)
> 
> BYE
> MAD

[asanka, 2017-02-09 19:25:01]

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:181: //
restricted_download_item is not null and danger_type is set to URL.
Rather than this, we can move block downloads at the DownloadTargetDeterminer
layer. That way we can factor in the download source and danger level as known
at the start of the download.

Another thing we should roll into this is to check with
IAttachmentExecute::CheckPolicy so that we can block downloads that are going to
be blocked anyway by Windows upon completion
(https://msdn.microsoft.com/en-us/library/windows/desktop/bb776294(v=vs.85).aspx).
This can also done in DownloadTargetDeterminer .

Currently DownloadTargetDeterminer can't mark a download as "blocked", but
that's being addressed in the pending CL.

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:361:
item->OnContentCheckCompleted(danger_type, false);
Yeah, we'll need something like this. The only change I could think of was to
use an interrupt reason instead of a boolean.

https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
File content/public/browser/download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.cc:50: bool
DownloadManagerDelegate::ShouldBlockAllDownloads() const {
Let's do all the blocking in //chrome.

[MAD, 2017-02-14 18:15:48]

Just realized I had not posted my answers to the recent comments as I uploaded
the changes last week...

I'm now waiting for Ansanka's related CL before I continue working on this one.

Thanks!

BYE
MAD

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:181: //
restricted_download_item is not null and danger_type is set to URL.
On 2017/02/09 19:25:01, asanka wrote:
> Rather than this, we can move block downloads at the DownloadTargetDeterminer
> layer. That way we can factor in the download source and danger level as known
> at the start of the download.
> 
> Another thing we should roll into this is to check with
> IAttachmentExecute::CheckPolicy so that we can block downloads that are going
to
> be blocked anyway by Windows upon completion
>
(https://msdn.microsoft.com/en-us/library/windows/desktop/bb776294(v=vs.85).aspx).
> This can also done in DownloadTargetDeterminer .
> 
> Currently DownloadTargetDeterminer can't mark a download as "blocked", but
> that's being addressed in the pending CL.

OK, I'll wait for the pending CL before continuing on this.

https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:361:
item->OnContentCheckCompleted(danger_type, false);
On 2017/02/09 19:25:01, asanka wrote:
> Yeah, we'll need something like this. The only change I could think of was to
> use an interrupt reason instead of a boolean.

Done.

https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
File content/public/browser/download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
content/public/browser/download_manager_delegate.cc:50: bool
DownloadManagerDelegate::ShouldBlockAllDownloads() const {
On 2017/02/09 19:25:01, asanka wrote:
> Let's do all the blocking in //chrome.

Can you point me where //chrome could block the download early enough yet still
show the UI?

[asanka, 2017-04-21 15:13:40]

On 2017/02/14 18:15:48, MAD wrote:
> Just realized I had not posted my answers to the recent comments as I uploaded
> the changes last week...
> 
> I'm now waiting for Ansanka's related CL before I continue working on this
one.

Whoa. Forgot to ping this thread.

The changes have landed and now DownloadTargetDeterminer can return an interrupt
reason when resolving a target.

> 
> Thanks!
> 
> BYE
> MAD
> 
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> chrome/browser/download/chrome_download_manager_delegate.cc:181: //
> restricted_download_item is not null and danger_type is set to URL.
> On 2017/02/09 19:25:01, asanka wrote:
> > Rather than this, we can move block downloads at the
DownloadTargetDeterminer
> > layer. That way we can factor in the download source and danger level as
known
> > at the start of the download.
> > 
> > Another thing we should roll into this is to check with
> > IAttachmentExecute::CheckPolicy so that we can block downloads that are
going
> to
> > be blocked anyway by Windows upon completion
> >
>
(https://msdn.microsoft.com/en-us/library/windows/desktop/bb776294(v=vs.85).aspx).
> > This can also done in DownloadTargetDeterminer .
> > 
> > Currently DownloadTargetDeterminer can't mark a download as "blocked", but
> > that's being addressed in the pending CL.
> 
> OK, I'll wait for the pending CL before continuing on this.
> 
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> chrome/browser/download/chrome_download_manager_delegate.cc:361:
> item->OnContentCheckCompleted(danger_type, false);
> On 2017/02/09 19:25:01, asanka wrote:
> > Yeah, we'll need something like this. The only change I could think of was
to
> > use an interrupt reason instead of a boolean.
> 
> Done.
> 
>
https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
> File content/public/browser/download_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
> content/public/browser/download_manager_delegate.cc:50: bool
> DownloadManagerDelegate::ShouldBlockAllDownloads() const {
> On 2017/02/09 19:25:01, asanka wrote:
> > Let's do all the blocking in //chrome.
> 
> Can you point me where //chrome could block the download early enough yet
still
> show the UI?

[MAD, 2017-04-21 15:35:57]

Thanks! I'm on a temporary assignment to another team until we branch m60,
so I'll get back to this then.

Le ven. 21 avr. 2017 à 11:13, <asanka@chromium.org> a écrit :

> On 2017/02/14 18:15:48, MAD wrote:
> > Just realized I had not posted my answers to the recent comments as I
> uploaded
> > the changes last week...
> >
> > I'm now waiting for Ansanka's related CL before I continue working on
> this
> one.
>
> Whoa. Forgot to ping this thread.
>
> The changes have landed and now DownloadTargetDeterminer can return an
> interrupt
> reason when resolving a target.
>
> >
> > Thanks!
> >
> > BYE
> > MAD
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> > File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> > chrome/browser/download/chrome_download_manager_delegate.cc:181: //
> > restricted_download_item is not null and danger_type is set to URL.
> > On 2017/02/09 19:25:01, asanka wrote:
> > > Rather than this, we can move block downloads at the
> DownloadTargetDeterminer
> > > layer. That way we can factor in the download source and danger level
> as
> known
> > > at the start of the download.
> > >
> > > Another thing we should roll into this is to check with
> > > IAttachmentExecute::CheckPolicy so that we can block downloads that are
> going
> > to
> > > be blocked anyway by Windows upon completion
> > >
> >
> (
>
https://msdn.microsoft.com/en-us/library/windows/desktop/bb776294(v=vs.85).aspx
> ).
> > > This can also done in DownloadTargetDeterminer .
> > >
> > > Currently DownloadTargetDeterminer can't mark a download as "blocked",
> but
> > > that's being addressed in the pending CL.
> >
> > OK, I'll wait for the pending CL before continuing on this.
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/160001/chrome/browser/downloa...
> > chrome/browser/download/chrome_download_manager_delegate.cc:361:
> > item->OnContentCheckCompleted(danger_type, false);
> > On 2017/02/09 19:25:01, asanka wrote:
> > > Yeah, we'll need something like this. The only change I could think of
> was
> to
> > > use an interrupt reason instead of a boolean.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
> > File content/public/browser/download_manager_delegate.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/160001/content/public/browser...
> > content/public/browser/download_manager_delegate.cc:50: bool
> > DownloadManagerDelegate::ShouldBlockAllDownloads() const {
> > On 2017/02/09 19:25:01, asanka wrote:
> > > Let's do all the blocking in //chrome.
> >
> > Can you point me where //chrome could block the download early enough yet
> still
> > show the UI?
>
>
>
> https://codereview.chromium.org/2674973003/
>
-- 
BYE
MAD...

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MAD, 2017-06-02 19:59:45]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 20:00:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-06-02 20:00:58]

OK, I finally got back to this CL and added code to benefit from the recent
changes in DetermineDownloadTarge and DownloadItem. I also added unit tests.

Let me know if this is what you had in mind asanka@ or if I misunderstood.

Thanks!

BYE
MAD

[MAD, 2017-06-02 20:22:28]

Description was changed from

==========
INCOMPLETE CL to validate ideas about "Adding a DownloadRestrictions group
policy."

BUG=683797
==========

to

==========
Adding a DownloadRestrictions group policy.

BUG=683797
==========

[commit-bot: I haz the power, 2017-06-02 20:53:00]

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

[commit-bot: I haz the power, 2017-06-02 20:53:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-06-05 17:28:50]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 17:29:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 18:54:22]

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

[commit-bot: I haz the power, 2017-06-05 18:54:23]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-06-06 03:00:35]

asanka@chromium.org changed reviewers:
+ dtrainor@chromium.org

[asanka, 2017-06-06 03:00:36]

Cool. In the meantime, I moved off of downloads :-)

I'll leave this in the capable hands of the current downloads owners.

https://codereview.chromium.org/2674973003/diff/260023/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/260023/components/policy/reso...
components/policy/resources/policy_templates.json:1742: ],
The policy value names and their descriptions should probably be run by UI
folks. In particular, the term "blocked" may be confusing since "blocked"
implies that the download can't proceed with or without the policy.

One way to phrase it would be to consider what the user is allowed or not
allowed to do. In the case of "DisableBlockedFiles", the user is not allowed to
bypass the SafeBrowsing warning. And with "DisableBlockedAndDangerousFiles", the
user is not allowed to bypass any download warning.

Also, I'd suggest sticking to Alllow/Disallow for consistency.

FTR: there are three download warning types -- in decreasing order of severity:
* Malicious downloads : Flagged by SB due to file content, file source or
something else.
* Potentially unwanted : Flagged by SB.
* Uncommon : SB doesn't have sufficient data to consider this file safe. Should
be considered more dangerous than 'Potentially dangerous'
* Potentially dangerous : Flagged simply because of the file type.

As enterprise use cases go, it's worth looking at how an enterprise might deploy
their custom binaries and installers as downloads. Also most MS Office file
types are in the "dangerous files" bucket, which may be an obstacle if folks are
expected to pass around documents over email etc... As currently laid out, such
use cases currently cannot be accommodated without opening the floodgates and
allowing all dangerous file types.

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
The only interrupt reasons that make sense here are those that result in a
ResumeMode of INVALID. Otherwise the download may resume automatically or wiggle
out of its interrupted state. The INVALID resume mode is also important because
it causes the partial state to be discarded.

Perhaps consider restricting the interrupt reason at this point to BLOCKED or
maybe just have a boolean or enum parameter indicating that the download should
be blocked.

https://codereview.chromium.org/2674973003/diff/260023/content/public/browser...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2674973003/diff/260023/content/public/browser...
content/public/browser/download_item.h:439: // file should be blocked.
Not accurate since it'll only be blocked if the interrupt reason is
FILE_BLOCKED. Other reasons may cause unintended behavior like resuming the
download automatically.

[MAD, 2017-06-06 18:00:28]

No code changes yet, just answers and more questions.

Thanks!

BYE
MAD

https://codereview.chromium.org/2674973003/diff/260023/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/260023/components/policy/reso...
components/policy/resources/policy_templates.json:1742: ],
On 2017/06/06 03:00:36, asanka wrote:
> The policy value names and their descriptions should probably be run by UI
> folks. In particular, the term "blocked" may be confusing since "blocked"
> implies that the download can't proceed with or without the policy.
> 
> One way to phrase it would be to consider what the user is allowed or not
> allowed to do. In the case of "DisableBlockedFiles", the user is not allowed
to
> bypass the SafeBrowsing warning. And with "DisableBlockedAndDangerousFiles",
the
> user is not allowed to bypass any download warning.
> 
> Also, I'd suggest sticking to Alllow/Disallow for consistency.
> 
> FTR: there are three download warning types -- in decreasing order of
severity:
> * Malicious downloads : Flagged by SB due to file content, file source or
> something else.
> * Potentially unwanted : Flagged by SB.
> * Uncommon : SB doesn't have sufficient data to consider this file safe.
Should
> be considered more dangerous than 'Potentially dangerous'
> * Potentially dangerous : Flagged simply because of the file type.
> 
> As enterprise use cases go, it's worth looking at how an enterprise might
deploy
> their custom binaries and installers as downloads. Also most MS Office file
> types are in the "dangerous files" bucket, which may be an obstacle if folks
are
> expected to pass around documents over email etc... As currently laid out,
such
> use cases currently cannot be accommodated without opening the floodgates and
> allowing all dangerous file types.

OK, I'll check with UX and Chrome Enterprise PM for the namings.

As for the floodgate, I have another CL I just started to add another policy to
complement this one, which will allow whitelisting certain downloads.

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
On 2017/06/06 03:00:36, asanka wrote:
> The only interrupt reasons that make sense here are those that result in a
> ResumeMode of INVALID. Otherwise the download may resume automatically or
wiggle
> out of its interrupted state. The INVALID resume mode is also important
because
> it causes the partial state to be discarded.
> 
> Perhaps consider restricting the interrupt reason at this point to BLOCKED or
> maybe just have a boolean or enum parameter indicating that the download
should
> be blocked.

Héhé... Interestingly, I initially had a bool and you suggested changing it to
specify the reason... :-D

I could also change the if to only interrupt the download when the value is
blocked, and keep it open for other interruptions in the future?

Though I don't mind bringing back the bool. :-)

https://codereview.chromium.org/2674973003/diff/260023/content/public/browser...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2674973003/diff/260023/content/public/browser...
content/public/browser/download_item.h:439: // file should be blocked.
On 2017/06/06 03:00:36, asanka wrote:
> Not accurate since it'll only be blocked if the interrupt reason is
> FILE_BLOCKED. Other reasons may cause unintended behavior like resuming the
> download automatically.

Acknowledged.

[asanka, 2017-06-06 18:34:18]

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
On 2017/06/06 18:00:28, MAD wrote:
> On 2017/06/06 03:00:36, asanka wrote:
> > The only interrupt reasons that make sense here are those that result in a
> > ResumeMode of INVALID. Otherwise the download may resume automatically or
> wiggle
> > out of its interrupted state. The INVALID resume mode is also important
> because
> > it causes the partial state to be discarded.
> > 
> > Perhaps consider restricting the interrupt reason at this point to BLOCKED
or
> > maybe just have a boolean or enum parameter indicating that the download
> should
> > be blocked.
> 
> Héhé... Interestingly, I initially had a bool and you suggested changing it to
> specify the reason... :-D
> 
> I could also change the if to only interrupt the download when the value is
> blocked, and keep it open for other interruptions in the future?
> 
> Though I don't mind bringing back the bool. :-)

LOL. Apologies for the runaround :). I'll defer to the current owners in that
case. The API probably looked too custom made for this use case. I'm tempted to
say we should fold the "BLOCKED" state into a danger type, but that'll make it
difficult to note *why* the download was blocked later.

[David Trainor- moved to gerrit, 2017-06-07 07:18:25]

On 2017/06/06 18:34:18, asanka wrote:
>
https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
> content/browser/download/download_item_impl.cc:845:
> InterruptAndDiscardPartialState(reason);
> On 2017/06/06 18:00:28, MAD wrote:
> > On 2017/06/06 03:00:36, asanka wrote:
> > > The only interrupt reasons that make sense here are those that result in a
> > > ResumeMode of INVALID. Otherwise the download may resume automatically or
> > wiggle
> > > out of its interrupted state. The INVALID resume mode is also important
> > because
> > > it causes the partial state to be discarded.
> > > 
> > > Perhaps consider restricting the interrupt reason at this point to BLOCKED
> or
> > > maybe just have a boolean or enum parameter indicating that the download
> > should
> > > be blocked.
> > 
> > Héhé... Interestingly, I initially had a bool and you suggested changing it
to
> > specify the reason... :-D
> > 
> > I could also change the if to only interrupt the download when the value is
> > blocked, and keep it open for other interruptions in the future?
> > 
> > Though I don't mind bringing back the bool. :-)
> 
> LOL. Apologies for the runaround :). I'll defer to the current owners in that
> case. The API probably looked too custom made for this use case. I'm tempted
to
> say we should fold the "BLOCKED" state into a danger type, but that'll make it
> difficult to note *why* the download was blocked later.

Just jumping on here quickly to reply!  I'm OOO at summits until Thursday.  Will
try to get to this by then.  Let me know if you need it done sooner.  Thanks!

[MAD, 2017-06-07 15:24:22]

No rush... Thanks!

Le mer. 7 juin 2017 à 03:18, <dtrainor@chromium.org> a écrit :

> On 2017/06/06 18:34:18, asanka wrote:
> >
>
>
https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
> > File content/browser/download/download_item_impl.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
> > content/browser/download/download_item_impl.cc:845:
> > InterruptAndDiscardPartialState(reason);
> > On 2017/06/06 18:00:28, MAD wrote:
> > > On 2017/06/06 03:00:36, asanka wrote:
> > > > The only interrupt reasons that make sense here are those that
> result in a
> > > > ResumeMode of INVALID. Otherwise the download may resume
> automatically or
> > > wiggle
> > > > out of its interrupted state. The INVALID resume mode is also
> important
> > > because
> > > > it causes the partial state to be discarded.
> > > >
> > > > Perhaps consider restricting the interrupt reason at this point to
> BLOCKED
> > or
> > > > maybe just have a boolean or enum parameter indicating that the
> download
> > > should
> > > > be blocked.
> > >
> > > Héhé... Interestingly, I initially had a bool and you suggested
> changing it
> to
> > > specify the reason... :-D
> > >
> > > I could also change the if to only interrupt the download when the
> value is
> > > blocked, and keep it open for other interruptions in the future?
> > >
> > > Though I don't mind bringing back the bool. :-)
> >
> > LOL. Apologies for the runaround :). I'll defer to the current owners in
> that
> > case. The API probably looked too custom made for this use case. I'm
> tempted
> to
> > say we should fold the "BLOCKED" state into a danger type, but that'll
> make it
> > difficult to note *why* the download was blocked later.
>
> Just jumping on here quickly to reply! I'm OOO at summits until Thursday.
> Will
> try to get to this by then. Let me know if you need it done sooner. Thanks!
>
> https://codereview.chromium.org/2674973003/
>
-- 
BYE
MAD...

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MAD, 2017-06-12 17:38:14]

mad@chromium.org changed reviewers:
+ blumberg@chromium.org

[MAD, 2017-06-12 17:38:16]

Updated the policy names and description to match the code (I hadn't realize
that I wrote them before writing the code, and that they didn't quite match
anymore, sorry about that). So +blumberg to double check the wording (others
feel free to comment there too :-).

And David, are you available to review now?

Thanks!

BYE
MAD

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
On 2017/06/06 18:34:18, asanka wrote:
> On 2017/06/06 18:00:28, MAD wrote:
> > On 2017/06/06 03:00:36, asanka wrote:
> > > The only interrupt reasons that make sense here are those that result in a
> > > ResumeMode of INVALID. Otherwise the download may resume automatically or
> > wiggle
> > > out of its interrupted state. The INVALID resume mode is also important
> > because
> > > it causes the partial state to be discarded.
> > > 
> > > Perhaps consider restricting the interrupt reason at this point to BLOCKED
> or
> > > maybe just have a boolean or enum parameter indicating that the download
> > should
> > > be blocked.
> > 
> > Héhé... Interestingly, I initially had a bool and you suggested changing it
to
> > specify the reason... :-D
> > 
> > I could also change the if to only interrupt the download when the value is
> > blocked, and keep it open for other interruptions in the future?
> > 
> > Though I don't mind bringing back the bool. :-)
> 
> LOL. Apologies for the runaround :). I'll defer to the current owners in that
> case. The API probably looked too custom made for this use case. I'm tempted
to
> say we should fold the "BLOCKED" state into a danger type, but that'll make it
> difficult to note *why* the download was blocked later.
> 

OK, thanks, I'll wait to see if the OWNERs prefer to change it to a boolean, or
restrict the interrupt reasons that can cause an interruption here.

[David Trainor- moved to gerrit, 2017-06-12 19:09:05]

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
On 2017/06/12 17:38:15, MAD wrote:
> On 2017/06/06 18:34:18, asanka wrote:
> > On 2017/06/06 18:00:28, MAD wrote:
> > > On 2017/06/06 03:00:36, asanka wrote:
> > > > The only interrupt reasons that make sense here are those that result in
a
> > > > ResumeMode of INVALID. Otherwise the download may resume automatically
or
> > > wiggle
> > > > out of its interrupted state. The INVALID resume mode is also important
> > > because
> > > > it causes the partial state to be discarded.
> > > > 
> > > > Perhaps consider restricting the interrupt reason at this point to
BLOCKED
> > or
> > > > maybe just have a boolean or enum parameter indicating that the download
> > > should
> > > > be blocked.
> > > 
> > > Héhé... Interestingly, I initially had a bool and you suggested changing
it
> to
> > > specify the reason... :-D
> > > 
> > > I could also change the if to only interrupt the download when the value
is
> > > blocked, and keep it open for other interruptions in the future?
> > > 
> > > Though I don't mind bringing back the bool. :-)
> > 
> > LOL. Apologies for the runaround :). I'll defer to the current owners in
that
> > case. The API probably looked too custom made for this use case. I'm tempted
> to
> > say we should fold the "BLOCKED" state into a danger type, but that'll make
it
> > difficult to note *why* the download was blocked later.
> > 
> 
> OK, thanks, I'll wait to see if the OWNERs prefer to change it to a boolean,
or
> restrict the interrupt reasons that can cause an interruption here.

I'm ok leaving the interrupt reason here.  Could we just DCHECK the expected
ResumeMode() after this call?

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:911: if
(download_restriction == DownloadPrefs::DOWNLOAD_RESTRICTION_ALL_FILES)
Would this be cleaner as a switch statement?  Fine either way though.

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:29: enum DOWNLOAD_RESTRICTION {
DownloadRestriction?  The all caps w/ _ feels more like a value rather than a
enum type.

[MAD, 2017-06-12 19:40:50]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 19:41:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[MAD, 2017-06-12 19:45:14]

Thanks! How about this now?

BYE
MAD...

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2674973003/diff/260023/content/browser/downlo...
content/browser/download/download_item_impl.cc:845:
InterruptAndDiscardPartialState(reason);
On 2017/06/12 19:09:04, David Trainor-ping after 12h wrote:
> On 2017/06/12 17:38:15, MAD wrote:
> > On 2017/06/06 18:34:18, asanka wrote:
> > > On 2017/06/06 18:00:28, MAD wrote:
> > > > On 2017/06/06 03:00:36, asanka wrote:
> > > > > The only interrupt reasons that make sense here are those that result
in
> a
> > > > > ResumeMode of INVALID. Otherwise the download may resume automatically
> or
> > > > wiggle
> > > > > out of its interrupted state. The INVALID resume mode is also
important
> > > > because
> > > > > it causes the partial state to be discarded.
> > > > > 
> > > > > Perhaps consider restricting the interrupt reason at this point to
> BLOCKED
> > > or
> > > > > maybe just have a boolean or enum parameter indicating that the
download
> > > > should
> > > > > be blocked.
> > > > 
> > > > Héhé... Interestingly, I initially had a bool and you suggested changing
> it
> > to
> > > > specify the reason... :-D
> > > > 
> > > > I could also change the if to only interrupt the download when the value
> is
> > > > blocked, and keep it open for other interruptions in the future?
> > > > 
> > > > Though I don't mind bringing back the bool. :-)
> > > 
> > > LOL. Apologies for the runaround :). I'll defer to the current owners in
> that
> > > case. The API probably looked too custom made for this use case. I'm
tempted
> > to
> > > say we should fold the "BLOCKED" state into a danger type, but that'll
make
> it
> > > difficult to note *why* the download was blocked later.
> > > 
> > 
> > OK, thanks, I'll wait to see if the OWNERs prefer to change it to a boolean,
> or
> > restrict the interrupt reasons that can cause an interruption here.
> 
> I'm ok leaving the interrupt reason here.  Could we just DCHECK the expected
> ResumeMode() after this call?

Done.

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:911: if
(download_restriction == DownloadPrefs::DOWNLOAD_RESTRICTION_ALL_FILES)
On 2017/06/12 19:09:05, David Trainor-ping after 12h wrote:
> Would this be cleaner as a switch statement?  Fine either way though.

I don't see a clean way of making it work well with the 
(download_restriction == DownloadPrefs::DOWNLOAD_RESTRICTION_NONE ||
 danger_type == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
condition.

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2674973003/diff/300001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:29: enum DOWNLOAD_RESTRICTION {
On 2017/06/12 19:09:05, David Trainor-ping after 12h wrote:
> DownloadRestriction?  The all caps w/ _ feels more like a value rather than a
> enum type.

Done.

[David Trainor- moved to gerrit, 2017-06-12 21:02:29]

lgtm

[MAD, 2017-06-12 21:09:54]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-06-12 21:10:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 21:23:23]

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

[commit-bot: I haz the power, 2017-06-12 21:23:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MAD, 2017-06-13 15:16:26]

I'm still missing OWNERs LGTM for:

chrome/browser/policy/configuration_policy_handler_list_factory.cc
components/policy/resources/policy_templates.json
pastarmovj@ ?

TBR jochen@ ?
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

[MAD, 2017-06-13 15:19:16]

Description was changed from

==========
Adding a DownloadRestrictions group policy.

BUG=683797
==========

to

==========
Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for tricial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797
==========

[MAD, 2017-06-13 15:19:27]

Description was changed from

==========
Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for tricial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797
==========

to

==========
Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for trivial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797
==========

[MAD, 2017-06-13 15:27:15]

The CQ bit was checked by mad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 15:27:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 16:46:54]

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

[commit-bot: I haz the power, 2017-06-13 16:46:56]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jochen (gone - plz use gerrit), 2017-06-14 08:16:50]

lgtm with comments

https://codereview.chromium.org/2674973003/diff/380001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/380001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:911: if
(download_restriction == DownloadPrefs::DownloadRestriction::ALL_FILES)
what about using a switch statement here, and having some handling for values
outside of the range of the enum?

https://codereview.chromium.org/2674973003/diff/380001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2674973003/diff/380001/chrome/common/pref_nam...
chrome/common/pref_names.cc:83: const char kDownloadRestrictions[] =
"download_restrictions";
s/integre/integer/

can you also add a comment saying that this corresponds to
DownloadPrefs::DownloadRestriction ?

https://codereview.chromium.org/2674973003/diff/380001/content/public/browser...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2674973003/diff/380001/content/public/browser...
content/public/browser/download_item.h:439: // file should be blocked.
I find it kinda odd to pass an interrupt reason to this method, however, that
appears to be a common pattern on download item..

Would be less odd to have a generic DownloadItem::Interrupt(reason) instead of
having several methods take an interrupt reason so you can interrupt at those
points. Probably not for this CL to fix.

[pastarmovj, 2017-06-14 08:48:35]

policy LGTM with a few requests for clarifications in the description text.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1743: 'supported_on':
['chrome.*:60-', 'chrome_os:60-'],
Shouldn't this be 61 by now?

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1761: When the 'Block all
downloads' option is chosen, all downloads are blocked.
Add another paragraph that explains what does it mean for the policy to not be
set. I know it is obvious but MS best practices dictate the default behavior to
be explicitly documented. See any other policy for reference.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1765: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.''',
nit: skip the "But ".

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1765: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.''',
Do we have a help center article about SafeBrowsing? It feels to me it will be
good to include a link here for people to know what does the restrictions above
really mean.

[MAD, 2017-06-14 17:50:22]

All done... Thanks!

Will go through CQ, but if you disagree with how I applied your comments, I
could update in another CL if this goes in before you reply.

BYE
MAD

https://codereview.chromium.org/2674973003/diff/380001/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2674973003/diff/380001/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:911: if
(download_restriction == DownloadPrefs::DownloadRestriction::ALL_FILES)
On 2017/06/14 08:16:49, jochen wrote:
> what about using a switch statement here, and having some handling for values
> outside of the range of the enum?

dtrainor asked for the same thing, and I thought it would be trickier, but after
a second suggestion, I decided to try it. :-)

https://codereview.chromium.org/2674973003/diff/380001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2674973003/diff/380001/chrome/common/pref_nam...
chrome/common/pref_names.cc:83: const char kDownloadRestrictions[] =
"download_restrictions";
On 2017/06/14 08:16:49, jochen wrote:
> s/integre/integer/
> 
> can you also add a comment saying that this corresponds to
> DownloadPrefs::DownloadRestriction ?

Done.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1743: 'supported_on':
['chrome.*:60-', 'chrome_os:60-'],
On 2017/06/14 08:48:35, pastarmovj wrote:
> Shouldn't this be 61 by now?

Done.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1761: When the 'Block all
downloads' option is chosen, all downloads are blocked.
On 2017/06/14 08:48:35, pastarmovj wrote:
> Add another paragraph that explains what does it mean for the policy to not be
> set. I know it is obvious but MS best practices dictate the default behavior
to
> be explicitly documented. See any other policy for reference.

Done.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1765: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.''',
On 2017/06/14 08:48:35, pastarmovj wrote:
> Do we have a help center article about SafeBrowsing? It feels to me it will be
> good to include a link here for people to know what does the restrictions
above
> really mean.

There are other mentions of safe browsing within other policies (some policies
being directly related to it). Should I add the link there too?

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1765: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.''',
On 2017/06/14 08:48:35, pastarmovj wrote:
> Do we have a help center article about SafeBrowsing? It feels to me it will be
> good to include a link here for people to know what does the restrictions
above
> really mean.

Done.

https://codereview.chromium.org/2674973003/diff/380001/content/public/browser...
File content/public/browser/download_item.h (right):

https://codereview.chromium.org/2674973003/diff/380001/content/public/browser...
content/public/browser/download_item.h:439: // file should be blocked.
On 2017/06/14 08:16:49, jochen wrote:
> I find it kinda odd to pass an interrupt reason to this method, however, that
> appears to be a common pattern on download item..
> 
> Would be less odd to have a generic DownloadItem::Interrupt(reason) instead of
> having several methods take an interrupt reason so you can interrupt at those
> points. Probably not for this CL to fix.

Agreed. Added a TODO. Thanks!

[MAD, 2017-06-14 17:50:31]

The CQ bit was checked by mad@chromium.org

[MAD, 2017-06-14 17:50:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, jochen@chromium.org, pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2674973003/#ps430001
(title: "Some oups.... :-)")

[commit-bot: I haz the power, 2017-06-14 17:51:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 19:27:12]

CQ is committing da patch.

Bot data: {"patchset_id": 430001, "attempt_start_ts": 1497462631100910,
"parent_rev": "90587f2f3ae61e1ff43e769f01294c8367661601", "commit_rev":
"4e9c44b9e4a05ba422bbe2f5d62b12d83071b94c"}

[commit-bot: I haz the power, 2017-06-14 19:27:28]

Description was changed from

==========
Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for trivial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797
==========

to

==========
Adding a DownloadRestrictions group policy.

TBR=jochen@chromium.org 
for trivial changes in:
chrome/browser/profiles/profile_impl.cc
content/public/browser/download_item.h
content/public/test/fake_download_item.h
content/public/test/fake_download_item.cc
content/public/test/mock_download_item.h

BUG=683797

Review-Url: https://codereview.chromium.org/2674973003
Cr-Commit-Position: refs/heads/master@{#479465}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e9c44b9e4a05ba422bbe2f5d62b...
==========

[commit-bot: I haz the power, 2017-06-14 19:27:30]

Committed patchset #22 (id:430001) as
https://chromium.googlesource.com/chromium/src/+/4e9c44b9e4a05ba422bbe2f5d62b...

[pastarmovj, 2017-06-16 07:17:13]

Thanks - the policy documentation looks great to me now! :) Still lgtm.

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2674973003/diff/380001/components/policy/reso...
components/policy/resources/policy_templates.json:1765: But these restrictions
do not apply to the save / download of the currently displayed page, nor does it
apply to saving as PDF from the printing options.''',
On 2017/06/14 17:50:21, MAD wrote:
> On 2017/06/14 08:48:35, pastarmovj wrote:
> > Do we have a help center article about SafeBrowsing? It feels to me it will
be
> > good to include a link here for people to know what does the restrictions
> above
> > really mean.
> 
> There are other mentions of safe browsing within other policies (some policies
> being directly related to it). Should I add the link there too?

If you want to you can do that in another CL. But at least this policy now has
great comprehensive description! :)