Implement Networking Private API VerifyAndEncryptCredentials method

Implementation of Networking Private API VerifyAndEncryptCredentials method on Windows and Mac OS X.

Adds methods to NetworkingPrivateService to verify device credentials and use NetworkingPrivateCredentialsGetter to get wifi password from system. 

On Windows that requires launching utility process with UAC privilege elevation, which is not implemented yet.

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

[mef, 2014-01-24 20:19:44]

Hi guys, please take a look. 

I've added initial implementation of Networking Private API
VerifyAndEncryptCredentials method on Windows.

I haven't implemented the launch of utility process to actually get the password
yet, and I'm thinking that maybe it would make sense to do it as a separate CL.

Please comment.

thanks,
-m

[mef, 2014-01-28 17:51:26]

Hi guys, please take a look. I've added WiFi password slurping using Utility
process. Once https://codereview.chromium.org/98603007/ lands the utility
process will be launched elevated.

I have a couple of questions:

1. Dependencies / layering. I've had to add dependencies to chrome/utility, but
that doesn't feel right. Is there a better way to package handling of
GetAndEncryptWiFiPassphrase message in the utility process.

2. Handling of VerifyAndEncryptCredentials seems to require performing 3
asynchronous task in sequence (exiting sequence if task fails): Verify device
credentials, check availability of WiFi Passphrase in the system, get encrypted
wifi passphrase using elevated utility process. Is there a nice way to express
this or just posting next task from the callback of previous task is fine?   

thanks,
-m

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:616:
// This will call StartProcessOnIOThread;
After https://codereview.chromium.org/98603007/ lands the utility process will
launch elevated.

https://codereview.chromium.org/102993002/diff/170001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/170001/chrome/utility/DEPS#new...
chrome/utility/DEPS:2: "+chrome/browser/extensions/api/networking_private",
This dependency is needed for use of NetworkingPrivateCrypto in
ChromeContentUtilityClient::OnGetAndEncryptWiFiPassphrase.

https://codereview.chromium.org/102993002/diff/170001/chrome/utility/DEPS#new...
chrome/utility/DEPS:5: "+components/wifi",
This dependency is needed for use of WiFiService in
ChromeContentUtilityClient::OnGetAndEncryptWiFiPassphrase.

[mef, 2014-02-06 16:26:35]

Hi,

I'm looking for suggestions whether or not it is ok to add dependencies to
"chrome/utility" (this is for WiFi password slurping using elevated utility
process on Windows):

"+chrome/browser/extensions/api/networking_private",
"+components/wifi",

I suspect that it is not ok, but I'm not sure what is the correct answer. 

I'll appreciate any comments.

thanks,
-m

