Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610953003:
Convert utility process WiFi Credentials IPC to mojo (Closed)
|
Created:
3 years, 11 months ago by Noel Gordon Modified:
3 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, qsr+mojo_chromium.org, stevenjb+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert utility process WiFi Credentials IPC to mojo
Add an OS_WIN mojo API to the utility process to allow callers to get the
WiFi credentials for a given network, an API that requires UAC elevation,
and used by the chrome extensions private networking API.
Add elevation support to the mojo utility process helper class and update
tests for that class. Use the helper class to control the utility process
(elevate, start, make request, terminate with the response).
Add a browser test: post a get credentials request from the blocking pool
to simulate how the existing call flow works. Internally post the work to
a different thread (IO) before making mojo calls (this to work-around the
fact that mojo does not currently support blocking pool callers).
Note the browser test does not cover the lower-level WiFi interface since
the bots have no access to any WiFi network (good). There's a manual tool
to test that aspect with [1] and, in my testing on Win10, that tool works
very well.
[1] components/wifi/wifi_test.cc
BUG=680928
Review-Url: https://codereview.chromium.org/2610953003
Cr-Commit-Position: refs/heads/master@{#444342}
Committed: https://chromium.googlesource.com/chromium/src/+/cd3f19adeb8b19062badb591ff33b770dd0eb21f
Patch Set 1 : Initial pass written diretctly in plain Mojo. #Patch Set 2 : Add elevation API to UtilityProcessMojoClient #Patch Set 3 : Remove UtilityProcessHostClient, use UtilityProcessMojoClient #Patch Set 4 : Make destructor private to satisfy win-clang, add friend class. #Patch Set 5 : Clean up and vet comments. #Patch Set 6 : Ditch AddRef etc: make the getter class self-deleting. #Patch Set 7 : Add code from dependent patch. #Patch Set 8 : Minor tweaks, minor comment changes. #Patch Set 9 : Add 2017 .mojom copyright. #Patch Set 10 : Add UtilityProcessMojoClientBrowserTest::ElevatedSuccess test. #Patch Set 11 : Add NetworkingPrivateCredentialsGetter browser test. #Patch Set 12 : Delete the credentials getter_ early in the browser test. #
Total comments: 20
Patch Set 13 : Review comments round #1. #
Total comments: 5
Patch Set 14 : Review comments round #2. #
Total comments: 8
Patch Set 15 : Review comments round #3. #Patch Set 16 : Add mojom test network name. #Messages
Total messages: 104 (83 generated)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded https://codereview.chromium.org/2606383006 for test assessment.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo BUG= ========== to ========== Convert utility process WiFi Credentials IPC to mojo BUG=597124 ==========
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:200001) has been deleted
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo BUG=597124 ========== to ========== Convert utility process WiFi Credentials IPC to mojo BUG=680928 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
noel@chromium.org changed reviewers: + sammc@chromium.org, stevenjb@chromium.org, tibell@chromium.org
This private networking IPC is OS_WIN only and also requires utility process elevation. There is no automated test and I found no good extension to test this with and manual testing was a PITA. So I added a browser test. Uses the mojo utility process client to control the utility process, add elevation support to it, and add a test. +sammc, +tibell for the mojo parts. +stevenjb for private networking.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... File chrome/common/extensions/wifi_credentials.mojom (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... chrome/common/extensions/wifi_credentials.mojom:14: GetWiFiCredentials(string ssid) => (bool success, string key_data); Could we instead return a `string?` (which translates to std::optional<string> in the generated C++ bindings) to communicate failure?
https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:22: network_guid_.clear(); Why? https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:24: network_guid_ = "chrome://test-wifi-get-key-from-system"; This should be a shared constant somewhere. Also, there should be one for success and one for failure. https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:38: getter_->Start( Why not create the getter here? In any case, store it in a unique_ptr. https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:161: ~NetworkingPrivateCredentialsGetterWin() override = default; Destructors before methods. https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... File chrome/common/extensions/wifi_credentials.mojom (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... chrome/common/extensions/wifi_credentials.mojom:10: interface WiFiCredentials { WifiCredentialsGetter? https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:102: static void Create(extensions::mojom::WiFiCredentialsRequest request) { Constructors and destructors before methods. https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:129: key_data.clear(); This wasn't here before. https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... content/browser/utility_process_mojo_client_browsertest.cc:23: void StartMojoService(bool disable_sandbox, bool run_elevated = false) { This would be nicer as an enum {SANDBOXED, UNSANDBOXED, ELEVATED}.
https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... File chrome/common/extensions/wifi_credentials.mojom (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... chrome/common/extensions/wifi_credentials.mojom:14: GetWiFiCredentials(string ssid) => (bool success, string key_data); On 2017/01/16 03:34:01, tibell wrote: > Could we instead return a `string?` (which translates to std::optional<string> > in the generated C++ bindings) to communicate failure? We could I suppose, but it would diverge from the mac and chrome_os uses of the same API (which do not need / use the utility process). So I decided to leave it it alone.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
New patch uploaded ... https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:22: network_guid_.clear(); On 2017/01/16 04:16:20, Sam McNally wrote: > Why? I was thinking ... nevermind, removed. https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:24: network_guid_ = "chrome://test-wifi-get-key-from-system"; On 2017/01/16 04:16:20, Sam McNally wrote: > This should be a shared constant somewhere. Ack /shrug. > Also, there should be one for success and one for failure. The empty network guid seems to handle the failure case, unless I'm missing something. https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:38: getter_->Start( On 2017/01/16 04:16:20, Sam McNally wrote: > Why not create the getter here? In any case, store it in a unique_ptr. Done. https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:161: ~NetworkingPrivateCredentialsGetterWin() override = default; On 2017/01/16 04:16:20, Sam McNally wrote: > Destructors before methods. Done. https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... File chrome/common/extensions/wifi_credentials.mojom (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/common/extensio... chrome/common/extensions/wifi_credentials.mojom:10: interface WiFiCredentials { On 2017/01/16 04:16:20, Sam McNally wrote: > WifiCredentialsGetter? Done. https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:102: static void Create(extensions::mojom::WiFiCredentialsRequest request) { On 2017/01/16 04:16:20, Sam McNally wrote: > Constructors and destructors before methods. Done. https://codereview.chromium.org/2610953003/diff/260001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:129: key_data.clear(); On 2017/01/16 04:16:20, Sam McNally wrote: > This wasn't here before. But is now because security. If this API fails, the content of key_data is now always empty. https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... content/browser/utility_process_mojo_client_browsertest.cc:23: void StartMojoService(bool disable_sandbox, bool run_elevated = false) { On 2017/01/16 04:16:20, Sam McNally wrote: > This would be nicer as an enum {SANDBOXED, UNSANDBOXED, ELEVATED}. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
noel@chromium.org changed reviewers: + dcheng@chromium.org
+ dcheng for IPC security.
lgtm
https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:105: if (success) { Invert and early exit https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:146: network_guid, public_key, callback)); Is posting this to the IO thread still necessary? At some point we may want to move this code out of src/chrome, in which case we would need to pass task runners to this class instead of using BrowserThread, but since we are just posting mojo calls, I don't think we need to do that on the IO thread?
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:105: if (success) { On 2017/01/17 17:06:54, stevenjb wrote: > Invert and early exit Prefer the early return is good style rule, IMHO. Gladly done, PTAL. https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:146: network_guid, public_key, callback)); On 2017/01/17 17:06:54, stevenjb wrote: > Is posting this to the IO thread still necessary? At some point we may want to > move this code out of src/chrome, in which case we would need to pass task > runners to this class instead of using BrowserThread, but since we are just > posting mojo calls, I don't think we need to do that on the IO thread? If your task runner was from the blocking pool, for example, then mojo would currently crash: it doesn't support blocking pool callers at this time. Until that is fixed, we have to bounce the work onto a different thread (here IO) to prevent such crashes. So yes, IO thread is still necessary. Also note that the browsertest calls Start() from the blocking pool for this reason.
owner lgtm https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:146: network_guid, public_key, callback)); On 2017/01/17 21:54:26, noel gordon wrote: > On 2017/01/17 17:06:54, stevenjb wrote: > > Is posting this to the IO thread still necessary? At some point we may want to > > move this code out of src/chrome, in which case we would need to pass task > > runners to this class instead of using BrowserThread, but since we are just > > posting mojo calls, I don't think we need to do that on the IO thread? > > If your task runner was from the blocking pool, for example, then mojo would > currently crash: it doesn't support blocking pool callers at this time. > > Until that is fixed, we have to bounce the work onto a different thread (here > IO) to prevent such crashes. So yes, IO thread is still necessary. Also note > that the browsertest calls Start() from the blocking pool for this reason. Ugh, I see, CryptoVerifyImpl is callong NetworkingPrivateCredentialsGetter::Start() from the blocking pool. We should probably fix that, but in the short term I guess this is fine.
On 2017/01/17 22:01:27, stevenjb wrote: > Ugh, I see, CryptoVerifyImpl is callong > NetworkingPrivateCredentialsGetter::Start() from the blocking pool. Yes, that was noted when creating this CL. I found out spectacularly on https://crbug.com/676960 that mojo and block'n pool don't play nice: my change crashed the chrome canary in production.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:14: #error This test is OS_WIN only. Do we get this for free if the test file is named something like _browsertest_win.cc? https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:36: : public_key_(public_key.begin(), public_key.end()) {} Sigh, more things that can't agree on passing data as string or vector =( https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:128: callback_.Run(base64_encoded_key_data, ""); Nit: "" => std::string(), here and elsewhere. https://codereview.chromium.org/2610953003/diff/300001/chrome/common/extensio... File chrome/common/extensions/BUILD.gn (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/common/extensio... chrome/common/extensions/BUILD.gn:29: "wifi_credentials_getter.mojom", Should we only conditionally compile this interface on Windows?
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:14: #error This test is OS_WIN only. On 2017/01/18 00:39:22, dcheng wrote: > Do we get this for free if the test file is named something like > _browsertest_win.cc? Maybe possible. I'm also hoping some private networking peeps will takeover and expand this test to cover CHROME_OS and OSX too, so I don't want to discourage them with _win.cc postfix :) https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:36: : public_key_(public_key.begin(), public_key.end()) {} On 2017/01/18 00:39:22, dcheng wrote: > Sigh, more things that can't agree on passing data as string or vector =( Ack. The focus of this patch is not to re-write private networking too, of course. That should be the subject of another bug if someone is keen to fix it. https://codereview.chromium.org/2610953003/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_win.cc:128: callback_.Run(base64_encoded_key_data, ""); On 2017/01/18 00:39:22, dcheng wrote: > Nit: "" => std::string(), here and elsewhere. Done. PTAL and double-check that I made no error doing so. My antennae twitch when I see encrypted bytes and chipertext and so on ;) https://codereview.chromium.org/2610953003/diff/300001/chrome/common/extensio... File chrome/common/extensions/BUILD.gn (right): https://codereview.chromium.org/2610953003/diff/300001/chrome/common/extensio... chrome/common/extensions/BUILD.gn:29: "wifi_credentials_getter.mojom", On 2017/01/18 00:39:22, dcheng wrote: > Should we only conditionally compile this interface on Windows? Yes, seems to work. Done.
mojo lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/networking_private/networking_private_credentials_getter_browsertest.cc:24: network_guid_ = "chrome://test-wifi-get-key-from-system"; On 2017/01/16 11:09:58, noel gordon wrote: > On 2017/01/16 04:16:20, Sam McNally wrote: > > This should be a shared constant somewhere. > > Ack /shrug. Since the question in my mind was: well, could you define "somewhere" a little more precisely? We discussed various options off-line, and came up with the idea of _somewhere_ == "in the .mojom file" [1]. Moral of this story: better to not guess about imprecisely stated things I feel, since I can't read your mind - unless you've installed a WiFi network in it recently and are cool with me calling GetWiFiCredentials() on it :D [1] filed https://crbug.com/682102 about the mojo generated string type. https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2610953003/diff/260001/content/browser/utilit... content/browser/utility_process_mojo_client_browsertest.cc:23: void StartMojoService(bool disable_sandbox, bool run_elevated = false) { On 2017/01/16 11:09:58, noel gordon wrote: > On 2017/01/16 04:16:20, Sam McNally wrote: > > This would be nicer as an enum {SANDBOXED, UNSANDBOXED, ELEVATED}. > > Acknowledged. ... but somewhat unrelated to this change: maybe best done on another patch as we discussed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks for review folks, t'was fun, let's try landing. +jochen for OWNERS.
noel@chromium.org changed reviewers: + jochen@chromium.org
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo BUG=680928 ========== to ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make mojo request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls: this to work-around the fact that mojo does not currently work for blocking pool callers. BUG=680928 ==========
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make mojo request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls: this to work-around the fact that mojo does not currently work for blocking pool callers. BUG=680928 ========== to ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). BUG=680928 ==========
lgtm
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). BUG=680928 ========== to ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). Note the browser test does not cover the lower-level WiFi interface since the bots have no access to any WiFi network (good). There's a manual tool to test that aspect with (see the build file) and in my testing on Win10, that tool works very well. BUG=680928 ==========
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). Note the browser test does not cover the lower-level WiFi interface since the bots have no access to any WiFi network (good). There's a manual tool to test that aspect with (see the build file) and in my testing on Win10, that tool works very well. BUG=680928 ========== to ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). Note the browser test does not cover the lower-level WiFi interface since the bots have no access to any WiFi network (good). There's a manual tool to test that aspect with [1] and, in my testing on Win10, that tool works very well. [1] components/wifi/wifi_test.cc BUG=680928 ==========
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tibell@chromium.org, stevenjb@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2610953003/#ps340001 (title: "Add mojom test network name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1484744859075370,
"parent_rev": "c8885e8afa5f667deab0d2f12ff93dbe3d7c3dd0", "commit_rev":
"cd3f19adeb8b19062badb591ff33b770dd0eb21f"}
Message was sent while issue was closed.
Description was changed from ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). Note the browser test does not cover the lower-level WiFi interface since the bots have no access to any WiFi network (good). There's a manual tool to test that aspect with [1] and, in my testing on Win10, that tool works very well. [1] components/wifi/wifi_test.cc BUG=680928 ========== to ========== Convert utility process WiFi Credentials IPC to mojo Add an OS_WIN mojo API to the utility process to allow callers to get the WiFi credentials for a given network, an API that requires UAC elevation, and used by the chrome extensions private networking API. Add elevation support to the mojo utility process helper class and update tests for that class. Use the helper class to control the utility process (elevate, start, make request, terminate with the response). Add a browser test: post a get credentials request from the blocking pool to simulate how the existing call flow works. Internally post the work to a different thread (IO) before making mojo calls (this to work-around the fact that mojo does not currently support blocking pool callers). Note the browser test does not cover the lower-level WiFi interface since the bots have no access to any WiFi network (good). There's a manual tool to test that aspect with [1] and, in my testing on Win10, that tool works very well. [1] components/wifi/wifi_test.cc BUG=680928 Review-Url: https://codereview.chromium.org/2610953003 Cr-Commit-Position: refs/heads/master@{#444342} Committed: https://chromium.googlesource.com/chromium/src/+/cd3f19adeb8b19062badb591ff33... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001) as https://chromium.googlesource.com/chromium/src/+/cd3f19adeb8b19062badb591ff33... |
