New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 832749 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Proj-Servicification

Blocked on:
issue 845683

Blocking:
issue 769401



Sign in to add a comment

Ensure DMServer header addition works with network service

Project Member Reported by jam@chromium.org, Apr 13 2018

Issue description

ChromeResourceDispatcherHostDelegateBrowserTest.PolicyHeader
ChromeResourceDispatcherHostDelegateBrowserTest.PolicyHeaderForRedirect
are tests to ensure that PolicyHeaderIOHelper::AddPolicyHeaders adds the correct headers for requests to a specific URL (DMServer).

This functionality was added in
https://codereview.chromium.org/99433004
https://codereview.chromium.org/307653006

PolicyHeaderIOHelper is currently called by ChromeResourceDispatcherHostDelegate, which isn't compatible with the network service (as net/ will be running in a different process).

The URL that's passed to PolicyHeaderIOHelper appears to be used in two places: net::URLFetcher in DeviceManagementService::StartJob and navigating a WebContents in ArcActiveDirectoryEnrollmentTokenFetcher::InitiateSamlFlow.

To figure out the best way to solve this, there are a few open questions:
1) The Arc use case came much later, so I'm wondering if the original implementation used ResourceDispatcherHost hooks (as opposed to just adding the header always in DeviceManagementRequestJobImpl::ConfigureRequest) because even then the user could navigate to that URL?
2) do we need to set this header on subresources (i.e. image, JS, stylesheets) or just frames and the URLFetcher?


If we need it just for frames and URLFetcher, that'd be easiest. We could then add the header in a ContentBrowserClient::NavigationRequestStarted for navigations and in DeviceManagementRequestJobImpl::ConfigureRequest for the fetcher.
 

Comment 1 by mmenke@chromium.org, Apr 13 2018

Components: Enterprise
Note that URLFetcher requests don't go through ChromeResourceDispatcherHostDelegates, so unless it's being called from somewhere some other location, this header is not being added for fetcher-initiated requests.

Comment 2 by jam@chromium.org, Apr 13 2018

Ah, thanks. Ok then we can put aside question 1 as it seems like we definitely need the headers added for navigations.

Comment 3 by jam@chromium.org, Apr 16 2018

Thinking about this more, if the header addition isn't needed for URLFetcher it seems like we only need to add it for navigations? And the ContentBrowserClient::NavigationRequestStarted delegate call might be enough.

One other question: is this for ChromeOS only or desktop as well?
Both Chrome OS and Desktop. And to follow up on our offline conversation, I think we only need it for navigations (not subresource loads).

Comment 5 by jam@chromium.org, Apr 16 2018

Owner: chongz@chromium.org
Status: Assigned (was: Available)
Great, thanks Andrew.

Chong: can you take care of this?

Comment 6 by chongz@chromium.org, Apr 16 2018

Components: Internals>Services>Network
Sure, will do. Thanks for the detailed backgrounds!

Comment 7 by chongz@chromium.org, Apr 27 2018

Status: Started (was: Assigned)

Comment 8 by chongz@chromium.org, May 18 2018

Labels: -Pri-2 Proj-Servicification-Canary Pri-1

Comment 9 by chongz@chromium.org, May 22 2018

Blockedon: 845683
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a4e12afac8d44d768890fb3b2b697341eec63861

commit a4e12afac8d44d768890fb3b2b697341eec63861
Author: Chong Zhang <chongz@chromium.org>
Date: Fri Jun 22 01:51:30 2018

[NetworkService] Support adding policy headers for DMServer

This CL:
1. Allows chrome to add policy headers for navigations and redirects
   via content API.
2. Removes the old |PolicyHeaderIOHelper| codepath and moves policy
   header tests to a new location.
3. Adds a move constructor/assignment for |net::HttpRequestHeaders|.

Bug:  832749 , 845683 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I713ed30da33ba5feb5f437f8927b173c2f84d91a
Reviewed-on: https://chromium-review.googlesource.com/1100393
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Drew Wilson <atwilson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569503}
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/chrome_content_browser_client_browsertest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/loader/chrome_resource_dispatcher_host_delegate_browsertest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/BUILD.gn
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper.cc
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper.h
[delete] https://crrev.com/3c724bcd2e4d2795518b725b937aa8a7f5d89362/components/policy/core/common/cloud/policy_header_io_helper_unittest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/components/policy/core/common/cloud/policy_header_service_unittest.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/detachable_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/intercepting_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/null_resource_controller.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/null_resource_controller.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_controller.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_handler.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_handler.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/browser/loader/resource_loader.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/content/public/browser/content_browser_client.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/net/http/http_request_headers.cc
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/net/http/http_request_headers.h
[modify] https://crrev.com/a4e12afac8d44d768890fb3b2b697341eec63861/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Status: Fixed (was: Started)

Sign in to add a comment