Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(428)

Issue 382143005: Supports DevTools socket access authentication based on Android permissions. (Closed)

Created:
6 years, 5 months ago by SeRya
Modified:
6 years, 4 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, vsevik, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Supports DevTools socket access authentication based on Android permissions. BUG=399567 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288273

Patch Set 1 #

Patch Set 2 : Fixed compilation error in gnubby_auth_handler_posix.cc #

Patch Set 3 : Merged #

Total comments: 16

Patch Set 4 : Code review fixes #

Patch Set 5 : Packing parameters to a stucture #

Total comments: 16

Patch Set 6 : Code review fixes #

Total comments: 9

Patch Set 7 : Non-static permission enabling #

Total comments: 3

Patch Set 8 : Commented process_id #

Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -70 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java View 1 2 3 4 5 6 2 chunks +31 lines, -2 lines 0 comments Download
M chrome/browser/android/dev_tools_server.h View 1 2 3 4 5 6 1 chunk +1 line, -4 lines 0 comments Download
M chrome/browser/android/dev_tools_server.cc View 1 2 3 4 5 6 7 chunks +26 lines, -21 lines 0 comments Download
M content/browser/android/devtools_auth.cc View 1 2 3 4 1 chunk +11 lines, -6 lines 0 comments Download
M content/public/browser/android/devtools_auth.h View 1 2 3 4 5 1 chunk +5 lines, -5 lines 0 comments Download
M net/socket/unix_domain_client_socket_posix_unittest.cc View 1 2 3 4 5 1 chunk +7 lines, -3 lines 0 comments Download
M net/socket/unix_domain_listen_socket_posix.cc View 1 2 3 4 5 1 chunk +3 lines, -4 lines 0 comments Download
M net/socket/unix_domain_listen_socket_posix_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M net/socket/unix_domain_server_socket_posix.h View 1 2 3 4 5 6 7 1 chunk +18 lines, -8 lines 0 comments Download
M net/socket/unix_domain_server_socket_posix.cc View 1 2 3 4 5 2 chunks +10 lines, -10 lines 0 comments Download
M net/socket/unix_domain_server_socket_posix_unittest.cc View 1 2 3 4 1 chunk +7 lines, -3 lines 0 comments Download
M remoting/host/gnubby_auth_handler_posix.cc View 1 2 3 4 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
SeRya
PTAL
6 years, 4 months ago (2014-08-01 12:47:06 UTC) #1
mmenke
https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc#newcode45 net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0; Not a huge fan of just ...
6 years, 4 months ago (2014-08-01 16:26:52 UTC) #2
aandrey
https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode30 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/org/chromium/chrome/browser/DevToolsServer.java#newcode32 ...
6 years, 4 months ago (2014-08-01 16:39:23 UTC) #3
byungchul
https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode30 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30: public static void requirePeermission( Is it possible to make ...
6 years, 4 months ago (2014-08-01 16:45:16 UTC) #4
SeRya
https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode30 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:30: public static void requirePeermission( On 2014/08/01 16:39:23, aandrey wrote: ...
6 years, 4 months ago (2014-08-04 10:47:04 UTC) #5
mmenke
https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc#newcode45 net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0; On 2014/08/04 10:47:03, SeRya wrote: > ...
6 years, 4 months ago (2014-08-04 15:45:58 UTC) #6
SeRya
https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/382143005/diff/140001/net/socket/unix_domain_server_socket_posix.cc#newcode45 net/socket/unix_domain_server_socket_posix.cc:45: *process_id = 0; On 2014/08/04 15:45:58, mmenke wrote: > ...
6 years, 4 months ago (2014-08-04 20:14:25 UTC) #7
mmenke
LGTM https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_server_socket_posix.h File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/382143005/diff/240001/net/socket/unix_domain_server_socket_posix.h#newcode67 net/socket/unix_domain_server_socket_posix.h:67: struct UnixDomainServerSocket::Credentials { In net/, at least, public ...
6 years, 4 months ago (2014-08-04 20:22:57 UTC) #8
mmenke
Oh, and that LGTM only applies to net/, didn't look at anything else.
6 years, 4 months ago (2014-08-04 20:23:22 UTC) #9
byungchul
https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/dev_tools_server.cc File chrome/browser/android/dev_tools_server.cc (right): https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/dev_tools_server.cc#newcode84 chrome/browser/android/dev_tools_server.cc:84: credentials.process_id, credentials.user_id) || wrong indentation. Should be aligned to ...
6 years, 4 months ago (2014-08-04 22:18:02 UTC) #10
SeRya
https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/dev_tools_server.cc File chrome/browser/android/dev_tools_server.cc (right): https://codereview.chromium.org/382143005/diff/240001/chrome/browser/android/dev_tools_server.cc#newcode84 chrome/browser/android/dev_tools_server.cc:84: credentials.process_id, credentials.user_id) || On 2014/08/04 22:18:02, byungchul wrote: > ...
6 years, 4 months ago (2014-08-05 10:32:27 UTC) #11
byungchul
lgtm
6 years, 4 months ago (2014-08-05 17:33:44 UTC) #12
SeRya
The CQ bit was checked by serya@chromium.org
6 years, 4 months ago (2014-08-05 18:48:14 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/382143005/280001
6 years, 4 months ago (2014-08-05 18:49:34 UTC) #14
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux ...
6 years, 4 months ago (2014-08-05 19:33:59 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-05 19:37:21 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/2155)
6 years, 4 months ago (2014-08-05 19:37:22 UTC) #17
SeRya
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
6 years, 4 months ago (2014-08-06 08:59:36 UTC) #18
Yaron
https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode20 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20: private static class SocketAuthentication { Do you anticipate more ...
6 years, 4 months ago (2014-08-06 21:39:10 UTC) #19
SeRya
https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode20 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:20: private static class SocketAuthentication { On 2014/08/06 21:39:10, Yaron ...
6 years, 4 months ago (2014-08-06 22:04:36 UTC) #20
Yaron
https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode18 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:18: private static volatile SocketAuthentication sAuthentication = new SocketAuthentication(); Which ...
6 years, 4 months ago (2014-08-07 01:38:06 UTC) #21
SeRya
https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java File chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java (right): https://codereview.chromium.org/382143005/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java#newcode18 chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java:18: private static volatile SocketAuthentication sAuthentication = new SocketAuthentication(); On ...
6 years, 4 months ago (2014-08-07 14:33:04 UTC) #22
Yaron
lgtm
6 years, 4 months ago (2014-08-07 18:07:52 UTC) #23
byungchul
https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/dev_tools_server.cc File chrome/browser/android/dev_tools_server.cc (right): https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/dev_tools_server.cc#newcode85 chrome/browser/android/dev_tools_server.cc:85: content::CanUserConnectToDevTools(credentials); Why not check content::CanUserConnectToDevTools(credentials) first which doesn't need ...
6 years, 4 months ago (2014-08-07 18:18:18 UTC) #24
SeRya
On 2014/08/07 18:18:18, byungchul wrote: > https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/dev_tools_server.cc > File chrome/browser/android/dev_tools_server.cc (right): > > https://codereview.chromium.org/382143005/diff/340001/chrome/browser/android/dev_tools_server.cc#newcode85 > ...
6 years, 4 months ago (2014-08-07 19:13:56 UTC) #25
garykac
lgtm
6 years, 4 months ago (2014-08-07 21:48:05 UTC) #26
mmenke
Forgot to publish this earlier. https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_server_socket_posix.h File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_server_socket_posix.h#newcode30 net/socket/unix_domain_server_socket_posix.h:30: #if defined(OS_LINUX) || defined(OS_ANDROID) ...
6 years, 4 months ago (2014-08-07 22:44:18 UTC) #27
SeRya
https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_server_socket_posix.h File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/382143005/diff/340001/net/socket/unix_domain_server_socket_posix.h#newcode30 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: ...
6 years, 4 months ago (2014-08-08 07:42:20 UTC) #28
SeRya
The CQ bit was checked by serya@chromium.org
6 years, 4 months ago (2014-08-08 07:42:57 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/serya@chromium.org/382143005/360001
6 years, 4 months ago (2014-08-08 07:45:34 UTC) #30
commit-bot: I haz the power
6 years, 4 months ago (2014-08-08 09:17:56 UTC) #31
Message was sent while issue was closed.
Change committed as 288273

Powered by Google App Engine
This is Rietveld 408576698