Implement argument validation for chrome.cast.channel.{open,send}

Implement argument validation for chrome.cast.channel.{open,send}

Cleanup namespace usage in cast_channel_api.h.

TESTED=Browser test.  Manually with Cast extension
BUG=331165
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266804

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

[imcheng, 2014-04-24 22:13:18]

lgtm

https://codereview.chromium.org/255443002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/255443002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:343: return true;
super nit: I'd prefer to see validation logic return true at the very end:

switch (params_->message.data->GetType()) {
  case base::Value::TYPE_STRING:
  case base::Value::TYPE_BINARY:
    break;
  default:
    SetError("Invalid type of message_info.data");
    return false;
}
return true;


Because that way it will be less error-prone to append other validation logic in
the future.

[mark a. foltz, 2014-04-25 17:38:31]

On 2014/04/24 22:13:18, imcheng1 wrote:
> lgtm
> 
>
https://codereview.chromium.org/255443002/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):
> 
>
https://codereview.chromium.org/255443002/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:343: return
true;
> super nit: I'd prefer to see validation logic return true at the very end:
> 
> switch (params_->message.data->GetType()) {
>   case base::Value::TYPE_STRING:
>   case base::Value::TYPE_BINARY:
>     break;
>   default:
>     SetError("Invalid type of message_info.data");
>     return false;
> }
> return true;
> 
> 
> Because that way it will be less error-prone to append other validation logic
in
> the future.

Done.  I also changed CCOF::Prepare() to return true at the end.

[Munjal (Google), 2014-04-28 17:19:51]

LGTM

[mark a. foltz, 2014-04-28 18:38:38]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfoltz@chromium.org/255443002/20001

[commit-bot: I haz the power, 2014-04-28 18:53:46]

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

[commit-bot: I haz the power, 2014-04-28 18:53:47]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mark a. foltz, 2014-04-28 20:27:52]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfoltz@chromium.org/255443002/20001

[commit-bot: I haz the power, 2014-04-28 22:21:42]

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

[commit-bot: I haz the power, 2014-04-28 22:21:42]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[mark a. foltz, 2014-04-28 23:22:18]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfoltz@chromium.org/255443002/20001

[mark a. foltz, 2014-04-29 04:28:01]

The CQ bit was unchecked by mfoltz@chromium.org

[mark a. foltz, 2014-04-29 04:28:03]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-04-29 04:28:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfoltz@chromium.org/255443002/20001

[commit-bot: I haz the power, 2014-04-29 09:36:44]

Change committed as 266804

[Michael Achenbach, 2014-04-29 10:47:50]

A revert of this CL has been created in
https://codereview.chromium.org/256333002/ by machenbach@chromium.org.

The reason for reverting is: Causing leaks:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[mark a. foltz, 2014-07-14 21:02:22]

Relanding this patch, converted test cases to browser tests as it's difficult to
correctly unit test extension functions.

[imcheng, 2014-07-15 21:26:41]

lgtm

https://codereview.chromium.org/255443002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api_unittest.cc
(right):

