Cast audio only policy enforcement support.

Cast audio only policy enforcement support.

BUG=447734
Committed: https://crrev.com/9d82cf614ff2b069f1094a394eaf750600ea4cc2
Cr-Commit-Position: refs/heads/master@{#311415}

[vadimgo, 2015-01-09 21:03:47]

vadimgo@chromium.org changed reviewers:
+ afontan@google.com, dougsteed@chromium.org, mfoltz@chromium.org

[mark a. foltz, 2015-01-12 22:01:56]

mfoltz@chromium.org changed reviewers:
+ kmarshall@chromium.org

[mark a. foltz, 2015-01-12 22:01:56]

mfoltz@chromium.org changed required reviewers:
+ kmarshall@chromium.org

[mark a. foltz, 2015-01-12 22:01:57]

Kevin should take a look as he's been in the code most recently.  Most of my
comments are around API design, the implementation looks functionally fine.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:49: std::string
client_auth_certificate;
A better API would test the client_auth_certificate for a set of policy keys
defined in cast_auth_util.cc (like kAudioOnlyPolicy) and store the result as an
enum bitmask (like AUDIO_ONLY_POLICY)...

Then have a method AuthResult::HasPolicy(AUDIO_ONLY_POLICY) to test the bitmask.

This way the mechanism for testing the authentication protocol for a specific
policy advertisement is hidden from users of AuthResult.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:35: VIDEO_OUT = (1 <<
0),
I don't think the parenthesis are necessary.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:361: // If device
capabilities are available in the connect information, pass the
Slightly prefer to pass the capabilities in the CastSocket ctor; perhaps you can
pass a default if they are not set in connect_info?

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:368:
socket->set_has_video_out(has_video_out);
ISTM we should have an API like

socket->HasCapability(VIDEO_OUT)

(assuming capabilities are set in the ctor as suggested above).

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:56: const char
kAudioOnlyPolicy[] =
This is an odd place to define a constant that is related to the certificate
contents.  cast_auth_util.cc would be better

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:232: if (has_video_out_) {
if (HasCapability(VIDEO_OUT) && result.HasPolicy(AuthResult::AUDIO_ONLY_POLICY))
{ ... }

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:232: if (has_video_out_) {
Slightly prefer to capture policy enforcement in its own method:

CastSocketImpl::EnforceConnectionPolicy(result);

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:237: LOG(ERROR) << "Audio
only policy enforced";
Please add an event to logging.proto and log it here so we capture the reason
the authenication failed.

Perhaps the call to LogSocketChallengeReplyEvent on success could be moved after
this block and instead we LogSocketChallengeReplyEvent here with a different
code on error.

https://codereview.chromium.org/807723004/diff/60001/extensions/common/api/ca...
File extensions/common/api/cast_channel.idl (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/common/api/ca...
extensions/common/api/cast_channel.idl:77: // If set, a bit mask describing
capability of the cast device.
A more self documenting API would accept an array of capability enums and
internally convert it to a bitmask for the protocol.  The downside is that we
would need to convert the IDL generated enum (which will be numbered 0 ... N-1)
to the bit shifted values internally.  I can see it either way.

If you stay with the bitmask, then please add a pointer to the canonical
definition of the values.

[vadimgo, 2015-01-13 00:08:28]

Please take another look.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:49: std::string
client_auth_certificate;
On 2015/01/12 22:01:56, mark a. foltz wrote:
> A better API would test the client_auth_certificate for a set of policy keys
> defined in cast_auth_util.cc (like kAudioOnlyPolicy) and store the result as
an
> enum bitmask (like AUDIO_ONLY_POLICY)...
> 
> Then have a method AuthResult::HasPolicy(AUDIO_ONLY_POLICY) to test the
bitmask.
> 
> This way the mechanism for testing the authentication protocol for a specific
> policy advertisement is hidden from users of AuthResult.
>  

Store the policy in AuthResult as suggested.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:35: VIDEO_OUT = (1 <<
0),
On 2015/01/12 22:01:57, mark a. foltz wrote:
> I don't think the parenthesis are necessary.

Removed parenthesis.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:361: // If device
capabilities are available in the connect information, pass the
On 2015/01/12 22:01:56, mark a. foltz wrote:
> Slightly prefer to pass the capabilities in the CastSocket ctor; perhaps you
can
> pass a default if they are not set in connect_info?

Changed to pass device capabilities to the socket constructor.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:368:
socket->set_has_video_out(has_video_out);
On 2015/01/12 22:01:56, mark a. foltz wrote:
> ISTM we should have an API like
> 
> socket->HasCapability(VIDEO_OUT)
> 
> (assuming capabilities are set in the ctor as suggested above).

This code is removed with the new api design.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:56: const char
kAudioOnlyPolicy[] =
On 2015/01/12 22:01:57, mark a. foltz wrote:
> This is an odd place to define a constant that is related to the certificate
> contents.  cast_auth_util.cc would be better

Done.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:232: if (has_video_out_) {
On 2015/01/12 22:01:57, mark a. foltz wrote:
> Slightly prefer to capture policy enforcement in its own method:
> 
> CastSocketImpl::EnforceConnectionPolicy(result);
> 

Done.

https://codereview.chromium.org/807723004/diff/60001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:237: LOG(ERROR) << "Audio
only policy enforced";
On 2015/01/12 22:01:57, mark a. foltz wrote:
> Please add an event to logging.proto and log it here so we capture the reason
> the authenication failed.
> 
> Perhaps the call to LogSocketChallengeReplyEvent on success could be moved
after
> this block and instead we LogSocketChallengeReplyEvent here with a different
> code on error.

Done.

https://codereview.chromium.org/807723004/diff/60001/extensions/common/api/ca...
File extensions/common/api/cast_channel.idl (right):

https://codereview.chromium.org/807723004/diff/60001/extensions/common/api/ca...
extensions/common/api/cast_channel.idl:77: // If set, a bit mask describing
capability of the cast device.
On 2015/01/12 22:01:57, mark a. foltz wrote:
> A more self documenting API would accept an array of capability enums and
> internally convert it to a bitmask for the protocol.  The downside is that we
> would need to convert the IDL generated enum (which will be numbered 0 ...
N-1)
> to the bit shifted values internally.  I can see it either way.
> 
> If you stay with the bitmask, then please add a pointer to the canonical
> definition of the values.
> 
> 

Now refer to the mask type in the comment.

[mark a. foltz, 2015-01-13 01:42:20]

Looks good modulo a few comments.   Also did the unit tests get dropped from the
patch?

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:36: enum PolicyType {
POLICY_NONE = 0, POLICY_AUDIO_ONLY = 1 << 0 };
Prefer a multi-line declaration; the style guide doesn't give definite guidance,
but it's more consistent with the surrounding code.

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:351:
connect_info_->capabilities.get() ? *connect_info_->capabilities : 0);
Consider declaring a constant value to provide the default.  It's not clear from
reading this that the default is "no capabilities".

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:231: if
((device_capabilities_ & CastDeviceCapability::VIDEO_OUT) != 0) {
No reason to have nested if() statements; use &&

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket_unittest.cc:200: 0),
Use constant suggested earlier

[vadimgo, 2015-01-13 02:21:45]

Please take another look.

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_auth_util.h:36: enum PolicyType {
POLICY_NONE = 0, POLICY_AUDIO_ONLY = 1 << 0 };
On 2015/01/13 01:42:19, mark a. foltz wrote:
> Prefer a multi-line declaration; the style guide doesn't give definite
guidance,
> but it's more consistent with the surrounding code.

"git cl format" formats this way, and since I run it every time before
uploading, it ends up this way,

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:351:
connect_info_->capabilities.get() ? *connect_info_->capabilities : 0);
On 2015/01/13 01:42:19, mark a. foltz wrote:
> Consider declaring a constant value to provide the default.  It's not clear
from
> reading this that the default is "no capabilities".

Done.

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket.cc:231: if
((device_capabilities_ & CastDeviceCapability::VIDEO_OUT) != 0) {
On 2015/01/13 01:42:19, mark a. foltz wrote:
> No reason to have nested if() statements; use &&

Done.

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/807723004/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_socket_unittest.cc:200: 0),
On 2015/01/13 01:42:19, mark a. foltz wrote:
> Use constant suggested earlier

Changed the cast socket unit test to use device capability VIDEO_OUT to exercise
the new code path by default.

[mark a. foltz, 2015-01-13 21:06:37]

lgtm

I do recommend a unit test for the negative case as well so that you can verify
that policy enforcement works as expected.

[vadimgo, 2015-01-13 21:48:26]

vadimgo@chromium.org changed reviewers:
+ rockot@chromium.org

[vadimgo, 2015-01-13 21:49:24]

+rockot for changes in

extensions/common/api/cast_channel.idl
extensions/common/api/cast_channel/logging.proto

[Ken Rockot(use gerrit already), 2015-01-13 21:51:35]

said files lgtm

[vadimgo, 2015-01-13 22:24:17]

Mark, I added two unit tests - TestChannelPolicyVerificationCapabilitiesNone and
TestChannelPolicyVerificationCapabilitiesVideoOut.

Could you please take another look?

Thanks!

[mark a. foltz, 2015-01-13 23:03:25]

lgtm

[vadimgo, 2015-01-13 23:28:25]

The CQ bit was checked by vadimgo@chromium.org

[commit-bot: I haz the power, 2015-01-13 23:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807723004/140001

[commit-bot: I haz the power, 2015-01-13 23:29:05]

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

[commit-bot: I haz the power, 2015-01-13 23:29:06]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Kevin M, 2015-01-14 00:26:16]

lgtm

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:53: unsigned int
channel_policy;
Policies, since there can be >1?

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:352:
connect_info_->capabilities.get() ? *connect_info_->capabilities
No need for get()

[vadimgo, 2015-01-14 01:16:15]

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.h (right):

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.h:53: unsigned int
channel_policy;
On 2015/01/14 00:26:16, Kevin M wrote:
> Policies, since there can be >1?

Done.

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/807723004/diff/140001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:352:
connect_info_->capabilities.get() ? *connect_info_->capabilities
On 2015/01/14 00:26:16, Kevin M wrote:
> No need for get()

Done.

[vadimgo, 2015-01-14 01:16:27]

The CQ bit was checked by vadimgo@chromium.org

[commit-bot: I haz the power, 2015-01-14 01:17:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807723004/160001

[commit-bot: I haz the power, 2015-01-14 02:15:33]

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

[commit-bot: I haz the power, 2015-01-14 02:15:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vadimgo, 2015-01-14 04:39:47]

The CQ bit was checked by vadimgo@chromium.org

[commit-bot: I haz the power, 2015-01-14 04:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807723004/180001

[commit-bot: I haz the power, 2015-01-14 05:38:34]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-01-14 05:39:28]

Patchset 10 (id:??) landed as
https://crrev.com/9d82cf614ff2b069f1094a394eaf750600ea4cc2
Cr-Commit-Position: refs/heads/master@{#311415}