Allow SAML logins to use the webcam

Allow SAML logins to use the webcam

Add a new opt-in policy, LoginVideoCaptureAllowedUrls, that works in a
similar way to the existing VideoCaptureAllowedUrls policy.  SAML
login pages listed in the whitelist will be allowed to access the
webcam (no audio).  If the policy is unset or the list is empty, all
SAML login pages will be denied webcam access.

BUG=606979
TEST=whitelist https://clever.com via YAPS and verify with
     mo@clever.academy login

Committed: https://crrev.com/cfcb9306e198a4e56facc865a60f45ce6d28f1aa
Cr-Commit-Position: refs/heads/master@{#391390}

[Kevin Cernekee, 2016-04-30 23:55:59]

cernekee@chromium.org changed reviewers:
+ bartfab@chromium.org, emaxx@chromium.org

[Kevin Cernekee, 2016-05-01 02:18:43]

cernekee@chromium.org changed reviewers:
+ isherman@chromium.org, zelidrag@gmail.com

[bartfab (slow), 2016-05-02 10:30:29]

Overall no concerns, but this needs a browser test.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:441:
controller.PermissionDenied();
Nit: It would still make sense to have a NOTREACHED() in here for debugging
purposes.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:445:
chromeos::CrosSettings* settings = chromeos::CrosSettings::Get();
Nit 1: const pointer.
Nit 2: #include "chrome/browser/chromeos/settings/cros_settings.h"

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:451: const base::Value*
raw_types_value =
Nit: const pointer.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:452:
settings->GetPref(chromeos::kLoginVideoCaptureAllowedUrls);
Nit: s/chromeos:://

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:459:
CHECK(raw_types_value->GetAsList(&types_value));
Nit: #include "base/logging.h"

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:332:
base::ListValue* urls = new base::ListValue();
Nit 1: const pointer.
Nit 2: A unique_ptr would be semantically cleaner.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:276:
std::unique_ptr<base::ListValue> list(new base::ListValue());
Nit: #include <memory>

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:286:
new_values_cache->SetValue(kLoginVideoCaptureAllowedUrls, std::move(list));
Nit: #include <utility>

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

https://codereview.chromium.org/1936903002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8498: 'tags': [],
Nit: Definitely "website-sharing", maybe more?

https://codereview.chromium.org/1936903002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8502: found, access will be
automatically denied.'''
Nit: Document that wildcard patterns are not allowed.

[Ilya Sherman, 2016-05-02 18:40:07]

histograms.xml lgtm

[emaxx, 2016-05-02 18:42:08]

lgtm with nits

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:460: for (size_t i = 0; i <
types_value->GetSize(); ++i) {
Nit: Range-based for loop?

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:282: for
(RepeatedPtrField<std::string>::const_iterator it = urls.begin();
Nit: Can the range-based for loop be used here?

[Kevin Cernekee, 2016-05-02 20:02:27]

cernekee@chromium.org changed reviewers:
+ zelidrag@chromium.org
- zelidrag@gmail.com

[Kevin Cernekee, 2016-05-02 20:02:30]

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:441:
controller.PermissionDenied();
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit: It would still make sense to have a NOTREACHED() in here for debugging
> purposes.

It isn't a Chrome bug (i.e. assertion failure) if the site requests audio. 
We'll just deny it.

Could log a warning, although we don't log the other denials.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:445:
chromeos::CrosSettings* settings = chromeos::CrosSettings::Get();
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit 1: const pointer.
> Nit 2: #include "chrome/browser/chromeos/settings/cros_settings.h"

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:451: const base::Value*
raw_types_value =
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit: const pointer.

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:452:
settings->GetPref(chromeos::kLoginVideoCaptureAllowedUrls);
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit: s/chromeos:://

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:459:
CHECK(raw_types_value->GetAsList(&types_value));
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:460: for (size_t i = 0; i <
types_value->GetSize(); ++i) {
On 2016/05/02 18:42:08, emaxx wrote:
> Nit: Range-based for loop?

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:332:
base::ListValue* urls = new base::ListValue();
On 2016/05/02 10:30:28, bartfab (slow) wrote:
> Nit 1: const pointer.
> Nit 2: A unique_ptr would be semantically cleaner.

1) Done
2) Don't think that will work here, because PolicyMap::Set() takes ownership of
the |value| argument.  (It crashed when I tried it.)

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:276:
std::unique_ptr<base::ListValue> list(new base::ListValue());
On 2016/05/02 10:30:29, bartfab (slow) wrote:
> Nit: #include <memory>

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:282: for
(RepeatedPtrField<std::string>::const_iterator it = urls.begin();
On 2016/05/02 18:42:08, emaxx wrote:
> Nit: Can the range-based for loop be used here?

Done.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider.cc:286:
new_values_cache->SetValue(kLoginVideoCaptureAllowedUrls, std::move(list));
On 2016/05/02 10:30:29, bartfab (slow) wrote:
> Nit: #include <utility>

Done.

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

https://codereview.chromium.org/1936903002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8498: 'tags': [],
On 2016/05/02 10:30:29, bartfab (slow) wrote:
> Nit: Definitely "website-sharing", maybe more?

Done.

https://codereview.chromium.org/1936903002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:8502: found, access will be
automatically denied.'''
On 2016/05/02 10:30:29, bartfab (slow) wrote:
> Nit: Document that wildcard patterns are not allowed.

Done.

[Kevin Cernekee, 2016-05-03 15:55:28]

cernekee@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Kevin Cernekee, 2016-05-03 15:55:28]