https://codereview.chromium.org/255443002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api_unittest.cc:12:
namespace extensions {
nit: Can this file be reverted?

[mark a. foltz, 2014-07-16 21:43:26]

https://codereview.chromium.org/255443002/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api_unittest.cc
(right):

https://codereview.chromium.org/255443002/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api_unittest.cc:12:
namespace extensions {
On 2014/07/15 21:26:41, imcheng1 wrote:
> nit: Can this file be reverted?

Done.

[Wez, 2014-07-16 22:42:33]

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:300:
SetError("Invalid connect_info");
nit: May be helpful to be able to distinguish this error (bad ConnectInfo
parameters) from the previous error (malformed ConnectInfo)?

It looks like ConnectInfo::FromValue() mostly just parses the fields, except for
the auth-type, which it enforces cannot be NONE; ParseConnectInfo() enforces
values on the fields (including auth-type) and then returns an IPEndpoint for
it. Can we break the latter into IsValidConnectInfo() and
IpEndpointFromConnectInfo(), and remove the auth-type check from FromValue, to
keep parsing, verifying and fetching fields well encapsulated?

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.h (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api.h:140:
cast_channel::ConnectInfo* connect_info);
nit: This is just cleanup, not related to parameter validation, so mention it in
the CL description.

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:288:
"[{\"ipAddress\": \"invalid_ip\", \"port\": 8009, \"auth\": \"ssl\"}]",
This tests a valid ConnectInfo with invalid ipAddress; consider adding a test
for invalid ConnectInfo, e.g. one missing ipAddress, port, etc.

[mark a. foltz, 2014-07-17 19:26:51]

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:300:
SetError("Invalid connect_info");
On 2014/07/16 22:42:33, Wez wrote:
> nit: May be helpful to be able to distinguish this error (bad ConnectInfo
> parameters) from the previous error (malformed ConnectInfo)

Done.
> 
> It looks like ConnectInfo::FromValue() mostly just parses the fields, except
for
> the auth-type, which it enforces cannot be NONE; ParseConnectInfo() enforces
> values on the fields (including auth-type) and then returns an IPEndpoint for
> it. Can we break the latter into IsValidConnectInfo() and
> IpEndpointFromConnectInfo(), and remove the auth-type check from FromValue, to
> keep parsing, verifying and fetching fields well encapsulated?

Done, at the expense of parsing the IP address twice.

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.h (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_api.h:140:
cast_channel::ConnectInfo* connect_info);
On 2014/07/16 22:42:33, Wez wrote:
> nit: This is just cleanup, not related to parameter validation, so mention it
in
> the CL description.

Done.

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc (right):

https://codereview.chromium.org/255443002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_channel_apitest.cc:288:
"[{\"ipAddress\": \"invalid_ip\", \"port\": 8009, \"auth\": \"ssl\"}]",
On 2014/07/16 22:42:33, Wez wrote:
> This tests a valid ConnectInfo with invalid ipAddress; consider adding a test
> for invalid ConnectInfo, e.g. one missing ipAddress, port, etc.

Done.

[Wez, 2014-07-17 21:16:48]

lgtm

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:305:
SetError("Invalid connect_info (unknown type)");
This can just go in the default: case, surely?

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:307:
SetError("connect_info.auth is required");
I was actually thinking we could just get rid of the AUTH NONE failure mode in
FromValue() entirely, since it'll be caught by the IsValidConnectInfoAuth(),
below, won't it?

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:309:
SetError("Invalid connect_info (invalid Cast URL " + cast_url + ")");
This could go in the TYPE_STRING failure block, if FromValue() is fixed to
never-fail.

[mark a. foltz, 2014-07-17 22:23:40]

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:305:
SetError("Invalid connect_info (unknown type)");
On 2014/07/17 21:16:47, Wez wrote:
> This can just go in the default: case, surely?

Done.

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:307:
SetError("connect_info.auth is required");
On 2014/07/17 21:16:47, Wez wrote:
> I was actually thinking we could just get rid of the AUTH NONE failure mode in
> FromValue() entirely, since it'll be caught by the IsValidConnectInfoAuth(),
> below, won't it?

FromValue() is generated code that can't be changed.  It is intended to verify
the enum is set.  I think it's clearer to explicitly check the value as well.

https://codereview.chromium.org/255443002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:309:
SetError("Invalid connect_info (invalid Cast URL " + cast_url + ")");
On 2014/07/17 21:16:47, Wez wrote:
> This could go in the TYPE_STRING failure block, if FromValue() is fixed to
> never-fail.

Done.

[mark a. foltz, 2014-07-18 18:02:23]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:04:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfoltz@chromium.org/255443002/190001

[commit-bot: I haz the power, 2014-07-18 21:24:52]

Change committed as 284200