Use extensions socket permission for TCP/UDP socket APIs in Pepper

Use extensions socket permission for TCP/UDP socket APIs in Pepper

This CL is first step in removing whitelist of extensions.
It makes permission check same as extensions do.

BUG=124311
TEST=browser_tests for TCP/UDP


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165376

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

[ygorshenin1, 2012-09-28 14:43:33]

LGTM with nits.

https://codereview.chromium.org/10993078/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/10993078/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:1749: &extension_params))
nit: delete single space before "&extension_params".

https://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_message_filter.cc:47: #include
"ppapi/cpp/private/net_address_private.h"
nit: sort, please.

https://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_message_filter.cc:126:
message.type() == PpapiHostMsg_PPBTCPServerSocket_Listen::ID) {
nit: message ID's could be sorted here, for better readability.

https://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/pepper_message_filter.cc:504: if
(routing_id == iter->second->routing_id() &&  allowed)
nit: delete single space before "allowed".

https://codereview.chromium.org/10993078/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/10993078/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:105: int port;
s/int/uint16_t?

[Dmitry Polukhin, 2012-10-01 11:00:24]

+ Brett for OWNER review

http://codereview.chromium.org/10993078/diff/1/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10993078/diff/1/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.cc:1749: &extension_params))
On 2012/09/28 14:43:34, ygorshenin1 wrote:
> nit: delete single space before "&extension_params".

Done.

http://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

http://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/pepper/pepper_message_filter.cc:47: #include
"ppapi/cpp/private/net_address_private.h"
On 2012/09/28 14:43:34, ygorshenin1 wrote:
> nit: sort, please.

Done.

http://codereview.chromium.org/10993078/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/pepper/pepper_message_filter.cc:126:
message.type() == PpapiHostMsg_PPBTCPServerSocket_Listen::ID) {
On 2012/09/28 14:43:34, ygorshenin1 wrote:
> nit: message ID's could be sorted here, for better readability.

Done.

http://codereview.chromium.org/10993078/diff/1/content/public/browser/content...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10993078/diff/1/content/public/browser/content...
content/public/browser/content_browser_client.h:105: int port;
On 2012/09/28 14:43:34, ygorshenin1 wrote:
> s/int/uint16_t?

Not sure. Port is declared as int in many other place. This struct is basically
the same as SocketPermission::CheckParam (see
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/pe...)
but I can't use it here because it will create illegal dependency.

[miket_OOO, 2012-10-05 18:46:01]

lgtm

http://codereview.chromium.org/10993078/diff/13001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10993078/diff/13001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1748: }
This block seems like it could be a generally useful utility function.

[Dmitry Polukhin, 2012-10-08 09:50:35]

Brett, please take a look as owner.

http://codereview.chromium.org/10993078/diff/13001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10993078/diff/13001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1748: }
On 2012/10/05 18:46:02, miket wrote:
> This block seems like it could be a generally useful utility function.

Done.

[brettw, 2012-10-09 21:44:54]

John should review nontrivial content API changes.

I'm wondering what the long-term plan for this is. It doesn't seem like this can
support all the permissions we'll be adding for different things. What are we
planning on doing for other APIs?

Or are we only going to be implementing the Pepper Apps v2 APIs in Chrome from
here on out, and this is just a hack to make the current implementation of
TCP/UDP work in this world?

[jam, 2012-10-10 20:48:30]

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: struct SocketPermissionParam
{
please move this to a separate file (the convention in the content api is that
every struct or class should be in a separate file)

also, it seems unfortunate that this duplicates a subset of the
SocketPermissionData structure in chrome. can the latter be made to contain this
struct and add whatever other data it needs to it?

[Dmitry Polukhin, 2012-10-17 09:25:54]

Sorry for delay with code review - was busy with M23 blocker issues.

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: struct SocketPermissionParam
{
On 2012/10/10 20:48:30, John Abd-El-Malek wrote:
> please move this to a separate file (the convention in the content api is that
> every struct or class should be in a separate file)

Done.

> also, it seems unfortunate that this duplicates a subset of the
> SocketPermissionData structure in chrome. can the latter be made to contain
this
> struct and add whatever other data it needs to it?

I agree that it looks unfortunate to have this duplication. But if extension
internals use things from content, it also looks a bit strange to me and any
change there will require OWNER LGTM from content... But I'll switch extensions
code to content::SocketPermissionRequest if extensions team is OK with it.

[jam, 2012-10-17 15:22:41]

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/10993078/diff/24001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: struct SocketPermissionParam
{
On 2012/10/17 09:25:54, Dmitry Polukhin wrote:
> On 2012/10/10 20:48:30, John Abd-El-Malek wrote:
> > please move this to a separate file (the convention in the content api is
that
> > every struct or class should be in a separate file)
> 
> Done.
> 
> > also, it seems unfortunate that this duplicates a subset of the
> > SocketPermissionData structure in chrome. can the latter be made to contain
> this
> > struct and add whatever other data it needs to it?
> 
> I agree that it looks unfortunate to have this duplication. But if extension
> internals use things from content, it also looks a bit strange to me

why do you say it's odd? extensions is in chrome, which depends on content (i.e.
like it depends on base). extensions code uses many objects from content
already.

> and any
> change there will require OWNER LGTM from content... But I'll switch
extensions
> code to content::SocketPermissionRequest if extensions team is OK with it.

[Dmitry Polukhin, 2012-10-18 08:57:38]

I removed duplication but it violates checkdeps rules.

http://codereview.chromium.org/10993078/diff/24001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10993078/diff/24001/content/public/browser/con...
content/public/browser/content_browser_client.h:86: struct SocketPermissionParam
{
On 2012/10/17 15:22:42, John Abd-El-Malek wrote:
> why do you say it's odd? extensions is in chrome, which depends on content
(i.e.
> like it depends on base). extensions code uses many objects from content
> already.

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  chrome/common/extensions/permissions/socket_permission_data.h
      Illegal include: "content/public/browser/socket_permission_request.h"
    Because of no rule applying.

Is it legal inclusion and I should add some checkdeps rule to remove this error?

[jam, 2012-10-18 15:37:15]

http://codereview.chromium.org/10993078/diff/24001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10993078/diff/24001/content/public/browser/con...
content/public/browser/content_browser_client.h:86: struct SocketPermissionParam
{
On 2012/10/18 08:57:38, Dmitry Polukhin wrote:
> On 2012/10/17 15:22:42, John Abd-El-Malek wrote:
> > why do you say it's odd? extensions is in chrome, which depends on content
> (i.e.
> > like it depends on base). extensions code uses many objects from content
> > already.
> 
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   chrome/common/extensions/permissions/socket_permission_data.h
>       Illegal include: "content/public/browser/socket_permission_request.h"
>     Because of no rule applying.
> 
> Is it legal inclusion and I should add some checkdeps rule to remove this
error?

this is because chrome\common is including from content\public\browser, which
isn't allowed. just move the new header to content\public\common instead

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
why only bring in the enum here? i.e. why not the whole SocketPermissionRequest
struct and remove host_/port_ below? (that's what i had meant by duplication)

[dpolukhin, 2012-10-18 18:01:00]

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
On 2012/10/18 15:37:15, John Abd-El-Malek wrote:
> why only bring in the enum here? i.e. why not the whole
SocketPermissionRequest
> struct and remove host_/port_ below? (that's what i had meant by duplication)

This class represents pattern for matching not a request. I did replace
<type,host,port> with new struct in SocketPermission::CheckParam (see
chrome/common/extensions/permissions/socket_permission.h, it was the
duplication).

[jam, 2012-10-18 18:17:06]

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
On 2012/10/18 18:01:00, dpolukhin wrote:
> On 2012/10/18 15:37:15, John Abd-El-Malek wrote:
> > why only bring in the enum here? i.e. why not the whole
> SocketPermissionRequest
> > struct and remove host_/port_ below? (that's what i had meant by
duplication)
> 
> This class represents pattern for matching not a request.

I'm not sure I follow the distinction, i.e. why does that make a difference of
whether this struct reuses an enum but not other members of the content struct,
vs using the whole struct?

> I did replace
> <type,host,port> with new struct in SocketPermission::CheckParam (see
> chrome/common/extensions/permissions/socket_permission.h, it was the
> duplication).

[dpolukhin, 2012-10-18 18:30:34]

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
On 2012/10/18 18:17:06, John Abd-El-Malek wrote:
> > This class represents pattern for matching not a request.
> 
> I'm not sure I follow the distinction, i.e. why does that make a difference of
> whether this struct reuses an enum but not other members of the content
struct,
> vs using the whole struct?

New SocketPermissionRequest has 1:1: correspondence with
SocketPermission::CheckParam and there I eliminated duplication. Patter and
request are different things even if they both have members with the same names.

[jam, 2012-10-18 21:21:34]

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
On 2012/10/18 18:30:34, dpolukhin wrote:
> On 2012/10/18 18:17:06, John Abd-El-Malek wrote:
> > > This class represents pattern for matching not a request.
> > 
> > I'm not sure I follow the distinction, i.e. why does that make a difference
of
> > whether this struct reuses an enum but not other members of the content
> struct,
> > vs using the whole struct?
> 
> New SocketPermissionRequest has 1:1: correspondence with
> SocketPermission::CheckParam and there I eliminated duplication. Patter and
> request are different things even if they both have members with the same
names.

so it's just a coincidence that they both have host_ and port_ members?

[Dmitry Polukhin, 2012-10-19 09:23:59]

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/socket_permission_data.h (right):

http://codereview.chromium.org/10993078/diff/37001/chrome/common/extensions/p...
chrome/common/extensions/permissions/socket_permission_data.h:56:
content::SocketPermissionRequest::OperationType type_;
On 2012/10/18 21:21:34, John Abd-El-Malek wrote:
> On 2012/10/18 18:30:34, dpolukhin wrote:
> > On 2012/10/18 18:17:06, John Abd-El-Malek wrote:
> > > > This class represents pattern for matching not a request.
> > > 
> > > I'm not sure I follow the distinction, i.e. why does that make a
difference
> of
> > > whether this struct reuses an enum but not other members of the content
> > struct,
> > > vs using the whole struct?
> > 
> > New SocketPermissionRequest has 1:1: correspondence with
> > SocketPermission::CheckParam and there I eliminated duplication. Patter and
> > request are different things even if they both have members with the same
> names.
> 
> so it's just a coincidence that they both have host_ and port_ members?

They are related but not the same. But I did it because you insist. IMHO, it
makes all accesses longer and doesn't increase code readability.

[jam, 2012-10-19 16:19:08]

lgtm

[commit-bot: I haz the power, 2012-10-19 18:00:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/10993078/37002

[commit-bot: I haz the power, 2012-10-19 18:00:27]

Presubmit check for 10993078-37002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ppapi/proxy
    ppapi

Presubmit checks took 2.9s to calculate.

[dpolukhin, 2012-10-29 04:14:14]

Brett, friendly ping, I need OWNER review in ppapi/, could you please take a
look?

[brettw, 2012-10-31 23:34:23]

ppapi lgtm. sorry for the delay.

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
ppapi/shared_impl/private/net_address_private_impl.cc:285: !!include_port);
Please use PP_ToBool(include_port) like the old code did.

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
ppapi/shared_impl/private/net_address_private_impl.cc:507: else
else
  return
return

should be just
"return ConvertIPv4..."
(We prefer no "else" if the previous arm has an early return and the second
return is redundant.)

[Dmitry Polukhin, 2012-11-01 07:59:52]

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/net_address_private_impl.cc (right):

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
ppapi/shared_impl/private/net_address_private_impl.cc:285: !!include_port);
On 2012/10/31 23:34:23, brettw wrote:
> Please use PP_ToBool(include_port) like the old code did.

Done.

http://codereview.chromium.org/10993078/diff/49001/ppapi/shared_impl/private/...
ppapi/shared_impl/private/net_address_private_impl.cc:507: else
On 2012/10/31 23:34:23, brettw wrote:
> else
>   return
> return
> 
> should be just
> "return ConvertIPv4..."
> (We prefer no "else" if the previous arm has an early return and the second
> return is redundant.)

Done.

[commit-bot: I haz the power, 2012-11-01 08:01:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/10993078/49002

[commit-bot: I haz the power, 2012-11-01 09:33:37]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 09:38:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/10993078/49002

[commit-bot: I haz the power, 2012-11-01 11:03:25]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-01 13:56:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/10993078/49002

[commit-bot: I haz the power, 2012-11-01 14:46:12]

Change committed as 165376

[Dmitry Polukhin, 2012-11-01 15:23:47]

+benm@ for OWNER review in android_webview/

[benm (inactive), 2012-11-01 15:37:45]

lgtm