Steven or Zel, could you please take a quick look?  We're trying to get this in
for M52 so it can be rolled out to schools in August.

Thanks!

[bartfab (slow), 2016-05-03 16:14:15]

LGTM, assuming you will follow-up with a test at a later time.

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:332:
base::ListValue* urls = new base::ListValue();
On 2016/05/02 20:02:29, Kevin Cernekee wrote:
> On 2016/05/02 10:30:28, bartfab (slow) wrote:
> > Nit 1: const pointer.
> > Nit 2: A unique_ptr would be semantically cleaner.
> 
> 1) Done
> 2) Don't think that will work here, because PolicyMap::Set() takes ownership
of
> the |value| argument.  (It crashed when I tried it.)

Sure, you need to relase() when hading over ownership to PolicyMap::Set().

[Kevin Cernekee, 2016-05-03 17:54:54]

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1936903002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:332:
base::ListValue* urls = new base::ListValue();
On 2016/05/03 16:14:15, bartfab (slow) wrote:
> On 2016/05/02 20:02:29, Kevin Cernekee wrote:
> > On 2016/05/02 10:30:28, bartfab (slow) wrote:
> > > Nit 1: const pointer.
> > > Nit 2: A unique_ptr would be semantically cleaner.
> > 
> > 1) Done
> > 2) Don't think that will work here, because PolicyMap::Set() takes ownership
> of
> > the |value| argument.  (It crashed when I tried it.)
> 
> Sure, you need to relase() when hading over ownership to PolicyMap::Set().

Done.

[stevenjb, 2016-05-03 19:53:41]

rs owner lgtm

[Kevin Cernekee, 2016-05-03 20:09:47]

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

[commit-bot: I haz the power, 2016-05-03 20:12:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936903002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936903002/60001

[commit-bot: I haz the power, 2016-05-03 20:16:36]

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

[commit-bot: I haz the power, 2016-05-03 20:16:38]

Dry run: This issue passed the CQ dry run.

[Kevin Cernekee, 2016-05-03 21:03:22]

The CQ bit was checked by cernekee@chromium.org

[Kevin Cernekee, 2016-05-03 21:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, emaxx@chromium.org, bartfab@chromium.org
Link to the patchset: https://codereview.chromium.org/1936903002/#ps60001
(title: "use unique_ptr for url list")

[commit-bot: I haz the power, 2016-05-03 21:03:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936903002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936903002/60001

[commit-bot: I haz the power, 2016-05-03 21:14:32]

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

[commit-bot: I haz the power, 2016-05-03 21:14:33]

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

[Kevin Cernekee, 2016-05-03 21:17:24]

cernekee@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Kevin Cernekee, 2016-05-03 21:17:25]

xiyuan - could you please review the change under
chrome/browser/resources/gaia_auth_host ?

Thanks!

[xiyuan, 2016-05-03 21:40:18]

lgtm

https://codereview.chromium.org/1936903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth_host/saml_handler.js (right):

https://codereview.chromium.org/1936903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/gaia_auth_host/saml_handler.js:153: e.request.allow();
nit: Add a comment about where the actual permission check happens to avoid
confusions from future reader of this code.

[Kevin Cernekee, 2016-05-03 21:52:03]

The CQ bit was checked by cernekee@chromium.org

[Kevin Cernekee, 2016-05-03 21:52:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, stevenjb@chromium.org, xiyuan@chromium.org,
emaxx@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1936903002/#ps80001
(title: "add comment to JS re: C++ permission check")

[commit-bot: I haz the power, 2016-05-03 21:52:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936903002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936903002/80001

[commit-bot: I haz the power, 2016-05-03 23:19:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-03 23:21:44]

Description was changed from

==========
Allow SAML logins to use the webcam

Add a new opt-in policy, LoginVideoCaptureAllowedUrls, that works in a
similar way to the existing VideoCaptureAllowedUrls policy.  SAML
login pages listed in the whitelist will be allowed to access the
webcam (no audio).  If the policy is unset or the list is empty, all
SAML login pages will be denied webcam access.

BUG=606979
TEST=whitelist https://clever.com via YAPS and verify with
     mo@clever.academy login
==========

to

==========
Allow SAML logins to use the webcam

Add a new opt-in policy, LoginVideoCaptureAllowedUrls, that works in a
similar way to the existing VideoCaptureAllowedUrls policy.  SAML
login pages listed in the whitelist will be allowed to access the
webcam (no audio).  If the policy is unset or the list is empty, all
SAML login pages will be denied webcam access.

BUG=606979
TEST=whitelist https://clever.com via YAPS and verify with
     mo@clever.academy login

Committed: https://crrev.com/cfcb9306e198a4e56facc865a60f45ce6d28f1aa
Cr-Commit-Position: refs/heads/master@{#391390}
==========

[commit-bot: I haz the power, 2016-05-03 23:21:45]

Patchset 5 (id:??) landed as
https://crrev.com/cfcb9306e198a4e56facc865a60f45ce6d28f1aa
Cr-Commit-Position: refs/heads/master@{#391390}