[tbarzic, 2014-02-07 19:25:00]

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:61:
std::string* ciphertext,
arg names should match those in h file

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:56:
virtual void VerifyAndEncryptCredentials(scoped_ptr<base::ListValue> args,
first arg to a new line, then align the rest with it.
(same for implementation in networking_private_apitest)

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc:85:
BrowserThread::UI,
instead of changing thread here, you can pass the UI message loop proxy to
StartProcessIOThread and use it to create the utility process host

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:876:
wifi_service->Initialize(NULL);
hm, wifi_service does much more than we need.
Can you just implement GetPassphraseFromSystem somewhere in chrome/utility? (it
doesn't seem that it would create too much extra code).

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:190:
// Chromecast's public key.
remove Chromecast from the comment (unless the method is really Cromecast
specific)

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS#new...
chrome/utility/DEPS:2: "+chrome/browser/extensions/api/networking_private",
Hm, I'm not sure if adding dependency on chrome/browser/ this is ok or not.
I guess you could avoid this by moving
chrome/browser/extensions/api/networking_private/networking_private_crypto
somewhere already accessible from chrome/utility.

[mef, 2014-02-07 19:53:30]

Toni, thanks a lot for your response! 

Would it work better from layering perspective if I add
chrome/utility/networking_private folder and add new DEPS there?

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:61:
std::string* ciphertext,
On 2014/02/07 19:25:00, tbarzic wrote:
> arg names should match those in h file

Done.

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:56:
virtual void VerifyAndEncryptCredentials(scoped_ptr<base::ListValue> args,
On 2014/02/07 19:25:00, tbarzic wrote:
> first arg to a new line, then align the rest with it.
> (same for implementation in networking_private_apitest)

Done.

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc:85:
BrowserThread::UI,
On 2014/02/07 19:25:00, tbarzic wrote:
> instead of changing thread here, you can pass the UI message loop proxy to
> StartProcessIOThread and use it to create the utility process host

Hmm, I suppose I can store current message loop proxy in the constructor, would
that work?

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:876:
wifi_service->Initialize(NULL);
On 2014/02/07 19:25:00, tbarzic wrote:
> hm, wifi_service does much more than we need.
> Can you just implement GetPassphraseFromSystem somewhere in chrome/utility?
(it
> doesn't seem that it would create too much extra code).
Yes and no. One of important functions of WiFiService on Windows is dynamic
loading of of WlanApi.dll to avoid function resolution issues on XP SP2.

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:190:
// Chromecast's public key.
On 2014/02/07 19:25:00, tbarzic wrote:
> remove Chromecast from the comment (unless the method is really Cromecast
> specific)
It is as 'Verify' step of all Verify* methods expects device credentials to be
signed by Google production key, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS#new...
chrome/utility/DEPS:2: "+chrome/browser/extensions/api/networking_private",
On 2014/02/07 19:25:00, tbarzic wrote:
> Hm, I'm not sure if adding dependency on chrome/browser/ this is ok or not.
> I guess you could avoid this by moving
> chrome/browser/extensions/api/networking_private/networking_private_crypto
> somewhere already accessible from chrome/utility.

I agree, but where? The most reasonable seems to be "extensions/common", but
even that doesn't seem to be right. OTOH chrome/utility/importer has its own
DEPS, so maybe I should add chrome/utility/networking_private and add DEPS
there?

[tbarzic, 2014-02-07 22:43:01]

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc:85:
BrowserThread::UI,
On 2014/02/07 19:53:31, mef wrote:
> On 2014/02/07 19:25:00, tbarzic wrote:
> > instead of changing thread here, you can pass the UI message loop proxy to
> > StartProcessIOThread and use it to create the utility process host
> 
> Hmm, I suppose I can store current message loop proxy in the constructor,
would
> that work?

I think you can just pass it to StartProcessOnIOThread from Start:

void StartProcessOnIOThread(const scoped_refptr<base::MessageLoopProxy>&
response_message_loop,...) {}

and in Start:

BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
Bind(&StartProcessOnIOThread, this, base::MessageLoopProxy::current()));

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:876:
wifi_service->Initialize(NULL);
On 2014/02/07 19:53:31, mef wrote:
> On 2014/02/07 19:25:00, tbarzic wrote:
> > hm, wifi_service does much more than we need.
> > Can you just implement GetPassphraseFromSystem somewhere in chrome/utility?
> (it
> > doesn't seem that it would create too much extra code).
> Yes and no. One of important functions of WiFiService on Windows is dynamic
> loading of of WlanApi.dll to avoid function resolution issues on XP SP2. 

OK then. Though it would be nice if we prevented the WiFiService from listening
to WLAN changes if it doesn't have to.
(e.g. extract wlan enumeration and listener registration from
WiFiServiceImpl::OpenClientHandle, and pass additional arg to Initialize that
would determine if the extracted part should be called or not). WDYT?

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc
(right):

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:52:
{ }
nuke the empty space

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:60:
callback_id,
nit: alignment

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:445:
service_callbacks->credentials_getter.reset(
does the getter have to be owned by service?
Why don't you just have VerifyAndEncryptCredentials own it?

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:44:
const GetCredentialsCallback& callback) : callback_(callback) {
nit:
move ': callback_...' to new line

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:190:
// Chromecast's public key.
On 2014/02/07 19:53:31, mef wrote:
> On 2014/02/07 19:25:00, tbarzic wrote:
> > remove Chromecast from the comment (unless the method is really Cromecast
> > specific)
> It is as 'Verify' step of all Verify* methods expects device credentials to be
> signed by Google production key, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

ack

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:234:
scoped_ptr<NetworkingPrivateCredentialsGetter> credentials_getter;
NetworkingPrivateCredentialsGetter is ref counted on win (through
UtilityProcessHostClient), so having it owned by a scoped_ptr seems a bit
dangerous.

I'd imagine this crashes (either when this is reset or when the
UtilityProcessHost gets deleted, whichever happens later).

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS#new...
chrome/utility/DEPS:2: "+chrome/browser/extensions/api/networking_private",
On 2014/02/07 19:53:31, mef wrote:
> On 2014/02/07 19:25:00, tbarzic wrote:
> > Hm, I'm not sure if adding dependency on chrome/browser/ this is ok or not.
> > I guess you could avoid this by moving
> > chrome/browser/extensions/api/networking_private/networking_private_crypto
> > somewhere already accessible from chrome/utility.
> 
> I agree, but where? The most reasonable seems to be "extensions/common", but
> even that doesn't seem to be right. OTOH chrome/utility/importer has its own
> DEPS, so maybe I should add chrome/utility/networking_private and add DEPS
> there?

I wouldn't move this to a new dir just for a sake of adding a new dep. If adding
a dependency to chrome/browser here is wrong, it would be wrong in
chrome/utility/foo :)
Though, it may be better to defer this question to jam@.

I'm not sure what would be the best alternative home for
networking_private_crypto. Somewhere in extension sounds OK to me, but there may
be people with stronger opinion against this.
Other places I'd consider are net/ or components/wifi (though this would
probably require some renaming/generalization).

[mef, 2014-02-11 23:20:03]

Toni, thanks, PTAL, I've addressed your comments and got it to work with
privilege elevation on windows..

jam@ - please advise about proper layering for utility process message handling.

jorgelo@ - chrome/common/chrome_utility_messages.h,
chrome/utility/chrome_content_utility_ipc_whitelist.cc and general security.

thanks,
-m

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc
(right):

https://codereview.chromium.org/102993002/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/wifi_passphrase_getter.cc:85:
BrowserThread::UI,
On 2014/02/07 22:43:02, tbarzic wrote:
> On 2014/02/07 19:53:31, mef wrote:
> > On 2014/02/07 19:25:00, tbarzic wrote:
> > > instead of changing thread here, you can pass the UI message loop proxy to
> > > StartProcessIOThread and use it to create the utility process host
> > 
> > Hmm, I suppose I can store current message loop proxy in the constructor,
> would
> > that work?
> 
> I think you can just pass it to StartProcessOnIOThread from Start:
> 
> void StartProcessOnIOThread(const scoped_refptr<base::MessageLoopProxy>&
> response_message_loop,...) {}
> 
> and in Start:
> 
> BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
> Bind(&StartProcessOnIOThread, this, base::MessageLoopProxy::current()));

Done. Start is called on the worker thread, but constructor is called on UI
thread, so I store message_loop_proxy there.

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc
(right):

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:52:
{ }
On 2014/02/07 22:43:02, tbarzic wrote:
> nuke the empty space

Done.

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:60:
callback_id,
On 2014/02/07 22:43:02, tbarzic wrote:
> nit: alignment

Done.

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/420001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:445:
service_callbacks->credentials_getter.reset(
On 2014/02/07 22:43:02, tbarzic wrote:
> does the getter have to be owned by service?
> Why don't you just have VerifyAndEncryptCredentials own it?

I think you are right, but I'm not sure how to express it. CredentialsGetter
must live after VerifyAndEncryptCredentials is done, because it spawns the
utility process, which could take forever to respond (given that user has to
click through UAC prompt).

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_mac.cc:44:
const GetCredentialsCallback& callback) : callback_(callback) {
On 2014/02/07 22:43:02, tbarzic wrote:
> nit:
> move ': callback_...' to new line

Done.

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/102993002/diff/440001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:234:
scoped_ptr<NetworkingPrivateCredentialsGetter> credentials_getter;
On 2014/02/07 22:43:02, tbarzic wrote:
> NetworkingPrivateCredentialsGetter is ref counted on win (through
> UtilityProcessHostClient), so having it owned by a scoped_ptr seems a bit
> dangerous.
> 
> I'd imagine this crashes (either when this is reset or when the
> UtilityProcessHost gets deleted, whichever happens later).
Done. You are absolutely correct. I couldn't inherit
NetworkingPrivateCredentialsGetter from RefCounted because it is refcounted
through UtilityProcessHostClient on win), so I had to separate implementation of
UtilityProcessHostClient and  NetworkingPrivateCredentialsGetter into separate
classes.

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/440001/chrome/utility/DEPS#new...
chrome/utility/DEPS:2: "+chrome/browser/extensions/api/networking_private",
On 2014/02/07 22:43:02, tbarzic wrote:
> On 2014/02/07 19:53:31, mef wrote:
> > On 2014/02/07 19:25:00, tbarzic wrote:
> > > Hm, I'm not sure if adding dependency on chrome/browser/ this is ok or
not.
> > > I guess you could avoid this by moving
> > > chrome/browser/extensions/api/networking_private/networking_private_crypto
> > > somewhere already accessible from chrome/utility.
> > 
> > I agree, but where? The most reasonable seems to be "extensions/common", but
> > even that doesn't seem to be right. OTOH chrome/utility/importer has its own
> > DEPS, so maybe I should add chrome/utility/networking_private and add DEPS
> > there?
> 
> I wouldn't move this to a new dir just for a sake of adding a new dep. If
adding
> a dependency to chrome/browser here is wrong, it would be wrong in
> chrome/utility/foo :)
> Though, it may be better to defer this question to jam@.
> 
> I'm not sure what would be the best alternative home for
> networking_private_crypto. Somewhere in extension sounds OK to me, but there
may
> be people with stronger opinion against this.
> Other places I'd consider are net/ or components/wifi (though this would
> probably require some renaming/generalization).

