Pavol, we no longer allow guest session if FRE is active in OOBE. But we should still allow guest sessions if FRE is not active, correct?

I didn't realize we were locking down this guest session somehow, can you talk to Julian and Mattias and figure out if this behavior is still useful?

It seems that the "limited guest session" feature got lost in refactorings for the network service, specifically:
ShouldBlockRequestForFrame calls are removed in https://chromium-review.googlesource.com/c/chromium/src/+/745326

My current understanding is:
- When the feature got introduced, the restriction applied to all guest sessions invoked before OOBE.
- Orthogonal to that, error screens invoked from some contexts (from AppLaunchSlpashScreenHandler) were not even showing the "Guest Session" link.
- Then the feature was effectively disable in the CL above. Now all Guest Sessions pre-OOBE can browse the web freely.
- Our response was to disable the Guest Session from the FRE Error Screen if FRE if VPD indicates that the device had been enrolled previously ("FRE explicitly required").


It seems that being able to enter the limited Guest Session from the FRE Error Screen could be useful, e.g. the user could open the terminal and run network_diag or whatever the command's name is to see if they can reach google servers.
I'm not sure if I like limiting all guest sessions before OOBE, but it would mean that there's no possibility we've missed some error screen entry point we should lock down for FRE.

BTW, it seems that this happened because:
- there was no browsertest testing this :)
- the assumption CL:745326 implicitly makes is that URLBlacklistManager::IsURLBlocked is consistent with URLBlacklistManager::ShouldBlockRequestForFrame.

This is subtly not true - ShouldBlockRequestForFrame checks the |override_blacklist| passed to URLBlacklistManager's ctor while IsURLBlocked doesn't.
More importantly, if IsURLBlocked returns true the semantics are probably "don't allow this URL no matter where it's requested from", while "ShouldBlockRequestForFrame" seems to not apply to subresources or background downloads[2].

Anyways, if we want this feature back, it seems that the 'frame-level' blocking (see rules[1]) intended here, i.e. the one |ShouldBlockRequestForFrame| was used for, should done in chrome and not in the net layer.
One place to consider would be e.g. | ChromeContentBrowserClient::ShouldAllowOpenURL| (I have not though this through yet though).

[1] https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_helpers.cc?rcl=bb3383fef616783132aa38c8211d2e364d2d9abe&l=29
[2] https://cs.chromium.org/chromium/src/components/policy/core/browser/url_blacklist_manager.h?rcl=bb3383fef616783132aa38c8211d2e364d2d9abe&l=146

> - there was no browsertest testing this :)

Yep. I discovered this because I'm trying to refactor this service, so I started writing a browser test and couldn't get it to pass!

+jam@/creis@ as content owners - do you think there's a good way to restrict which URLs are requestable by 'frames', as it used to be done here?

https://chromium.googlesource.com/chromium/src/+/574253081eebc50b94efb2e1d46fc83d2a75c8f1%5E%21/#F11

It *feels* like this should be something that could be specified in a chrome-content API boundary, not involving the network stack, but I may be wrong here.

Drew, assigning to you for now so you can aressign to whoever will work on it in case we decide it's urgent to bring this feature back, as I'll be OOO for two weeks.

Note: If we bring this feature back, we'll probably want to revert disabling guest session from the FRE error screen, i.e. https://chromium-review.googlesource.com/1059788

FYI: we will discuss this topic again once pmarko@ is back (two weeks), but we will probably abandon "limited guest sessions"

Taking this on my plate - After discussing this, we've decided to do two things:
(AI_1) remove limited guest session code
(AI_2) ensure that a guest session can not be entered if the device is not owned/enrolled and is in "forced re-enrollment explicitly required" state.

The limited guest session had two main advantages:
- it's automatically not possible to browse the web if we end up on a guest session from forced-re enrollment OOBE.
  -> AI_2 fixes this.

- it's possible to run network diagnostics tools when Forced Re-Enrollment breaks.
  -> It seems that the maintenance cost for supporting limited guest session is too high to achieve this. The same network diag tools could be run from another Chromebook (not in FRE mode) on the same network. If this turns out to be an issue (unlikely), we can try to expose network diag tools in a different way.

All in all, we've decided that the complexity & maintenance cost of supporting limited guest session is not worth the small benefit.

Uploaded https://chromium-review.googlesource.com/c/chromium/src/+/1117884 to remove unused policy code to simplify a refactoring.

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

commit edf345243b3b6285d3450ebac100fd3546cfc188
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Sat Jul 14 03:36:08 2018

Remove unused policy blacklist code

The URLBlacklistManager used to take a callback for overriding the
blacklist. This was used for limited guest sessions in OOBE, but all
code paths that actually triggered this code have been removed, and
these limited guest sessions will no longer be supported.

Bug: 850213
Change-Id: I299ccace10529696ba2b581ed861e8c6a6cd769b
Reviewed-on: https://chromium-review.googlesource.com/1119073
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575146}
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/chrome/browser/policy/policy_helpers.cc
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/chrome/browser/policy/policy_helpers.h
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/components/policy/content/policy_blacklist_navigation_throttle.cc
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/components/policy/content/policy_blacklist_navigation_throttle.h
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/components/policy/core/browser/url_blacklist_manager.cc
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/components/policy/core/browser/url_blacklist_manager.h
[modify] https://crrev.com/edf345243b3b6285d3450ebac100fd3546cfc188/components/policy/core/browser/url_blacklist_manager_unittest.cc