Add chromium pdf viewer to whitelist.

Add chromium pdf viewer to whitelist.

BUG=626342, 694545
TEST=manual

Review-Url: https://codereview.chromium.org/2702223003
Cr-Commit-Position: refs/heads/master@{#451958}
Committed: https://chromium.googlesource.com/chromium/src/+/e4813d56655ddf9f7d85e4b8985c7011d99898ce

[achuithb, 2017-02-20 15:51:57]

achuith@chromium.org changed reviewers:
+ emaxx@chromium.org

[achuithb, 2017-02-20 15:51:58]

Maksim, I noticed this extension was being blocked on the signin screen. PTAL.

[achuithb, 2017-02-20 15:52:04]

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

[commit-bot: I haz the power, 2017-02-20 15:52:17]

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

[emaxx, 2017-02-20 15:54:59]

On 2017/02/20 15:51:58, achuithb wrote:
> Maksim, I noticed this extension was being blocked on the signin screen. PTAL.

Good catch. Have you checked whether anything else is not actually whitelisted
from the list documented here:
http://crbug.com/626342#c10
?

[achuithb, 2017-02-20 16:04:04]

On 2017/02/20 15:54:59, emaxx wrote:
> On 2017/02/20 15:51:58, achuithb wrote:
> > Maksim, I noticed this extension was being blocked on the signin screen.
PTAL.
> 
> Good catch. Have you checked whether anything else is not actually whitelisted
> from the list documented here:
> http://crbug.com/626342#c10
> ?

It looks like everything else is in, from what I can tell anyway.

[emaxx, 2017-02-20 16:06:55]

On 2017/02/20 16:04:04, achuithb wrote:
> On 2017/02/20 15:54:59, emaxx wrote:
> > On 2017/02/20 15:51:58, achuithb wrote:
> > > Maksim, I noticed this extension was being blocked on the signin screen.
> PTAL.
> > 
> > Good catch. Have you checked whether anything else is not actually
whitelisted
> > from the list documented here:
> > http://crbug.com/626342#c10
> > ?
> 
> It looks like everything else is in, from what I can tell anyway.

LGTM.

I'm wondering now whether this needs to be merged into M57.
Are you aware of the use cases for the PDF Viewer on the login screen?

[achuithb, 2017-02-20 16:15:22]

On 2017/02/20 16:06:55, emaxx wrote:
> On 2017/02/20 16:04:04, achuithb wrote:
> > On 2017/02/20 15:54:59, emaxx wrote:
> > > On 2017/02/20 15:51:58, achuithb wrote:
> > > > Maksim, I noticed this extension was being blocked on the signin screen.
> > PTAL.
> > > 
> > > Good catch. Have you checked whether anything else is not actually
> whitelisted
> > > from the list documented here:
> > > http://crbug.com/626342#c10
> > > ?
> > 
> > It looks like everything else is in, from what I can tell anyway.
> 
> LGTM.
> 
> I'm wondering now whether this needs to be merged into M57.
> Are you aware of the use cases for the PDF Viewer on the login screen?

Will investigate based on offline discussion.

[achuithb, 2017-02-20 16:15:52]

achuith@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[achuithb, 2017-02-20 16:15:52]

Devlin, could you please take a look?

[commit-bot: I haz the power, 2017-02-20 16:44:44]

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

[commit-bot: I haz the power, 2017-02-20 16:44:45]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-21 15:53:24]

Can you help me understand when a user would view a PDF on the sign-in screen?

[achuithb, 2017-02-21 16:19:28]

Description was changed from

==========
Add chromium pdf viewer to whitelist.

BUG=626342
TEST=manual
==========

to

==========
Add chromium pdf viewer to whitelist.

BUG=626342,694545
TEST=manual
==========

[achuithb, 2017-02-21 16:27:42]

On 2017/02/21 15:53:24, Devlin (ooo Feb20-US Holiday) wrote:
> Can you help me understand when a user would view a PDF on the sign-in screen?

Devlin, I've now also attached bug 694545 to this CL.
https://bugs.chromium.org/p/chromium/issues/detail?id=694545

We've always loaded this extension in the signin profile. We introduced this
whitelist for the signin profile a few months ago with this CL:
https://codereview.chromium.org/2159103006

There was another followup CL to update the whitelist with more extensions:
https://codereview.chromium.org/2539213002

But this pdf extension was left out of this whitelist.

The current behavior is that we have code that loads this component extension
into the signin profile, and it immediately gets blocked by the whitelist.