Ack. I really need an advise from jam@ at this point.

[tbarzic, 2014-02-13 18:54:19]

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:144:
base::Unretained(this),
This doesn't seem safe. NPCredentialsGetter is owned by a profile keyed service
on UI thread. This may run on IO thread after it's been destroyed.

Wy not just move this logic to CredentialsGetterHostClient::Start, and just call
host_client_->Start(network_guid, public_key);

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:159:
host_client_, base::MessageLoopProxy::current().get());
pass result_loop_proxy_ instead of the current message loop proxy.

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:165:
void NetworkingPrivateCredentialsGetterWin::ReportResult(
If you pass callback to the host client and invoke it directly from there, you
will not need this method.

[mef, 2014-02-14 16:37:38]

Thanks, Toni, PTAL.

I'm still eagerly looking forward to comments from jam@ and jorgelo@.

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:144:
base::Unretained(this),
On 2014/02/13 18:54:19, tbarzic wrote:
> This doesn't seem safe. NPCredentialsGetter is owned by a profile keyed
service
> on UI thread. This may run on IO thread after it's been destroyed.
> 
> Wy not just move this logic to CredentialsGetterHostClient::Start, and just
call
> host_client_->Start(network_guid, public_key);

Done. I've moved |StartProcessOnIOThread| to host_client_ and post it from here,
which I think should be fine as it is refcounted...

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:159:
host_client_, base::MessageLoopProxy::current().get());
On 2014/02/13 18:54:19, tbarzic wrote:
> pass result_loop_proxy_ instead of the current message loop proxy.

