Supports DevTools socket access authentication based on Android permissions.

Supports DevTools socket access authentication based on Android permissions.

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

[SeRya, 2014-08-01 12:47:06]

PTAL

[mmenke, 2014-08-01 16:26:52]

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0;
Not a huge fan of just silently making process_id 0 on certain platforms.  Don't
suppose there's a way to get PID here?

[aandrey, 2014-08-01 16:39:23]

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30:
public static void requirePeermission(
typo: Peermission -> Permission

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:32:
sContext = context;
should it be guarded by a lock?

[byungchul, 2014-08-01 16:45:16]

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30:
public static void requirePeermission(
Is it possible to make this non-static? Can't you pass the global reference of
devtools server to AuthenticateSocketAccess?

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:31:
Context context, String androidPermission) {
It must fit in a line (100 chars).

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:57: &&
sContext.checkPermission(sPermission, pid, uid)
wrong indetation, need 8  whitespaces.

https://codereview.chromium.org/382143005/diff/140001/chrome/browser/android/...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/382143005/diff/140001/chrome/browser/android/...
chrome/browser/android/dev_tools_server.cc:83:
content::CanUserConnectToDevTools(pid, uid, gid);
wrong indentation, 4 whitespaces.

[SeRya, 2014-08-04 10:47:04]

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30:
public static void requirePeermission(
On 2014/08/01 16:39:23, aandrey wrote:
> typo: Peermission -> Permission

Done.

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30:
public static void requirePeermission(
On 2014/08/01 16:45:16, byungchul wrote:
> Is it possible to make this non-static? Can't you pass the global reference of
> devtools server to AuthenticateSocketAccess?

I guess making this method nonstatic only make sens with doing the same with
"checkPermission". It's possible but would require the C++ part to keep
reference to the java object. It complicates code with little benefit.
Configuring permission makes sense on application startup.

Not sure what do you mean by "the global reference of devtools server". While at
most one instance of DevToolsServer presents at time there is no global
reference to DevToolsServer.

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:31:
Context context, String androidPermission) {
On 2014/08/01 16:45:16, byungchul wrote:
> It must fit in a line (100 chars).

Done.

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:32:
sContext = context;
On 2014/08/01 16:39:23, aandrey wrote:
> should it be guarded by a lock?

Instead of synchronizing I added an immutable class.

https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:57: &&
sContext.checkPermission(sPermission, pid, uid)
On 2014/08/01 16:45:16, byungchul wrote:
> wrong indetation, need 8  whitespaces.

Done.

https://codereview.chromium.org/382143005/diff/140001/chrome/browser/android/...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/382143005/diff/140001/chrome/browser/android/...
chrome/browser/android/dev_tools_server.cc:83:
content::CanUserConnectToDevTools(pid, uid, gid);
On 2014/08/01 16:45:16, byungchul wrote:
> wrong indentation, 4 whitespaces.

Done.

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0;
On 2014/08/01 16:26:52, mmenke wrote:
> Not a huge fan of just silently making process_id 0 on certain platforms. 
Don't
> suppose there's a way to get PID here?

It seems there is no consensus among UNIX systems that kind of credentials could
be retrieved. As an alternative I can introduce a structure SocketCredential
with fileds user_id, group_id and process_id only on Linux(Android).

[mmenke, 2014-08-04 15:45:58]

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0;
On 2014/08/04 10:47:03, SeRya wrote:
> On 2014/08/01 16:26:52, mmenke wrote:
> > Not a huge fan of just silently making process_id 0 on certain platforms. 
> Don't
> > suppose there's a way to get PID here?
> 
> It seems there is no consensus among UNIX systems that kind of credentials
could
> be retrieved. As an alternative I can introduce a structure SocketCredential
> with fileds user_id, group_id and process_id only on Linux(Android).

I'd prefer that approach.  Setting it to 0 on some platforms makes me worry
about broken checks on platforms where it's not set.  Open to other ideas.

[SeRya, 2014-08-04 20:14:25]

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0;
On 2014/08/04 15:45:58, mmenke wrote:
> On 2014/08/04 10:47:03, SeRya wrote:
> > On 2014/08/01 16:26:52, mmenke wrote:
> > > Not a huge fan of just silently making process_id 0 on certain platforms. 
> > Don't
> > > suppose there's a way to get PID here?
> > 
> > It seems there is no consensus among UNIX systems that kind of credentials
> could
> > be retrieved. As an alternative I can introduce a structure SocketCredential
> > with fileds user_id, group_id and process_id only on Linux(Android).
> 
> I'd prefer that approach.  Setting it to 0 on some platforms makes me worry
> about broken checks on platforms where it's not set.  Open to other ideas.

Done.

[mmenke, 2014-08-04 20:22:57]

LGTM

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:67: struct
UnixDomainServerSocket::Credentials {
In net/, at least, public inner classes and structs are generally inlined in the
declaration.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:69: pid_t process_id;
Think this is worth a comment (Code is pretty self-explanatory, but a comment
for something unexpected like this still seems a good idea to me).

[mmenke, 2014-08-04 20:23:22]

Oh, and that LGTM only applies to net/, didn't look at anything else.

[byungchul, 2014-08-04 22:18:02]

https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/...
chrome/browser/android/dev_tools_server.cc:84: credentials.process_id,
credentials.user_id) ||
wrong indentation. Should be aligned to '(', or line-wrap before env.

https://codereview.chromium.org/382143005/diff/240001/content/public/browser/...
File content/public/browser/android/devtools_auth.h (right):

https://codereview.chromium.org/382143005/diff/240001/content/public/browser/...
content/public/browser/android/devtools_auth.h:13: // Returns true if the given
user/group pair is authorized to connect to the
Update the comment.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:25: const
UnixDomainServerSocket::Credentials& credentials) {
wrong indentation

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:44: &credentials->user_id,
&credentials->group_id) == 0;
wrong indentation

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:40: static bool
GetPeerIds(SocketDescriptor socket_fd,
GetCredentials

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:67: struct
UnixDomainServerSocket::Credentials {
need NET_EXPORT

[SeRya, 2014-08-05 10:32:27]

https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/...
chrome/browser/android/dev_tools_server.cc:84: credentials.process_id,
credentials.user_id) ||
On 2014/08/04 22:18:02, byungchul wrote:
> wrong indentation. Should be aligned to '(', or line-wrap before env.

Done.

https://codereview.chromium.org/382143005/diff/240001/content/public/browser/...
File content/public/browser/android/devtools_auth.h (right):

https://codereview.chromium.org/382143005/diff/240001/content/public/browser/...
content/public/browser/android/devtools_auth.h:13: // Returns true if the given
user/group pair is authorized to connect to the
On 2014/08/04 22:18:02, byungchul wrote:
> Update the comment.

Done.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:25: const
UnixDomainServerSocket::Credentials& credentials) {
On 2014/08/04 22:18:02, byungchul wrote:
> wrong indentation

Done.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:44: &credentials->user_id,
&credentials->group_id) == 0;
On 2014/08/04 22:18:02, byungchul wrote:
> wrong indentation

Done.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:40: static bool
GetPeerIds(SocketDescriptor socket_fd,
On 2014/08/04 22:18:02, byungchul wrote:
> GetCredentials

Renamed to GetPeerCredentials

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:67: struct
UnixDomainServerSocket::Credentials {
On 2014/08/04 20:22:57, mmenke wrote:
> In net/, at least, public inner classes and structs are generally inlined in
the
> declaration.

Moved inside.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:67: struct
UnixDomainServerSocket::Credentials {
On 2014/08/04 22:18:02, byungchul wrote:
> need NET_EXPORT

Done.

https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:69: pid_t process_id;
On 2014/08/04 20:22:57, mmenke wrote:
> Think this is worth a comment (Code is pretty self-explanatory, but a comment
> for something unexpected like this still seems a good idea to me).

Done.

[byungchul, 2014-08-05 17:33:44]

lgtm

[SeRya, 2014-08-05 18:48:14]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-08-05 18:49:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/serya@chromium.org/382143005/280001

[commit-bot: I haz the power, 2014-08-05 19:33:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-05 19:37:21]

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

[commit-bot: I haz the power, 2014-08-05 19:37:22]

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

[SeRya, 2014-08-06 08:59:36]

Owners review needed:

@garykac for:
    remoting/host/gnubby_auth_handler_posix.cc
@yfriedman for:
    chrome/browser/android/dev_tools_server.cc
    content/browser/android/devtools_auth.cc
    content/public/browser/android/devtools_auth.h

[Yaron, 2014-08-06 21:39:10]

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20:
private static class SocketAuthentication {
Do you anticipate more sophisticated work here? Why not just have a single
member String with the required permission?

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:34:
public static void requirePermission(final Context context, final String
androidPermission) {
I don't see where this is called from. If it's from the app, you should have a
DevToolsServer instance and this doesn't need to be static.

[SeRya, 2014-08-06 22:04:36]

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20:
private static class SocketAuthentication {
On 2014/08/06 21:39:10, Yaron wrote:
> Do you anticipate more sophisticated work here? Why not just have a single
> member String with the required permission?

There only 2 scenarios expected: standard check (allowing Shell and Root users
to pass; also recently added same user) and standard check + permission check.

Since the class is private it may not be extended unless subclass is inside
DevToolsServer.

Checking permission needs Context. So it's why this code is not cover both
cases.

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:34:
public static void requirePermission(final Context context, final String
androidPermission) {
On 2014/08/06 21:39:09, Yaron wrote:
> I don't see where this is called from. If it's from the app, you should have a
> DevToolsServer instance and this doesn't need to be static.

It supposed to be called from Clank (reference to the patch placed to the design
doc). The only reason it's static is simplification. Native implementation
doesn't keep reference to the java instance. To make it non-static I would need
to introduce one.

[Yaron, 2014-08-07 01:38:06]

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:18:
private static volatile SocketAuthentication sAuthentication = new
SocketAuthentication();
Which threads are actually involved here? I assume requirePermission and
checkPermission are on separate threads but can you elaborate?

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20:
private static class SocketAuthentication {
On 2014/08/06 22:04:35, SeRya wrote:
> On 2014/08/06 21:39:10, Yaron wrote:
> > Do you anticipate more sophisticated work here? Why not just have a single
> > member String with the required permission?
> 
> There only 2 scenarios expected: standard check (allowing Shell and Root
users
> to pass; also recently added same user) and standard check + permission
check.
> 
> Since the class is private it may not be extended unless subclass is inside
> DevToolsServer.
> 
> Checking permission needs Context. So it's why this code is not cover both
> cases.

Ah. I missed the context bit but you can still simplify it.
 
You could replace sAuthentication with:
private static String sRequiredPremission;

and then just assign it in requirePermission.

then checkPermission can include the context as one param (you can get from
base::android::GetApplicationContext()

But actually, why do we need this at all? Can you not just do something like:
context.getPackageName() + ".permission.DEBUG" or something? That way it's all
nicely localized here.

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30: /*
/**

[SeRya, 2014-08-07 14:33:04]

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java
(right):

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:18:
private static volatile SocketAuthentication sAuthentication = new
SocketAuthentication();
On 2014/08/07 01:38:06, Yaron wrote:
> Which threads are actually involved here? I assume requirePermission and
> checkPermission are on separate threads but can you elaborate?

Callback originates from "Chrome_DevToolsHandlerThread".

initialization (as well as DevToolsServer creation) happens on the thread where
ChromeTabbedActivity initialized (presumably android ui thread).

https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20:
private static class SocketAuthentication {
On 2014/08/07 01:38:06, Yaron wrote:
> On 2014/08/06 22:04:35, SeRya wrote:
> > On 2014/08/06 21:39:10, Yaron wrote:
> > > Do you anticipate more sophisticated work here? Why not just have a single
> > > member String with the required permission?
> > 
> > There only 2 scenarios expected: standard check (allowing Shell and Root
users
> > to pass; also recently added same user) and standard check + permission
check.
> > 
> > Since the class is private it may not be extended unless subclass is inside
> > DevToolsServer.
> > 
> > Checking permission needs Context. So it's why this code is not cover both
> > cases.
> 
> Ah. I missed the context bit but you can still simplify it.
>  
> You could replace sAuthentication with:
> private static String sRequiredPremission;
> 
> and then just assign it in requirePermission.
> 
> then checkPermission can include the context as one param (you can get from
> base::android::GetApplicationContext()
> 
> But actually, why do we need this at all? Can you not just do something like:
> context.getPackageName() + ".permission.DEBUG" or something? That way it's all
> nicely localized here.

Good point. With GetApplicationContext() and derived permission name it's much
easier to make this non-static.

[Yaron, 2014-08-07 18:07:52]

lgtm

[byungchul, 2014-08-07 18:18:18]

https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/...
chrome/browser/android/dev_tools_server.cc:85:
content::CanUserConnectToDevTools(credentials);
Why not check content::CanUserConnectToDevTools(credentials) first which doesn't
need base::android::AttachCurrentThread(). It would be more readable.

[SeRya, 2014-08-07 19:13:56]

On 2014/08/07 18:18:18, byungchul wrote:
>
https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/...
> File chrome/browser/android/dev_tools_server.cc (right):
> 
>
https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/...
> chrome/browser/android/dev_tools_server.cc:85:
> content::CanUserConnectToDevTools(credentials);
> Why not check content::CanUserConnectToDevTools(credentials) first which
doesn't
> need base::android::AttachCurrentThread(). It would be more readable.

CanUserConnectToDevTools logs a warning if it rejects connection.

[garykac, 2014-08-07 21:48:05]

lgtm

[mmenke, 2014-08-07 22:44:18]

Forgot to publish this earlier.

https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:30: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
Sorry, was thinking about a comment specifically about process_id only being
available on Linux/Android.

[SeRya, 2014-08-08 07:42:20]

https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:30: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
On 2014/08/07 22:44:18, mmenke wrote:
> Sorry, was thinking about a comment specifically about process_id only being
> available on Linux/Android.

Done.

[SeRya, 2014-08-08 07:42:57]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-08-08 07:45:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/serya@chromium.org/382143005/360001

[commit-bot: I haz the power, 2014-08-08 09:17:56]

Change committed as 288273