We haven't really found a use case for the pdf viewer - it's possible that
there's some scenario involving SAML login (third party identity providers), or
the terms of service, or some obscure Google login flow that may need pdf
support. Probably unlikely though.

The purpose of this CL is to just put things back to where they were.

[Devlin, 2017-02-21 17:41:27]

On 2017/02/21 16:27:42, achuithb wrote:
> On 2017/02/21 15:53:24, Devlin (ooo Feb20-US Holiday) wrote:
> > Can you help me understand when a user would view a PDF on the sign-in
screen?
> 
> Devlin, I've now also attached bug 694545 to this CL.
> https://bugs.chromium.org/p/chromium/issues/detail?id=694545
> 
> We've always loaded this extension in the signin profile. We introduced this
> whitelist for the signin profile a few months ago with this CL:
> https://codereview.chromium.org/2159103006
> 
> There was another followup CL to update the whitelist with more extensions:
> https://codereview.chromium.org/2539213002
> 
> But this pdf extension was left out of this whitelist.
> 
> The current behavior is that we have code that loads this component extension
> into the signin profile, and it immediately gets blocked by the whitelist.
> 
> We haven't really found a use case for the pdf viewer - it's possible that
> there's some scenario involving SAML login (third party identity providers),
or
> the terms of service, or some obscure Google login flow that may need pdf
> support. Probably unlikely though.
> 
> The purpose of this CL is to just put things back to where they were.

Thanks for the background.

LGTM, but I wonder if we should be trying to prune this list.  Is there any
chance of working towards a smaller whitelist?

[achuithb, 2017-02-21 18:30:11]

On 2017/02/21 17:41:27, Devlin (ooo Feb20-US Holiday) wrote:
> On 2017/02/21 16:27:42, achuithb wrote:
> > On 2017/02/21 15:53:24, Devlin (ooo Feb20-US Holiday) wrote:
> > > Can you help me understand when a user would view a PDF on the sign-in
> screen?
> > 
> > Devlin, I've now also attached bug 694545 to this CL.
> > https://bugs.chromium.org/p/chromium/issues/detail?id=694545
> > 
> > We've always loaded this extension in the signin profile. We introduced this
> > whitelist for the signin profile a few months ago with this CL:
> > https://codereview.chromium.org/2159103006
> > 
> > There was another followup CL to update the whitelist with more extensions:
> > https://codereview.chromium.org/2539213002
> > 
> > But this pdf extension was left out of this whitelist.
> > 
> > The current behavior is that we have code that loads this component
extension
> > into the signin profile, and it immediately gets blocked by the whitelist.
> > 
> > We haven't really found a use case for the pdf viewer - it's possible that
> > there's some scenario involving SAML login (third party identity providers),
> or
> > the terms of service, or some obscure Google login flow that may need pdf
> > support. Probably unlikely though.
> > 
> > The purpose of this CL is to just put things back to where they were.
> 
> Thanks for the background.
> 
> LGTM, but I wonder if we should be trying to prune this list.  Is there any
> chance of working towards a smaller whitelist?

Sure, I'll file a bug to kick out some of these extensions.

I believe in some cases, component extensions were added to chrome and ended up
unwittingly in the ChromeOS signin profile. 

Thanks for the review.

[achuithb, 2017-02-21 18:30:18]

The CQ bit was checked by achuith@chromium.org

[achuithb, 2017-02-21 18:30:19]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2702223003/#ps20001
(title: "Add bug id")

[commit-bot: I haz the power, 2017-02-21 18:30:50]

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-21 21:58:40]

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

[commit-bot: I haz the power, 2017-02-21 21:58:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achuithb, 2017-02-22 09:38:05]

The CQ bit was checked by achuith@chromium.org

[commit-bot: I haz the power, 2017-02-22 09:38:55]

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-22 09:45:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487756285581450,
"parent_rev": "1886d611f3f71c0ebe55a6d8fc49a40297ef7f99", "commit_rev":
"e4813d56655ddf9f7d85e4b8985c7011d99898ce"}

[commit-bot: I haz the power, 2017-02-22 09:45:46]

Description was changed from

==========
Add chromium pdf viewer to whitelist.

BUG=626342,694545
TEST=manual
==========

to

==========
Add chromium pdf viewer to whitelist.

BUG=626342,694545
TEST=manual

Review-Url: https://codereview.chromium.org/2702223003
Cr-Commit-Position: refs/heads/master@{#451958}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4813d56655ddf9f7d85e4b8985c...
==========

[commit-bot: I haz the power, 2017-02-22 09:45:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e4813d56655ddf9f7d85e4b8985c...