Done.

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:165:
void NetworkingPrivateCredentialsGetterWin::ReportResult(
On 2014/02/13 18:54:19, tbarzic wrote:
> If you pass callback to the host client and invoke it directly from there, you
> will not need this method.
Actually I still do. It is an interface method that is also called by
CryptoVerifyImpl::VerifyAndEncryptCredentials if credentials are not valid.

[tbarzic, 2014-02-21 19:02:50]

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:876:
wifi_service->Initialize(NULL);
On 2014/02/07 22:43:02, tbarzic wrote:
> On 2014/02/07 19:53:31, mef wrote:
> > On 2014/02/07 19:25:00, tbarzic wrote:
> > > hm, wifi_service does much more than we need.
> > > Can you just implement GetPassphraseFromSystem somewhere in
chrome/utility?
> > (it
> > > doesn't seem that it would create too much extra code).
> > Yes and no. One of important functions of WiFiService on Windows is dynamic
> > loading of of WlanApi.dll to avoid function resolution issues on XP SP2. 
> 
> OK then. Though it would be nice if we prevented the WiFiService from
listening
> to WLAN changes if it doesn't have to.
> (e.g. extract wlan enumeration and listener registration from
> WiFiServiceImpl::OpenClientHandle, and pass additional arg to Initialize that
> would determine if the extracted part should be called or not). WDYT?

I think you missed this comment :)

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:144:
base::Unretained(this),
On 2014/02/14 16:37:38, mef wrote:
> On 2014/02/13 18:54:19, tbarzic wrote:
> > This doesn't seem safe. NPCredentialsGetter is owned by a profile keyed
> service
> > on UI thread. This may run on IO thread after it's been destroyed.
> > 
> > Wy not just move this logic to CredentialsGetterHostClient::Start, and just
> call
> > host_client_->Start(network_guid, public_key);
> 
> Done. I've moved |StartProcessOnIOThread| to host_client_ and post it from
here,
> which I think should be fine as it is refcounted...

sg

https://codereview.chromium.org/102993002/diff/970001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:165:
void NetworkingPrivateCredentialsGetterWin::ReportResult(
On 2014/02/14 16:37:38, mef wrote:
> On 2014/02/13 18:54:19, tbarzic wrote:
> > If you pass callback to the host client and invoke it directly from there,
you
> > will not need this method.
> Actually I still do. It is an interface method that is also called by
> CryptoVerifyImpl::VerifyAndEncryptCredentials if credentials are not valid.

oh, right.. though, see may comment in networking_private_service_client.cc

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:82:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
won't these now be called on result_loop_proxy_ (you passed that message loop to
the UtilityProcessHost)

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:65:
if (!VerifyDestination(params->properties)) {
ReportResult seems like something that should be internal to credentials_getter.
Also, it's a bit weird to use credentials getter to report error not related to
the getter and before the getter has even started (and effectively hiding the
fact that this method is async in the credentials getter implementation).

I think that it would be cleaner to pass callback directly to
VerifyAndEncryptCredentials, and do something like:

virtual void VerifyAndEncryptCredentials(
    args,
    credentails_getter,
    callback) {
  if (!VerifyDestination()) {
    // callback should take care of changing thread to UI.
    callback.Run("","verify error");
    return;
  }
  if (!Base64Decode()) {
    callback.Run("", "decode error");
    return;
  }

  // Possibly require for the credentials_getter implementation to allow the
  // getter to be deleted after calling |Run|, and make credentials_getter
  // scoped_ptr (so you don't have to keep it alive in the service client).
  credentials_getter->Run(network_guid, public_key, callback);
}

[mef, 2014-02-25 19:57:58]

Toni, thanks for your suggestions, they've simplified things. PTAL.

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/270001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:876:
wifi_service->Initialize(NULL);
> I think you missed this comment :)
Sorry, you are right. I've moved call to WlanRegisterNotification into
WiFiServiceImpl::SetEventObservers, so it doesn't register in utility process.

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:82:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/02/21 19:02:52, tbarzic wrote:
> won't these now be called on result_loop_proxy_ (you passed that message loop
to
> the UtilityProcessHost)

