[CloudAP SSO] Add support for header injection to the network service. [chromium/src : main]
Igor Ruvinov (Gerrit)
1 comment:
Patchset:
mmenke@ this is the network-side of the changes. The other CLs are also up if you want to get a full picture of the feature. PTAL when you get a chance -- thanks!
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Igor Ruvinov would like Matt Menke to review this change.
[CloudAP SSO] Add support for header injection to the network service.
Give the network service the ability to fetch cookies/headers and attach
them to requests which are destined for an header injection-enabled
origin. The set of origins is configured on the URLRequestContext when
built and may be updated dynamically.
The cookies/headers will be fetched from the browser process using the
NetworkContextClient interface.
The first application for this feature will be Azure AD SSO. On requests
to a Microsoft IdP origin, auth data (in the form of cookies/headers)
can be fetched from relevant accounts signed into Windows via the
CloudAP API and injected into the request.
Bug: 1246839
Change-Id: Iabdc1cd48f6455a2deceb9849f7390d5e9352f23
---
M chrome/browser/preloading/prefetch/prefetch_proxy/prefetch_proxy_network_context_client.cc
M chrome/browser/preloading/prefetch/prefetch_proxy/prefetch_proxy_network_context_client.h
M content/browser/network_context_client_base_impl.cc
M content/browser/preloading/prefetch/prefetch_network_context_client.cc
M content/browser/preloading/prefetch/prefetch_network_context_client.h
M content/browser/storage_partition_impl.cc
M content/browser/storage_partition_impl.h
M content/public/browser/network_context_client_base.h
M services/network/network_context.cc
M services/network/network_context.h
M services/network/network_context_unittest.cc
M services/network/network_service_network_delegate.cc
M services/network/network_service_network_delegate.h
M services/network/network_service_unittest.cc
M services/network/public/cpp/BUILD.gn
A services/network/public/cpp/header_injection_types.h
M services/network/public/mojom/network_context.mojom
M services/network/test/test_network_context.h
M services/network/test/test_network_context_client.h
M services/network/url_loader.cc
M services/network/url_loader.h
21 files changed, 524 insertions(+), 10 deletions(-)
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
1 comment:
Patchset:
mmenke@ this is the network-side of the changes. […]
Forgot to add you as a reviewer 😊
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
6 comments:
Patchset:
Initial comments - still want to think a bit about the exact Mojo API (using the same struct for headers and cookies seems really confusing, but want to think about it a bit more).
Test coverage looks really good!
File services/network/network_context.cc:
Patch Set #1, Line 588:
header_injection_origins_ = std::move(*params_->header_injection_origins);Maybe SetHeaderInjectionOrigins(std::move(*params_->header_injection_origins))? It doesn't do anything extra that's interesting, but in case it ever does, means we'll reuse the logic.
if (origins)
header_injection_origins_ = std::move(origins.value());I don't think origins should be optional? We do nothing if it's nullopt, making the call useless in that case, so seems better to make it non-optional.
File services/network/network_service_network_delegate.cc:
Patch Set #1, Line 70:
IsValidHeaderNameWhat are we hoping to accomplish here? Safety from buggy 3P modules? This isn't sufficient for that (e.g., the Host header would very, very badly break things). Just avoiding DCHECKs?
We should probably check for invalid headers in the browser process, since we generally consider it a bug if Chrome code is generating invalid headers, and this API should be no exception. It should be up to the caller to know that they're getting data from an untrusted source, and to fix it.
Patch Set #1, Line 182:
OnGetHeaderInjectionDataWhy do we call this before the NetworkDelegate callback, but apply its modifications after?
Also, I think this would be a lot cleaner if we moved client->OnGetHeaderInjectionData() into OnBeforeStartTransaction(), and made this method strictly what's in the lambda - would need nested bind calls in OnBeforeStartTransaction(), but think that's clearer, and the inline function here is quite hard to read.
File services/network/url_loader.cc:
Patch Set #1, Line 1898:
header_injection_enabledThis name is not very clear. Maybe call it invoked_asynchronously, or must_invoke_callback, and document it in the header file?
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 2:Commit-Queue +1
6 comments:
Patchset:
Initial comments - still want to think a bit about the exact Mojo API (using the same struct for hea […]
I'm definitely open to API recommendations. Thanks for taking a look!
File services/network/network_context.cc:
Patch Set #1, Line 588:
header_injection_origins_ = std::move(*params_->header_injection_origins);
Maybe SetHeaderInjectionOrigins(std::move(*params_->header_injection_origins))? It doesn't do anyth […]
Done
if (origins)
header_injection_origins_ = std::move(origins.value());
I don't think origins should be optional? We do nothing if it's nullopt, making the call useless in […]
SG, I made origins non-optional.
File services/network/network_service_network_delegate.cc:
Patch Set #1, Line 70:
IsValidHeaderName
What are we hoping to accomplish here? Safety from buggy 3P modules? This isn't sufficient for tha […]
SG, I removed these checks from here and will add them to the browser process instead.
Patch Set #1, Line 182:
OnGetHeaderInjectionDataWhy do we call this before the NetworkDelegate callback, but apply its modifications after?
I'm not sure I understand -- are you referring to `OnBeforeStartTransactionCallback`? That callback is called in `URLLoader` after the new header data is added to the request. LMK if I misunderstood.
Also, I think this would be a lot cleaner if we moved client->OnGetHeaderInjectionData()...
Done. Thanks for the suggestion!
File services/network/url_loader.cc:
Patch Set #1, Line 1898:
header_injection_enabled
This name is not very clear. […]
Yeah I was also torn between this and what you suggested, `invoked_asynchronously` SGTM. I moved the explanation to the header. Thanks!
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
2 comments:
File services/network/network_service_network_delegate.cc:
Patch Set #2, Line 196:
OnGetHeaderInjectionDataCallbackDoes this need to be separate from the above method? I had thought one was being passed to OnBeforeStartTransaction (Hence my question about setting cookies after), but as-is, I don't think there's a need to make them separate methods?
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headersLet's just make this an network.mojom.HttpRequestHeaders modified_headers, and use net::HttpRequestHeaders::MergeFrom (which is what it's typemapped to), and take it as a single argument. That will unconditionally overwrite headers with new values. It more closely resembles what URLLoader::FollowRedirect does, and is more flexible (this can't delete headers). If the caller wants to merge headers.
We'd need to pass it the original HttpRequestHeaders as well, if we want it to be able to merge cookie headers like the current code does, of course.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headers
Let's just make this an network.mojom. […]
Just to clarify before I update the design/implementation, we would:
1. Pass all request headers to the browser process
2. The browser process updates the headers however it wants (adds/deletes/modifies)
3. The browser process sends back the `modified_headers` which the network service uses to replace the original headersAre we okay with sending all headers to the browser process? If we are, then SGTM!
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
1 comment:
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headers
Just to clarify before I update the design/implementation, we would:
1. Pass all request headers to the browser process
2. The browser process updates the headers however it wants (adds/deletes/modifies)
3. The browser process sends back the `modified_headers` which the network service uses to replace the original headersAre we okay with sending all headers to the browser process? If we are, then SGTM!
Almost....the way URLLoader::FollowRedirect works is that only the modified headers are sent back to the network process, which are then merged with the original headers.
I assume this is done to reduce copies/overhead.
I think we're fine sending all headers to the browser process - it's the most trusted process, after all, and it can already get them by hooking up devtools, anyways.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headersJust to clarify before I update the design/implementation, we would:
1. Pass all request headers to the browser process
2. The browser process updates the headers however it wants (adds/deletes/modifies)
3. The browser process sends back the `modified_headers` which the network service uses to replace the original headersAre we okay with sending all headers to the browser process? If we are, then SGTM!
Almost....the way URLLoader::FollowRedirect works is that only the modified headers are sent back to the network process, which are then merged with the original headers.
I assume this is done to reduce copies/overhead.
I think we're fine sending all headers to the browser process - it's the most trusted process, after all, and it can already get them by hooking up devtools, anyways.
(Note that this means there's no way to remove a header - just replace it with an empty one. FollowRedirect adds a list of headers to remove for that, but we don't currently need it here, so don't need to bother)
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
1 comment:
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headers
> > Just to clarify before I update the design/implementation, we would: […]
Got it, thanks for clarifying. I'll update the code and re-request review.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 4:Commit-Queue +1
2 comments:
File services/network/network_service_network_delegate.cc:
Patch Set #2, Line 196:
OnGetHeaderInjectionDataCallback
Does this need to be separate from the above method? I had thought one was being passed to OnBefore […]
You're right, we don't need both; I combined them into a single callback.
File services/network/public/mojom/network_context.mojom:
Patch Set #2, Line 858:
array<HeaderInjectionData> cookies, array<HeaderInjectionData> headers
Got it, thanks for clarifying. I'll update the code and re-request review.
Done!
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 5:Commit-Queue +1
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
7 comments:
File services/network/network_service_network_delegate.cc:
Patch Set #5, Line 112:
requestNull check not needed.
base::Contains(network_context_->header_injection_origins(),
url::Origin::Create(request->url()))Hrm...Wonder if it's better to use IsSameOriginWith(request->url()) for each origin, to avoid the string copy. Guess it probably doesn't matter much.
Patch Set #5, Line 122:
weak_ptr_factory_.GetWeakPtr()BUG: This doesn't handle URLLoader deletion - NetworkServiceNetworkDelegate::OnGetHeaderInjectionDataCallback() would be invoked with a deleted URLRequest (Since the NetworkDelegate is a per-NetworkContext object, it can outlive a cancelled URLLoader), and then URLLoader::ForRequest() would crash.
Think the simplest fix is to just move this into URLLoader - I can't imagine this matters in the WebSocket case (Those aren't HTTP requests, though they look like HTTP. I'm not entirely sure if they follow redirects, though they may). Other requests are mostly browser internal requests, which this should also not matter for.
.
We need a test for the call-after-delete case, too.
#if !BUILDFLAG(IS_IOS)
WebSocket* web_socket = WebSocket::ForRequest(*request);
if (web_socket)
web_socket->OnBeforeStartTransaction(headers, std::move(callback));
#endif // !BUILDFLAG(IS_IOS)This case is buggy - it needs an `invoked_asynchronously` bit. Above suggested crash fix would work around this issue.
Similarly, we're not handling the case there is no web socket or URLLoader.
File services/network/public/mojom/network_context.mojom:
uses the same scheme, port, and host as any of the header injection
// URLs in the network context=> "has an origin matching one of the `header_injection_origins` passed to the NetworkContext on construction."
Patch Set #5, Line 850:
modified_headersShould document modified headers (Mention that they overwrite any matching headers on the request, leaving other headers unmodified).
File services/network/url_loader.h:
`invoked_asynchronously` ensures that the request is processed correctly
// when header injection is enabled in `NetworkServiceNetworkDelegate` since
// the request is handled asynchronously to allow for injection data fetching.Let's cover what `invoked_asynchronously` does, not just how it's used. Maybe something like:
If `invoked_asynchronously` is true, then `callback` must be invoked with the result, even on synchronous completion, as the source URLRequest is no longer on the callstack to receive the return value. This is needed in the case that the NetworkDelegate to applies its own asynchronous logic to the request.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 7:Commit-Queue +1
7 comments:
File services/network/network_service_network_delegate.cc:
Patch Set #5, Line 112:
requestNull check not needed.
Done
base::Contains(network_context_->header_injection_origins(),
url::Origin::Create(request->url()))
Hrm... […]
Sure that works as well, done
Patch Set #5, Line 122:
weak_ptr_factory_.GetWeakPtr()
BUG: This doesn't handle URLLoader deletion - NetworkServiceNetworkDelegate::OnGetHeaderInjectionDa […]
SG, I moved this into `URLLoader` since we didn't inject headers for `WebSocket`/other requests anyways.
I also added a call-after-delete test.
#if !BUILDFLAG(IS_IOS)
WebSocket* web_socket = WebSocket::ForRequest(*request);
if (web_socket)
web_socket->OnBeforeStartTransaction(headers, std::move(callback));
#endif // !BUILDFLAG(IS_IOS)
This case is buggy - it needs an `invoked_asynchronously` bit. […]
I moved the header injection logic into `URLLoader`, so this should no longer be buggy.
File services/network/public/mojom/network_context.mojom:
uses the same scheme, port, and host as any of the header injection
// URLs in the network context
=> "has an origin matching one of the `header_injection_origins` passed to the NetworkContext on con […]
Done, thanks for the suggestion. I removed `"on construction"` since the origins can also be updated after construction.
Patch Set #5, Line 850:
modified_headers
Should document modified headers (Mention that they overwrite any matching headers on the request, l […]
Done, LMKWYT
File services/network/url_loader.h:
`invoked_asynchronously` ensures that the request is processed correctly
// when header injection is enabled in `NetworkServiceNetworkDelegate` since
// the request is handled asynchronously to allow for injection data fetching.
Let's cover what `invoked_asynchronously` does, not just how it's used. Maybe something like: […]
SG, thanks for the suggestion.
I moved the async part into `URLLoader` like you suggested above so, since `invoked_asynchronously` no longer exists, I also moved this explanation there and modified it slightly.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
4 comments:
File services/network/network_context_unittest.cc:
nit: Prefer .test origins in tests. This avoids accidentally depending on a real origin, both in terms of if a request accidentally makes real DNS requests, and, with google origins especially, avoids interacting with any google-specific magic in Chrome.
File services/network/network_service_network_delegate.cc:
Patch Set #8, Line 111:
network_context_->header_injection_origins()Let's instead add a method to get this to URLLoaderContext() (Which the URLLoader has access to, and the implementation of it in URLLoaderFactory has access to the NetworkContext. (I'm skeptical of the value of going to such lengths not to expose the NetworkContext to the URLLoader, but that's the way things are currently designed...)
File services/network/url_loader.cc:
Patch Set #8, Line 1931:
net::HttpRequestHeaders merged_modified_headersSeems cleaner to me to make this an optional, rather than to make IsEmpty() a magic value.
header_client_->OnBeforeSendHeaders(
merged_modified_headers.IsEmpty() ? headers : merged_modified_headers,
base::BindOnce(&URLLoader::OnBeforeSendHeadersComplete,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
return;We should have test coverage of this block - we should actually have at least four tests, one where headers are modified and one where they are not (By both delegates).
I think there's a bug here - if OnGetHeaderInjectionData() modifies headers, but header_client_->OnBeforeSendHeaders() returns a nullopt, then the header modifications are discarded, right? I don't think that's the behavior we want.
We probably need to pass the merged headers, in non-null to OnBeforeSendHeadersComplete(), and have it use headers from the header client, if present, otherwise, using merged_modified_headers, if present, and otherwise pass in nullopt.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 9:Commit-Queue +1
4 comments:
File services/network/network_context_unittest.cc:
nit: Prefer .test origins in tests. […]
SG, updated to `.test`
File services/network/network_service_network_delegate.cc:
Patch Set #8, Line 111:
network_context_->header_injection_origins()
Let's instead add a method to get this to URLLoaderContext() (Which the URLLoader has access to, and […]
Nice, that's much better -- done
File services/network/url_loader.cc:
Patch Set #8, Line 1931:
net::HttpRequestHeaders merged_modified_headersSeems cleaner to me to make this an optional, rather than to make IsEmpty() a magic value.
Done
header_client_->OnBeforeSendHeaders(
merged_modified_headers.IsEmpty() ? headers : merged_modified_headers,
base::BindOnce(&URLLoader::OnBeforeSendHeadersComplete,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
return;We should have test coverage of this block - we should actually have at least four tests, one where headers are modified and one where they are not (By both delegates).
Done, I added some tests for the header client blocks. I also moved the entire test suite into `network_context_unittest.cc` since there's already a test header client defined there.
I think there's a bug here - if OnGetHeaderInjectionData() modifies headers, but header_client_->OnBeforeSendHeaders() returns a nullopt, then the header modifications are discarded, right? I don't think that's the behavior we want.
To be honest I'm not sure what exactly the header client does, but if that behavior makes sense to you then SGTM. I updated the behavior and added some tests (`Success_HeaderClientNull` and `EmptyInjectionData_HeaderClientNull`).
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 10:Commit-Queue +1
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
10 comments:
File services/network/network_context_unittest.cc:
Patch Set #10, Line 7690:
NetworkContextHeaderInjectionTest() {}use '= default' to define a trivial default constructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Lint observed on `android-clang-tidy-rel`, `linux-chromeos-clang-tidy-rel`, and `linux-clang-tidy-rel`)
Please fix.
Patch Set #10, Line 7691:
~NetworkContextHeaderInjectionTest() override {}use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Lint observed on `android-clang-tidy-rel`, `linux-chromeos-clang-tidy-rel`, and `linux-clang-tidy-rel`)
Please fix.
Maybe add `SCOPED_TRACE(name);`?
Patch Set #10, Line 7827:
OriginMismatchThis test should set an injection origin, right? Otherwise, it's not exactly a mismatch, since there's nothing to match the origin to. At least I assume we'd prefer to test the case when there's an injection origin that doesn't match over there being no injection origin. Same with other mismatch tests
Patch Set #10, Line 7879:
Success_HeaderClientI think we either need a comment or a better test name. This is the case where we inject headers via NetworkContext, but and then HeaderClient also modifies them, right?
Patch Set #10, Line 7928:
ValidateHeader(kNewHeaderName, kNewHeaderValue);Test for the header HeaderClient adds, too? That's the most important bit. Same for the next test.
File services/network/public/mojom/network_context.mojom:
Remove space (`git cl format` doesn't work on mojom files, sadly)
File services/network/url_loader.cc:
header_client_->OnBeforeSendHeaders(
merged_modified_headers.IsEmpty() ? headers : merged_modified_headers,
base::BindOnce(&URLLoader::OnBeforeSendHeadersComplete,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
return;We should have test coverage of this block - we should actually have at least four tests, one where headers are modified and one where they are not (By both delegates).
Done, I added some tests for the header client blocks. I also moved the entire test suite into `network_context_unittest.cc` since there's already a test header client defined there.
I think there's a bug here - if OnGetHeaderInjectionData() modifies headers, but header_client_->OnBeforeSendHeaders() returns a nullopt, then the header modifications are discarded, right? I don't think that's the behavior we want.
To be honest I'm not sure what exactly the header client does, but if that behavior makes sense to you then SGTM. I updated the behavior and added some tests (`Success_HeaderClientNull` and `EmptyInjectionData_HeaderClientNull`).
It's a method that takes an optional that, if present, entirely overwrites headers (We could have done that here, too, instead of taking the deltas - wasn't aware of the details of this API when I made the suggestion of the current arrangement, but regardless, the two APIs need to work together).
HeaderClient interposes on *all* requests that a URLLoaderFactory makes, however, and is designed for extensions. (Adding a roundtrip whenever it's in use is the reason it's not suitable for our purposes)
File services/network/url_loader.cc:
Patch Set #10, Line 1943:
merged_modified_headersLet's std::move this here, to save a copy.
net::NetworkDelegate::OnBeforeStartTransactionCallback callback,nit: Let's put this second, making it the last of the bound parameters, to match other methods.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Attention is currently required from: Matt Menke.
Patch set 11:Commit-Queue +1
10 comments:
File services/network/network_context_unittest.cc:
Patch Set #10, Line 7690:
NetworkContextHeaderInjectionTest() {}
> use '= default' to define a trivial default constructor (https://clang.llvm. […]
Done
Patch Set #10, Line 7691:
~NetworkContextHeaderInjectionTest() override {}
> use '= default' to define a trivial destructor (https://clang.llvm. […]
Done
Maybe add `SCOPED_TRACE(name);`?
Good idea, done!
Patch Set #10, Line 7827:
OriginMismatch
This test should set an injection origin, right? Otherwise, it's not exactly a mismatch, since ther […]
Sure, done
Patch Set #10, Line 7879:
Success_HeaderClient
I think we either need a comment or a better test name. […]
That's correct, I added a comment to better describe the test.
Patch Set #10, Line 7928:
ValidateHeader(kNewHeaderName, kNewHeaderValue);Test for the header HeaderClient adds, too? That's the most important bit. Same for the next test.
Done!
File services/network/public/mojom/network_context.mojom:
Remove space (`git cl format` doesn't work on mojom files, sadly)
Good catch, fixed
File services/network/url_loader.cc:
header_client_->OnBeforeSendHeaders(
merged_modified_headers.IsEmpty() ? headers : merged_modified_headers,
base::BindOnce(&URLLoader::OnBeforeSendHeadersComplete,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
return;
> > We should have test coverage of this block - we should actually have at least four tests, one wh […]
Got it, thanks for explaining!
File services/network/url_loader.cc:
Patch Set #10, Line 1943:
merged_modified_headersLet's std::move this here, to save a copy.
Adding `std::move` here seems to break the earlier part (`merged_modified_headers.value_or(headers)`) so the expected headers don't end up being passed in.
I did add `std::move` below to the `OnBeforeSendHeadersComplete` call.
net::NetworkDelegate::OnBeforeStartTransactionCallback callback,nit: Let's put this second, making it the last of the bound parameters, to match other methods.
Done
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Attention is currently required from: Igor Ruvinov.
Patch set 11:Code-Review +1
1 comment:
Patchset:
LGTM! Sorry for slowness - last 2-3 weeks have been particularly busy for me.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.
Igor Ruvinov (Gerrit)
Igor Ruvinov abandoned this change.
To view, visit change 3921827. To unsubscribe, or for help writing mail filters, visit settings.