Done. Pass base::MessageLoopProxy::current() from IO thread.

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1110001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:65:
if (!VerifyDestination(params->properties)) {
On 2014/02/21 19:02:52, tbarzic wrote:
> ReportResult seems like something that should be internal to
credentials_getter.
> Also, it's a bit weird to use credentials getter to report error not related
to
> the getter and before the getter has even started (and effectively hiding the
> fact that this method is async in the credentials getter implementation).
> 
> I think that it would be cleaner to pass callback directly to
> VerifyAndEncryptCredentials, and do something like:
> 
> virtual void VerifyAndEncryptCredentials(
>     args,
>     credentails_getter,
>     callback) {
>   if (!VerifyDestination()) {
>     // callback should take care of changing thread to UI.
>     callback.Run("","verify error");
>     return;
>   }
>   if (!Base64Decode()) {
>     callback.Run("", "decode error");
>     return;
>   }
> 
>   // Possibly require for the credentials_getter implementation to allow the
>   // getter to be deleted after calling |Run|, and make credentials_getter
>   // scoped_ptr (so you don't have to keep it alive in the service client).
>   credentials_getter->Run(network_guid, public_key, callback);
> }

Done.

[tbarzic, 2014-02-25 20:40:49]

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:28:
// Start getting credentials. Call |callback| with |key_data| or |error|.
nit: Starts; Calls

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:29:
virtual void Start(
Comment that the method should work even if |this| goes out of scope right after
the method call (and maybe add a test for this).

Also, please comment about thread the callback is called on.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:67:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Please, consider calling the callback on the thread
NetworkingPrivateCredentialsGetterWin::Start was called (so Start always
responds on the same thread it's called).
I guess this works too, but should be documented.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:88:
{
nit: { fits to the previous line

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:105:
explicit NetworkingPrivateCredentialsGetterWin();
you can remove explicit

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:438:
static void AfterVerifyAndEncryptCredentialsRelayOnWorkerThread(
move this to namespace {}

Also, this doesn't necessary have to be called on Worker thread, so the name
should probably be changed.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:456:
scoped_ptr<NetworkingPrivateCredentialsGetter> credentials_getter(
you can probably move the getter creation to |VerifyAndEncryptCredentials|.

https://codereview.chromium.org/102993002/diff/1360001/components/wifi/wifi_s...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/102993002/diff/1360001/components/wifi/wifi_s...
components/wifi/wifi_service_win.cc:710:
WlanRegisterNotification_function_(client_,
Should this be protected from multiple |SetEventObservers| call (or maybe there
should be a check that this is not called more than one)?

[mef, 2014-02-25 22:36:25]

Toni, thanks! PTAL.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:28:
// Start getting credentials. Call |callback| with |key_data| or |error|.
On 2014/02/25 20:40:49, tbarzic wrote:
> nit: Starts; Calls

Done.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:29:
virtual void Start(
On 2014/02/25 20:40:49, tbarzic wrote:
> Comment that the method should work even if |this| goes out of scope right
after
> the method call (and maybe add a test for this).
> 
> Also, please comment about thread the callback is called on.

Done.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:67:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/02/25 20:40:49, tbarzic wrote:
> Please, consider calling the callback on the thread
> NetworkingPrivateCredentialsGetterWin::Start was called (so Start always
> responds on the same thread it's called).
> I guess this works too, but should be documented.

I've tried that, but base::MessageLoopProxy::current() is NULL on the worker
thread, so I had to do it on IO thread.
I'm probably missing something, any suggestions?

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:88:
{
On 2014/02/25 20:40:49, tbarzic wrote:
> nit: { fits to the previous line

Done.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:105:
explicit NetworkingPrivateCredentialsGetterWin();
On 2014/02/25 20:40:49, tbarzic wrote:
> you can remove explicit

Done.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:438:
static void AfterVerifyAndEncryptCredentialsRelayOnWorkerThread(
On 2014/02/25 20:40:49, tbarzic wrote:
> move this to namespace {}
> 
> Also, this doesn't necessary have to be called on Worker thread, so the name
> should probably be changed.

Done.

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:456:
scoped_ptr<NetworkingPrivateCredentialsGetter> credentials_getter(
On 2014/02/25 20:40:49, tbarzic wrote:
> you can probably move the getter creation to |VerifyAndEncryptCredentials|.

Done.

https://codereview.chromium.org/102993002/diff/1360001/components/wifi/wifi_s...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/102993002/diff/1360001/components/wifi/wifi_s...
components/wifi/wifi_service_win.cc:710:
WlanRegisterNotification_function_(client_,
On 2014/02/25 20:40:49, tbarzic wrote:
> Should this be protected from multiple |SetEventObservers| call (or maybe
there
> should be a check that this is not called more than one)?

Done.

[tbarzic, 2014-02-25 23:07:24]

lgtm (module utility/DEPS)

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc
(right):

https://codereview.chromium.org/102993002/diff/1360001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:67:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/02/25 22:36:26, mef wrote:
> On 2014/02/25 20:40:49, tbarzic wrote:
> > Please, consider calling the callback on the thread
> > NetworkingPrivateCredentialsGetterWin::Start was called (so Start always
> > responds on the same thread it's called).
> > I guess this works too, but should be documented.
> 
> I've tried that, but base::MessageLoopProxy::current() is NULL on the worker
> thread, so I had to do it on IO thread.
> I'm probably missing something, any suggestions?

Hm, in that case, I'm ok with this.

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h
(right):

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:28:
// Starts getting credentials. Calls |callback| on random thread with
I'd reword this to something like:

// Starts getting credentials. The credentials and , in case of an error, the
error code are returned using |callback|.
// The NetworkingPrivateCredentialsGetter implementation should ensure that the
credentials request can be successfully processed even if |this| gets deleted
immediately after calling this method.
// Note that there are no guarantees about the thread on which |callback| is
run. The caller should make sure that the result is processed on the right
thread.

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:444:
void AfterVerifyAndEncryptCredentialsRelay(
nit: move this to line 121

https://codereview.chromium.org/102993002/diff/1380001/components/wifi/wifi_s...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/102993002/diff/1380001/components/wifi/wifi_s...
components/wifi/wifi_service_win.cc:711: WLAN_NOTIFICATION_SOURCE_NONE,
nit: indent

[jam, 2014-02-26 00:14:50]

back from leave, just saw this.

chrome/utility can't depend on chrome/browser. subdirectories of chrome
correspond to a process, and the only other subdirectory they can depend on is
chrome/common which is for shared code. another note is that this approach
wouldn't even link on windows where we have a separate dll for browser code and
for child process code.

since
chrome/browser/extensions/api/networking_private/networking_private_crypto.h is
used in chrome/browser and chrome/utility, it should be moved to chrome/common.
luckily this appears to be straightforward since it doesn't include any other
files from chrome/browser

[mef, 2014-02-26 17:43:36]

Thanks, PTAL! 

I've moved networking_private_crypto* to
chrome/common/extensions/api/networking_private.

Everything builds and tests fine, but I get git cl presubmit errors:

======================================================================
FAIL: testSiteVerificationFile (__main__.IntegrationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "docs\server2\integration_test.py", line 263, in testSiteVerificationFile
    response = LocalRenderer.Render('/' + SITE_VERIFICATION_FILE)
  File "S:\chrome\src\chrome\common\extensions\docs\server2\local_renderer.py",
line 20, in Render
    _LocalRenderServletDelegate()).Get()
  File "S:\chrome\src\chrome\common\extensions\docs\server2\render_servlet.py",
line 49, in Get
    server_instance = self._delegate.CreateServerInstance()
  File "S:\chrome\src\chrome\common\extensions\docs\server2\local_renderer.py",
line 11, in CreateServerInstance
    return ServerInstance.ForLocal()
  File "S:\chrome\src\chrome\common\extensions\docs\server2\server_instance.py",
line 195, in ForLocal
    CloudStorageFileSystemProvider(object_store_creator))
  File "S:\chrome\src\chrome\common\extensions\docs\server2\server_instance.py",
line 63, in __init__
    host_fs_at_trunk = host_file_system_provider.GetTrunk()
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\third_party\json_schema_compiler\memoize.py",
line 12, in im
pl
    memory[full_args] = fn(*args, **optargs)
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\host_file_system_provider.py",
line 61, in GetTrunk
    return self._Create('trunk', revision=self._max_trunk_revision)
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\host_file_system_provider.py",
line 86, in _Create
    file_system = self._constructor_for_test(branch=branch, revision=revision)
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\host_file_system_provider.py",
line 100, in <lambda>
    constructor_for_test=lambda **_: LocalFileSystem.Create(),
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\local_file_system.py", line
77, in Create
    return LocalFileSystem(ChromiumPath(*path))
  File
"S:\chrome\src\chrome\common\extensions\docs\server2\local_file_system.py", line
72, in __init__
    AssertIsDirectory(base_path)
  File "S:\chrome\src\chrome\common\extensions\docs\server2\path_util.py", line
57, in AssertIsDirectory
    assert IsDirectory(path), '"%s" is not a directory' % path
  File "S:\chrome\src\chrome\common\extensions\docs\server2\path_util.py", line
16, in IsDirectory
    AssertIsValid(path)
  File "S:\chrome\src\chrome\common\extensions\docs\server2\path_util.py", line
29, in AssertIsValid
    assert IsValid(path), 'Path "%s" is invalid' % path
AssertionError: Path "..\..\.." is invalid

Do you have any suggestions / ideas about that?

thanks,
-m

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h
(right):

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_credentials_getter.h:28:
// Starts getting credentials. Calls |callback| on random thread with
On 2014/02/25 23:07:25, tbarzic wrote:
> I'd reword this to something like:
> 
> // Starts getting credentials. The credentials and , in case of an error, the
> error code are returned using |callback|.
> // The NetworkingPrivateCredentialsGetter implementation should ensure that
the
> credentials request can be successfully processed even if |this| gets deleted
> immediately after calling this method.
> // Note that there are no guarantees about the thread on which |callback| is
> run. The caller should make sure that the result is processed on the right
> thread.

Done.

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1380001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:444:
void AfterVerifyAndEncryptCredentialsRelay(
On 2014/02/25 23:07:25, tbarzic wrote:
> nit: move this to line 121

Done.

https://codereview.chromium.org/102993002/diff/1380001/components/wifi/wifi_s...
File components/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/102993002/diff/1380001/components/wifi/wifi_s...
components/wifi/wifi_service_win.cc:711: WLAN_NOTIFICATION_SOURCE_NONE,
On 2014/02/25 23:07:25, tbarzic wrote:
> nit: indent

Done.

[jam, 2014-02-28 17:34:16]

lgtm

https://codereview.chromium.org/102993002/diff/1330002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/1330002/chrome/utility/DEPS#ne...
chrome/utility/DEPS:2: "+chrome/common/extensions/api/networking_private",
this isn't needed since chrome/DEPS has "+chrome/common"

[mef, 2014-02-28 21:28:04]

Thanks, guys!

Chris, could you OWNER-review 

    chrome/common/chrome_utility_messages.h

and

    chrome/utility/chrome_content_utility_ipc_whitelist.cc

thanks,
-m

https://codereview.chromium.org/102993002/diff/1330002/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/102993002/diff/1330002/chrome/utility/DEPS#ne...
chrome/utility/DEPS:2: "+chrome/common/extensions/api/networking_private",
On 2014/02/28 17:34:17, jam wrote:
> this isn't needed since chrome/DEPS has "+chrome/common"

Done.

[palmer, 2014-03-03 22:17:41]

We like to use the most specific types available, avoiding "blob" types like
strings. When we have to use strings, we like to limit their size, validate them
in some simple way, et c.

http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:479: std::string /* public_key */)
vector<uint8> would be a better type for a key; better yet, some particular key
type.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:482: // caller's public key. |error| is
not empty if error occured..
Nit: Just 1 ".".

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:484: std::string /* encrypted_key_data
*/,
vector<uint8> is a better type for encrypted data.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:485: std::string /* error */)
How many possible errors are there? An enumerated constant type would be better
here. (Then let the privileged recipient stringify it according to its policy.)

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
File
chrome/common/extensions/api/networking_private/networking_private_crypto.cc
(right):

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:91:
reinterpret_cast<unsigned char*>(const_cast<char*>(cert_data.c_str()));
These casts look a bit abusive, at least the const_cast.

Isn't string::data a better choice than string::c_str for this? (Of course,
really, GetDERFromPEM should fill a byte array/vector, not a string.)

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:174:
reinterpret_cast<unsigned char*>(const_cast<char*>(pub_key_der.c_str()));
Same concerns as above.

[Jorge Lucangeli Obes, 2014-03-03 23:31:38]

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:481: // Reply after gettng WiFi
credentials from the system and encrypting it with
nits: typo in "getting", "encrypting them" for verb-object agreement.

[mef, 2014-03-04 21:57:03]

Thanks a lot, PTAL!

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:479: std::string /* public_key */)
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> vector<uint8> would be a better type for a key; better yet, some particular
key
> type.

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:481: // Reply after gettng WiFi
credentials from the system and encrypting it with
On 2014/03/03 23:31:39, Jorge Lucangeli Obes wrote:
> nits: typo in "getting", "encrypting them" for verb-object agreement.

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:482: // caller's public key. |error| is
not empty if error occured..
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> Nit: Just 1 ".".

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:484: std::string /* encrypted_key_data
*/,
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> vector<uint8> is a better type for encrypted data.

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:485: std::string /* error */)
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> How many possible errors are there? An enumerated constant type would be
better
> here. (Then let the privileged recipient stringify it according to its
policy.)

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
File
chrome/common/extensions/api/networking_private/networking_private_crypto.cc
(right):

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:91:
reinterpret_cast<unsigned char*>(const_cast<char*>(cert_data.c_str()));
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> These casts look a bit abusive, at least the const_cast.
> 
> Isn't string::data a better choice than string::c_str for this? (Of course,
> really, GetDERFromPEM should fill a byte array/vector, not a string.)

Done.

https://codereview.chromium.org/102993002/diff/1430001/chrome/common/extensio...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:174:
reinterpret_cast<unsigned char*>(const_cast<char*>(pub_key_der.c_str()));
On 2014/03/03 22:17:42, Chromium Palmer wrote:
> Same concerns as above.

Done.

[palmer, 2014-03-05 00:20:54]

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:914: const std::string&
network_guid,
Can we use the GUID type in base/guid.h instead?

If not, for some reason:

Since we call this thing a "GUID", we should check that it actually meets the
lexical definition of a GUID (including enforcing the expected size).

Or perhaps wifi_service->GetKeyFromSystem does that? If so, document that fact
with a comment so that people don't wonder.

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:921:
wifi_service->GetKeyFromSystem(network_guid, &key_data, &error);
Ah, I see. This function expects strings. Is it possible to refactor that later
(not in this CL)? E.g the |network_guid| should be a GUID, the |key_data| should
be a byte vector, and you don't need |error| because the callee only ever sets
it to |kWiFiServiceError|, and checking for an error string to be empty is a bit
odd (I'd just use a bool).

[mef, 2014-03-05 01:21:39]

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:914: const std::string&
network_guid,
On 2014/03/05 00:20:54, Chromium Palmer wrote:
> Can we use the GUID type in base/guid.h instead?
> 
> If not, for some reason:
> 
> Since we call this thing a "GUID", we should check that it actually meets the
> lexical definition of a GUID (including enforcing the expected size).
> 
> Or perhaps wifi_service->GetKeyFromSystem does that? If so, document that fact
> with a comment so that people don't wonder.
It is (admittedly confusingly) called |network_guid| to match naming defined by
chromeos implementation. 
I think stevejb@ can provide some background. In windows and mac implementation
it equals to network ssid.

https://codereview.chromium.org/102993002/diff/1470001/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:921:
wifi_service->GetKeyFromSystem(network_guid, &key_data, &error);
On 2014/03/05 00:20:54, Chromium Palmer wrote:
> Ah, I see. This function expects strings. Is it possible to refactor that
later
> (not in this CL)? E.g the |network_guid| should be a GUID, the |key_data|
should
> be a byte vector, and you don't need |error| because the callee only ever sets
> it to |kWiFiServiceError|, and checking for an error string to be empty is a
bit
> odd (I'd just use a bool).
Strictly speaking WiFiService is a component that could be used by other
modules. 
Returning |error| string matches other interface functions, so I'd rather keep
it.
I agree that in this particular case |bool| is sufficient, but it is not always
the case.

[stevenjb, 2014-03-07 19:54:58]

lgtm

https://codereview.chromium.org/102993002/diff/1290003/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:476:
base::Passed(&args),
alignment looks off here?
Might be more clear to declare both callbacks above, rather than Bind one inline
but not the other.

[Jorge Lucangeli Obes, 2014-03-13 20:07:52]

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:481: // Reply after gettng WiFi
credentials from the system and encrypting them with
"getting"

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:482: // caller's public key. |success|
is false if error occured.
"occurred"

https://codereview.chromium.org/102993002/diff/1290003/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:927: NetworkingPrivateCrypto
crypto;
Who reviewed NetworkingPrivateCrypto?

[mef, 2014-03-13 20:22:28]

Thanks, PTAL!

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:481: // Reply after gettng WiFi
credentials from the system and encrypting them with
On 2014/03/13 20:07:53, Jorge Lucangeli Obes wrote:
> "getting"

Done.

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/chrome_u...
chrome/common/chrome_utility_messages.h:482: // caller's public key. |success|
is false if error occured.
On 2014/03/13 20:07:53, Jorge Lucangeli Obes wrote:
> "occurred"

Done.

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/extensio...
File
chrome/common/extensions/api/networking_private/networking_private_crypto.cc
(right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/common/extensio...
chrome/common/extensions/api/networking_private/networking_private_crypto.cc:5:
#include
"chrome/common/extensions/api/networking_private/networking_private_crypto.h"
NetworkingPrivateCrypto was originally reviewed by rsleevi@ and gspencer@ here:
https://codereview.chromium.org/23710003.
Moved to chrome/common in order to use in utility process.

https://codereview.chromium.org/102993002/diff/1290003/chrome/utility/chrome_...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/102993002/diff/1290003/chrome/utility/chrome_...
chrome/utility/chrome_content_utility_client.cc:927: NetworkingPrivateCrypto
crypto;
On 2014/03/13 20:07:53, Jorge Lucangeli Obes wrote:
> Who reviewed NetworkingPrivateCrypto?
rsleevi@ here: https://codereview.chromium.org/23710003.

[Jorge Lucangeli Obes, 2014-03-13 20:27:54]

lgtm

[mef, 2014-03-13 20:30:00]

On 2014/03/13 20:27:54, Jorge Lucangeli Obes wrote:
> lgtm

Thanks!

[mef, 2014-03-14 12:49:22]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-03-14 12:50:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/102993002/1540002

[commit-bot: I haz the power, 2014-03-14 12:51:06]

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

[commit-bot: I haz the power, 2014-03-14 12:51:07]

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

[mef, 2014-03-14 16:36:35]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-03-14 16:37:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/102993002/1540002

[commit-bot: I haz the power, 2014-03-14 21:32:13]

Change committed as 257209